Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add metrics for negotiated group with client #11844

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add metrics for negotiated group with client #11844

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
245a01e
Add metrics for negotiated group with client
masaori335 Nov 5, 2024
0ff7688
Use SSL_get_all_group_names for BoringSSL
masaori335 Nov 6, 2024
809329a
Fix format
masaori335 Nov 6, 2024
30aa894
Fix for clang-analyzer build
masaori335 Nov 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 80 additions & 0 deletions src/iocore/net/SSLStats.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,75 @@
SSLStatsBlock ssl_rsb;
std::unordered_map<std::string, Metrics::Counter::AtomicType *> cipher_map;

#ifdef OPENSSL_IS_BORINGSSL
std::unordered_map<std::string, Metrics::Counter::AtomicType *> tls_group_map;
#elif defined(SSL_get_negotiated_group)
std::unordered_map<int, Metrics::Counter::AtomicType *> tls_group_map;
#endif

namespace
{
DbgCtl dbg_ctl_ssl{"ssl"};

#if defined(OPENSSL_IS_BORINGSSL) || defined(SSL_get_negotiated_group)

template <typename T>
void
add_group_stat(T key, const std::string &name)
{
// If not already registered ...
if (tls_group_map.find(key) == tls_group_map.end()) {
Metrics::Counter::AtomicType *metric = Metrics::Counter::createPtr("proxy.process.ssl.group.user_agent." + name);

tls_group_map.emplace(key, metric);
Dbg(dbg_ctl_ssl, "registering SSL group metric '%s'", name.c_str());
}
}

#endif // OPENSSL_IS_BORINGSSL or SSL_get_negotiated_group

#if not defined(OPENSSL_IS_BORINGSSL) and defined(SSL_get_negotiated_group) // OPENSSL 3.x

struct TLSGroup {
int nid;
std::string name;
};

// NID and Group table. Some groups are not defined by some library.
const TLSGroup TLS_GROUPS[] = {
{SSL_GROUP_STAT_OTHER_KEY, "OTHER" },
{NID_X9_62_prime256v1, "P-256" },
{NID_secp384r1, "P-384" },
{NID_secp521r1, "P-521" },
{NID_X25519, "X25519" },
#ifdef NID_secp224r1
{NID_secp224r1, "P-224" },
#endif
#ifdef NID_X448
{NID_X448, "X448" },
#endif
#ifdef NID_ffdhe2048
{NID_ffdhe2048, "ffdhe2048" },
#endif
#ifdef NID_ffdhe3072
{NID_ffdhe3072, "ffdhe3072" },
#endif
#ifdef NID_ffdhe4096
{NID_ffdhe4096, "ffdhe4096" },
#endif
#ifdef NID_ffdhe6144
{NID_ffdhe6144, "ffdhe6144" },
#endif
#ifdef NID_ffdhe8192
{NID_ffdhe8192, "ffdhe8192" },
#endif
#ifdef NID_X25519MLKEM768
{NID_X25519MLKEM768, "X25519MLKEM768"},
#endif
};

#endif // OPENSSL 3.x

} // end anonymous namespace

// ToDo: This gets called once per global sync, for now at least.
Expand Down Expand Up @@ -175,6 +240,21 @@ SSLInitializeStatistics()
// Add "OTHER" for ciphers not on the map
add_cipher_stat(SSL_CIPHER_STAT_OTHER.c_str(), "proxy.process.ssl.cipher.user_agent." + SSL_CIPHER_STAT_OTHER);

// TLS Group
#if defined(OPENSSL_IS_BORINGSSL)
size_t list_size = SSL_get_all_group_names(nullptr, 0);
std::vector<const char *> group_list(list_size);
SSL_get_all_group_names(group_list.data(), group_list.size());

for (const char *name : group_list) {
add_group_stat<std::string>(name, name);
}
#elif defined(SSL_get_negotiated_group)
for (auto group : TLS_GROUPS) {
add_group_stat<int>(group.nid, group.name);
}
#endif // OPENSSL_IS_BORINGSSL or SSL_get_negotiated_group

SSL_free(ssl);
SSLReleaseContext(ctx);
}
9 changes: 9 additions & 0 deletions src/iocore/net/SSLStats.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@

#include "tsutil/Metrics.h"

#include <openssl/ssl.h>

using ts::Metrics;

// For some odd reason, these have to be initialized with nullptr, because the order
Expand Down Expand Up @@ -100,6 +102,13 @@ struct SSLStatsBlock {
extern SSLStatsBlock ssl_rsb;
extern std::unordered_map<std::string, Metrics::Counter::AtomicType *> cipher_map;

#if defined(OPENSSL_IS_BORINGSSL)
extern std::unordered_map<std::string, Metrics::Counter::AtomicType *> tls_group_map;
#elif defined(SSL_get_negotiated_group)
extern std::unordered_map<int, Metrics::Counter::AtomicType *> tls_group_map;
constexpr int SSL_GROUP_STAT_OTHER_KEY = 0;
#endif

// Initialize SSL statistics.
void SSLInitializeStatistics();

Expand Down
22 changes: 22 additions & 0 deletions src/iocore/net/SSLUtils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1086,6 +1086,28 @@ ssl_callback_info(const SSL *ssl, int where, int ret)
}
Metrics::Counter::increment(it->second);
}

#if defined(OPENSSL_IS_BORINGSSL)
uint16_t group_id = SSL_get_group_id(ssl);
if (group_id != 0) {
const char *group_name = SSL_get_group_name(group_id);
if (auto it = tls_group_map.find(group_name); it != tls_group_map.end()) {
Metrics::Counter::increment(it->second);
} else {
Warning("Unknown TLS Group");
}
}
#elif defined(SSL_get_negotiated_group)
int nid = SSL_get_negotiated_group(const_cast<SSL *>(ssl));
if (nid != NID_undef) {
if (auto it = tls_group_map.find(nid); it != tls_group_map.end()) {
Copy link
Member

@maskit maskit Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have OTHER, otherwise we'd see a mysterious drop when SSL libraries and clients support a new group.
#9623

And the table could be autogenerated if the library used has SSL_get_all_group_names. Only users who use SSL library that doesn't have the function would be affected if new groups are added.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeh, I was thinking the same thing. Will do them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BoringSSL has an API to get NID from group name?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it doesn't. Look at cipher_map. Its key is a string, not NID. It may not be great in terms of calculating the hash, but if we really care about it, I suppose we could use the pointer for the const char.

Metrics::Counter::increment(it->second);
} else {
auto other = tls_group_map.find(SSL_GROUP_STAT_OTHER_KEY);
Metrics::Counter::increment(other->second);
}
}
#endif // OPENSSL_IS_BORINGSSL or SSL_get_negotiated_group
}
}

Expand Down