rustls / rustls-ffi

Use Rustls from any language
Other
127 stars 30 forks source link

client: NoneVerifier UnknownIssuer instead of BadSignature #421

Closed cpu closed 5 months ago

cpu commented 5 months ago

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 https://github.com/rustls/rustls-ffi/issues/409

cpu commented 5 months ago

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? :thinking:

ctz commented 5 months ago

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 commented 5 months ago

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: https://github.com/rustls/rustls-ffi/issues/422).