[PATCH] 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-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 <redacted>
---
 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

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!

On Tue, Sep 21, 2021 at 2:41 PM Jeff King [off-list ref] 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 <redacted>

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

On Tue, Sep 21, 2021 at 2:41 PM Jeff King [off-list ref] wrote:

quoted

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

On Tue, Sep 21, 2021 at 12:48 PM Jeff King [off-list ref] 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

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

On Tue, Sep 21, 2021 at 12:48 PM Jeff King [off-list ref] wrote:

quoted

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

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

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

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

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

quoted

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.

quoted

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

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

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

quoted

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

On Wed, Sep 22 2021, Jeff King wrote:

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

quoted

quoted

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.

quoted

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.

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

quoted

quoted

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.

quoted

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