Bug: git behind proxy is broken in 2.34.1

Bug: git behind proxy is broken in 2.34.1

[tm-uzr3z]

Hi all,

I just realized that git cloning behind a webproxy is broken in version
2.34.1 (Ubuntu 22.04).

The environment bash variables http_proxy, https_proxy, HTTP_PROXY and
HTTPS_PROXY are all set with the value
"http://myusername:mypassword@ourwebproxy:3128/".

git gives me the following error message on cloning:
$git clone https://github.com/XXXX/YYYY
fatal: unable to access 'https://github.com/XXXX/YYYY/': Received HTTP
code 407 from proxy after CONNECT

For example wget or curl http/https requests in the same shell work
without any issues and use the same proxy settings from the environment
variables.

In pcap traces I see that git requests the URL through the proxy, receives
an http 407 authentication required, and repeats the same request again
without credentials, which gets denied a second time.

On another very old machine with git 1.9.1 it requests the URL through the
proxy, receives an http 407 authentication required, and repeats the
request with credentials, which is allowed then.

Even with git config --global and --system variables http.proxy and
https.proxy the authentication required reply is ignored.

The git configuration is all default, except the proxy variables.
Hope this helps to reproduce and fix this issue. Thank you!

Best regards
Holger

Re: Bug: git behind proxy is broken in 2.34.1

[Jeff King]

On Thu, Feb 16, 2023 at 06:55:42PM +0100, tm-uzr3z@entrap.de wrote:

> I just realized that git cloning behind a webproxy is broken in version
> 2.34.1 (Ubuntu 22.04).
> 
> The environment bash variables http_proxy, https_proxy, HTTP_PROXY and
> HTTPS_PROXY are all set with the value
> "http://myusername:mypassword@ourwebproxy:3128/".
> 
> git gives me the following error message on cloning:
> $git clone https://github.com/XXXX/YYYY
> fatal: unable to access 'https://github.com/XXXX/YYYY/': Received HTTP
> code 407 from proxy after CONNECT

Hmm. We do not have any test coverage for proxies at all. I took a stab
at adding some, and it was not too bad. But...it works!

However, I think there may be a number of possible configurations here.
In my tests, it is an http proxy over http, so it is issuing a GET with
a full URL in it, rather than using CONNECT.

So it may be that CONNECT is broken, but http-over-http proxying is not.
I couldn't get a working CONNECT set up at all (before even worrying
about the authentication step).

> On another very old machine with git 1.9.1 it requests the URL through the
> proxy, receives an http 407 authentication required, and repeats the
> request with credentials, which is allowed then.

That could be good news, as that would mean we might be able to bisect
between 1.9.1 and 2.34.1. However, since you said it was an older
machine, presumably other packages are old, too. I would not be
surprised if the difference is actually in libcurl, not Git.

Still, if you're able to build Git from source and bisect, that would be
worth trying.

-Peff

[PATCH] add basic http proxy tests

[Jeff King]

On Thu, Feb 16, 2023 at 03:46:48PM -0500, Jeff King wrote:

> Hmm. We do not have any test coverage for proxies at all. I took a stab
> at adding some, and it was not too bad.

Here is that patch. Even though it didn't lead me anywhere useful in
debugging your problem, it may be worth picking up anyway just to get
better test coverage.

-- >8 --
Subject: [PATCH] add basic http proxy tests

We do not test our http proxy functionality at all in the test suite, so
this is a pretty big blind spot. Let's at least add a basic check that
we can go through an authenticating proxy to perform a clone.

A few notes on the implementation:

  - I'm using a single apache instance to proxy to itself. This seems to
    work fine in practice, and we can check with a test that this rather
    unusual setup is doing what we expect.

  - I've put the proxy tests into their own script, and it's the only
    one which loads the apache proxy config. If any platform can't
    handle this (e.g., doesn't have the right modules), the start_httpd
    step should fail and gracefully skip the rest of the script (but all
    the other http tests in existing scripts will continue to run).

  - I used a separate passwd file to make sure we don't ever get
    confused between proxy and regular auth credentials. It's using the
    antiquated crypt() format. This is a terrible choice security-wise
    in the modern age, but it's what our existing passwd file uses, and
    should be portable. It would probably be reasonable to switch both
    of these to bcrypt, but we can do that in a separate patch.

  - On the client side, we test two situations with credentials: when
    they are present in the url, and when the username is present but we
    prompt for the password. I think we should be able to handle the
    case that _neither_ is present, but an HTTP 407 causes us to prompt
    for them. However, this doesn't seem to work. That's either a bug,
    or at the very least an opportunity for a feature, but I punted on
    it for now. The point of this patch is just getting basic coverage,
    and we can explore possible deficiencies later.

  - this doesn't work with LIB_HTTPD_SSL. This probably would be
    valuable to have, as https over an http proxy is totally different
    (it uses CONNECT to tunnel the session). But adding in
    mod_proxy_connect and some basic config didn't seem to work for me,
    so I punted for now. Much of the rest of the test suite does not
    currently work with LIB_HTTPD_SSL either, so we shouldn't be making
    anything much worse here.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/lib-httpd.sh           |  7 +++++++
 t/lib-httpd/apache.conf  | 16 ++++++++++++++++
 t/lib-httpd/proxy-passwd |  1 +
 t/t5563-http-proxy.sh    | 41 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 65 insertions(+)
 create mode 100644 t/lib-httpd/proxy-passwd
 create mode 100755 t/t5563-http-proxy.sh

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 5d2d56c445..059fd74adb 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -25,6 +25,7 @@
 #    LIB_HTTPD_DAV               enable DAV
 #    LIB_HTTPD_SVN               enable SVN at given location (e.g. "svn")
 #    LIB_HTTPD_SSL               enable SSL
+#    LIB_HTTPD_PROXY             enable proxy
 #
 # Copyright (c) 2008 Clemens Buchacher <drizzd@aon.at>
 #
@@ -133,6 +134,7 @@ install_script () {
 prepare_httpd() {
 	mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH"
 	cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH"
+	cp "$TEST_PATH"/proxy-passwd "$HTTPD_ROOT_PATH"
 	install_script incomplete-length-upload-pack-v2-http.sh
 	install_script incomplete-body-upload-pack-v2-http.sh
 	install_script error-no-report.sh
@@ -176,6 +178,11 @@ prepare_httpd() {
 			export LIB_HTTPD_SVN LIB_HTTPD_SVNPATH
 		fi
 	fi
+
+	if test -n "$LIB_HTTPD_PROXY"
+	then
+		HTTPD_PARA="$HTTPD_PARA -DPROXY"
+	fi
 }
 
 enable_http2 () {
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 51a4fbcf62..e31293a45f 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -47,6 +47,22 @@ Protocols h2c
 	LoadModule authz_host_module modules/mod_authz_host.so
 </IfModule>
 
+<IfDefine PROXY>
+<IfModule !mod_proxy.c>
+	LoadModule proxy_module modules/mod_proxy.so
+</IfModule>
+<IfModule !mod_proxy_http.c>
+	LoadModule proxy_http_module modules/mod_proxy_http.so
+</IfModule>
+ProxyRequests On
+<Proxy "*">
+	AuthType Basic
+	AuthName "proxy-auth"
+	AuthUserFile proxy-passwd
+	Require valid-user
+</Proxy>
+</IfDefine>
+
 <IfModule !mod_authn_core.c>
 	LoadModule authn_core_module modules/mod_authn_core.so
 </IfModule>
diff --git a/t/lib-httpd/proxy-passwd b/t/lib-httpd/proxy-passwd
new file mode 100644
index 0000000000..77c25138e0
--- /dev/null
+++ b/t/lib-httpd/proxy-passwd
@@ -0,0 +1 @@
+proxuser:2x7tAukjAED5M
diff --git a/t/t5563-http-proxy.sh b/t/t5563-http-proxy.sh
new file mode 100755
index 0000000000..9da5134614
--- /dev/null
+++ b/t/t5563-http-proxy.sh
@@ -0,0 +1,41 @@
+#!/bin/sh
+
+test_description="test fetching through http proxy"
+
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-httpd.sh
+
+LIB_HTTPD_PROXY=1
+start_httpd
+
+test_expect_success 'setup repository' '
+	test_commit foo &&
+	git init --bare "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+	git push --mirror "$HTTPD_DOCUMENT_ROOT_PATH/repo.git"
+'
+
+setup_askpass_helper
+
+# sanity check that our test setup is correctly using proxy
+test_expect_success 'proxy requires password' '
+	test_config_global http.proxy $HTTPD_DEST &&
+	test_must_fail git clone $HTTPD_URL/smart/repo.git 2>err &&
+	grep "error.*407" err
+'
+
+test_expect_success 'clone through proxy with auth' '
+	test_when_finished "rm -rf clone" &&
+	test_config_global http.proxy http://proxuser:proxpass@$HTTPD_DEST &&
+	GIT_TRACE_CURL=$PWD/trace git clone $HTTPD_URL/smart/repo.git clone &&
+	grep -i "Proxy-Authorization: Basic <redacted>" trace
+'
+
+test_expect_success 'clone can prompt for proxy password' '
+	test_when_finished "rm -rf clone" &&
+	test_config_global http.proxy http://proxuser@$HTTPD_DEST &&
+	set_askpass nobody proxpass &&
+	GIT_TRACE_CURL=$PWD/trace git clone $HTTPD_URL/smart/repo.git clone &&
+	expect_askpass pass proxuser
+'
+
+test_done
-- 
2.39.2.920.gf3beb43ccf

Re: [PATCH] add basic http proxy tests

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Here is that patch. Even though it didn't lead me anywhere useful in
> debugging your problem, it may be worth picking up anyway just to get
> better test coverage.
> ...
>   - I'm using a single apache instance to proxy to itself. This seems to
>     work fine in practice, and we can check with a test that this rather
>     unusual setup is doing what we expect.

Cute.

>   - I've put the proxy tests into their own script, and it's the only
>     one which loads the apache proxy config. If any platform can't
>     handle this (e.g., doesn't have the right modules), the start_httpd
>     step should fail and gracefully skip the rest of the script (but all
>     the other http tests in existing scripts will continue to run).

Nice.  I have to move it from 5563 to 5564, though.

>   - On the client side, we test two situations with credentials: when
>     they are present in the url, and when the username is present but we
>     prompt for the password. I think we should be able to handle the
>     case that _neither_ is present, but an HTTP 407 causes us to prompt
>     for them. However, this doesn't seem to work. That's either a bug,
>     or at the very least an opportunity for a feature, but I punted on
>     it for now. The point of this patch is just getting basic coverage,
>     and we can explore possible deficiencies later.

OK.

Thanks.

Re: [PATCH] add basic http proxy tests

[Jeff King]

On Thu, Feb 16, 2023 at 04:30:28PM -0800, Junio C Hamano wrote:

> >   - I've put the proxy tests into their own script, and it's the only
> >     one which loads the apache proxy config. If any platform can't
> >     handle this (e.g., doesn't have the right modules), the start_httpd
> >     step should fail and gracefully skip the rest of the script (but all
> >     the other http tests in existing scripts will continue to run).
> 
> Nice.  I have to move it from 5563 to 5564, though.

Ah, yeah. These number collisions are annoying.

Also, I note that our http client tests have spilled over into t556x
now, making the t556x_common script a bit of a misnomer (it handles
http-backend stuff). I'm not sure if we should be reordering more (which
is a lot of hassle) or perhaps just giving that helper a script a more
descriptive name. Or doing nothing. It is not that big a deal, just
something I noticed while searching for an unused number.

-Peff