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.

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

Add metrics for negotiated group with client #11844

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

Conversation

masaori335
Copy link
Contributor

Like TLS ciphers.

proxy.process.ssl.group.user_agent.P-256
proxy.process.ssl.group.user_agent.P-384
proxy.process.ssl.group.user_agent.P-521
proxy.process.ssl.group.user_agent.X25519
proxy.process.ssl.group.user_agent.P-224
proxy.process.ssl.group.user_agent.X448
proxy.process.ssl.group.user_agent.X25519MLKEM768

The SSL_get_negotiated_group is introduced by OpenSSL 3.0.0 (and BoringSSL), so this doesn't work with OpenSSL 1.1.1 and older.

@masaori335 masaori335 self-assigned this Nov 5, 2024
@masaori335 masaori335 added this to the 10.1.0 milestone Nov 5, 2024
@masaori335 masaori335 marked this pull request as draft November 5, 2024 07:18
int nid = SSL_get_negotiated_group(const_cast<SSL *>(ssl));
#endif
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

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.

@masaori335 masaori335 marked this pull request as ready for review November 7, 2024 05:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants