-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Proxy auth with custom HTTP transport #11433
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(""))?; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a comment about why these need to be called with empty string (instead of just not calling them)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment is in the commit message [0] (second paragraph). Do you want another one in the code? [0] e7a5905 |
||
handle.proxy_password(http.proxy_password.as_deref().unwrap_or(""))?; | ||
if let Some(cainfo) = &http.cainfo { | ||
let cainfo = cainfo.resolve_path(config); | ||
handle.cainfo(&cainfo)?; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,7 +77,7 @@ use crate::util::{internal, toml as cargo_toml}; | |
use crate::util::{FileLock, Filesystem, IntoUrl, IntoUrlWithBase, Rustc}; | ||
use anyhow::{anyhow, bail, format_err, Context as _}; | ||
use cargo_util::paths; | ||
use curl::easy::Easy; | ||
use curl::easy::{Auth, Easy}; | ||
use lazycell::LazyCell; | ||
use serde::Deserialize; | ||
use toml_edit::{easy as toml, Item}; | ||
|
@@ -2215,10 +2215,41 @@ impl Drop for PackageCacheLock<'_> { | |
} | ||
} | ||
|
||
#[derive(Debug, Default, Deserialize, PartialEq)] | ||
#[serde(rename_all = "kebab-case")] | ||
pub enum CargoHttpProxyAuth { | ||
#[default] | ||
Auto, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does the default of The curl docs indicate that it might:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change focuses on proxy authentication specifically, not authentication to arbitrary HTTP servers. The 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 (?). |
||
Disable, | ||
Basic, | ||
Digest, | ||
Gss, | ||
Ntlm, | ||
} | ||
|
||
impl CargoHttpProxyAuth { | ||
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 | ||
} | ||
} | ||
|
||
#[derive(Debug, Default, Deserialize, PartialEq)] | ||
#[serde(rename_all = "kebab-case")] | ||
pub struct CargoHttpConfig { | ||
pub proxy: Option<String>, | ||
#[serde(default)] | ||
pub proxy_auth: CargoHttpProxyAuth, | ||
pub proxy_username: Option<String>, | ||
pub proxy_password: Option<String>, | ||
pub low_speed_limit: Option<u32>, | ||
pub timeout: Option<u64>, | ||
pub cainfo: Option<ConfigRelativePath>, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is
CargoHttpProxyAuth::Disable
should we avoid calling any of theproxy_
methods?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not calling
handle.proxy_auth
is the same as calling it withAuth::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
andproxy_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