Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add "native-certs" feature flag #944

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

drewkett
Copy link

This feature flag enables the "native-certs" feature from tame-index which enables the use of the host OS certificate store with reqwest. I'm separately working on a PR to support third party registries. This feature flag is needed to support third party registries on some corporate networks.

This feature flag enables the "native-certs" feature from tame-index which
enables the use of the host OS certificate store with reqwest.
@obi1kenobi
Copy link
Owner

Thanks for the PR! A couple of questions for you:

enables the use of the host OS certificate store with reqwest

Does this mean it only works with the gix-reqwest feature, and would not work with the gix-curl feature?

Also, is there a good way to add some tests to ensure this feature continues to work correctly in the future? I'm afraid I'm not familiar with the details of OS certificate stores, so I'd like to proactively minimize the risk of something breaking down the line in a way that I as the maintainer would be on the hook to fix.

@drewkett
Copy link
Author

Hmm so looking at it a bit more, it's pretty complicated. Let me see if I can explain whats going on. I can't guarantee that all of this information is 100% accurate, but it is my understanding of the situation.

I'm not sure what your familiarity level is with TLS certificates, but basically when making an https request, you'll likely have some number of root certificates locally that are used to validate the remote server's TLS certificate. The default behavior with browsers and things like wget/curl (which typically use openssl on linux for example) is to use the root certificates installed on the host OS. A lot of the rust ecosystem however uses rustls which is rust implementation of TLS. With rustls it is common to use the webpki roots which is a set of root certificates provided by Mozilla. Usually there is a ton of overlap between the webpki roots as your host OS root certificates which is why queries tend to just work no matter what you use. However, its common in corporate networks to have custom tls certificates that are created based on an internal root certificate. (VPNs also do this too to MITM corporate network connections). This internal root certificate can be installed in the host OS to make it easy for the user to communicate with internal servers. When this happens, the webpki root certs will not work unless those custom tls certificates are also signed by something that rolls up the webpki root certificates which is not always the case. So with rustls in this scenario, you need to enable the native-certs feature so that rustls will query the host OS certs as well when validating https queries.

And now on the specifics here.

cargo-semver-checks depends on both gix and tame-index, and tame-index also depends on gix.

The existing feature flag for gix-curl only enforces the usage of curl for gix and not tame-index. The default behavior for curl as currently configured is to use openssl for tls, which would by default use the host OS certificates. The default behavior gix-reqwest would be to use rustls for tls, which as configured uses the webpki root certificates (not the host OS ones).

So the change I did, really only affects the reqwest based http requests and would not affect the curl based ones (and the curl based ones already have different behavior with respect to certificates compared to the reqwest ones).

An additional complication though is that even if you do --no-default-features --features gix-curl, the reqwest backend is still used by tame-index for sparse registries. So if you want to use cargo-semver-checks with curl, you still need to enable this feature flag if you're using sparse registries because those requests don't route through curl.

I'm happy to update the comment on the feature to try to describe this since it is complicated. I could also rename the feature to reqwest-native-certs to make it more clear that its not touching the curl side of things.

Regarding tests, I'm sure there is some way to do that, but I suspect it would be complicated. I haven't looked deeply at how this project handles tests, but you would likely need a registry behind an http server with a self signed certificate and then have a container which with the self signed certificate installed to prove that it can talk to it. I'm not even sure if this is possible given that there's no third party registry support (though you could do it at the tame_index level and not the overall program level). I am working on a PR for third party registries that I hope to have up soon, so that might make it a bit easier to write a test. In the meantime, I personally would also be fine putting this behind an experimental feature flag to make it clear that its not well tested, though I at least confirmed it works locally in my one use case.

Sorry for the long comment, but let me know what you would like me to do with this or if you have any other questions

@obi1kenobi
Copy link
Owner

Thanks for the detailed comment, I appreciate it!

Any chance you might be up for chatting about the two PRs over a video call one of these days?

I'd love to learn more about your use cases and figure out how cargo-semver-checks can best support them while continuing to minimize maintainability risk — which is the main existential threat to open-source software like this.

@drewkett
Copy link
Author

Yep. Happy to. Shoot me an email and we can coordinate on timing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants