expose verify helper functions #1325
Conversation
|
@Fritiofhedstrom It looks like your branch doesn't build. Could you take a look at fixing that (maybe also rebase on main)? |
0f21927 to
a5868f4
Compare
|
rebased on main and adapted to new DnsName struct |
rustls/tests/server_cert_verifier.rs
Outdated
| use rustls::client::{ | ||
| HandshakeSignatureValid, ServerCertVerified, ServerCertVerifier, WebPkiVerifier, | ||
| }; | ||
| use rustls::client::WebPkiVerifier; |
There was a problem hiding this comment.
This seems unrelated? I'd rather keep all imports from the same module on the same line.
rustls/src/verify.rs
Outdated
| ) | ||
| .map_err(pki_error) | ||
| .map(|_| cert)?; | ||
| cert.verify_is_valid_tls_server_cert( |
There was a problem hiding this comment.
I feel like both this and the server name check should use the newly created functions, to avoid us having to keep them in sync manually.
01a67ec to
408be12
Compare
rustls/src/verify.rs
Outdated
| intermediates: &[Certificate], | ||
| now: SystemTime, | ||
| ) -> Result<(), Error> { | ||
| let chain: Vec<&[u8]> = intermediates |
There was a problem hiding this comment.
Could we reuse prepare() here, maybe by changing its API a bit to fit better?
There was a problem hiding this comment.
I split it to seperate functions for each argument
There was a problem hiding this comment.
This looks like a fair compromise to me. Thanks for putting in the work :-) I think we should hold merge on Ctz's opinion but from my perspective I'm +1.
I left a couple small nits to consider RE: tightening up the unreachable_pub allows but I don't think that needs to be blocking feedback.
Could you also rebase to resolve conflicts? Thanks!
|
I like this too. It allows us to use rustls library code for the chain validation, but custom code for the name verification. This will allow us to shrink our custom verifiers a bit and depend on the default Rustls chain validation piece. |
acd38b5 to
6cff280
Compare
Codecov Report
@@ Coverage Diff @@
## main #1325 +/- ##
=======================================
Coverage 96.31% 96.32%
=======================================
Files 61 61
Lines 14282 14299 +17
=======================================
+ Hits 13756 13773 +17
Misses 526 526
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Thank you! |
Alternative to: #1320 that makes it less easy to create a ServerCertVerifier that skips domain name verification.
I chose not to call the new public functions from the WebPkiVerifier impl block because they need to parse the Certificate to webpki::EndEntity. If I did call the new functions, then the cert would be parsed twice.