Skip to content

Explicitly configure noproxy support in Curl #6115

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Explicitly configure noproxy support in Curl #6115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ bytesize = "1.0"
crates-io = { path = "src/crates-io", version = "0.20" }
crossbeam-utils = "0.5"
crypto-hash = "0.3.1"
curl = { version = "0.4.17", features = ['http2'] }
curl = { version = "0.4.18", features = ['http2'] }
env_logger = "0.5.11"
failure = "0.1.2"
filetime = "0.2"
Expand Down
8 changes: 8 additions & 0 deletions src/cargo/ops/registry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,14 @@ pub fn configure_http_handle(config: &Config, handle: &mut Easy) -> CargoResult<
} else {
handle.useragent(&version().to_string())?;
}

let noproxy = env::var("NO_PROXY").ok()
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand the original motivation for this. It looks like libcurl reads these environment variables automatically (https://github.com/curl/curl/blob/835a2fe694214341b601b2439082a30a24cc9da9/lib/url.c#L2707-L2723) and has for a while (though it looks like its been changed a bit over the years). Testing on my system, stable cargo reads them just fine. I have libcurl 7.54 which isn't exactly new.

What exactly is the issue here? I'm concerned without understanding why it doesn't work for the original reporter that this might not fix it for them. Maybe they have an old version of libcurl? Or a misconfiguration?

Copy link

Choose a reason for hiding this comment

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

I am the original reporter, and I have tried to get the variable to work for me, but it just doesn't. Neither on my mac, or in a CI environment with CentOS 7. Here is the version of libraries that Cargo is using:

λ otool -L ~/.cargo/bin/cargo
/Users/nate/.cargo/bin/cargo:
	/System/Library/Frameworks/Security.framework/Versions/A/Security (compatibility version 1.0.0, current version 57740.60.18)
	/usr/lib/libcurl.4.dylib (compatibility version 7.0.0, current version 9.0.0)
	/usr/lib/libz.1.dylib (compatibility version 1.0.0, current version 1.2.8)
	/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation (compatibility version 150.0.0, current version 1349.8.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1238.60.2)
	/usr/lib/libresolv.9.dylib (compatibility version 1.0.0, current version 1.0.0)

Copy link

Choose a reason for hiding this comment

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

You know what, I may have been using lowercased no_proxy. Let me try tomorrow with NO_PROXY

Copy link
Member Author

Choose a reason for hiding this comment

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

@natemara any luck with tests to see how it went?

Copy link

Choose a reason for hiding this comment

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

@alexcrichton here are some logging traces that I captured. There is one trace in a completely clean environment (with env -i), and one with proxy environment variables set, and NO_PROXY, and no_proxy set to git.corp.com. The one with proxy variables fails, and the one without proxy variables succeeds to download this dependency. I also used mitmproxy to see what was going on with the requests - it looks like the requests are not proxied when NO_PROXY is set, but the package still fails to download. Is there some different logic that activates if any of the proxy variables are set?

https://gist.github.com/natemara/817bb64a08f8403305cf0563768ca51c

Copy link

Choose a reason for hiding this comment

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

Adding .git should fix it, but that is chopped off at https://github.com/rust-lang/cargo/blob/master/src/cargo/sources/git/source.rs#L102.

Copy link

Choose a reason for hiding this comment

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

And yes, I believe our git server (gitlab) requires the .git at the end.

Copy link

Choose a reason for hiding this comment

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

If I comment out the block that I linked above, it works perfectly.

Copy link
Member Author

Choose a reason for hiding this comment

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

@natemara do you have other dependencies on the same crate elsewhere? That canonicalization is purely used for hashing and comparison, it's not actually used for fetching dependencies. Instead what's written down exactly in Cargo.toml is used to fetch dependencies. If you have one dependency on /logger and one on /logger.git (for example) then they'll has to the same bucket and resolve to the same dependency (and sort of nondeterministically pick one of the dependencies to fetch).

I don't really understand how commenting out the logic would fix anything because we don't use the "canonical URL" to actually fetch anything. Do you know of any other dependencies active throughout the system on this /logger.git url?

Copy link

Choose a reason for hiding this comment

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

My mistake - when I got it to work I had the proxy settings disabled. I think I've made a small example of my problem that can be distributed so that you can see the behavior for yourself. If you download this project https://gitlab.com/natemara/cargo-proxy-consumer, then run the run script in the root dir, you should be able to download the dependency just fine. If you uncomment lines 10-15 and run it again, the download should fail with 404s.

Were you able to reproduce this?

.or_else(|| env::var("no_proxy").ok())
.or(config.get::<Option<Vec<String>>>("http.noproxy")?.map(|s| s.join(",")));
if let Some(noproxy) = noproxy {
handle.noproxy(&noproxy)
.chain_err(|| "malformed `noproxy` configuration")?;
}
Ok(())
}

Expand Down
3 changes: 3 additions & 0 deletions src/doc/src/reference/config.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,9 @@ check-revoke = true # Indicates whether SSL certs are checked for revocation
low-speed-limit = 5 # Lower threshold for bytes/sec (10 = default, 0 = disabled)
multiplexing = false # whether or not to use HTTP/2 multiplexing where possible

# A list of hosts which do not use the proxy configuration, if any.
noproxy = ['github.com']

[build]
jobs = 1 # number of parallel jobs, defaults to # of CPUs
rustc = "rustc" # the rust compiler tool
Expand Down
2 changes: 2 additions & 0 deletions src/doc/src/reference/environment-variables.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ system:
will otherwise be used.
* `CARGO_CACHE_RUSTC_INFO` — If this is set to 0 then Cargo will not try to cache
compiler version information.
* `HTTP_PROXY`, `HTTPS_PROXY`, `NO_PROXY` - these variables are all ready by
Copy link
Contributor

Choose a reason for hiding this comment

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

ready → read
trafic → traffic

Also, it's a bit redundant to say "read by Cargo" since all of these vars are read by cargo. I would split NO_PROXY into a separate entry, and explain its format a little.

Cargo and correspond to proxy configuration for HTTP trafic.

Note that Cargo will also read environment variables for `.cargo/config`
configuration values, as described in [that documentation][config-env]
Expand Down