Skip to content

Conversation

@cpu
Copy link
Member

@cpu cpu commented Apr 14, 2024

The NoneVerifier that's used by default if a rustls client config builder is built without a verifier being specified was configured to return Error::InvalidCertificate(CertificateError::BadSignature) from all of its trait methods.

This branch updates the verify_server_cert() trait method to instead return Error::InvalidCertificate(CertificateError::UnknownIssuer). This will better match what would happen if you configured an empty root certificate store with a real verifier and is perhaps less confusing to debug than an error indicating a cryptographic signature validation error. BadSignature is still used for verify_tls12_signature() and verify_tls13_signature() where it feels like an appropriate default error. In practice neither of these trait methods come into play in NoneVerifier use.

Resolves #409

The `NoneVerifier` that's used by default if a rustls client config
builder is built without a verifier being specified was configured to
return `Error::InvalidCertificate(CertificateError::BadSignature` from
all of its trait methods.

This commit updates the `verify_server_cert` trait method to instead
return `Error::InvalidCertificate(CertificateError::UnknownIssuer)`.
This will better match what would happen if you configured an empty root
certificate store with a real verifier and is perhaps less confusing to
debug than an error indicating a cryptographic signature validation
error.
@cpu
Copy link
Member Author

cpu commented Apr 14, 2024

Alternatively, maybe rustls_client_config_builder_build should error if there hasn't been a verifier configured? The rustls_client_config_builder_new docs say:

/// This starts out with no trusted roots.
/// Caller must add roots with rustls_client_config_builder_load_roots_from_file
/// or provide a custom verifier.

If the caller must set up a verifier, why not error at build-time instead of verification time with the NoneVerifier? 🤔

@ctz
Copy link
Member

ctz commented Apr 15, 2024

Alternatively, maybe rustls_client_config_builder_build should error if there hasn't been a verifier configured?

I think I prefer this. Though if that option doesn't eliminate NoneVerifier (for some reason) we probably should land this PR too?

@cpu
Copy link
Member Author

cpu commented Apr 15, 2024

I started looking at the alternative and unfortunately it's a breaking change: rustls_client_config_builder_build is infallible right now and would need to be reworked to use an out pointer and return a rustls_result.

I think my inclination is to file an issue for that, tag it as a breaking change, and then hold for the next rustls update. (Ediit: #422).

Copy link
Collaborator

@jsha jsha left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@cpu cpu merged commit 9af30f4 into rustls:main Apr 15, 2024
@cpu cpu deleted the cpu-409-badsig-err branch April 15, 2024 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants