Proxy auth with custom HTTP transport #11433

hhirtz · 2022-11-28T11:19:41Z

What does this PR try to resolve?

This PR makes cargo authenticate against proxies when needed.
Closes #7330

Add three settings:

http.proxy-auth
http.proxy-username
http.proxy-password

proxy-auth defaults to "auto" so that most use cases work out of the box. I don't know why you would disable authentication support, but I added a "disable" option in this case. You can also force the use of a mechanism and abort if the proxy doesn't advertise it.

proxy-username and proxy-password: String vs Option<String>?
curl doesn't negotiate (SPNEGO, here "gss") if handle.proxy_username and handle.proxy_password aren't called. This authentication mechanism doesn't require a user/pass in cargo config. This shouldn't hurt (?) when the proxy doesn't require authentication.

About curl-rust's spnego and ntlm features:
https://github.com/alexcrichton/curl-rust/blob/b50c5d355aab69483752db51815c5a679b29d6c4/curl-sys/Cargo.toml#L54-L60
This is the last blocker I think. If we require those features, then vendored curl builds will require "a suitable GSS-API library or SSPI on Windows" [0] for the "gss" setting to work (which is what corporate proxies are using most of the time). I think this PR is blocked until we can either vendor those dependencies, or we change the official cargo builds to link dynamically to libcurl.

The official cargo builds seems to be made with vendored curl.

How should we test and review this PR?

Complete testing is a bit tedious since you'd need e.g. squid [1] to test each authentication mechanism. With such proxy, run cargo update with http-proxy environment variables (plus --config http.proxy-{username,password}= if needed).

[0] https://docs.rs/curl/0.4/curl/easy/struct.Auth.html#method.gssnegotiate
[1] http://www.squid-cache.org/

rustbot · 2022-11-28T11:19:46Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

Please see the contribution instructions for more information.

Add three settings: - http.proxy-auth - http.proxy-username - http.proxy-password proxy-auth defaults to "auto" so that most use cases work out of the box. I don't know why you would disable authentication support, but I added a "disable" option in this case. You can also force the use of a mechanism and abort if the proxy doesn't advertise it. proxy-username and proxy-password: String vs Option<String>? curl doesn't negotiate (SPNEGO, here "gss") if handle.proxy_username and handle.proxy_password aren't called. This authentication mechanism doesn't require a user/pass in cargo config. This shouldn't hurt (?) when the proxy doesn't require authentication. About curl-rust's spnego and ntlm features: https://github.com/alexcrichton/curl-rust/blob/b50c5d355aab69483752db51815c5a679b29d6c4/curl-sys/Cargo.toml#L54-L60 **This is the last blocker I think.** If we require those features, then vendored curl builds will require "a suitable GSS-API library or SSPI on Windows" [0] for the "gss" setting to work (which is what corporate proxies are using most of the time). I think this PR is blocked until we can either vendor those dependencies, or we change the official cargo builds to link dynamically to libcurl.

arlosi · 2022-11-30T17:20:54Z

src/cargo/util/config/mod.rs

+    #[serde(default)]
+    pub proxy_username: String,
+    #[serde(default)]
+    pub proxy_password: String,


I'd prefer the username and password be Options rather than empty strings.

Suggested change

#[serde(default)]

pub proxy_username: String,

#[serde(default)]

pub proxy_password: String,

pub proxy_username: Option<String>,

pub proxy_password: Option<String>,

arlosi · 2022-11-30T17:26:18Z

src/cargo/util/config/mod.rs

+impl CargoHttpProxyAuth {
+    pub fn to_easy(&self) -> Auth {
+        let mut easy = Auth::new();
+        let easy = match self {


Avoid the easy.clone() below by not redefining easy here. Also consider calling it auth instead of easy

Suggested change

let easy = match self {

match self {

It's not possible to avoid the clone because match self { .. } is a &mut Auth. Also Auth does not implement Copy.

https://docs.rs/curl/0.4.44/curl/easy/struct.Auth.html

You don't need to capture the result of the match in a variable. The basic(), digest(), etc methods mutate the Auth struct, so you can return the one you created with Auth::new() without cloning.

pub fn to_easy(&self) -> Auth { let mut auth = Auth::new(); match self { Self::Auto => auth.basic(true).digest(true).gssnegotiate(true).ntlm(true), Self::Disable => &auth, Self::Basic => auth.basic(true), Self::Digest => auth.digest(true), Self::Gss => auth.gssnegotiate(true), Self::Ntlm => auth.ntlm(true), }; auth }

arlosi · 2022-11-30T17:28:54Z

src/cargo/util/config/mod.rs

+#[serde(rename_all = "kebab-case")]
+pub enum CargoHttpProxyAuth {
+    #[default]
+    Auto,


Does the default of Auto introduce any additional HTTP requests for the non-proxy case?

The curl docs indicate that it might:

If more than one bit is set, libcurl will first query the site to see what authentication methods it supports and then pick the best one you allow it to use. For some methods, this will induce an extra network round-trip

This change focuses on proxy authentication specifically, not authentication to arbitrary HTTP servers. The Auth API is used for both problems, thus this paragraph I think. In the non-proxy case there is no additional request to be made.

This could well send an additional request to a proxy that doesn't need authentication, however I don't think it would make a noticeable difference in performance compared to, e.g. the time it takes to update the registry or download kilobytes of crates (?).

this is true! my bad! <rust-lang#11433 (comment)>

handle.proxy_username and handle.proxy_password still need to be called when options are "None", otherwise curl won't negotiate in cases it should (eg when using "gss") <rust-lang#11433 (comment)>

arlosi · 2022-12-09T13:06:24Z

src/cargo/ops/registry.rs

@@ -560,6 +560,9 @@ pub fn configure_http_handle(config: &Config, handle: &mut Easy) -> CargoResult<
    if let Some(proxy) = http_proxy(config)? {
        handle.proxy(&proxy)?;
    }
+    handle.proxy_auth(&http.proxy_auth.to_easy())?;


If this is CargoHttpProxyAuth::Disable should we avoid calling any of the proxy_ methods?

Not calling handle.proxy_auth is the same as calling it with Auth::new().basic(true) [0,1] (support only basic authentication). The idea with "disable" is to tell curl to not negotiate even if the user has a login/pwd in cargo config file.

We could avoid calling proxy_username and proxy_password but I don't think it would change anything, at least from the official curl docs.

[0] https://curl.se/libcurl/c/CURLOPT_PROXYAUTH.html
[1] https://docs.rs/curl/latest/curl/easy/struct.Easy2.html#method.proxy_auth

arlosi · 2022-12-09T13:07:07Z

src/cargo/ops/registry.rs

@@ -560,6 +560,9 @@ pub fn configure_http_handle(config: &Config, handle: &mut Easy) -> CargoResult<
    if let Some(proxy) = http_proxy(config)? {
        handle.proxy(&proxy)?;
    }
+    handle.proxy_auth(&http.proxy_auth.to_easy())?;
+    handle.proxy_username(http.proxy_username.as_deref().unwrap_or(""))?;


Can you add a comment about why these need to be called with empty string (instead of just not calling them)?

The comment is in the commit message [0] (second paragraph). Do you want another one in the code?

[0] e7a5905

ehuss · 2022-12-13T19:07:38Z

Unfortunately this seems like a relatively meaty change, so it can be difficult to nail everything down.

@hhirtz Is there a specific authentication scheme that you personally are interested in using? I was wondering if there is some way to narrow the scope of what is being added here. For example, if cargo needs to move to a different backend, then it would need to support all the mechanisms added here, which can be a large burden. Additionally, it looks like building curl with all the appropriate support looks potentially complicated. Exactly what will be supported on all of Cargo's platforms with the current build system?

Another issue is that we try to avoid including secrets in the configuration files. Adding the password here seems like an anti-pattern, so I think we may need to look at some alternate approach. I'm not sure if that involves using credentials.toml or something else.

In general, it can also be difficult for me to review and understand a change like this since I am not deeply familiar with all the different proxy authentication mechanisms. I think to move forward, I would need some kind of ELI5 introduction to what is being added here to help evaluate how these will be used and the consequences.

bors · 2023-02-14T11:05:50Z

☔ The latest upstream changes (presumably #11646) made this pull request unmergeable. Please resolve the merge conflicts.

ehuss · 2023-05-09T16:38:47Z

I'm going to close since this hasn't seen any movement in a while. If you want to pick this back up, I would suggest following up on #7330 with a specific design proposal that addresses any of the concerns raised above. Thanks!

rustbot assigned ehuss Nov 28, 2022

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 28, 2022

This was referenced Nov 28, 2022

Vendor libgss when static-curl and spnego features are set alexcrichton/curl-rust#481

Open

Proxy authentication mechanism support in cargo #7330

Open

hhirtz force-pushed the proxy-auth branch from c8d158e to e7a5905 Compare November 28, 2022 11:29

arlosi reviewed Nov 30, 2022

View reviewed changes

hhirtz added 2 commits December 7, 2022 09:36

remove clone

b50ce9b

this is true! my bad! <rust-lang#11433 (comment)>

use options instead of empty strings

ebaa009

handle.proxy_username and handle.proxy_password still need to be called when options are "None", otherwise curl won't negotiate in cases it should (eg when using "gss") <rust-lang#11433 (comment)>

arlosi reviewed Dec 9, 2022

View reviewed changes

ehuss added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 19, 2022

ehuss closed this May 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proxy auth with custom HTTP transport #11433

Proxy auth with custom HTTP transport #11433

hhirtz commented Nov 28, 2022

rustbot commented Nov 28, 2022

arlosi Nov 30, 2022

arlosi Nov 30, 2022

hhirtz Dec 2, 2022

arlosi Dec 2, 2022

arlosi Nov 30, 2022

hhirtz Dec 2, 2022

arlosi Dec 9, 2022

hhirtz Dec 9, 2022

arlosi Dec 9, 2022

hhirtz Dec 9, 2022

ehuss commented Dec 13, 2022

bors commented Feb 14, 2023

ehuss commented May 9, 2023

Proxy auth with custom HTTP transport #11433

Proxy auth with custom HTTP transport #11433

Conversation

hhirtz commented Nov 28, 2022

What does this PR try to resolve?

How should we test and review this PR?

rustbot commented Nov 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehuss commented Dec 13, 2022

bors commented Feb 14, 2023

ehuss commented May 9, 2023