[PATCH] http: match headers case-insensitively when redacting

[PATCH] http: match headers case-insensitively when redacting

[Jeff King]

When HTTP/2 is in use, we fail to correctly redact "Authorization" (and
other) headers in our GIT_TRACE_CURL output.

We get the headers in our CURLOPT_DEBUGFUNCTION callback, curl_trace().
It passes them along to curl_dump_header(), which in turn checks
redact_sensitive_header(). We see the headers as a text buffer like:

  Host: ...
  Authorization: Basic ...

After breaking it into lines, we match each header using skip_prefix().
This is case-insensitive, even though HTTP headers are case-insensitive.
This has worked reliably in the past because these headers are generated
by curl itself, which is predictable in what it sends.

But when HTTP/2 is in use, instead we get a lower-case "authorization:"
header, and we fail to match it. The fix is simple: we should match with
skip_iprefix().

Testing is more complicated, though. We do have a test for the redacting
feature, but we don't hit the problem case because our test Apache setup
does not understand HTTP/2. You can reproduce the issue by applying this
on top of the test change in this patch:

	diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
	index afa91e38b0..19267c7107 100644
	--- a/t/lib-httpd/apache.conf
	+++ b/t/lib-httpd/apache.conf
	@@ -29,6 +29,9 @@ ErrorLog error.log
	 	LoadModule setenvif_module modules/mod_setenvif.so
	 </IfModule>

	+LoadModule http2_module modules/mod_http2.so
	+Protocols h2c
	+
	 <IfVersion < 2.4>
	 LockFile accept.lock
	 </IfVersion>
	@@ -64,8 +67,8 @@ LockFile accept.lock
	 <IfModule !mod_access_compat.c>
	 	LoadModule access_compat_module modules/mod_access_compat.so
	 </IfModule>
	-<IfModule !mod_mpm_prefork.c>
	-	LoadModule mpm_prefork_module modules/mod_mpm_prefork.so
	+<IfModule !mod_mpm_event.c>
	+	LoadModule mpm_event_module modules/mod_mpm_event.so
	 </IfModule>
	 <IfModule !mod_unixd.c>
	 	LoadModule unixd_module modules/mod_unixd.so
	diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
	index 1c2a444ae7..ff74f0ae8a 100755
	--- a/t/t5551-http-fetch-smart.sh
	+++ b/t/t5551-http-fetch-smart.sh
	@@ -24,6 +24,10 @@ test_expect_success 'create http-accessible bare repository' '
	 	git push public main:main
	 '

	+test_expect_success 'prefer http/2' '
	+	git config --global http.version HTTP/2
	+'
	+
	 setup_askpass_helper

	 test_expect_success 'clone http repository' '

but this has a few issues:

  - it's not necessarily portable. The http2 apache module might not be
    available on all systems. Further, the http2 module isn't compatible
    with the prefork mpm, so we have to switch to something else. But we
    don't necessarily know what's available. It would be nice if we
    could have conditional config, but IfModule only tells us if a
    module is already loaded, not whether it is available at all.

    This might be a non-issue. The http tests are already optional, and
    modern-enough systems may just have both of these. But...

  - if we do this, then we'd no longer be testing HTTP/1.1 at all. I'm
    not sure how much that matters since it's all handled by curl under
    the hood, but I'd worry that some detail leaks through. We'd
    probably want two scripts running similar tests, one with HTTP/2 and
    one with HTTP/1.1.

  - speaking of which, a later test fails with the patch above! The
    problem is that it is making sure we used a chunked
    transfer-encoding by looking for that header in the trace. But
    HTTP/2 doesn't support that, as it has its own streaming mechanisms
    (the overall operation works fine; we just don't see the header in
    the trace).

On top of that, we also need the test change that this patch _does_ do:
grepping the trace file case-insensitively. Otherwise the test continues
to pass even over HTTP/2, because it sees _both_ forms of the header
(redacted and unredacted), as we upgrade from HTTP/1.1 to HTTP/2. So our
double grep:

	# Ensure that there is no "Basic" followed by a base64 string, but that
	# the auth details are redacted
	! grep "Authorization: Basic [0-9a-zA-Z+/]" trace &&
	grep "Authorization: Basic <redacted>" trace

gets confused. It sees the "<redacted>" one from the pre-upgrade
HTTP/1.1 request, but fails to see the unredacted HTTP/2 one, because it
does not match the lower-case "authorization". Even without the rest of
the test changes, we can still make this test more robust by matching
case-insensitively. That will future-proof the test for a day when
HTTP/2 is finally enabled by default, and doesn't hurt in the meantime.

And finally, there's one other way to demonstrate the issue (and how I
actually found it originally). Looking at GIT_TRACE_CURL output against
github.com, you'll see the unredacted output, even if you didn't set
http.version. That's because setting it is only necessary for curl to
send the extra headers in its HTTP/1.1 request that say "Hey, I speak
HTTP/2; upgrade if you do, too". But for a production site speaking
https, the server advertises via ALPN, a TLS extension, that it supports
HTTP/2, and the client can immediately start using it.

Signed-off-by: Jeff King <peff@peff.net>
---
 http.c                      |  6 +++---
 t/t5551-http-fetch-smart.sh | 24 ++++++++++++------------
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/http.c b/http.c
index a0f169d2fe..4f6a32165f 100644
--- a/http.c
+++ b/http.c
@@ -550,8 +550,8 @@ static void redact_sensitive_header(struct strbuf *header)
 	const char *sensitive_header;
 
 	if (trace_curl_redact &&
-	    (skip_prefix(header->buf, "Authorization:", &sensitive_header) ||
-	     skip_prefix(header->buf, "Proxy-Authorization:", &sensitive_header))) {
+	    (skip_iprefix(header->buf, "Authorization:", &sensitive_header) ||
+	     skip_iprefix(header->buf, "Proxy-Authorization:", &sensitive_header))) {
 		/* The first token is the type, which is OK to log */
 		while (isspace(*sensitive_header))
 			sensitive_header++;
@@ -561,7 +561,7 @@ static void redact_sensitive_header(struct strbuf *header)
 		strbuf_setlen(header,  sensitive_header - header->buf);
 		strbuf_addstr(header, " <redacted>");
 	} else if (trace_curl_redact &&
-		   skip_prefix(header->buf, "Cookie:", &sensitive_header)) {
+		   skip_iprefix(header->buf, "Cookie:", &sensitive_header)) {
 		struct strbuf redacted_header = STRBUF_INIT;
 		const char *cookie;
 
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 4f87d90c5b..4e54226162 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -196,8 +196,8 @@ test_expect_success 'GIT_TRACE_CURL redacts auth details' '
 
 	# Ensure that there is no "Basic" followed by a base64 string, but that
 	# the auth details are redacted
-	! grep "Authorization: Basic [0-9a-zA-Z+/]" trace &&
-	grep "Authorization: Basic <redacted>" trace
+	! grep -i "Authorization: Basic [0-9a-zA-Z+/]" trace &&
+	grep -i "Authorization: Basic <redacted>" trace
 '
 
 test_expect_success 'GIT_CURL_VERBOSE redacts auth details' '
@@ -208,8 +208,8 @@ test_expect_success 'GIT_CURL_VERBOSE redacts auth details' '
 
 	# Ensure that there is no "Basic" followed by a base64 string, but that
 	# the auth details are redacted
-	! grep "Authorization: Basic [0-9a-zA-Z+/]" trace &&
-	grep "Authorization: Basic <redacted>" trace
+	! grep -i "Authorization: Basic [0-9a-zA-Z+/]" trace &&
+	grep -i "Authorization: Basic <redacted>" trace
 '
 
 test_expect_success 'GIT_TRACE_CURL does not redact auth details if GIT_TRACE_REDACT=0' '
@@ -219,7 +219,7 @@ test_expect_success 'GIT_TRACE_CURL does not redact auth details if GIT_TRACE_RE
 		git clone --bare "$HTTPD_URL/auth/smart/repo.git" redact-auth &&
 	expect_askpass both user@host &&
 
-	grep "Authorization: Basic [0-9a-zA-Z+/]" trace
+	grep -i "Authorization: Basic [0-9a-zA-Z+/]" trace
 '
 
 test_expect_success 'disable dumb http on server' '
@@ -474,10 +474,10 @@ test_expect_success 'cookies are redacted by default' '
 	GIT_TRACE_CURL=true \
 		git -c "http.cookieFile=$(pwd)/cookies" clone \
 		$HTTPD_URL/smart/repo.git clone 2>err &&
-	grep "Cookie:.*Foo=<redacted>" err &&
-	grep "Cookie:.*Bar=<redacted>" err &&
-	! grep "Cookie:.*Foo=1" err &&
-	! grep "Cookie:.*Bar=2" err
+	grep -i "Cookie:.*Foo=<redacted>" err &&
+	grep -i "Cookie:.*Bar=<redacted>" err &&
+	! grep -i "Cookie:.*Foo=1" err &&
+	! grep -i "Cookie:.*Bar=2" err
 '
 
 test_expect_success 'empty values of cookies are also redacted' '
@@ -486,7 +486,7 @@ test_expect_success 'empty values of cookies are also redacted' '
 	GIT_TRACE_CURL=true \
 		git -c "http.cookieFile=$(pwd)/cookies" clone \
 		$HTTPD_URL/smart/repo.git clone 2>err &&
-	grep "Cookie:.*Foo=<redacted>" err
+	grep -i "Cookie:.*Foo=<redacted>" err
 '
 
 test_expect_success 'GIT_TRACE_REDACT=0 disables cookie redaction' '
@@ -496,8 +496,8 @@ test_expect_success 'GIT_TRACE_REDACT=0 disables cookie redaction' '
 	GIT_TRACE_REDACT=0 GIT_TRACE_CURL=true \
 		git -c "http.cookieFile=$(pwd)/cookies" clone \
 		$HTTPD_URL/smart/repo.git clone 2>err &&
-	grep "Cookie:.*Foo=1" err &&
-	grep "Cookie:.*Bar=2" err
+	grep -i "Cookie:.*Foo=1" err &&
+	grep -i "Cookie:.*Bar=2" err
 '
 
 test_expect_success 'GIT_TRACE_CURL_NO_DATA prevents data from being traced' '
-- 
2.33.0.1029.g2347ba282e

Re: [PATCH] http: match headers case-insensitively when redacting

[Jeff King]

On Tue, Sep 21, 2021 at 02:41:16PM -0400, Jeff King wrote:

> When HTTP/2 is in use, we fail to correctly redact "Authorization" (and
> other) headers in our GIT_TRACE_CURL output.
> 
> We get the headers in our CURLOPT_DEBUGFUNCTION callback, curl_trace().
> It passes them along to curl_dump_header(), which in turn checks
> redact_sensitive_header(). We see the headers as a text buffer like:
> 
>   Host: ...
>   Authorization: Basic ...
> 
> After breaking it into lines, we match each header using skip_prefix().
> This is case-insensitive, even though HTTP headers are case-insensitive.
> This has worked reliably in the past because these headers are generated
> by curl itself, which is predictable in what it sends.
> 
> But when HTTP/2 is in use, instead we get a lower-case "authorization:"
> header, and we fail to match it. The fix is simple: we should match with
> skip_iprefix().

Daniel,

I cc'd you here mostly as an FYI. I think Git was doing the wrong thing
in assuming case here (we're only expecting these particular headers
coming from the client, but for response headers, I thnk curl will give
us whatever form the server sent us).

But certainly I found the behavior surprising. :) I'd guess it's because
HTTP/2 is sending some binary goo instead of text headers, and the names
we get are just coming from some lookup table? Or maybe I'm just showing
my ignorance of HTTP/2.

At any rate, I wonder if it would be friendlier for curl to hand strings
to the debug function with the usual capitalization.

-Peff

PS This nit aside, it is totally cool that I have been seamlessly using
   HTTP/2 to talk to github.com without even realizing it. I wonder for
   how long!

Re: [PATCH] http: match headers case-insensitively when redacting

[Eric Sunshine]

On Tue, Sep 21, 2021 at 2:41 PM Jeff King <peff@peff.net> wrote:
> When HTTP/2 is in use, we fail to correctly redact "Authorization" (and
> other) headers in our GIT_TRACE_CURL output.
>
> We get the headers in our CURLOPT_DEBUGFUNCTION callback, curl_trace().
> It passes them along to curl_dump_header(), which in turn checks
> redact_sensitive_header(). We see the headers as a text buffer like:
>
>   Host: ...
>   Authorization: Basic ...
>
> After breaking it into lines, we match each header using skip_prefix().
> This is case-insensitive, even though HTTP headers are case-insensitive.
> This has worked reliably in the past because these headers are generated
> by curl itself, which is predictable in what it sends.

Did you mean "This is case-sensitive..."?

> But when HTTP/2 is in use, instead we get a lower-case "authorization:"
> header, and we fail to match it. The fix is simple: we should match with
> skip_iprefix().
> [...]
> Signed-off-by: Jeff King <peff@peff.net>

Re: [PATCH] http: match headers case-insensitively when redacting

[Jeff King]

On Tue, Sep 21, 2021 at 03:06:20PM -0400, Eric Sunshine wrote:

> On Tue, Sep 21, 2021 at 2:41 PM Jeff King <peff@peff.net> wrote:
> > When HTTP/2 is in use, we fail to correctly redact "Authorization" (and
> > other) headers in our GIT_TRACE_CURL output.
> >
> > We get the headers in our CURLOPT_DEBUGFUNCTION callback, curl_trace().
> > It passes them along to curl_dump_header(), which in turn checks
> > redact_sensitive_header(). We see the headers as a text buffer like:
> >
> >   Host: ...
> >   Authorization: Basic ...
> >
> > After breaking it into lines, we match each header using skip_prefix().
> > This is case-insensitive, even though HTTP headers are case-insensitive.
> > This has worked reliably in the past because these headers are generated
> > by curl itself, which is predictable in what it sends.
> 
> Did you mean "This is case-sensitive..."?

Whoops, yes. It probably makes a lot more sense with that fix. :)

-Peff

Re: [PATCH] http: match headers case-insensitively when redacting

[Carlo Arenas]

On Tue, Sep 21, 2021 at 12:48 PM Jeff King <peff@peff.net> wrote:
> But certainly I found the behavior surprising. :) I'd guess it's because
> HTTP/2 is sending some binary goo instead of text headers, and the names
> we get are just coming from some lookup table? Or maybe I'm just showing
> my ignorance of HTTP/2.

AFAIK headers in HTTP/2 MUST be lowercase as per the SPEC[1]

Carlo

[1] https://datatracker.ietf.org/doc/html/rfc7540#section-8.1.2

Re: [PATCH] http: match headers case-insensitively when redacting

[Jeff King]

On Tue, Sep 21, 2021 at 01:14:21PM -0700, Carlo Arenas wrote:

> On Tue, Sep 21, 2021 at 12:48 PM Jeff King <peff@peff.net> wrote:
> > But certainly I found the behavior surprising. :) I'd guess it's because
> > HTTP/2 is sending some binary goo instead of text headers, and the names
> > we get are just coming from some lookup table? Or maybe I'm just showing
> > my ignorance of HTTP/2.
> 
> AFAIK headers in HTTP/2 MUST be lowercase as per the SPEC[1]
> 
> Carlo
> 
> [1] https://datatracker.ietf.org/doc/html/rfc7540#section-8.1.2

Yeah, I did some more reading and found that, too. From Daniel on
StackOverflow, no less:

  https://stackoverflow.com/questions/54067796/preserving-case-of-http-headers-with-curl

So it probably is reasonable to present them to the debug code in that
way. It is a bit weird that we may see them differently depending on
whether curl decided to use HTTP/1.1 or HTTP/2 under the hood, but
matching the on-the-wire format is probably the least-bad thing.

-Peff

Re: [PATCH] http: match headers case-insensitively when redacting

[Taylor Blau]

On Tue, Sep 21, 2021 at 02:41:15PM -0400, Jeff King wrote:
> When HTTP/2 is in use, we fail to correctly redact "Authorization" (and
> other) headers in our GIT_TRACE_CURL output.
>
> We get the headers in our CURLOPT_DEBUGFUNCTION callback, curl_trace().
> It passes them along to curl_dump_header(), which in turn checks
> redact_sensitive_header(). We see the headers as a text buffer like:
>
>   Host: ...
>   Authorization: Basic ...
>
> After breaking it into lines, we match each header using skip_prefix().
> This is case-insensitive, even though HTTP headers are case-insensitive.
> This has worked reliably in the past because these headers are generated
> by curl itself, which is predictable in what it sends.
>
> But when HTTP/2 is in use, instead we get a lower-case "authorization:"
> header, and we fail to match it. The fix is simple: we should match with
> skip_iprefix().
>
> Testing is more complicated, though. We do have a test for the redacting
> feature, but we don't hit the problem case because our test Apache setup
> does not understand HTTP/2. You can reproduce the issue by applying this
> on top of the test change in this patch:
>
> [...]
>
> but this has a few issues:

I'd be fine with assuming that the http2 module is available everywhere,
but only because the tests are optional in the first place. I agree that
we'd want to run our suite of HTTP-related tests in both HTTP/2 and
HTTP/1.1 mode.

But that doesn't mean we have to reconfigure our Apache server midway
through the test, since HTTP/2 servers should keep the HTTP/1.1
conversation going if the client doesn't reply with 'Connection:
upgrade; Upgrade: h2c'. At least, I think that's the case based on my
fairly rudimentary understanding of HTTP/2 ;).

>   - speaking of which, a later test fails with the patch above! The
>     problem is that it is making sure we used a chunked
>     transfer-encoding by looking for that header in the trace. But
>     HTTP/2 doesn't support that, as it has its own streaming mechanisms
>     (the overall operation works fine; we just don't see the header in
>     the trace)

Yeah, presumably we'd want to have a few protocol-specific tests.

> On top of that, we also need the test change that this patch _does_ do:
> grepping the trace file case-insensitively. Otherwise the test continues
> to pass even over HTTP/2, because it sees _both_ forms of the header
> (redacted and unredacted), as we upgrade from HTTP/1.1 to HTTP/2. So our
> double grep:
>
> 	# Ensure that there is no "Basic" followed by a base64 string, but that
> 	# the auth details are redacted
> 	! grep "Authorization: Basic [0-9a-zA-Z+/]" trace &&
> 	grep "Authorization: Basic <redacted>" trace
>
> gets confused. It sees the "<redacted>" one from the pre-upgrade
> HTTP/1.1 request, but fails to see the unredacted HTTP/2 one, because it
> does not match the lower-case "authorization". Even without the rest of
> the test changes, we can still make this test more robust by matching
> case-insensitively. That will future-proof the test for a day when
> HTTP/2 is finally enabled by default, and doesn't hurt in the meantime.

Yeah. We could probably rewrite this test as:

    grep '^[Aa]uthorization:' trace >headers &&
    ! grep 'Basic [0-9a-zA-Z+/]$' headers &&
    grep 'Basic <redacted>$' headers

which I even think is a little clearer to read (but I could equally
understand how other readers find the existing version easier to grok).

Anyway, all of these musings could just as easily be ignored in the
meantime. It's certainly neat to see HTTP/2 more often in the wild :).

This patch looks obviously correct to me.

Thanks,
Taylor

Re: [PATCH] http: match headers case-insensitively when redacting

[Daniel Stenberg]

On Tue, 21 Sep 2021, Jeff King wrote:

> I cc'd you here mostly as an FYI. I think Git was doing the wrong thing
> in assuming case here (we're only expecting these particular headers
> coming from the client, but for response headers, I thnk curl will give
> us whatever form the server sent us).

That'd be correct, yes.

> But certainly I found the behavior surprising. :) I'd guess it's because 
> HTTP/2 is sending some binary goo instead of text headers, and the names we 
> get are just coming from some lookup table? Or maybe I'm just showing my 
> ignorance of HTTP/2.
>
> At any rate, I wonder if it would be friendlier for curl to hand strings
> to the debug function with the usual capitalization.

Maybe that could've been a good idea if we had done it when we introduced 
HTTP/2 support. Now, I think that ship has sailed already as libcurl has 
supported HTTP/2 since late 2013 and changing anything like that now will just 
risk introducing the reverse surprise in applications. Better not rock that 
boat now methinks.

> PS This nit aside, it is totally cool that I have been seamlessly using
>   HTTP/2 to talk to github.com without even realizing it. I wonder for
>   how long!

I don't know when github.com started supporting h2, but since libcurl 7.62.0 
(released Oct 31, 2018) it has negotiated h2 by default over HTTPS.

-- 

  / daniel.haxx.se

Re: [PATCH] http: match headers case-insensitively when redacting

[Jeff King]

On Tue, Sep 21, 2021 at 05:20:45PM -0400, Taylor Blau wrote:

> I'd be fine with assuming that the http2 module is available everywhere,
> but only because the tests are optional in the first place. I agree that
> we'd want to run our suite of HTTP-related tests in both HTTP/2 and
> HTTP/1.1 mode.

Yeah, it's really only a problem if we lose some coverage of http on
particular platforms. But I suspect it's relatively rare for people to
run the http tests in the first place.

> But that doesn't mean we have to reconfigure our Apache server midway
> through the test, since HTTP/2 servers should keep the HTTP/1.1
> conversation going if the client doesn't reply with 'Connection:
> upgrade; Upgrade: h2c'. At least, I think that's the case based on my
> fairly rudimentary understanding of HTTP/2 ;).

Right. If we were doing ALPN, curl would automatically do HTTP/2 if the
server supports it. But since we're not, then yes, we can control it
from the client side. I think I'd probably break it into two scripts
anyway, though, like:

  #!/bin/sh

  test_description='variant of t5551 for http2'
  . ./test-lib.sh

  test_expect_success 'turn on http/2' '
	git config --global http.version HTTP/2 &&
	test_set_prereq HTTP2
  '

  # presumably it learns to skip its preamble if test_description is
  # already set. Or we could pull it out to a common lib-t5551 file.
  . t5551-http-fetch-smart.sh

But TBH I'm not sure if it's even worth the effort. We did find one
obscure case here, but AFAICT this would be unlikely to turn up
anything useful. I dunno. And really, you'd want to do it for all
http-related test scripts, not just this one. That's quite a bit more
work.

-Peff

Re: [PATCH] http: match headers case-insensitively when redacting

[Jeff King]

On Wed, Sep 22, 2021 at 12:00:03AM +0200, Daniel Stenberg wrote:

> > At any rate, I wonder if it would be friendlier for curl to hand strings
> > to the debug function with the usual capitalization.
> 
> Maybe that could've been a good idea if we had done it when we introduced
> HTTP/2 support. Now, I think that ship has sailed already as libcurl has
> supported HTTP/2 since late 2013 and changing anything like that now will
> just risk introducing the reverse surprise in applications. Better not rock
> that boat now methinks.

Oof, that's much older than I realized. I agree the ship has long
sailed, and we are better off leaving things as-is.

> > PS This nit aside, it is totally cool that I have been seamlessly using
> >   HTTP/2 to talk to github.com without even realizing it. I wonder for
> >   how long!
> 
> I don't know when github.com started supporting h2, but since libcurl 7.62.0
> (released Oct 31, 2018) it has negotiated h2 by default over HTTPS.

I dug a bit. Looks like it was enabled at the load-balancing layer of
github.com around January of this year.

-Peff

Re: [PATCH] http: match headers case-insensitively when redacting

[Bagas Sanjaya]

On 22/09/21 01.41, Jeff King wrote:
> But when HTTP/2 is in use, instead we get a lower-case "authorization:"
> header, and we fail to match it. The fix is simple: we should match with
> skip_iprefix().
> 
> Testing is more complicated, though. We do have a test for the redacting
> feature, but we don't hit the problem case because our test Apache setup
> does not understand HTTP/2. You can reproduce the issue by applying this
> on top of the test change in this patch:
> 
> 	diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
> 	index afa91e38b0..19267c7107 100644
> 	--- a/t/lib-httpd/apache.conf
> 	+++ b/t/lib-httpd/apache.conf
> 	@@ -29,6 +29,9 @@ ErrorLog error.log
> 	 	LoadModule setenvif_module modules/mod_setenvif.so
> 	 </IfModule>
> 
> 	+LoadModule http2_module modules/mod_http2.so
> 	+Protocols h2c
> 	+
> 	 <IfVersion < 2.4>
> 	 LockFile accept.lock
> 	 </IfVersion>
> 	@@ -64,8 +67,8 @@ LockFile accept.lock
> 	 <IfModule !mod_access_compat.c>
> 	 	LoadModule access_compat_module modules/mod_access_compat.so
> 	 </IfModule>
> 	-<IfModule !mod_mpm_prefork.c>
> 	-	LoadModule mpm_prefork_module modules/mod_mpm_prefork.so
> 	+<IfModule !mod_mpm_event.c>
> 	+	LoadModule mpm_event_module modules/mod_mpm_event.so
> 	 </IfModule>
> 	 <IfModule !mod_unixd.c>
> 	 	LoadModule unixd_module modules/mod_unixd.so
> 	diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> 	index 1c2a444ae7..ff74f0ae8a 100755
> 	--- a/t/t5551-http-fetch-smart.sh
> 	+++ b/t/t5551-http-fetch-smart.sh
> 	@@ -24,6 +24,10 @@ test_expect_success 'create http-accessible bare repository' '
> 	 	git push public main:main
> 	 '
> 
> 	+test_expect_success 'prefer http/2' '
> 	+	git config --global http.version HTTP/2
> 	+'
> 	+
> 	 setup_askpass_helper
> 
> 	 test_expect_success 'clone http repository' '
> 
> but this has a few issues:
> 
>    - it's not necessarily portable. The http2 apache module might not be
>      available on all systems. Further, the http2 module isn't compatible
>      with the prefork mpm, so we have to switch to something else. But we
>      don't necessarily know what's available. It would be nice if we
>      could have conditional config, but IfModule only tells us if a
>      module is already loaded, not whether it is available at all.
> 
>      This might be a non-issue. The http tests are already optional, and
>      modern-enough systems may just have both of these. But...
> 
>    - if we do this, then we'd no longer be testing HTTP/1.1 at all. I'm
>      not sure how much that matters since it's all handled by curl under
>      the hood, but I'd worry that some detail leaks through. We'd
>      probably want two scripts running similar tests, one with HTTP/2 and
>      one with HTTP/1.1.

Maybe for httpd config we can say that if mpm_prefork isn't loaded, load 
mpm_event and mod_http2.

And for testing both HTTP/2 and HTTP/1.1 did you mean sharing the same 
test code (with adjustments for each protocol)?

-- 
An old man doll... just what I always wanted! - Clara

Re: [PATCH] http: match headers case-insensitively when redacting

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Sep 21, 2021 at 03:06:20PM -0400, Eric Sunshine wrote:
>
>> On Tue, Sep 21, 2021 at 2:41 PM Jeff King <peff@peff.net> wrote:
>> > When HTTP/2 is in use, we fail to correctly redact "Authorization" (and
>> > other) headers in our GIT_TRACE_CURL output.
>> >
>> > We get the headers in our CURLOPT_DEBUGFUNCTION callback, curl_trace().
>> > It passes them along to curl_dump_header(), which in turn checks
>> > redact_sensitive_header(). We see the headers as a text buffer like:
>> >
>> >   Host: ...
>> >   Authorization: Basic ...
>> >
>> > After breaking it into lines, we match each header using skip_prefix().
>> > This is case-insensitive, even though HTTP headers are case-insensitive.
>> > This has worked reliably in the past because these headers are generated
>> > by curl itself, which is predictable in what it sends.
>> 
>> Did you mean "This is case-sensitive..."?
>
> Whoops, yes. It probably makes a lot more sense with that fix. :)

Yeah, I was wondering about the same thing when I read it the first
time.

Re: [PATCH] http: match headers case-insensitively when redacting

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 	# Ensure that there is no "Basic" followed by a base64 string, but that
> 	# the auth details are redacted
> 	! grep "Authorization: Basic [0-9a-zA-Z+/]" trace &&
> 	grep "Authorization: Basic <redacted>" trace
>
> gets confused. It sees the "<redacted>" one from the pre-upgrade
> HTTP/1.1 request, but fails to see the unredacted HTTP/2 one, because it
> does not match the lower-case "authorization".

Neither pattern of the above two will not match the HTTP/2 one, so
the first one would report "there is no leakage of Auth with a
caplital letter"; the second one may see only one pre-upgrade Auth
with a capital letter, but as long as it does find one, it should be
happy, no?

I am a bit puzzled how the test gets confused.

Re: [PATCH] http: match headers case-insensitively when redacting

[Jeff King]

On Wed, Sep 22, 2021 at 12:19:26PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > 	# Ensure that there is no "Basic" followed by a base64 string, but that
> > 	# the auth details are redacted
> > 	! grep "Authorization: Basic [0-9a-zA-Z+/]" trace &&
> > 	grep "Authorization: Basic <redacted>" trace
> >
> > gets confused. It sees the "<redacted>" one from the pre-upgrade
> > HTTP/1.1 request, but fails to see the unredacted HTTP/2 one, because it
> > does not match the lower-case "authorization".
> 
> Neither pattern of the above two will not match the HTTP/2 one, so
> the first one would report "there is no leakage of Auth with a
> caplital letter"; the second one may see only one pre-upgrade Auth
> with a capital letter, but as long as it does find one, it should be
> happy, no?
> 
> I am a bit puzzled how the test gets confused.

The first one matches nothing, because the HTTP/2 one which fails to
redact has a lower-case "A". The second one _does_ match, because we do
issue an HTTP/1.1 request in addition to the HTTP/2 one. We have to in
order to probe the server to say "this is HTTP/1.1, but by the way, we
support HTTP/2".

I am a little surprised that we get as far as sending auth info via
HTTP/1.1, since the initial probe that results in a 401 (causing us to
send the auth) could in theory let us know the server speaks HTTP/2. But
in practice it doesn't.  It looks like the server does not do the
upgrade for a 401 (perhaps that's true for any non-success code, I don't
know).

You can see it in action if you use the test changes I mentioned earlier
_without_ my patch applied (so neither the "grep -i" fix, nor the actual
code change). And then do:

  ./t5551-http-fetch-smart.sh --run=1-17 --debug
  egrep 'Send header:|Recv header:' trash*/trace

I get (with some extraneous headers omitted):

  => Send header: GET /auth/smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
  => Send header: Connection: Upgrade, HTTP2-Settings
  => Send header: Upgrade: h2c
  => Send header: HTTP2-Settings: AAMAAABkAAQCAAAAAAIAAAAA

  <= Recv header: HTTP/1.1 401 Unauthorized
  <= Recv header: Date: Wed, 22 Sep 2021 20:03:32 GMT
  <= Recv header: Server: Apache/2.4.49 (Debian)
  <= Recv header: WWW-Authenticate: Basic realm="git-auth"

  => Send header: GET /auth/smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
  => Send header: Authorization: Basic <redacted>
  => Send header: Connection: Upgrade, HTTP2-Settings
  => Send header: Upgrade: h2c
  => Send header: HTTP2-Settings: AAMAAABkAAQCAAAAAAIAAAAA

  <= Recv header: HTTP/1.1 101 Switching Protocols
  <= Recv header: Upgrade: h2c
  <= Recv header: Connection: Upgrade
  <= Recv header: HTTP/2 200
  <= Recv header: content-type: application/x-git-upload-pack-advertisement

  => Send header: POST /auth/smart/repo.git/git-upload-pack HTTP/2
  => Send header: authorization: Basic dXNlckBob3N0OnBhc3NAaG9zdA==
  => Send header: content-type: application/x-git-upload-pack-request

  <= Recv header: HTTP/2 200
  <= Recv header: content-type: application/x-git-upload-pack-result
  [...and so on...]

So you can see both the redacted and unredacted lines in that output.
I'm happy to include that in the commit message if it helps; I avoided
it earlier because it was already getting quite long. ;)

-Peff

Re: [PATCH] http: match headers case-insensitively when redacting

[Jeff King]

On Wed, Sep 22, 2021 at 09:32:41AM +0700, Bagas Sanjaya wrote:

> > but this has a few issues:
> > 
> >    - it's not necessarily portable. The http2 apache module might not be
> >      available on all systems. Further, the http2 module isn't compatible
> >      with the prefork mpm, so we have to switch to something else. But we
> >      don't necessarily know what's available. It would be nice if we
> >      could have conditional config, but IfModule only tells us if a
> >      module is already loaded, not whether it is available at all.
> > 
> >      This might be a non-issue. The http tests are already optional, and
> >      modern-enough systems may just have both of these. But...
> > 
> >    - if we do this, then we'd no longer be testing HTTP/1.1 at all. I'm
> >      not sure how much that matters since it's all handled by curl under
> >      the hood, but I'd worry that some detail leaks through. We'd
> >      probably want two scripts running similar tests, one with HTTP/2 and
> >      one with HTTP/1.1.
> 
> Maybe for httpd config we can say that if mpm_prefork isn't loaded, load
> mpm_event and mod_http2.

That doesn't work. We can say "is mpm_prefork" loaded, and indeed we
already do, in order to load mpm_prefork! That's because the module may
or may not be built-in, and if not, we have to load it (or some mpm
module). See 296f0b3ea9 (t/lib-httpd/apache.conf: configure an MPM
module for apache 2.4, 2013-06-09).

But we have no way of knowing _which_ modules are available. It may just
be that "event" or "worker" (both of which support mod_http2) are
available close enough to everywhere that we can just guess.

> And for testing both HTTP/2 and HTTP/1.1 did you mean sharing the same test
> code (with adjustments for each protocol)?

Yes. I'd literally run the same battery of tests against both protocols
(see my other response to Taylor with a sketched-out example). I'm still
not sure it's entirely worth the effort, though. The underlying
transport should be pretty transparent to Git, with the exception of
things like debugging output.

-Peff

Re: [PATCH] http: match headers case-insensitively when redacting

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Sep 22, 2021 at 12:19:26PM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > 	# Ensure that there is no "Basic" followed by a base64 string, but that
>> > 	# the auth details are redacted
>> > 	! grep "Authorization: Basic [0-9a-zA-Z+/]" trace &&
>> > 	grep "Authorization: Basic <redacted>" trace
>> >
>> > gets confused. It sees the "<redacted>" one from the pre-upgrade
>> > HTTP/1.1 request, but fails to see the unredacted HTTP/2 one, because it
>> > does not match the lower-case "authorization".
>> 
>> Neither pattern of the above two will not match the HTTP/2 one, so
>> the first one would report "there is no leakage of Auth with a
>> caplital letter"; the second one may see only one pre-upgrade Auth
>> with a capital letter, but as long as it does find one, it should be
>> happy, no?
>> 
>> I am a bit puzzled how the test gets confused.
>
> The first one matches nothing, because the HTTP/2 one which fails to
> redact has a lower-case "A". The second one _does_ match, because ...

I thought we were talking about the original case sensitive test
getting confused when testing the software that is fixed,
i.e. HTTP/2 lowercase "authorization" line properly redacted.

> I get (with some extraneous headers omitted):
> ...
>   => Send header: GET /auth/smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
>   => Send header: Authorization: Basic <redacted>

So, this is what we see in HTTP/1.1 (with capitalization).  And then
...

> ...
>   => Send header: POST /auth/smart/repo.git/git-upload-pack HTTP/2
>   => Send header: authorization: Basic dXNlckBob3N0OnBhc3NAaG9zdA==

this one, once the redaction code is fixed by applying this patch,
would show that we redacted it, too, no?

With or without the fix in the code, I agree that neither of the two
"grep" patterns without "grep -i" change will match this line.  So
the end result is that the test finds no unredacted line, and one
redacted one (instead of two).

I agree that it is *not* testing what we want to test, and if you
said so, I wouldn't have been puzzled.  I just wanted to know if
there is something _else_ (other than "gee, we are not testing the
HTTP/2 case at all") going on that I failed to read in your
"... gets confused".

Thanks.

Re: [PATCH] http: match headers case-insensitively when redacting

[Jeff King]

On Wed, Sep 22, 2021 at 01:51:39PM -0700, Junio C Hamano wrote:

> >> Neither pattern of the above two will not match the HTTP/2 one, so
> >> the first one would report "there is no leakage of Auth with a
> >> caplital letter"; the second one may see only one pre-upgrade Auth
> >> with a capital letter, but as long as it does find one, it should be
> >> happy, no?
> >> 
> >> I am a bit puzzled how the test gets confused.
> >
> > The first one matches nothing, because the HTTP/2 one which fails to
> > redact has a lower-case "A". The second one _does_ match, because ...
> 
> I thought we were talking about the original case sensitive test
> getting confused when testing the software that is fixed,
> i.e. HTTP/2 lowercase "authorization" line properly redacted.

No, sorry. I meant: before the fix, even if we were running HTTP/2, the
test does not detect the bug. And thus it is hard to realize that the
fix is indeed making the bug go away. :)

> > I get (with some extraneous headers omitted):
> > ...
> >   => Send header: GET /auth/smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
> >   => Send header: Authorization: Basic <redacted>
> 
> So, this is what we see in HTTP/1.1 (with capitalization).  And then
> ...
> 
> > ...
> >   => Send header: POST /auth/smart/repo.git/git-upload-pack HTTP/2
> >   => Send header: authorization: Basic dXNlckBob3N0OnBhc3NAaG9zdA==
> 
> this one, once the redaction code is fixed by applying this patch,
> would show that we redacted it, too, no?

Correct. But the test, without switching to "grep -i", does not realize
that.

> With or without the fix in the code, I agree that neither of the two
> "grep" patterns without "grep -i" change will match this line.  So
> the end result is that the test finds no unredacted line, and one
> redacted one (instead of two).
> 
> I agree that it is *not* testing what we want to test, and if you
> said so, I wouldn't have been puzzled.  I just wanted to know if
> there is something _else_ (other than "gee, we are not testing the
> HTTP/2 case at all") going on that I failed to read in your
> "... gets confused".

No, I think we are on the same page now. Do you want me to take
another stab at writing the commit message to clarify things (i.e., do
we think it's badly written, or was it just mis-interpreted)?

-Peff

Re: [PATCH] http: match headers case-insensitively when redacting

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> No, I think we are on the same page now. Do you want me to take
> another stab at writing the commit message to clarify things (i.e., do
> we think it's badly written, or was it just mis-interpreted)?

I am not sure which, but perhaps more of the latter.  So I would not
insist.

Thanks.

[PATCH v2] http: match headers case-insensitively when redacting

[Jeff King]

On Wed, Sep 22, 2021 at 02:42:57PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > No, I think we are on the same page now. Do you want me to take
> > another stab at writing the commit message to clarify things (i.e., do
> > we think it's badly written, or was it just mis-interpreted)?
> 
> I am not sure which, but perhaps more of the latter.  So I would not
> insist.

Well, I did it anyway. :) Here's the updated patch. I think it explains
things more clearly by showing the example output from our discussion
(and reframes the text around it to explain it more). I'll send a
range-diff in a moment.

(It also fixes the s/insensitive/sensitive/ typo).

-- >8 --
Subject: http: match headers case-insensitively when redacting

When HTTP/2 is in use, we fail to correctly redact "Authorization" (and
other) headers in our GIT_TRACE_CURL output.

We get the headers in our CURLOPT_DEBUGFUNCTION callback, curl_trace().
It passes them along to curl_dump_header(), which in turn checks
redact_sensitive_header(). We see the headers as a text buffer like:

  Host: ...
  Authorization: Basic ...

After breaking it into lines, we match each header using skip_prefix().
This is case-sensitive, even though HTTP headers are case-insensitive.
This has worked reliably in the past because these headers are generated
by curl itself, which is predictable in what it sends.

But when HTTP/2 is in use, instead we get a lower-case "authorization:"
header, and we fail to match it. The fix is simple: we should match with
skip_iprefix().

Testing is more complicated, though. We do have a test for the redacting
feature, but we don't hit the problem case because our test Apache setup
does not understand HTTP/2. You can reproduce the issue by applying this
on top of the test change in this patch:

	diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
	index afa91e38b0..19267c7107 100644
	--- a/t/lib-httpd/apache.conf
	+++ b/t/lib-httpd/apache.conf
	@@ -29,6 +29,9 @@ ErrorLog error.log
	 	LoadModule setenvif_module modules/mod_setenvif.so
	 </IfModule>

	+LoadModule http2_module modules/mod_http2.so
	+Protocols h2c
	+
	 <IfVersion < 2.4>
	 LockFile accept.lock
	 </IfVersion>
	@@ -64,8 +67,8 @@ LockFile accept.lock
	 <IfModule !mod_access_compat.c>
	 	LoadModule access_compat_module modules/mod_access_compat.so
	 </IfModule>
	-<IfModule !mod_mpm_prefork.c>
	-	LoadModule mpm_prefork_module modules/mod_mpm_prefork.so
	+<IfModule !mod_mpm_event.c>
	+	LoadModule mpm_event_module modules/mod_mpm_event.so
	 </IfModule>
	 <IfModule !mod_unixd.c>
	 	LoadModule unixd_module modules/mod_unixd.so
	diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
	index 1c2a444ae7..ff74f0ae8a 100755
	--- a/t/t5551-http-fetch-smart.sh
	+++ b/t/t5551-http-fetch-smart.sh
	@@ -24,6 +24,10 @@ test_expect_success 'create http-accessible bare repository' '
	 	git push public main:main
	 '

	+test_expect_success 'prefer http/2' '
	+	git config --global http.version HTTP/2
	+'
	+
	 setup_askpass_helper

	 test_expect_success 'clone http repository' '

but this has a few issues:

  - it's not necessarily portable. The http2 apache module might not be
    available on all systems. Further, the http2 module isn't compatible
    with the prefork mpm, so we have to switch to something else. But we
    don't necessarily know what's available. It would be nice if we
    could have conditional config, but IfModule only tells us if a
    module is already loaded, not whether it is available at all.

    This might be a non-issue. The http tests are already optional, and
    modern-enough systems may just have both of these. But...

  - if we do this, then we'd no longer be testing HTTP/1.1 at all. I'm
    not sure how much that matters since it's all handled by curl under
    the hood, but I'd worry that some detail leaks through. We'd
    probably want two scripts running similar tests, one with HTTP/2 and
    one with HTTP/1.1.

  - speaking of which, a later test fails with the patch above! The
    problem is that it is making sure we used a chunked
    transfer-encoding by looking for that header in the trace. But
    HTTP/2 doesn't support that, as it has its own streaming mechanisms
    (the overall operation works fine; we just don't see the header in
    the trace).

Furthermore, even with the changes above, this test still does not
detect the current failure, because we see _both_ HTTP/1.1 and HTTP/2
requests, which confuse it. Quoting only the interesting bits from the
resulting trace file, we first see:

  => Send header: GET /auth/smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
  => Send header: Connection: Upgrade, HTTP2-Settings
  => Send header: Upgrade: h2c
  => Send header: HTTP2-Settings: AAMAAABkAAQCAAAAAAIAAAAA

  <= Recv header: HTTP/1.1 401 Unauthorized
  <= Recv header: Date: Wed, 22 Sep 2021 20:03:32 GMT
  <= Recv header: Server: Apache/2.4.49 (Debian)
  <= Recv header: WWW-Authenticate: Basic realm="git-auth"

So the client asks for HTTP/2, but Apache does not do the upgrade for
the 401 response. Then the client repeats with credentials:

  => Send header: GET /auth/smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
  => Send header: Authorization: Basic <redacted>
  => Send header: Connection: Upgrade, HTTP2-Settings
  => Send header: Upgrade: h2c
  => Send header: HTTP2-Settings: AAMAAABkAAQCAAAAAAIAAAAA

  <= Recv header: HTTP/1.1 101 Switching Protocols
  <= Recv header: Upgrade: h2c
  <= Recv header: Connection: Upgrade
  <= Recv header: HTTP/2 200
  <= Recv header: content-type: application/x-git-upload-pack-advertisement

So the client does properly redact there, because we're speaking
HTTP/1.1, and the server indicates it can do the upgrade. And then the
client will make further requests using HTTP/2:

  => Send header: POST /auth/smart/repo.git/git-upload-pack HTTP/2
  => Send header: authorization: Basic dXNlckBob3N0OnBhc3NAaG9zdA==
  => Send header: content-type: application/x-git-upload-pack-request

And there we can see that the credential is _not_ redacted. This part of
the test is what gets confused:

	# Ensure that there is no "Basic" followed by a base64 string, but that
	# the auth details are redacted
	! grep "Authorization: Basic [0-9a-zA-Z+/]" trace &&
	grep "Authorization: Basic <redacted>" trace

The first grep does not match the un-redacted HTTP/2 header, because
it insists on an uppercase "A". And the second one does find the
HTTP/1.1 header. So as far as the test is concerned, everything is OK,
but it failed to notice the un-redacted lines.

We can make this test (and the other related ones) more robust by adding
"-i" to grep case-insensitively. This isn't really doing anything for
now, since we're not actually speaking HTTP/2, but it future-proofs the
tests for a day when we do (either we add explicit HTTP/2 test support,
or it's eventually enabled by default by our Apache+curl test setup).
And it doesn't hurt in the meantime for the tests to be more careful.

The change to use "grep -i", coupled with the changes to use HTTP/2
shown above, causes the test to fail with the current code, and pass
after this patch is applied.

And finally, there's one other way to demonstrate the issue (and how I
actually found it originally). Looking at GIT_TRACE_CURL output against
github.com, you'll see the unredacted output, even if you didn't set
http.version. That's because setting it is only necessary for curl to
send the extra headers in its HTTP/1.1 request that say "Hey, I speak
HTTP/2; upgrade if you do, too". But for a production site speaking
https, the server advertises via ALPN, a TLS extension, that it supports
HTTP/2, and the client can immediately start using it.

Signed-off-by: Jeff King <peff@peff.net>
---
 http.c                      |  6 +++---
 t/t5551-http-fetch-smart.sh | 24 ++++++++++++------------
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/http.c b/http.c
index a0f169d2fe..4f6a32165f 100644
--- a/http.c
+++ b/http.c
@@ -550,8 +550,8 @@ static void redact_sensitive_header(struct strbuf *header)
 	const char *sensitive_header;
 
 	if (trace_curl_redact &&
-	    (skip_prefix(header->buf, "Authorization:", &sensitive_header) ||
-	     skip_prefix(header->buf, "Proxy-Authorization:", &sensitive_header))) {
+	    (skip_iprefix(header->buf, "Authorization:", &sensitive_header) ||
+	     skip_iprefix(header->buf, "Proxy-Authorization:", &sensitive_header))) {
 		/* The first token is the type, which is OK to log */
 		while (isspace(*sensitive_header))
 			sensitive_header++;
@@ -561,7 +561,7 @@ static void redact_sensitive_header(struct strbuf *header)
 		strbuf_setlen(header,  sensitive_header - header->buf);
 		strbuf_addstr(header, " <redacted>");
 	} else if (trace_curl_redact &&
-		   skip_prefix(header->buf, "Cookie:", &sensitive_header)) {
+		   skip_iprefix(header->buf, "Cookie:", &sensitive_header)) {
 		struct strbuf redacted_header = STRBUF_INIT;
 		const char *cookie;
 
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 4f87d90c5b..4e54226162 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -196,8 +196,8 @@ test_expect_success 'GIT_TRACE_CURL redacts auth details' '
 
 	# Ensure that there is no "Basic" followed by a base64 string, but that
 	# the auth details are redacted
-	! grep "Authorization: Basic [0-9a-zA-Z+/]" trace &&
-	grep "Authorization: Basic <redacted>" trace
+	! grep -i "Authorization: Basic [0-9a-zA-Z+/]" trace &&
+	grep -i "Authorization: Basic <redacted>" trace
 '
 
 test_expect_success 'GIT_CURL_VERBOSE redacts auth details' '
@@ -208,8 +208,8 @@ test_expect_success 'GIT_CURL_VERBOSE redacts auth details' '
 
 	# Ensure that there is no "Basic" followed by a base64 string, but that
 	# the auth details are redacted
-	! grep "Authorization: Basic [0-9a-zA-Z+/]" trace &&
-	grep "Authorization: Basic <redacted>" trace
+	! grep -i "Authorization: Basic [0-9a-zA-Z+/]" trace &&
+	grep -i "Authorization: Basic <redacted>" trace
 '
 
 test_expect_success 'GIT_TRACE_CURL does not redact auth details if GIT_TRACE_REDACT=0' '
@@ -219,7 +219,7 @@ test_expect_success 'GIT_TRACE_CURL does not redact auth details if GIT_TRACE_RE
 		git clone --bare "$HTTPD_URL/auth/smart/repo.git" redact-auth &&
 	expect_askpass both user@host &&
 
-	grep "Authorization: Basic [0-9a-zA-Z+/]" trace
+	grep -i "Authorization: Basic [0-9a-zA-Z+/]" trace
 '
 
 test_expect_success 'disable dumb http on server' '
@@ -474,10 +474,10 @@ test_expect_success 'cookies are redacted by default' '
 	GIT_TRACE_CURL=true \
 		git -c "http.cookieFile=$(pwd)/cookies" clone \
 		$HTTPD_URL/smart/repo.git clone 2>err &&
-	grep "Cookie:.*Foo=<redacted>" err &&
-	grep "Cookie:.*Bar=<redacted>" err &&
-	! grep "Cookie:.*Foo=1" err &&
-	! grep "Cookie:.*Bar=2" err
+	grep -i "Cookie:.*Foo=<redacted>" err &&
+	grep -i "Cookie:.*Bar=<redacted>" err &&
+	! grep -i "Cookie:.*Foo=1" err &&
+	! grep -i "Cookie:.*Bar=2" err
 '
 
 test_expect_success 'empty values of cookies are also redacted' '
@@ -486,7 +486,7 @@ test_expect_success 'empty values of cookies are also redacted' '
 	GIT_TRACE_CURL=true \
 		git -c "http.cookieFile=$(pwd)/cookies" clone \
 		$HTTPD_URL/smart/repo.git clone 2>err &&
-	grep "Cookie:.*Foo=<redacted>" err
+	grep -i "Cookie:.*Foo=<redacted>" err
 '
 
 test_expect_success 'GIT_TRACE_REDACT=0 disables cookie redaction' '
@@ -496,8 +496,8 @@ test_expect_success 'GIT_TRACE_REDACT=0 disables cookie redaction' '
 	GIT_TRACE_REDACT=0 GIT_TRACE_CURL=true \
 		git -c "http.cookieFile=$(pwd)/cookies" clone \
 		$HTTPD_URL/smart/repo.git clone 2>err &&
-	grep "Cookie:.*Foo=1" err &&
-	grep "Cookie:.*Bar=2" err
+	grep -i "Cookie:.*Foo=1" err &&
+	grep -i "Cookie:.*Bar=2" err
 '
 
 test_expect_success 'GIT_TRACE_CURL_NO_DATA prevents data from being traced' '
-- 
2.33.0.1031.gb334554566

Re: [PATCH v2] http: match headers case-insensitively when redacting

[Jeff King]

On Wed, Sep 22, 2021 at 06:11:54PM -0400, Jeff King wrote:

> Well, I did it anyway. :) Here's the updated patch. I think it explains
> things more clearly by showing the example output from our discussion
> (and reframes the text around it to explain it more). I'll send a
> range-diff in a moment.

Here's the range-diff. I split it out because the commit message is
already so long and full of sample diffs and output that I thought it
would get hard to tell what was range-diff and what was actual diff. :)

1:  faa6e6d28e ! 1:  ea064beb32 http: match headers case-insensitively when redacting
    @@ Commit message
           Authorization: Basic ...
     
         After breaking it into lines, we match each header using skip_prefix().
    -    This is case-insensitive, even though HTTP headers are case-insensitive.
    +    This is case-sensitive, even though HTTP headers are case-insensitive.
         This has worked reliably in the past because these headers are generated
         by curl itself, which is predictable in what it sends.
     
    @@ Commit message
             (the overall operation works fine; we just don't see the header in
             the trace).
     
    -    On top of that, we also need the test change that this patch _does_ do:
    -    grepping the trace file case-insensitively. Otherwise the test continues
    -    to pass even over HTTP/2, because it sees _both_ forms of the header
    -    (redacted and unredacted), as we upgrade from HTTP/1.1 to HTTP/2. So our
    -    double grep:
    +    Furthermore, even with the changes above, this test still does not
    +    detect the current failure, because we see _both_ HTTP/1.1 and HTTP/2
    +    requests, which confuse it. Quoting only the interesting bits from the
    +    resulting trace file, we first see:
    +
    +      => Send header: GET /auth/smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
    +      => Send header: Connection: Upgrade, HTTP2-Settings
    +      => Send header: Upgrade: h2c
    +      => Send header: HTTP2-Settings: AAMAAABkAAQCAAAAAAIAAAAA
    +
    +      <= Recv header: HTTP/1.1 401 Unauthorized
    +      <= Recv header: Date: Wed, 22 Sep 2021 20:03:32 GMT
    +      <= Recv header: Server: Apache/2.4.49 (Debian)
    +      <= Recv header: WWW-Authenticate: Basic realm="git-auth"
    +
    +    So the client asks for HTTP/2, but Apache does not do the upgrade for
    +    the 401 response. Then the client repeats with credentials:
    +
    +      => Send header: GET /auth/smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
    +      => Send header: Authorization: Basic <redacted>
    +      => Send header: Connection: Upgrade, HTTP2-Settings
    +      => Send header: Upgrade: h2c
    +      => Send header: HTTP2-Settings: AAMAAABkAAQCAAAAAAIAAAAA
    +
    +      <= Recv header: HTTP/1.1 101 Switching Protocols
    +      <= Recv header: Upgrade: h2c
    +      <= Recv header: Connection: Upgrade
    +      <= Recv header: HTTP/2 200
    +      <= Recv header: content-type: application/x-git-upload-pack-advertisement
    +
    +    So the client does properly redact there, because we're speaking
    +    HTTP/1.1, and the server indicates it can do the upgrade. And then the
    +    client will make further requests using HTTP/2:
    +
    +      => Send header: POST /auth/smart/repo.git/git-upload-pack HTTP/2
    +      => Send header: authorization: Basic dXNlckBob3N0OnBhc3NAaG9zdA==
    +      => Send header: content-type: application/x-git-upload-pack-request
    +
    +    And there we can see that the credential is _not_ redacted. This part of
    +    the test is what gets confused:
     
                 # Ensure that there is no "Basic" followed by a base64 string, but that
                 # the auth details are redacted
                 ! grep "Authorization: Basic [0-9a-zA-Z+/]" trace &&
                 grep "Authorization: Basic <redacted>" trace
     
    -    gets confused. It sees the "<redacted>" one from the pre-upgrade
    -    HTTP/1.1 request, but fails to see the unredacted HTTP/2 one, because it
    -    does not match the lower-case "authorization". Even without the rest of
    -    the test changes, we can still make this test more robust by matching
    -    case-insensitively. That will future-proof the test for a day when
    -    HTTP/2 is finally enabled by default, and doesn't hurt in the meantime.
    +    The first grep does not match the un-redacted HTTP/2 header, because
    +    it insists on an uppercase "A". And the second one does find the
    +    HTTP/1.1 header. So as far as the test is concerned, everything is OK,
    +    but it failed to notice the un-redacted lines.
    +
    +    We can make this test (and the other related ones) more robust by adding
    +    "-i" to grep case-insensitively. This isn't really doing anything for
    +    now, since we're not actually speaking HTTP/2, but it future-proofs the
    +    tests for a day when we do (either we add explicit HTTP/2 test support,
    +    or it's eventually enabled by default by our Apache+curl test setup).
    +    And it doesn't hurt in the meantime for the tests to be more careful.
    +
    +    The change to use "grep -i", coupled with the changes to use HTTP/2
    +    shown above, causes the test to fail with the current code, and pass
    +    after this patch is applied.
     
         And finally, there's one other way to demonstrate the issue (and how I
         actually found it originally). Looking at GIT_TRACE_CURL output against

Re: [PATCH] http: match headers case-insensitively when redacting

[Ævar Arnfjörð Bjarmason]

On Wed, Sep 22 2021, Jeff King wrote:

> On Wed, Sep 22, 2021 at 09:32:41AM +0700, Bagas Sanjaya wrote:
>
>> > but this has a few issues:
>> > 
>> >    - it's not necessarily portable. The http2 apache module might not be
>> >      available on all systems. Further, the http2 module isn't compatible
>> >      with the prefork mpm, so we have to switch to something else. But we
>> >      don't necessarily know what's available. It would be nice if we
>> >      could have conditional config, but IfModule only tells us if a
>> >      module is already loaded, not whether it is available at all.
>> > 
>> >      This might be a non-issue. The http tests are already optional, and
>> >      modern-enough systems may just have both of these. But...
>> > 
>> >    - if we do this, then we'd no longer be testing HTTP/1.1 at all. I'm
>> >      not sure how much that matters since it's all handled by curl under
>> >      the hood, but I'd worry that some detail leaks through. We'd
>> >      probably want two scripts running similar tests, one with HTTP/2 and
>> >      one with HTTP/1.1.
>> 
>> Maybe for httpd config we can say that if mpm_prefork isn't loaded, load
>> mpm_event and mod_http2.
>
> That doesn't work. We can say "is mpm_prefork" loaded, and indeed we
> already do, in order to load mpm_prefork! That's because the module may
> or may not be built-in, and if not, we have to load it (or some mpm
> module). See 296f0b3ea9 (t/lib-httpd/apache.conf: configure an MPM
> module for apache 2.4, 2013-06-09).
>
> But we have no way of knowing _which_ modules are available. It may just
> be that "event" or "worker" (both of which support mod_http2) are
> available close enough to everywhere that we can just guess.
>
>> And for testing both HTTP/2 and HTTP/1.1 did you mean sharing the same test
>> code (with adjustments for each protocol)?
>
> Yes. I'd literally run the same battery of tests against both protocols
> (see my other response to Taylor with a sketched-out example). I'm still
> not sure it's entirely worth the effort, though. The underlying
> transport should be pretty transparent to Git, with the exception of
> things like debugging output.

Maybe I'm missing something, but it seems to me that trying to figure
out if we support http v2 or not beforehand is the wrong thing to do in
this case. Why don't we simply try to start the server, and fail and
skip_all="sorry, no httpv2" if it fails?

Then have 2 test files:

t1234-http-v1.sh
t1235-http-v2.sh

Where the latter includes the former (or is a symlink with a $0 check),
or both include a library. Doing it this way also means you'll get a
message you notice via "prove", since you won't run all v1 tests in one
file, then skip some v2.

It also means we could add "ssl" in that mix and have 4x files, and
unlike a GIT_TEST_* mode or shoving it all in one test we can run these
in parallel and test all combinations in one test run.

Re: [PATCH] http: match headers case-insensitively when redacting

[Jeff King]

On Thu, Sep 23, 2021 at 03:22:04AM +0200, Ævar Arnfjörð Bjarmason wrote:

> >> Maybe for httpd config we can say that if mpm_prefork isn't loaded, load
> >> mpm_event and mod_http2.
> >
> > That doesn't work. We can say "is mpm_prefork" loaded, and indeed we
> > already do, in order to load mpm_prefork! That's because the module may
> > or may not be built-in, and if not, we have to load it (or some mpm
> > module). See 296f0b3ea9 (t/lib-httpd/apache.conf: configure an MPM
> > module for apache 2.4, 2013-06-09).
> >
> > But we have no way of knowing _which_ modules are available. It may just
> > be that "event" or "worker" (both of which support mod_http2) are
> > available close enough to everywhere that we can just guess.
> >
> >> And for testing both HTTP/2 and HTTP/1.1 did you mean sharing the same test
> >> code (with adjustments for each protocol)?
> >
> > Yes. I'd literally run the same battery of tests against both protocols
> > (see my other response to Taylor with a sketched-out example). I'm still
> > not sure it's entirely worth the effort, though. The underlying
> > transport should be pretty transparent to Git, with the exception of
> > things like debugging output.
> 
> Maybe I'm missing something, but it seems to me that trying to figure
> out if we support http v2 or not beforehand is the wrong thing to do in
> this case. Why don't we simply try to start the server, and fail and
> skip_all="sorry, no httpv2" if it fails?
> 
> Then have 2 test files:
> 
> t1234-http-v1.sh
> t1235-http-v2.sh

Sure. I was assuming we'd just have one server config (which _does_
work), but if we are spinning up two servers anyway for the separate
scripts, it would be easy enough to customize them. And I do think it
would make sense to do it in separate scripts.

And this dual-script thing might need to be repeated for others besides
t5551. I didn't look at which other ones might potentially benefit (or
if it's diminishing returns as we just add more basically-identical
tests that spend a bunch of CPU). This is why I say "it might not be
worth the effort".

> Where the latter includes the former (or is a symlink with a $0 check),
> or both include a library. Doing it this way also means you'll get a
> message you notice via "prove", since you won't run all v1 tests in one
> file, then skip some v2.

This does work oddly with GIT_TEST_HTTPD=Yes, which complains about
skipping (intentionally; it's how we notice when http setup code
breaks).  That might be acceptable, though, if the folks setting that
option (like me, or the linux CI jobs) are likely to have http2 support.

-Peff