Conversation
ahl
changed the title
davepacheco
left a comment
davepacheco
left a comment
There was a problem hiding this comment.
Thanks so much for doing this! It looks like it was probably annoying to do. I found the release notes extremely helpful for reviewing this -- made me realize how much of this was mechanical.
As @sunshowers mentioned in #832, this is a breaking change for consumers that are providing a rustls::ServerConfig directly. We should add a CHANGELOG entry saying so. I think it's fine to just say we updated rustls from X to Y and if you're providing ServerConfig directly then you need to apply any corresponding changes.
Do we want to make sure Omicron can be updated smoothly to this before we land it? (I'm a little less worried about other stuff because I don't expect anybody else is using rustls::ServerConfig.)
| fn verify_tls12_signature( | ||
| &self, | ||
| _message: &[u8], | ||
| _cert: &rustls::pki_types::CertificateDer<'_>, | ||
| _dss: &rustls::DigitallySignedStruct, | ||
| ) -> Result<rustls::client::danger::HandshakeSignatureValid, rustls::Error> | ||
| { | ||
| Ok(rustls::client::danger::HandshakeSignatureValid::assertion()) | ||
| } | ||
|
|
||
| fn verify_tls13_signature( | ||
| &self, | ||
| _message: &[u8], | ||
| _cert: &rustls::pki_types::CertificateDer<'_>, | ||
| _dss: &rustls::DigitallySignedStruct, | ||
| ) -> Result<rustls::client::danger::HandshakeSignatureValid, rustls::Error> | ||
| { | ||
| Ok(rustls::client::danger::HandshakeSignatureValid::assertion()) | ||
| } | ||
|
|
||
| fn supported_verify_schemes(&self) -> Vec<rustls::SignatureScheme> { | ||
| // Default algorithm from rcgen | ||
| vec![rustls::SignatureScheme::ECDSA_NISTP256_SHA256] |
There was a problem hiding this comment.
What's the deal with these implementations?
There was a problem hiding this comment.
I wasn't 100% sure on this, but according to https://docs.rs/rustls/latest/rustls/client/danger/trait.ServerCertVerifier.html#tymethod.verify_tls12_signature
cert has already been validated by ServerCertVerifier::verify_server_cert.
So it seemed like waving the signature through preserved the existing behavior. I'm happy to add a comment to that effect; also happy to discuss if this doesn't seem like the right policy.
Will do. Do we want to potentially abstract away that external dependency... or not worth it at the moment?
Yes; I'll give that a shot. |
No. That's what we did before. What we built was not general enough and we spent a lot of effort to make things work with our abstraction when rustls provided (via |
|
Wonder if we can petition rustls to stabilize just their types in a separate crate at some point. There's no difference between https://docs.rs/rustls/0.21.10/rustls/server/struct.ServerConfig.html and https://docs.rs/rustls/0.22.2/rustls/server/struct.ServerConfig.html as far as I can tell. |
3 participants
Closes #832