Log instead of error MissingOrMalformedExtensions in rustls_native_certs::load_native_certs

[TjeuKayim]

Only log errors because rustls-native-certs doesn't work with certificates that don't define any extensions, this is caused by the webpki crate: https://github.com/briansmith/webpki/blob/17d9189981a618120fd8217a913828e7418e2484/src/error.rs#L78-L83
However, such certs might be present in the trusted root store, like for Windows systems in an enterprise domain. So, reqwest should not throw reqwest::Error { kind: Builder, source: MissingOrMalformedExtensions } in that case, but rather log only a warning.
See also this comment https://github.com/rustls/rustls/blob/3c390ef7c459cc1ef2504bd9d1fefdcb7eea1c20/rustls/src/anchors.rs#L115-L125

[mefellows]

Thanks for doing this for us 😆

[mefellows]

Hi @seanmonstar, what's required for this PR to come in?

I'm happy to deal with the conflicts if that would help?

[seanmonstar]

Ah, this looks interesting! I suppose it's a good idea, I admit I haven't given it too much thought. Are there any decent instances where someone would prefer or be relying on this to error and NOT log? I guess, is there any reason we shouldn't just merge this? I don't have the time to investigate that answer very well.

[TjeuKayim]

In the meantime, pull request #1388 already made a change so that ClientBuilder::build() no longer panics but returns an error instead. So, this pull request needs to be renamed. Also, since the rustls dependencies are updated, I had to refactor the code in this pull request.

Are there any decent instances where someone would prefer or be relying on this to error and NOT log?
@seanmonstar

I can imagine a situation like this: A software project uses OCI containers that contain the reqwest library with the 'rustls-tls-native-roots' feature, and also a fixed version of the ca-certificates installed with the package manager. In the future, a new root certificate appears in ca-certificates incompatible with the used rustls version. The developers would like to spot the incompatibility during automated testing of the container image, instead of encountering crashes months later in production when a server moved to a certificate issued by the Root CA incompatible with the used rustls version. So, this imaginary developer would like to keep the error an error, so that it fails already during testing.

On the other hand: For many other users of this library, the native root store is updated independently of the application. Let's say, in the future a Windows update adds a new root certificate that is incompatible, while the rust application using reqwest is not updated. The next time the application starts, it crashes because of the error. In such cases, the imaginary developer would have liked that the error was only a warning in the logs instead, because the Windows updates are hard to account for.

Also, it seems unlikely to me that such an incompatibility will occur in the near future, as much other software depends on parsing root certificates.

[mefellows]

💯

The error we're encountering is specifically the second one. We distribute a CLI to users, and can't control the runtime environment. Erroring with a really nice warning may not be sufficient either, in our testing (this is second hand, I can probably dig up the details) it can fail on a valid certificate, removing which may not be desirable. This happened to be on Windows, but I suspect it isn't unique to Windows. I suspect it may have been a development or self-signed certificate, but that shouldn't matter.

Logging a warn is probably better, because it can communicate the problem without forcing a user action.

[seanmonstar]

Right on, thanks for the write-up, I agree with the assessment. And thanks for the code, as well! Let's go!

[mefellows]

Amazing, thanks @seanmonstar !