Match `NO_PROXY` formats to libcurl behaviour #501

mjcheetham · 2021-10-20T14:00:23Z

We aim to be compatible with the behaviour of Git as much as possible when it comes to network settings. This enables users to setup Git proxy settings and get the same setup "for free" with GCM.

Git uses libcurl to provide it's HTTP interactions. The NO_PROXY setting is used by libcurl to disable proxy settings for specific hosts.

We previously attempted to plumb the value of NO_PROXY through to the .NET WebProxy class' list of "bypassed addresses" (the set of hosts that should not be proxied). However, the .NET class expects a set of regular expressions which is unlike libcurl!

As a result, libcurl permitted values for NO_PROXY were throwing errors inside of GCM since they are not valid regexs.

In this commit we perform a transformation of the NO_PROXY list and construct a set of regular expressions that match addresses in the same way as libcurl does.

The transformation is as follows:

strip any leading periods . or wildcards *.
escape the remaining input to match literally (e.g.: . becomes \.)
prepend a group that matches either a period '.' or a URI scheme delimiter :// - this prevents partial domain matching
append a end-of-string symbol $ to ensure we only match to the specified TLD and port

See the libcurl documentation on NO_PROXY behaviour: https://curl.se/libcurl/c/CURLOPT_NOPROXY.html

Fixes #497

jeffhostetler · 2021-10-20T15:29:10Z

src/shared/Microsoft.Git.CredentialManager/Settings.cs

+                // For example: "example.com" should match "http://example.com" and
+                // "http://www.example.com" but not "http://notanexample.com".
+                regex.Append(@"(\.|\:\/\/)");
+


do we care about things like "http://*.example.com" ?

This is covered in the case of the \. arm of the group. See this example: https://regex101.com/r/Q1kE2R/1
There is also a unit test for this case "www.example.com".

Does curl strip off the "http" vs "https" so we don't need to worry about one being handled one way and the other differently?

This group at the start says either it must be . and then the user value "example.com" OR it must form the start of the hostname. I'd like to have used ^ here, but the .NET code actually matches against the full URL (so it includes "scheme://hostname"). To match the "start" of the hostname, I use the URL scheme delimiter ://.

WRT "http://*.example.com" my concern was that we have explicit code to strip off leading "star dot", but an inline "star" here will pass thru (and maybe be escaped).

Ahh right.. so where the user has set NO_PROXY=http://*.example.com? I.e., they've put both the protocol/scheme prefix and an inline *.? That is not supported, at least AFAIK, with libcurl either.

Or did you mean NO_PROXY=example.*.com? In that case we would escape the .*. to \.\*\., which I would again consider "invalid". libcurl says it does not support any wildcards except just NO_PROXY=* (which turns off the proxying all together).

Removing a leading *. is a convenience here. Not sure what libcurl would do here. I know it does ignore a leading . (for example .example.com == example.com).

Yeah, I was thinking about NO_PROXY=http://*.example.com and was afraid that we'd get an escaped * passed to the RegEx. I was not thinking about the example.*.com case.

I can confirm that libcurl does not match NO_PROXY values that include the scheme (http[s]://) to hosts.

$ https_proxy=http://localhost:8080/proxy no_proxy=https://google.com curl -v https://google.com * Uses proxy env variable no_proxy == 'https://google.com' * Uses proxy env variable https_proxy == 'http://localhost:8080/proxy' * Trying ::1... * TCP_NODELAY set * Connection failed * connect to ::1 port 8080 failed: Connection refused * Trying 127.0.0.1... * TCP_NODELAY set * Connection failed * connect to 127.0.0.1 port 8080 failed: Connection refused * Failed to connect to localhost port 8080: Connection refused * Closing connection 0 curl: (7) Failed to connect to localhost port 8080: Connection refused

Above I set the HTTPS proxy to "http://localhost:8080/proxy", and a NO_PROXY value of https://google.com. Attempting to make the request to https://google.com I am still attempting to proxy via the fake localhost proxy. The NO_PROXY value is not matched.

In fact, libcurl doesn't match with no_proxy=*.google.com either:

$ https_proxy=http://localhost:8080/proxy no_proxy=*.google.com curl -v https://google.com * Uses proxy env variable no_proxy == '*.google.com' * Uses proxy env variable https_proxy == 'http://localhost:8080/proxy' * Trying ::1... * TCP_NODELAY set * Connection failed * connect to ::1 port 8080 failed: Connection refused [...]

.. so GCM is actually allowing more formats than libcurl does here. I don't think this is a major problem since it's a superset? We can also just drop the lead *. trimming code?

@jeffhostetler what do you think?

Note that no_proxy=.google.com and no_proxy=google.com work as expected (the request to https://google.com is not proxied).

src/shared/Microsoft.Git.CredentialManager/Settings.cs

derrickstolee

I have just a small suggested addition to your test data.

src/shared/Microsoft.Git.CredentialManager.Tests/SettingsTests.cs

We aim to be compatible with the behaviour of Git as much as possible when it comes to network settings. This enables users to setup Git proxy settings and get the same setup "for free" with GCM. Git uses libcurl to provide it's HTTP interactions. The NO_PROXY setting is used by libcurl to disable proxy settings for specific hosts. We previously attempted to plumb the value of NO_PROXY through to the .NET WebProxy class' list of "bypassed addresses" (the set of hosts that should not be proxied). However, the .NET class expects a set of _regular expressions_ which is unlike libcurl! As a result, libcurl permitted values for NO_PROXY were throwing errors inside of GCM since they are not valid regexs. In this commit we perform a transformation of the NO_PROXY list and construct a set of regular expressions that match addresses in the same way as libcurl does. The transformation is as follows: 1. strip any leading periods '.' or wildcards '*.' 2. escape the remaining input to match literally (e.g.: '.' becomes '\.') 3. prepend a group that matches either a period '.' or a URI scheme delimiter '://' - this prevents partial domain matching 4. append a end-of-string symbol '$' to ensure we only match to the specified TLD and port See the libcurl documentation on NO_PROXY behaviour: https://curl.se/libcurl/c/CURLOPT_NOPROXY.html

mjcheetham added bug A bug in Git Credential Manager network Related to networking (VPN, TCP/IP, etc) labels Oct 20, 2021

mjcheetham requested review from dscho, derrickstolee, jeffhostetler, vdye and ldennington October 20, 2021 14:02

jeffhostetler reviewed Oct 20, 2021

View reviewed changes

src/shared/Microsoft.Git.CredentialManager/Settings.cs Outdated Show resolved Hide resolved

jeffhostetler approved these changes Oct 20, 2021

View reviewed changes

derrickstolee approved these changes Oct 21, 2021

View reviewed changes

src/shared/Microsoft.Git.CredentialManager.Tests/SettingsTests.cs Outdated Show resolved Hide resolved

mjcheetham force-pushed the noproxy-globs branch from f466150 to 8b19495 Compare October 22, 2021 12:26

derrickstolee approved these changes Oct 22, 2021

View reviewed changes

mjcheetham force-pushed the noproxy-globs branch from 8b19495 to 8cd79a3 Compare November 2, 2021 10:22

mjcheetham merged commit a391785 into git-ecosystem:main Nov 2, 2021

mjcheetham deleted the noproxy-globs branch November 2, 2021 10:32

mjcheetham mentioned this pull request Nov 2, 2021

GCM core fails when NO_PROXY contains a glob pattern #497

Closed

This was referenced Nov 15, 2021

New release #530

Merged

New release (attempt 2) #534

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Match `NO_PROXY` formats to libcurl behaviour #501

Match `NO_PROXY` formats to libcurl behaviour #501

mjcheetham commented Oct 20, 2021

jeffhostetler Oct 20, 2021

mjcheetham Oct 20, 2021

jeffhostetler Oct 20, 2021

mjcheetham Oct 20, 2021

jeffhostetler Oct 20, 2021

mjcheetham Oct 20, 2021

mjcheetham Oct 20, 2021

jeffhostetler Oct 20, 2021

mjcheetham Oct 22, 2021

mjcheetham Oct 22, 2021

derrickstolee left a comment

Match NO_PROXY formats to libcurl behaviour #501

Match NO_PROXY formats to libcurl behaviour #501

Conversation

mjcheetham commented Oct 20, 2021

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

derrickstolee left a comment

Choose a reason for hiding this comment

Match `NO_PROXY` formats to libcurl behaviour #501

Match `NO_PROXY` formats to libcurl behaviour #501