crate-ci / cargo-release

Cargo subcommand `release`: everything about releasing a rust crate.
Apache License 2.0
1.31k stars 109 forks source link

"Invalid peer certificate" when run on network which substitutes certs #764

Closed alilleybrinker closed 2 months ago

alilleybrinker commented 6 months ago

This issue appears to have arisen between v0.20.5 and v0.25.6 (it worked for me on the former version, and now doesn't on the latter).

Basically, I'm running on a network where certificates for sites are substituted with a chain managed by the network operator. I have the root for this chain installed in my system certificate store.

I can confirm with the -vv flag that rustls under the hood is picking up my network's substitute chain, which it then fails to verify.

It looks like cargo-release uses webpki_roots through reqwest, which only considers the Mozilla root certificate list, and does not incorporate what certs are trusted in the system store.

I believe the solution would be to modify how reqwest is used to incorporate rustls_native_certs instead.

epage commented 5 months ago

Likely happened after #719.

As I don't know much about these topics and can't reproduce, I leave this to others to figure out and fix.

alilleybrinker commented 5 months ago

I believe all that should be needed would be to update the feature selection on reqwest in the Cargo.toml for cargo-release. Right now it activates the rustls-tls feature, which activates rustls-tls-webpki-roots. Adding the rustls-tls-native-roots feature in the list for your Cargo.toml should automatically cause reqwest and thereby cargo-release to start picking up the system certs as well.

If you want to enable their use but make it opt-in for users, then you'd add the feature but call ClientBuilder::use_rustls_tls to force only use of the webpki certs by default, and then optionally call ClientBuilder::use_native_tls to cause cargo-release to pick up native certs based on a CLI flag.

Hope that helps!

AnnZ2021 commented 5 months ago

I believe all that should be needed would be to update the feature selection on reqwest in the Cargo.toml for cargo-release. Right now it activates the rustls-tls feature, which activates rustls-tls-webpki-roots. Adding the rustls-tls-native-roots feature in the list for your Cargo.toml should automatically cause reqwest and thereby cargo-release to start picking up the system certs as well.

If you want to enable their use but make it opt-in for users, then you'd add the feature but call ClientBuilder::use_rustls_tls to force only use of the webpki certs by default, and then optionally call ClientBuilder::use_native_tls to cause cargo-release to pick up native certs based on a CLI flag.

Hope that helps!

I got the same error, I still don't understand what should be done on client side to get around with it.

alilleybrinker commented 5 months ago

I think applying the following patch might work, but I haven't checked it.

diff --git a/Cargo.toml b/Cargo.toml
index c955962..a5f8c92 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -60,7 +60,7 @@ vendored-openssl = ["git2/vendored-openssl"]
 [dependencies]
 cargo_metadata = "0.18"
 tame-index = "0.9"
-reqwest = { version = "0.11", default-features = false, features = ["blocking", "rustls-tls", "gzip"] }
+reqwest = { version = "0.11", default-features = false, features = ["blocking", "rustls-tls-native-roots", "gzip"] }
 git2 = { version = "0.18.2", default-features = false }
 toml_edit = { version = "0.22.6", features = ["serde"] }
 toml = "0.8.10"
AnnZ2021 commented 5 months ago

@alilleybrinker

Screenshot 2024-03-26 at 10 24 12 PM 1

Unfortunately it does NOT work.

My laptop is using Zscaler VPN.

My rust version is: rustc 1.77.0, cargo version is: cargo 1.77.0

OS: MacOS Sonoma version 14.4.1 Chip: Apple M3 max

My older MacBook Pro installed rust 1.75.0 but the same version of cargo-release which works well using the same VPN.

Is there any way to specify Zscaler Certificate for usage when using cargo-release?

Or can the TLS be turned off?

alilleybrinker commented 5 months ago

(I should say that my attempt at a patch was my initial guess from reviewing the way things appeared to work; I'm not a cargo-release maintainer, sorry it didn't work!)

Reqwest (and Rustls under the hood) definitely allows for configuration of the trusted certificates, so the question I think is how to expose that functionality to cargo-release users. This is where I'd basically want to defer to the maintainers as far as the right API.

Some things I've seen in other tools:

These could then be done via:

I haven't explored how cargo-release does config to see how they'd likely want this to work.


In short: yes I believe this could be implemented, but there'd be implementation work to figure out:

epage commented 5 months ago

These could then be done via:

In our config layering, we support a user-wide config which it seems something like this would work. We could potentially also have a CLI flag that is layered in with this.

Whether to use CLI or config is dependent on how transient the state is and if there are one time exceptions. If this is a set-and-forget type of thing, config makes sense.

In proposing a config or CLI, I would like prior art shown of how other commands tend to represent this so we can be consistent on terms to make adoption easier.

alilleybrinker commented 4 months ago

I think putting it in configuration makes sense. In general I don't expect that modifications of the set of trusted certificates are a common thing to set transiently. Rather, you'd want to do a one-time configuration to link cargo-release up to the proper certificate store, and then expect it to use that in the future.

For prior art, I'd say Git's configuration around this is a good example. You can see some of the relevant configuration here: https://git-scm.com/docs/git-config#Documentation/git-config.txt-httpsslCAInfo

alilleybrinker commented 4 months ago

I'm happy to do an implementation around this if that's helpful. This is now blocking for me to publish versions of a crate I'm working on, so I'd like to get it resolved.

epage commented 4 months ago

If you'd be able to. As I can't reproduce this problem, I'm not in a good position to verify that its fixed.