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

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

Open
alilleybrinker opened this issue Mar 6, 2024 · 10 comments · May be fixed by #787
Open

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

alilleybrinker opened this issue Mar 6, 2024 · 10 comments · May be fixed by #787
Labels
bug Not as expected

Comments

@alilleybrinker
Copy link

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
Copy link
Collaborator

epage commented Mar 12, 2024

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.

@epage epage added the bug Not as expected label Mar 12, 2024
@alilleybrinker
Copy link
Author

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
Copy link

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
Copy link
Author

alilleybrinker commented Mar 26, 2024

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
Copy link

AnnZ2021 commented Mar 27, 2024

@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
Copy link
Author

(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:

  • Offering some mechanism to turn off certificate checking entirely.
    • This is questionable because:
      1. It's insecure
      2. It's overkill for what people actually want (to let specific certificate pass validation)
  • Offering some mechanism to specify integration with the system trust store
    • Git has this, where for example you can tell it to use the system certificate store on Windows, which it doesn't by default.
  • Offering some mechanism to specify additional accepted certificates outside of any system store

These could then be done via:

  • Environment variables
  • Configuration files
  • CLI flags
  • Any mix of the above, with some precedence rules

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:

  • How to teach reqwest to pick up different / additional certificates and/or turn off certificate checking entirely
  • Deciding what sort of configuration to expose to the user
  • Deciding where / how that configuration should be set

@epage
Copy link
Collaborator

epage commented Mar 27, 2024

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
Copy link
Author

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
Copy link
Author

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
Copy link
Collaborator

epage commented May 8, 2024

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.

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

Successfully merging a pull request may close this issue.

3 participants