-
-
Notifications
You must be signed in to change notification settings - Fork 384
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
crypto: expose negotiated_cipher_suite in the hadshake data #2001
base: main
Are you sure you want to change the base?
Conversation
@@ -256,6 +261,9 @@ pub struct HandshakeData { | |||
/// | |||
/// Always `None` for outgoing connections | |||
pub server_name: Option<String>, | |||
|
|||
/// The ciphersuite negotiated with the peer. | |||
pub negotiated_cipher_suite: CipherSuite, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be better to avoid using a rustls-specific type here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but on the other hand why not? :D
This is rustls.rs
module, which already depends on rustls
specific (and quinn
re-export rustls
as well)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if we're going to have a rustls-specific type, why not SupportedCipherSuite
instead of CipherSuite
?
@Ralith have opinions on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SupportedCipherSuite
also contains the implementation.
I thought the scope of this crypto::rustls::HandshakeData
for quinn
is just give the information about the negotiated cipher and implementation is not needed (reducing overhead of the struct); that's why I went for CipherSuite
instead.
Anyway lemme know what do you prefer
Once |
As title.
I am not totally convinced by that
.expect("cipher is negotiated")
.It makes sense to me: as AFAIK reaching that point always implies the cipher is negotiated, but I might have missed a edge scenario