rustls / rustls-platform-verifier

A certificate verification library for rustls that uses the operating system's verifier
Apache License 2.0
66 stars 20 forks source link

feat: add Verifier::set_provider and Verifier::with_provider #81

Closed jbr closed 6 months ago

jbr commented 6 months ago

This is offered as a possible solution for #79, which I just encountered.

Implementation notes, let me know if any of these assumptions were wrong:

cpu commented 6 months ago

Thank you for picking this up! The general approach seems like the right solution.

nnmkhang commented 6 months ago

Hi Everyone!

Thanks @jbr for picking this up so fast, I'm not sure what type of testing you have done but I tried to verify this on my end with a custom crypto provider but I am running into some issues.

Here is the snippet of code that I am trying to get working.

let mut config = rustls::ClientConfig::builder_with_provider(Arc::new(default_symcrypt_provider()))
        .with_safe_default_protocol_versions()
        .unwrap()
        .dangerous()
        .with_custom_certificate_verifier(Arc::new(Verifier::set_provider(Arc::new(default_symcrypt_provider))))
        .with_no_client_auth();

And im getting the following error, what am I doing wrong here?

error[E0277]: the trait bound `(): ServerCertVerifier` is not satisfied
  --> bin/sample_internet_client_platform.rs:20:43
   |
20 |         .with_custom_certificate_verifier(Arc::new(Verifier::set_provider(Arc::new(default_symcrypt_provider))))
   |                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `ServerCertVerifier` is not implemented for `()`
   |
   = help: the following other types implement trait `ServerCertVerifier`:
             Verifier
             WebPkiServerVerifier
   = note: required for the cast from `Arc<()>` to `Arc<(dyn ServerCertVerifier + 'static)>`

I've also noticed on the rustls documentation for ClientConfig that builder_with_provider does not take in the WantsVerifier struct.

Please forgive my new-ness to rust but would this be the cause of the error? Or is there something wrong with the configuration on my end?

Appreciate the help here!

jbr commented 6 months ago

@nnmkhang It's still a PR ~and CI is currently failing~, but with the current iteration, I think you want code that looks like

let provider = Arc::new(default_symcrypt_provider());

ClientConfig::builder_with_provider(provider.clone())
   .with_safe_default_protocol_versions()
   .unwrap()
   .dangerous()
   .with_custom_certificate_verifier(Arc::new(Verifier::new().with_provider(provider)))
   .with_no_client_auth()

Note the Verifier::new().with_provider(provider) instead of Verifier::set_provider(provider)

The awkwardness of this construction motivates #86, which that code was directly taken from

Alternatively, if you wanted to use Verifier::set_provider instead of Verifier::new().with_provider,

let provider = Arc::new(default_symcrypt_provider());

let mut verifier = Verifier::new();
verifier.set_provider(provider.clone());

ClientConfig::builder_with_provider(provider)
   .with_safe_default_protocol_versions()
   .unwrap()
   .dangerous()
   .with_custom_certificate_verifier(Arc::new(verifier))
   .with_no_client_auth()
nnmkhang commented 6 months ago

@jbr, Just tried it again on my my machine and it works with the custom provider and the platform verifier enabled! Verified with event viewer on windows as well.

Thanks for your help and sorry for my silly mistake.

cpu commented 6 months ago

I think we should square away #86 and #78 and then consider a 0.3.1 point release.

complexspaces commented 6 months ago

Sounds good to me.

cpu commented 5 months ago

v/0.3.1 is now available with this change included.