Should/could Verifier::new_with_extra_roots take rustls::RootCertStore?

[paolobarbolini]

Hey! I'm trying to add support for this crate in lettre at lettre/lettre#1081.

Is there a reason why Verifier::new_with_extra_roots takes impl IntoIterator<Item = CertificateDer<'static>> instead of RootCertStore?
What I'm finding is that the implementation ignores invalid certs. The way I've always interpreted this is that we should ignore invalid certificates coming from the root certificate store, which is why RootCertStore::add_parsable_certificates exists (and I'm using it here), but we wouldn't want to do the same for certificates that the user manually decided to configure (here).

Does this make sense, and if so given that rustls already provides an API for doing it, do you think rustls-platform-verifier should use it instead of doing it's own thing with the iterator?

[ctz]

I don't think we want to take a rustls::RootCertStore here, because that means we cannot actually pass certificates to the verifier on platforms with a "real" platform verifier (these need the whole-form CertificateDer, they cannot operate on the compact pki-types::TrustAnchor representation held by RootCertStore).

What I'm finding is that the implementation ignores invalid certs. <...> but we wouldn't want to do the same for certificates that the user manually decided to configure (here).

Agree on that point. I think this is happening due to the flat_map and Result being an iterator over the contents of Ok?

[djc]

What I'm finding is that the implementation ignores invalid certs. <...> but we wouldn't want to do the same for certificates that the user manually decided to configure (here).

#171 already makes a bunch of changes in this area (but it still ignores invalid extra roots). I will add a commit there to fix this.