Skip to content

Add http.proxy-cainfo config for proxy certs #15374

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

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

Conversation

koxu1996
Copy link

@koxu1996 koxu1996 commented Apr 1, 2025

This adds a http.proxy-cainfo option to Cargo which reads CA information from a bundle to pass through to the underlying libcurl call. This should allow configuration of Cargo in situations where SSL proxy is used.

Similar to #2917.

This adds a `http.proxy-cainfo` option to Cargo which reads CA
information from a bundle to pass through to the underlying `libcurl`
call. This should allow configuration of Cargo in situations where SSL
proxy is used.
@rustbot
Copy link
Collaborator

rustbot commented Apr 1, 2025

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-configuration Area: cargo config files and env vars A-documenting-cargo-itself Area: Cargo's documentation A-networking Area: networking issues, curl, etc. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 1, 2025
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. This looks reasonable, though I saw your comment in #13460 (comment), and wonder what people want.
Do people really need the ability to configure custom proxy CA info, or just want to disable all the CA checks?

There is also git's CA check discussion in #1180 as well, somehow relevant.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #13460 (comment).

Let's discuss over there :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to disable CA checks - although I think it is really useful option, definitely worth introducing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I faced the problem where cargo doesn't work with an SSL proxy, even though the custom CA is trusted (curl doesn't complain about the certificates). This stems from the fact that the automatically vendored libcurl has no built-in information about the system trust store. I wrote a fairly detailed description of this issue in #15376; the ability to set a proxy cainfo is a real need to solve it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weihanglo Do you need more info?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say that the change standalone looks reasonable. Have you tested it satisfies and fixes issues on your side?

I would kick off a straw poll for the Cargo team, as this would add a new insta-stable config field to Cargo.

Copy link
Author

@koxu1996 koxu1996 Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it fixes the problem 🫡:

# Step 1) Setup TLS proxy and set CA info.
export CARGO_HTTP_PROXY=https://localhost:8002
export CARGO_HTTP_CAINFO=/etc/pki/tls/certs/ca-bundle.crt
# Step 2) Try cargo compiled from `koxu1996:feature/http-proxy-cainfo`.
/tmp/cargo/target/debug/cargo add [email protected]
    Updating crates.io index
error: download of config.json failed

Caused by:
  failed to download from `https://index.crates.io/config.json`

Caused by:
  [60] SSL peer certificate or SSH remote key was not OK (SSL certificate problem: unable to get local issuer certificate)
# Step 3) Set **proxy** CA info
export CARGO_HTTP_PROXY_CAINFO=/etc/pki/tls/certs/ca-bundle.crt
# Step 4) Try cargo again.
/tmp/cargo/target/debug/cargo add [email protected]
    Updating crates.io index
      Adding proc-macro2 v1.0.94 to dependencies
             Features:
             + proc-macro
             - nightly
             - span-locations

@weihanglo
Copy link
Member

@rfcbot fcp merge

I propose to the team that we merge and insta-stabilize the http.proxy-cainfo config.

Possible counterarguments:

  • Instead, we just provide an --insecure equivalent flag to disable all checks.
    • This doesn't work if people do want some checks for proxies.
  • As Fails to connect through trusted SSL proxy #15376 pointing out, We should fix this in upstream curl crate to align with curl CLI's behavior.
    • Yeah, but that only works if you have no cainfo set I guess.

@rfcbot
Copy link
Collaborator

rfcbot commented Apr 9, 2025

Team member @weihanglo has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Apr 9, 2025
@ehuss ehuss moved this to FCP merge in Cargo status tracker Apr 15, 2025
@joshtriplett
Copy link
Member

joshtriplett commented Apr 15, 2025

@rfcbot concern need-rationale-other-than-curl-bug

According to #15376 , it sounds like the curl command-line tool automatically uses the non-proxy CA info as the proxy CA info, if no proxy CA info is specifically provided. This behavior doesn't occur in libcurl, or in other clients like cargo.

I think it'd be appropriate for cargo to implement the same behavior as the curl command-line tool. That would make this use case work automatically, without requiring configuration.

There may still be a reason to have this separate configuration, but we'd need a separate motivation for it other than the case where the proxy needs a system CA certificate and this config is being used to work around us not automatically using that.

@koxu1996
Copy link
Author

koxu1996 commented Apr 16, 2025

@joshtriplett http.cainfo was introduced with the following motivation:

This should allow configuration of Cargo in situations where the default certificate store doesn't contain the relevant certificates, such as behind corporate proxies.

Given that proxies - especially in corporate environments - are expected to use TLS for secure communication, introducing http.proxy_cainfo is a reasonable extension that supports this more secure setup.

@ehuss ehuss moved this from FCP merge to FCP blocked in Cargo status tracker Apr 22, 2025
@arlosi
Copy link
Contributor

arlosi commented Apr 22, 2025

Regardless of whether we decide to add proxy_cainfo as a separate configuration option, I think cargo should match curl's behavior of using cainfo for proxy_cainfo (if an explicit proxy_cainfo is not set).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-configuration Area: cargo config files and env vars A-documenting-cargo-itself Area: Cargo's documentation A-networking Area: networking issues, curl, etc. disposition-merge FCP with intent to merge proposed-final-comment-period An FCP proposal has started, but not yet signed off. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cargo Team: Cargo
Projects
Status: FCP blocked
Development

Successfully merging this pull request may close these issues.

6 participants