{"thread":{"id":"56558","subject":"[PATCH] http: match headers case-insensitively when redacting","startedAt":"2021-09-21T18:41:17Z","lastAt":"2021-09-23T21:56:44Z","messageCount":22,"participants":["Jeff King","Eric Sunshine","Carlo Arenas","Taylor Blau","Daniel Stenberg","Bagas Sanjaya","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"436653","messageId":"YUonS1uoZlZEt+Yd@coredump.intra.peff.net","threadId":"56558","inReplyTo":null,"subject":"[PATCH] http: match headers case-insensitively when redacting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-21T18:41:15Z","receivedAt":"2021-09-21T18:41:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When HTTP/2 is in use, we fail to correctly redact \"Authorization\" (and\nother) headers in our GIT_TRACE_CURL output.\n\nWe get the headers in our CURLOPT_DEBUGFUNCTION callback, curl_trace().\nIt passes them along to curl_dump_header(), which in turn checks\nredact_sensitive_header(). We see the headers as a text buffer like:\n\n  Host: ...\n  Authorization: Basic ...\n\nAfter breaking it into lines, we match each header using skip_prefix().\nThis is case-insensitive, even though HTTP headers are case-insensitive.\nThis has worked reliably in the past because these headers are generated\nby curl itself, which is predictable in what it sends.\n\nBut when HTTP/2 is in use, instead we get a lower-case \"authorization:\"\nheader, and we fail to match it. The fix is simple: we should match with\nskip_iprefix().\n\nTesting is more complicated, though. We do have a test for the redacting\nfeature, but we don't hit the problem case because our test Apache setup\ndoes not understand HTTP/2. You can reproduce the issue by applying this\non top of the test change in this patch:\n\n\tdiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\n\tindex afa91e38b0..19267c7107 100644\n\t--- a/t/lib-httpd/apache.conf\n\t+++ b/t/lib-httpd/apache.conf\n\t@@ -29,6 +29,9 @@ ErrorLog error.log\n\t \tLoadModule setenvif_module modules/mod_setenvif.so\n\t </IfModule>\n\n\t+LoadModule http2_module modules/mod_http2.so\n\t+Protocols h2c\n\t+\n\t <IfVersion < 2.4>\n\t LockFile accept.lock\n\t </IfVersion>\n\t@@ -64,8 +67,8 @@ LockFile accept.lock\n\t <IfModule !mod_access_compat.c>\n\t \tLoadModule access_compat_module modules/mod_access_compat.so\n\t </IfModule>\n\t-<IfModule !mod_mpm_prefork.c>\n\t-\tLoadModule mpm_prefork_module modules/mod_mpm_prefork.so\n\t+<IfModule !mod_mpm_event.c>\n\t+\tLoadModule mpm_event_module modules/mod_mpm_event.so\n\t </IfModule>\n\t <IfModule !mod_unixd.c>\n\t \tLoadModule unixd_module modules/mod_unixd.so\n\tdiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\n\tindex 1c2a444ae7..ff74f0ae8a 100755\n\t--- a/t/t5551-http-fetch-smart.sh\n\t+++ b/t/t5551-http-fetch-smart.sh\n\t@@ -24,6 +24,10 @@ test_expect_success 'create http-accessible bare repository' '\n\t \tgit push public main:main\n\t '\n\n\t+test_expect_success 'prefer http/2' '\n\t+\tgit config --global http.version HTTP/2\n\t+'\n\t+\n\t setup_askpass_helper\n\n\t test_expect_success 'clone http repository' '\n\nbut this has a few issues:\n\n  - it's not necessarily portable. The http2 apache module might not be\n    available on all systems. Further, the http2 module isn't compatible\n    with the prefork mpm, so we have to switch to something else. But we\n    don't necessarily know what's available. It would be nice if we\n    could have conditional config, but IfModule only tells us if a\n    module is already loaded, not whether it is available at all.\n\n    This might be a non-issue. The http tests are already optional, and\n    modern-enough systems may just have both of these. But...\n\n  - if we do this, then we'd no longer be testing HTTP/1.1 at all. I'm\n    not sure how much that matters since it's all handled by curl under\n    the hood, but I'd worry that some detail leaks through. We'd\n    probably want two scripts running similar tests, one with HTTP/2 and\n    one with HTTP/1.1.\n\n  - speaking of which, a later test fails with the patch above! The\n    problem is that it is making sure we used a chunked\n    transfer-encoding by looking for that header in the trace. But\n    HTTP/2 doesn't support that, as it has its own streaming mechanisms\n    (the overall operation works fine; we just don't see the header in\n    the trace).\n\nOn top of that, we also need the test change that this patch _does_ do:\ngrepping the trace file case-insensitively. Otherwise the test continues\nto pass even over HTTP/2, because it sees _both_ forms of the header\n(redacted and unredacted), as we upgrade from HTTP/1.1 to HTTP/2. So our\ndouble grep:\n\n\t# Ensure that there is no \"Basic\" followed by a base64 string, but that\n\t# the auth details are redacted\n\t! grep \"Authorization: Basic [0-9a-zA-Z+/]\" trace &&\n\tgrep \"Authorization: Basic <redacted>\" trace\n\ngets confused. It sees the \"<redacted>\" one from the pre-upgrade\nHTTP/1.1 request, but fails to see the unredacted HTTP/2 one, because it\ndoes not match the lower-case \"authorization\". Even without the rest of\nthe test changes, we can still make this test more robust by matching\ncase-insensitively. That will future-proof the test for a day when\nHTTP/2 is finally enabled by default, and doesn't hurt in the meantime.\n\nAnd finally, there's one other way to demonstrate the issue (and how I\nactually found it originally). Looking at GIT_TRACE_CURL output against\ngithub.com, you'll see the unredacted output, even if you didn't set\nhttp.version. That's because setting it is only necessary for curl to\nsend the extra headers in its HTTP/1.1 request that say \"Hey, I speak\nHTTP/2; upgrade if you do, too\". But for a production site speaking\nhttps, the server advertises via ALPN, a TLS extension, that it supports\nHTTP/2, and the client can immediately start using it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n http.c                      |  6 +++---\n t/t5551-http-fetch-smart.sh | 24 ++++++++++++------------\n 2 files changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex a0f169d2fe..4f6a32165f 100644\n--- a/http.c\n+++ b/http.c\n@@ -550,8 +550,8 @@ static void redact_sensitive_header(struct strbuf *header)\n \tconst char *sensitive_header;\n \n \tif (trace_curl_redact &&\n-\t    (skip_prefix(header->buf, \"Authorization:\", &sensitive_header) ||\n-\t     skip_prefix(header->buf, \"Proxy-Authorization:\", &sensitive_header))) {\n+\t    (skip_iprefix(header->buf, \"Authorization:\", &sensitive_header) ||\n+\t     skip_iprefix(header->buf, \"Proxy-Authorization:\", &sensitive_header))) {\n \t\t/* The first token is the type, which is OK to log */\n \t\twhile (isspace(*sensitive_header))\n \t\t\tsensitive_header++;\n@@ -561,7 +561,7 @@ static void redact_sensitive_header(struct strbuf *header)\n \t\tstrbuf_setlen(header,  sensitive_header - header->buf);\n \t\tstrbuf_addstr(header, \" <redacted>\");\n \t} else if (trace_curl_redact &&\n-\t\t   skip_prefix(header->buf, \"Cookie:\", &sensitive_header)) {\n+\t\t   skip_iprefix(header->buf, \"Cookie:\", &sensitive_header)) {\n \t\tstruct strbuf redacted_header = STRBUF_INIT;\n \t\tconst char *cookie;\n \ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 4f87d90c5b..4e54226162 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -196,8 +196,8 @@ test_expect_success 'GIT_TRACE_CURL redacts auth details' '\n \n \t# Ensure that there is no \"Basic\" followed by a base64 string, but that\n \t# the auth details are redacted\n-\t! grep \"Authorization: Basic [0-9a-zA-Z+/]\" trace &&\n-\tgrep \"Authorization: Basic <redacted>\" trace\n+\t! grep -i \"Authorization: Basic [0-9a-zA-Z+/]\" trace &&\n+\tgrep -i \"Authorization: Basic <redacted>\" trace\n '\n \n test_expect_success 'GIT_CURL_VERBOSE redacts auth details' '\n@@ -208,8 +208,8 @@ test_expect_success 'GIT_CURL_VERBOSE redacts auth details' '\n \n \t# Ensure that there is no \"Basic\" followed by a base64 string, but that\n \t# the auth details are redacted\n-\t! grep \"Authorization: Basic [0-9a-zA-Z+/]\" trace &&\n-\tgrep \"Authorization: Basic <redacted>\" trace\n+\t! grep -i \"Authorization: Basic [0-9a-zA-Z+/]\" trace &&\n+\tgrep -i \"Authorization: Basic <redacted>\" trace\n '\n \n test_expect_success 'GIT_TRACE_CURL does not redact auth details if GIT_TRACE_REDACT=0' '\n@@ -219,7 +219,7 @@ test_expect_success 'GIT_TRACE_CURL does not redact auth details if GIT_TRACE_RE\n \t\tgit clone --bare \"$HTTPD_URL/auth/smart/repo.git\" redact-auth &&\n \texpect_askpass both user@host &&\n \n-\tgrep \"Authorization: Basic [0-9a-zA-Z+/]\" trace\n+\tgrep -i \"Authorization: Basic [0-9a-zA-Z+/]\" trace\n '\n \n test_expect_success 'disable dumb http on server' '\n@@ -474,10 +474,10 @@ test_expect_success 'cookies are redacted by default' '\n \tGIT_TRACE_CURL=true \\\n \t\tgit -c \"http.cookieFile=$(pwd)/cookies\" clone \\\n \t\t$HTTPD_URL/smart/repo.git clone 2>err &&\n-\tgrep \"Cookie:.*Foo=<redacted>\" err &&\n-\tgrep \"Cookie:.*Bar=<redacted>\" err &&\n-\t! grep \"Cookie:.*Foo=1\" err &&\n-\t! grep \"Cookie:.*Bar=2\" err\n+\tgrep -i \"Cookie:.*Foo=<redacted>\" err &&\n+\tgrep -i \"Cookie:.*Bar=<redacted>\" err &&\n+\t! grep -i \"Cookie:.*Foo=1\" err &&\n+\t! grep -i \"Cookie:.*Bar=2\" err\n '\n \n test_expect_success 'empty values of cookies are also redacted' '\n@@ -486,7 +486,7 @@ test_expect_success 'empty values of cookies are also redacted' '\n \tGIT_TRACE_CURL=true \\\n \t\tgit -c \"http.cookieFile=$(pwd)/cookies\" clone \\\n \t\t$HTTPD_URL/smart/repo.git clone 2>err &&\n-\tgrep \"Cookie:.*Foo=<redacted>\" err\n+\tgrep -i \"Cookie:.*Foo=<redacted>\" err\n '\n \n test_expect_success 'GIT_TRACE_REDACT=0 disables cookie redaction' '\n@@ -496,8 +496,8 @@ test_expect_success 'GIT_TRACE_REDACT=0 disables cookie redaction' '\n \tGIT_TRACE_REDACT=0 GIT_TRACE_CURL=true \\\n \t\tgit -c \"http.cookieFile=$(pwd)/cookies\" clone \\\n \t\t$HTTPD_URL/smart/repo.git clone 2>err &&\n-\tgrep \"Cookie:.*Foo=1\" err &&\n-\tgrep \"Cookie:.*Bar=2\" err\n+\tgrep -i \"Cookie:.*Foo=1\" err &&\n+\tgrep -i \"Cookie:.*Bar=2\" err\n '\n \n test_expect_success 'GIT_TRACE_CURL_NO_DATA prevents data from being traced' '\n-- \n2.33.0.1029.g2347ba282e\n"},{"id":"436654","messageId":"YUoorS6UwA1DmwBm@coredump.intra.peff.net","threadId":"56558","inReplyTo":"YUonS1uoZlZEt+Yd@coredump.intra.peff.net","subject":"Re: [PATCH] http: match headers case-insensitively when redacting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-21T18:47:09Z","receivedAt":"2021-09-21T18:47:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 21, 2021 at 02:41:16PM -0400, Jeff King wrote:\n\n> When HTTP/2 is in use, we fail to correctly redact \"Authorization\" (and\n> other) headers in our GIT_TRACE_CURL output.\n> \n> We get the headers in our CURLOPT_DEBUGFUNCTION callback, curl_trace().\n> It passes them along to curl_dump_header(), which in turn checks\n> redact_sensitive_header(). We see the headers as a text buffer like:\n> \n>   Host: ...\n>   Authorization: Basic ...\n> \n> After breaking it into lines, we match each header using skip_prefix().\n> This is case-insensitive, even though HTTP headers are case-insensitive.\n> This has worked reliably in the past because these headers are generated\n> by curl itself, which is predictable in what it sends.\n> \n> But when HTTP/2 is in use, instead we get a lower-case \"authorization:\"\n> header, and we fail to match it. The fix is simple: we should match with\n> skip_iprefix().\n\nDaniel,\n\nI cc'd you here mostly as an FYI. I think Git was doing the wrong thing\nin assuming case here (we're only expecting these particular headers\ncoming from the client, but for response headers, I thnk curl will give\nus whatever form the server sent us).\n\nBut certainly I found the behavior surprising. :) I'd guess it's because\nHTTP/2 is sending some binary goo instead of text headers, and the names\nwe get are just coming from some lookup table? Or maybe I'm just showing\nmy ignorance of HTTP/2.\n\nAt any rate, I wonder if it would be friendlier for curl to hand strings\nto the debug function with the usual capitalization.\n\n-Peff\n\nPS This nit aside, it is totally cool that I have been seamlessly using\n   HTTP/2 to talk to github.com without even realizing it. I wonder for\n   how long!\n"},{"id":"436657","messageId":"CAPig+cS6DZ5DtSpvdrjjQVs5f=pCKkNwaGxU558Qvt50mi9z-A@mail.gmail.com","threadId":"56558","inReplyTo":"YUonS1uoZlZEt+Yd@coredump.intra.peff.net","subject":"Re: [PATCH] http: match headers case-insensitively when redacting","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-09-21T19:06:20Z","receivedAt":"2021-09-21T19:06:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Sep 21, 2021 at 2:41 PM Jeff King <peff@peff.net> wrote:\n> When HTTP/2 is in use, we fail to correctly redact \"Authorization\" (and\n> other) headers in our GIT_TRACE_CURL output.\n>\n> We get the headers in our CURLOPT_DEBUGFUNCTION callback, curl_trace().\n> It passes them along to curl_dump_header(), which in turn checks\n> redact_sensitive_header(). We see the headers as a text buffer like:\n>\n>   Host: ...\n>   Authorization: Basic ...\n>\n> After breaking it into lines, we match each header using skip_prefix().\n> This is case-insensitive, even though HTTP headers are case-insensitive.\n> This has worked reliably in the past because these headers are generated\n> by curl itself, which is predictable in what it sends.\n\nDid you mean \"This is case-sensitive...\"?\n\n> But when HTTP/2 is in use, instead we get a lower-case \"authorization:\"\n> header, and we fail to match it. The fix is simple: we should match with\n> skip_iprefix().\n> [...]\n> Signed-off-by: Jeff King <peff@peff.net>\n"},{"id":"436658","messageId":"YUovLNjkFilkcTAU@coredump.intra.peff.net","threadId":"56558","inReplyTo":"CAPig+cS6DZ5DtSpvdrjjQVs5f=pCKkNwaGxU558Qvt50mi9z-A@mail.gmail.com","subject":"Re: [PATCH] http: match headers case-insensitively when redacting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-21T19:14:52Z","receivedAt":"2021-09-21T19:14:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 21, 2021 at 03:06:20PM -0400, Eric Sunshine wrote:\n\n> On Tue, Sep 21, 2021 at 2:41 PM Jeff King <peff@peff.net> wrote:\n> > When HTTP/2 is in use, we fail to correctly redact \"Authorization\" (and\n> > other) headers in our GIT_TRACE_CURL output.\n> >\n> > We get the headers in our CURLOPT_DEBUGFUNCTION callback, curl_trace().\n> > It passes them along to curl_dump_header(), which in turn checks\n> > redact_sensitive_header(). We see the headers as a text buffer like:\n> >\n> >   Host: ...\n> >   Authorization: Basic ...\n> >\n> > After breaking it into lines, we match each header using skip_prefix().\n> > This is case-insensitive, even though HTTP headers are case-insensitive.\n> > This has worked reliably in the past because these headers are generated\n> > by curl itself, which is predictable in what it sends.\n> \n> Did you mean \"This is case-sensitive...\"?\n\nWhoops, yes. It probably makes a lot more sense with that fix. :)\n\n-Peff\n"},{"id":"436660","messageId":"CAPUEspgrqxrhp-5diEenH+vevWi3QtxpjPqTwDuU5J-JHOXg9A@mail.gmail.com","threadId":"56558","inReplyTo":"YUoorS6UwA1DmwBm@coredump.intra.peff.net","subject":"Re: [PATCH] http: match headers case-insensitively when redacting","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-09-21T20:14:21Z","receivedAt":"2021-09-21T20:14:35Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Sep 21, 2021 at 12:48 PM Jeff King <peff@peff.net> wrote:\n> But certainly I found the behavior surprising. :) I'd guess it's because\n> HTTP/2 is sending some binary goo instead of text headers, and the names\n> we get are just coming from some lookup table? Or maybe I'm just showing\n> my ignorance of HTTP/2.\n\nAFAIK headers in HTTP/2 MUST be lowercase as per the SPEC[1]\n\nCarlo\n\n[1] https://datatracker.ietf.org/doc/html/rfc7540#section-8.1.2\n"},{"id":"436663","messageId":"YUpDRyfYN8gBcTOh@coredump.intra.peff.net","threadId":"56558","inReplyTo":"CAPUEspgrqxrhp-5diEenH+vevWi3QtxpjPqTwDuU5J-JHOXg9A@mail.gmail.com","subject":"Re: [PATCH] http: match headers case-insensitively when redacting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-21T20:40:39Z","receivedAt":"2021-09-21T20:40:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 21, 2021 at 01:14:21PM -0700, Carlo Arenas wrote:\n\n> On Tue, Sep 21, 2021 at 12:48 PM Jeff King <peff@peff.net> wrote:\n> > But certainly I found the behavior surprising. :) I'd guess it's because\n> > HTTP/2 is sending some binary goo instead of text headers, and the names\n> > we get are just coming from some lookup table? Or maybe I'm just showing\n> > my ignorance of HTTP/2.\n> \n> AFAIK headers in HTTP/2 MUST be lowercase as per the SPEC[1]\n> \n> Carlo\n> \n> [1] https://datatracker.ietf.org/doc/html/rfc7540#section-8.1.2\n\nYeah, I did some more reading and found that, too. From Daniel on\nStackOverflow, no less:\n\n  https://stackoverflow.com/questions/54067796/preserving-case-of-http-headers-with-curl\n\nSo it probably is reasonable to present them to the debug code in that\nway. It is a bit weird that we may see them differently depending on\nwhether curl decided to use HTTP/1.1 or HTTP/2 under the hood, but\nmatching the on-the-wire format is probably the least-bad thing.\n\n-Peff\n"},{"id":"436670","messageId":"YUpMreNwBDSygFSf@nand.local","threadId":"56558","inReplyTo":"YUonS1uoZlZEt+Yd@coredump.intra.peff.net","subject":"Re: [PATCH] http: match headers case-insensitively when redacting","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-21T21:20:45Z","receivedAt":"2021-09-21T21:20:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 21, 2021 at 02:41:15PM -0400, Jeff King wrote:\n> When HTTP/2 is in use, we fail to correctly redact \"Authorization\" (and\n> other) headers in our GIT_TRACE_CURL output.\n>\n> We get the headers in our CURLOPT_DEBUGFUNCTION callback, curl_trace().\n> It passes them along to curl_dump_header(), which in turn checks\n> redact_sensitive_header(). We see the headers as a text buffer like:\n>\n>   Host: ...\n>   Authorization: Basic ...\n>\n> After breaking it into lines, we match each header using skip_prefix().\n> This is case-insensitive, even though HTTP headers are case-insensitive.\n> This has worked reliably in the past because these headers are generated\n> by curl itself, which is predictable in what it sends.\n>\n> But when HTTP/2 is in use, instead we get a lower-case \"authorization:\"\n> header, and we fail to match it. The fix is simple: we should match with\n> skip_iprefix().\n>\n> Testing is more complicated, though. We do have a test for the redacting\n> feature, but we don't hit the problem case because our test Apache setup\n> does not understand HTTP/2. You can reproduce the issue by applying this\n> on top of the test change in this patch:\n>\n> [...]\n>\n> but this has a few issues:\n\nI'd be fine with assuming that the http2 module is available everywhere,\nbut only because the tests are optional in the first place. I agree that\nwe'd want to run our suite of HTTP-related tests in both HTTP/2 and\nHTTP/1.1 mode.\n\nBut that doesn't mean we have to reconfigure our Apache server midway\nthrough the test, since HTTP/2 servers should keep the HTTP/1.1\nconversation going if the client doesn't reply with 'Connection:\nupgrade; Upgrade: h2c'. At least, I think that's the case based on my\nfairly rudimentary understanding of HTTP/2 ;).\n\n>   - speaking of which, a later test fails with the patch above! The\n>     problem is that it is making sure we used a chunked\n>     transfer-encoding by looking for that header in the trace. But\n>     HTTP/2 doesn't support that, as it has its own streaming mechanisms\n>     (the overall operation works fine; we just don't see the header in\n>     the trace)\n\nYeah, presumably we'd want to have a few protocol-specific tests.\n\n> On top of that, we also need the test change that this patch _does_ do:\n> grepping the trace file case-insensitively. Otherwise the test continues\n> to pass even over HTTP/2, because it sees _both_ forms of the header\n> (redacted and unredacted), as we upgrade from HTTP/1.1 to HTTP/2. So our\n> double grep:\n>\n> \t# Ensure that there is no \"Basic\" followed by a base64 string, but that\n> \t# the auth details are redacted\n> \t! grep \"Authorization: Basic [0-9a-zA-Z+/]\" trace &&\n> \tgrep \"Authorization: Basic <redacted>\" trace\n>\n> gets confused. It sees the \"<redacted>\" one from the pre-upgrade\n> HTTP/1.1 request, but fails to see the unredacted HTTP/2 one, because it\n> does not match the lower-case \"authorization\". Even without the rest of\n> the test changes, we can still make this test more robust by matching\n> case-insensitively. That will future-proof the test for a day when\n> HTTP/2 is finally enabled by default, and doesn't hurt in the meantime.\n\nYeah. We could probably rewrite this test as:\n\n    grep '^[Aa]uthorization:' trace >headers &&\n    ! grep 'Basic [0-9a-zA-Z+/]$' headers &&\n    grep 'Basic <redacted>$' headers\n\nwhich I even think is a little clearer to read (but I could equally\nunderstand how other readers find the existing version easier to grok).\n\nAnyway, all of these musings could just as easily be ignored in the\nmeantime. It's certainly neat to see HTTP/2 more often in the wild :).\n\nThis patch looks obviously correct to me.\n\nThanks,\nTaylor\n"},{"id":"436672","messageId":"nycvar.QRO.7.76.2109212351440.26668@fvyyl","threadId":"56558","inReplyTo":"YUoorS6UwA1DmwBm@coredump.intra.peff.net","subject":"Re: [PATCH] http: match headers case-insensitively when redacting","fromName":"Daniel Stenberg","fromEmail":"daniel@haxx.se","sentAt":"2021-09-21T22:00:03Z","receivedAt":"2021-09-21T22:00:08Z","isPatch":true,"sender":{"key":"daniel@haxx.se","avatar":"https://gravatar.com/avatar/69fdca87edd17cee21ca2e79fc2ff671d644603c3dc27167430f3cd3dbab7ba8?d=mp&s=160"},"body":"On Tue, 21 Sep 2021, Jeff King wrote:\n\n> I cc'd you here mostly as an FYI. I think Git was doing the wrong thing\n> in assuming case here (we're only expecting these particular headers\n> coming from the client, but for response headers, I thnk curl will give\n> us whatever form the server sent us).\n\nThat'd be correct, yes.\n\n> But certainly I found the behavior surprising. :) I'd guess it's because \n> HTTP/2 is sending some binary goo instead of text headers, and the names we \n> get are just coming from some lookup table? Or maybe I'm just showing my \n> ignorance of HTTP/2.\n>\n> At any rate, I wonder if it would be friendlier for curl to hand strings\n> to the debug function with the usual capitalization.\n\nMaybe that could've been a good idea if we had done it when we introduced \nHTTP/2 support. Now, I think that ship has sailed already as libcurl has \nsupported HTTP/2 since late 2013 and changing anything like that now will just \nrisk introducing the reverse surprise in applications. Better not rock that \nboat now methinks.\n\n> PS This nit aside, it is totally cool that I have been seamlessly using\n>   HTTP/2 to talk to github.com without even realizing it. I wonder for\n>   how long!\n\nI don't know when github.com started supporting h2, but since libcurl 7.62.0 \n(released Oct 31, 2018) it has negotiated h2 by default over HTTPS.\n\n-- \n\n  / daniel.haxx.se\n"},{"id":"436705","messageId":"YUqVMbhqiwMFmbqg@coredump.intra.peff.net","threadId":"56558","inReplyTo":"YUpMreNwBDSygFSf@nand.local","subject":"Re: [PATCH] http: match headers case-insensitively when redacting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-22T02:30:09Z","receivedAt":"2021-09-22T02:30:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 21, 2021 at 05:20:45PM -0400, Taylor Blau wrote:\n\n> I'd be fine with assuming that the http2 module is available everywhere,\n> but only because the tests are optional in the first place. I agree that\n> we'd want to run our suite of HTTP-related tests in both HTTP/2 and\n> HTTP/1.1 mode.\n\nYeah, it's really only a problem if we lose some coverage of http on\nparticular platforms. But I suspect it's relatively rare for people to\nrun the http tests in the first place.\n\n> But that doesn't mean we have to reconfigure our Apache server midway\n> through the test, since HTTP/2 servers should keep the HTTP/1.1\n> conversation going if the client doesn't reply with 'Connection:\n> upgrade; Upgrade: h2c'. At least, I think that's the case based on my\n> fairly rudimentary understanding of HTTP/2 ;).\n\nRight. If we were doing ALPN, curl would automatically do HTTP/2 if the\nserver supports it. But since we're not, then yes, we can control it\nfrom the client side. I think I'd probably break it into two scripts\nanyway, though, like:\n\n  #!/bin/sh\n\n  test_description='variant of t5551 for http2'\n  . ./test-lib.sh\n\n  test_expect_success 'turn on http/2' '\n\tgit config --global http.version HTTP/2 &&\n\ttest_set_prereq HTTP2\n  '\n\n  # presumably it learns to skip its preamble if test_description is\n  # already set. Or we could pull it out to a common lib-t5551 file.\n  . t5551-http-fetch-smart.sh\n\nBut TBH I'm not sure if it's even worth the effort. We did find one\nobscure case here, but AFAICT this would be unlikely to turn up\nanything useful. I dunno. And really, you'd want to do it for all\nhttp-related test scripts, not just this one. That's quite a bit more\nwork.\n\n-Peff\n"},{"id":"436706","messageId":"YUqVoVgt47E80HhV@coredump.intra.peff.net","threadId":"56558","inReplyTo":"nycvar.QRO.7.76.2109212351440.26668@fvyyl","subject":"Re: [PATCH] http: match headers case-insensitively when redacting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-22T02:32:01Z","receivedAt":"2021-09-22T02:32:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 22, 2021 at 12:00:03AM +0200, Daniel Stenberg wrote:\n\n> > At any rate, I wonder if it would be friendlier for curl to hand strings\n> > to the debug function with the usual capitalization.\n> \n> Maybe that could've been a good idea if we had done it when we introduced\n> HTTP/2 support. Now, I think that ship has sailed already as libcurl has\n> supported HTTP/2 since late 2013 and changing anything like that now will\n> just risk introducing the reverse surprise in applications. Better not rock\n> that boat now methinks.\n\nOof, that's much older than I realized. I agree the ship has long\nsailed, and we are better off leaving things as-is.\n\n> > PS This nit aside, it is totally cool that I have been seamlessly using\n> >   HTTP/2 to talk to github.com without even realizing it. I wonder for\n> >   how long!\n> \n> I don't know when github.com started supporting h2, but since libcurl 7.62.0\n> (released Oct 31, 2018) it has negotiated h2 by default over HTTPS.\n\nI dug a bit. Looks like it was enabled at the load-balancing layer of\ngithub.com around January of this year.\n\n-Peff\n"},{"id":"436707","messageId":"afd7bd6b-52bf-7fd8-d13e-6dcd660c4100@gmail.com","threadId":"56558","inReplyTo":"YUonS1uoZlZEt+Yd@coredump.intra.peff.net","subject":"Re: [PATCH] http: match headers case-insensitively when redacting","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2021-09-22T02:32:41Z","receivedAt":"2021-09-22T02:32:48Z","isPatch":true,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"On 22/09/21 01.41, Jeff King wrote:\n> But when HTTP/2 is in use, instead we get a lower-case \"authorization:\"\n> header, and we fail to match it. The fix is simple: we should match with\n> skip_iprefix().\n> \n> Testing is more complicated, though. We do have a test for the redacting\n> feature, but we don't hit the problem case because our test Apache setup\n> does not understand HTTP/2. You can reproduce the issue by applying this\n> on top of the test change in this patch:\n> \n> \tdiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\n> \tindex afa91e38b0..19267c7107 100644\n> \t--- a/t/lib-httpd/apache.conf\n> \t+++ b/t/lib-httpd/apache.conf\n> \t@@ -29,6 +29,9 @@ ErrorLog error.log\n> \t \tLoadModule setenvif_module modules/mod_setenvif.so\n> \t </IfModule>\n> \n> \t+LoadModule http2_module modules/mod_http2.so\n> \t+Protocols h2c\n> \t+\n> \t <IfVersion < 2.4>\n> \t LockFile accept.lock\n> \t </IfVersion>\n> \t@@ -64,8 +67,8 @@ LockFile accept.lock\n> \t <IfModule !mod_access_compat.c>\n> \t \tLoadModule access_compat_module modules/mod_access_compat.so\n> \t </IfModule>\n> \t-<IfModule !mod_mpm_prefork.c>\n> \t-\tLoadModule mpm_prefork_module modules/mod_mpm_prefork.so\n> \t+<IfModule !mod_mpm_event.c>\n> \t+\tLoadModule mpm_event_module modules/mod_mpm_event.so\n> \t </IfModule>\n> \t <IfModule !mod_unixd.c>\n> \t \tLoadModule unixd_module modules/mod_unixd.so\n> \tdiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\n> \tindex 1c2a444ae7..ff74f0ae8a 100755\n> \t--- a/t/t5551-http-fetch-smart.sh\n> \t+++ b/t/t5551-http-fetch-smart.sh\n> \t@@ -24,6 +24,10 @@ test_expect_success 'create http-accessible bare repository' '\n> \t \tgit push public main:main\n> \t '\n> \n> \t+test_expect_success 'prefer http/2' '\n> \t+\tgit config --global http.version HTTP/2\n> \t+'\n> \t+\n> \t setup_askpass_helper\n> \n> \t test_expect_success 'clone http repository' '\n> \n> but this has a few issues:\n> \n>    - it's not necessarily portable. The http2 apache module might not be\n>      available on all systems. Further, the http2 module isn't compatible\n>      with the prefork mpm, so we have to switch to something else. But we\n>      don't necessarily know what's available. It would be nice if we\n>      could have conditional config, but IfModule only tells us if a\n>      module is already loaded, not whether it is available at all.\n> \n>      This might be a non-issue. The http tests are already optional, and\n>      modern-enough systems may just have both of these. But...\n> \n>    - if we do this, then we'd no longer be testing HTTP/1.1 at all. I'm\n>      not sure how much that matters since it's all handled by curl under\n>      the hood, but I'd worry that some detail leaks through. We'd\n>      probably want two scripts running similar tests, one with HTTP/2 and\n>      one with HTTP/1.1.\n\nMaybe for httpd config we can say that if mpm_prefork isn't loaded, load \nmpm_event and mod_http2.\n\nAnd for testing both HTTP/2 and HTTP/1.1 did you mean sharing the same \ntest code (with adjustments for each protocol)?\n\n-- \nAn old man doll... just what I always wanted! - Clara\n"},{"id":"436755","messageId":"xmqqczp077ez.fsf@gitster.g","threadId":"56558","inReplyTo":"YUovLNjkFilkcTAU@coredump.intra.peff.net","subject":"Re: [PATCH] http: match headers case-insensitively when redacting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-22T19:10:44Z","receivedAt":"2021-09-22T19:10:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Sep 21, 2021 at 03:06:20PM -0400, Eric Sunshine wrote:\n>\n>> On Tue, Sep 21, 2021 at 2:41 PM Jeff King <peff@peff.net> wrote:\n>> > When HTTP/2 is in use, we fail to correctly redact \"Authorization\" (and\n>> > other) headers in our GIT_TRACE_CURL output.\n>> >\n>> > We get the headers in our CURLOPT_DEBUGFUNCTION callback, curl_trace().\n>> > It passes them along to curl_dump_header(), which in turn checks\n>> > redact_sensitive_header(). We see the headers as a text buffer like:\n>> >\n>> >   Host: ...\n>> >   Authorization: Basic ...\n>> >\n>> > After breaking it into lines, we match each header using skip_prefix().\n>> > This is case-insensitive, even though HTTP headers are case-insensitive.\n>> > This has worked reliably in the past because these headers are generated\n>> > by curl itself, which is predictable in what it sends.\n>> \n>> Did you mean \"This is case-sensitive...\"?\n>\n> Whoops, yes. It probably makes a lot more sense with that fix. :)\n\nYeah, I was wondering about the same thing when I read it the first\ntime.\n"},{"id":"436758","messageId":"xmqq8rzo770h.fsf@gitster.g","threadId":"56558","inReplyTo":"YUonS1uoZlZEt+Yd@coredump.intra.peff.net","subject":"Re: [PATCH] http: match headers case-insensitively when redacting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-22T19:19:26Z","receivedAt":"2021-09-22T19:19:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> \t# Ensure that there is no \"Basic\" followed by a base64 string, but that\n> \t# the auth details are redacted\n> \t! grep \"Authorization: Basic [0-9a-zA-Z+/]\" trace &&\n> \tgrep \"Authorization: Basic <redacted>\" trace\n>\n> gets confused. It sees the \"<redacted>\" one from the pre-upgrade\n> HTTP/1.1 request, but fails to see the unredacted HTTP/2 one, because it\n> does not match the lower-case \"authorization\".\n\nNeither pattern of the above two will not match the HTTP/2 one, so\nthe first one would report \"there is no leakage of Auth with a\ncaplital letter\"; the second one may see only one pre-upgrade Auth\nwith a capital letter, but as long as it does find one, it should be\nhappy, no?\n\nI am a bit puzzled how the test gets confused.\n"},{"id":"436768","messageId":"YUuNXOb5blV7iN6P@coredump.intra.peff.net","threadId":"56558","inReplyTo":"xmqq8rzo770h.fsf@gitster.g","subject":"Re: [PATCH] http: match headers case-insensitively when redacting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-22T20:09:00Z","receivedAt":"2021-09-22T20:09:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 22, 2021 at 12:19:26PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > \t# Ensure that there is no \"Basic\" followed by a base64 string, but that\n> > \t# the auth details are redacted\n> > \t! grep \"Authorization: Basic [0-9a-zA-Z+/]\" trace &&\n> > \tgrep \"Authorization: Basic <redacted>\" trace\n> >\n> > gets confused. It sees the \"<redacted>\" one from the pre-upgrade\n> > HTTP/1.1 request, but fails to see the unredacted HTTP/2 one, because it\n> > does not match the lower-case \"authorization\".\n> \n> Neither pattern of the above two will not match the HTTP/2 one, so\n> the first one would report \"there is no leakage of Auth with a\n> caplital letter\"; the second one may see only one pre-upgrade Auth\n> with a capital letter, but as long as it does find one, it should be\n> happy, no?\n> \n> I am a bit puzzled how the test gets confused.\n\nThe first one matches nothing, because the HTTP/2 one which fails to\nredact has a lower-case \"A\". The second one _does_ match, because we do\nissue an HTTP/1.1 request in addition to the HTTP/2 one. We have to in\norder to probe the server to say \"this is HTTP/1.1, but by the way, we\nsupport HTTP/2\".\n\nI am a little surprised that we get as far as sending auth info via\nHTTP/1.1, since the initial probe that results in a 401 (causing us to\nsend the auth) could in theory let us know the server speaks HTTP/2. But\nin practice it doesn't.  It looks like the server does not do the\nupgrade for a 401 (perhaps that's true for any non-success code, I don't\nknow).\n\nYou can see it in action if you use the test changes I mentioned earlier\n_without_ my patch applied (so neither the \"grep -i\" fix, nor the actual\ncode change). And then do:\n\n  ./t5551-http-fetch-smart.sh --run=1-17 --debug\n  egrep 'Send header:|Recv header:' trash*/trace\n\nI get (with some extraneous headers omitted):\n\n  => Send header: GET /auth/smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1\n  => Send header: Connection: Upgrade, HTTP2-Settings\n  => Send header: Upgrade: h2c\n  => Send header: HTTP2-Settings: AAMAAABkAAQCAAAAAAIAAAAA\n\n  <= Recv header: HTTP/1.1 401 Unauthorized\n  <= Recv header: Date: Wed, 22 Sep 2021 20:03:32 GMT\n  <= Recv header: Server: Apache/2.4.49 (Debian)\n  <= Recv header: WWW-Authenticate: Basic realm=\"git-auth\"\n\n  => Send header: GET /auth/smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1\n  => Send header: Authorization: Basic <redacted>\n  => Send header: Connection: Upgrade, HTTP2-Settings\n  => Send header: Upgrade: h2c\n  => Send header: HTTP2-Settings: AAMAAABkAAQCAAAAAAIAAAAA\n\n  <= Recv header: HTTP/1.1 101 Switching Protocols\n  <= Recv header: Upgrade: h2c\n  <= Recv header: Connection: Upgrade\n  <= Recv header: HTTP/2 200\n  <= Recv header: content-type: application/x-git-upload-pack-advertisement\n\n  => Send header: POST /auth/smart/repo.git/git-upload-pack HTTP/2\n  => Send header: authorization: Basic dXNlckBob3N0OnBhc3NAaG9zdA==\n  => Send header: content-type: application/x-git-upload-pack-request\n\n  <= Recv header: HTTP/2 200\n  <= Recv header: content-type: application/x-git-upload-pack-result\n  [...and so on...]\n\nSo you can see both the redacted and unredacted lines in that output.\nI'm happy to include that in the commit message if it helps; I avoided\nit earlier because it was already getting quite long. ;)\n\n-Peff\n"},{"id":"436770","messageId":"YUuN+KguN0WetC49@coredump.intra.peff.net","threadId":"56558","inReplyTo":"afd7bd6b-52bf-7fd8-d13e-6dcd660c4100@gmail.com","subject":"Re: [PATCH] http: match headers case-insensitively when redacting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-22T20:11:36Z","receivedAt":"2021-09-22T20:11:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 22, 2021 at 09:32:41AM +0700, Bagas Sanjaya wrote:\n\n> > but this has a few issues:\n> > \n> >    - it's not necessarily portable. The http2 apache module might not be\n> >      available on all systems. Further, the http2 module isn't compatible\n> >      with the prefork mpm, so we have to switch to something else. But we\n> >      don't necessarily know what's available. It would be nice if we\n> >      could have conditional config, but IfModule only tells us if a\n> >      module is already loaded, not whether it is available at all.\n> > \n> >      This might be a non-issue. The http tests are already optional, and\n> >      modern-enough systems may just have both of these. But...\n> > \n> >    - if we do this, then we'd no longer be testing HTTP/1.1 at all. I'm\n> >      not sure how much that matters since it's all handled by curl under\n> >      the hood, but I'd worry that some detail leaks through. We'd\n> >      probably want two scripts running similar tests, one with HTTP/2 and\n> >      one with HTTP/1.1.\n> \n> Maybe for httpd config we can say that if mpm_prefork isn't loaded, load\n> mpm_event and mod_http2.\n\nThat doesn't work. We can say \"is mpm_prefork\" loaded, and indeed we\nalready do, in order to load mpm_prefork! That's because the module may\nor may not be built-in, and if not, we have to load it (or some mpm\nmodule). See 296f0b3ea9 (t/lib-httpd/apache.conf: configure an MPM\nmodule for apache 2.4, 2013-06-09).\n\nBut we have no way of knowing _which_ modules are available. It may just\nbe that \"event\" or \"worker\" (both of which support mod_http2) are\navailable close enough to everywhere that we can just guess.\n\n> And for testing both HTTP/2 and HTTP/1.1 did you mean sharing the same test\n> code (with adjustments for each protocol)?\n\nYes. I'd literally run the same battery of tests against both protocols\n(see my other response to Taylor with a sketched-out example). I'm still\nnot sure it's entirely worth the effort, though. The underlying\ntransport should be pretty transparent to Git, with the exception of\nthings like debugging output.\n\n-Peff\n"},{"id":"436780","messageId":"xmqqk0j85o6c.fsf@gitster.g","threadId":"56558","inReplyTo":"YUuNXOb5blV7iN6P@coredump.intra.peff.net","subject":"Re: [PATCH] http: match headers case-insensitively when redacting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-22T20:51:39Z","receivedAt":"2021-09-22T20:51:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Sep 22, 2021 at 12:19:26PM -0700, Junio C Hamano wrote:\n>\n>> Jeff King <peff@peff.net> writes:\n>> \n>> > \t# Ensure that there is no \"Basic\" followed by a base64 string, but that\n>> > \t# the auth details are redacted\n>> > \t! grep \"Authorization: Basic [0-9a-zA-Z+/]\" trace &&\n>> > \tgrep \"Authorization: Basic <redacted>\" trace\n>> >\n>> > gets confused. It sees the \"<redacted>\" one from the pre-upgrade\n>> > HTTP/1.1 request, but fails to see the unredacted HTTP/2 one, because it\n>> > does not match the lower-case \"authorization\".\n>> \n>> Neither pattern of the above two will not match the HTTP/2 one, so\n>> the first one would report \"there is no leakage of Auth with a\n>> caplital letter\"; the second one may see only one pre-upgrade Auth\n>> with a capital letter, but as long as it does find one, it should be\n>> happy, no?\n>> \n>> I am a bit puzzled how the test gets confused.\n>\n> The first one matches nothing, because the HTTP/2 one which fails to\n> redact has a lower-case \"A\". The second one _does_ match, because ...\n\nI thought we were talking about the original case sensitive test\ngetting confused when testing the software that is fixed,\ni.e. HTTP/2 lowercase \"authorization\" line properly redacted.\n\n> I get (with some extraneous headers omitted):\n> ...\n>   => Send header: GET /auth/smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1\n>   => Send header: Authorization: Basic <redacted>\n\nSo, this is what we see in HTTP/1.1 (with capitalization).  And then\n...\n\n> ...\n>   => Send header: POST /auth/smart/repo.git/git-upload-pack HTTP/2\n>   => Send header: authorization: Basic dXNlckBob3N0OnBhc3NAaG9zdA==\n\nthis one, once the redaction code is fixed by applying this patch,\nwould show that we redacted it, too, no?\n\nWith or without the fix in the code, I agree that neither of the two\n\"grep\" patterns without \"grep -i\" change will match this line.  So\nthe end result is that the test finds no unredacted line, and one\nredacted one (instead of two).\n\nI agree that it is *not* testing what we want to test, and if you\nsaid so, I wouldn't have been puzzled.  I just wanted to know if\nthere is something _else_ (other than \"gee, we are not testing the\nHTTP/2 case at all\") going on that I failed to read in your\n\"... gets confused\".\n\nThanks.\n"},{"id":"436782","messageId":"YUudqYmzy9N3e0Bk@coredump.intra.peff.net","threadId":"56558","inReplyTo":"xmqqk0j85o6c.fsf@gitster.g","subject":"Re: [PATCH] http: match headers case-insensitively when redacting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-22T21:18:33Z","receivedAt":"2021-09-22T21:18:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 22, 2021 at 01:51:39PM -0700, Junio C Hamano wrote:\n\n> >> Neither pattern of the above two will not match the HTTP/2 one, so\n> >> the first one would report \"there is no leakage of Auth with a\n> >> caplital letter\"; the second one may see only one pre-upgrade Auth\n> >> with a capital letter, but as long as it does find one, it should be\n> >> happy, no?\n> >> \n> >> I am a bit puzzled how the test gets confused.\n> >\n> > The first one matches nothing, because the HTTP/2 one which fails to\n> > redact has a lower-case \"A\". The second one _does_ match, because ...\n> \n> I thought we were talking about the original case sensitive test\n> getting confused when testing the software that is fixed,\n> i.e. HTTP/2 lowercase \"authorization\" line properly redacted.\n\nNo, sorry. I meant: before the fix, even if we were running HTTP/2, the\ntest does not detect the bug. And thus it is hard to realize that the\nfix is indeed making the bug go away. :)\n\n> > I get (with some extraneous headers omitted):\n> > ...\n> >   => Send header: GET /auth/smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1\n> >   => Send header: Authorization: Basic <redacted>\n> \n> So, this is what we see in HTTP/1.1 (with capitalization).  And then\n> ...\n> \n> > ...\n> >   => Send header: POST /auth/smart/repo.git/git-upload-pack HTTP/2\n> >   => Send header: authorization: Basic dXNlckBob3N0OnBhc3NAaG9zdA==\n> \n> this one, once the redaction code is fixed by applying this patch,\n> would show that we redacted it, too, no?\n\nCorrect. But the test, without switching to \"grep -i\", does not realize\nthat.\n\n> With or without the fix in the code, I agree that neither of the two\n> \"grep\" patterns without \"grep -i\" change will match this line.  So\n> the end result is that the test finds no unredacted line, and one\n> redacted one (instead of two).\n> \n> I agree that it is *not* testing what we want to test, and if you\n> said so, I wouldn't have been puzzled.  I just wanted to know if\n> there is something _else_ (other than \"gee, we are not testing the\n> HTTP/2 case at all\") going on that I failed to read in your\n> \"... gets confused\".\n\nNo, I think we are on the same page now. Do you want me to take\nanother stab at writing the commit message to clarify things (i.e., do\nwe think it's badly written, or was it just mis-interpreted)?\n\n-Peff\n"},{"id":"436784","messageId":"xmqqbl4k5lsu.fsf@gitster.g","threadId":"56558","inReplyTo":"YUudqYmzy9N3e0Bk@coredump.intra.peff.net","subject":"Re: [PATCH] http: match headers case-insensitively when redacting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-22T21:42:57Z","receivedAt":"2021-09-22T21:43:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> No, I think we are on the same page now. Do you want me to take\n> another stab at writing the commit message to clarify things (i.e., do\n> we think it's badly written, or was it just mis-interpreted)?\n\nI am not sure which, but perhaps more of the latter.  So I would not\ninsist.\n\nThanks.\n"},{"id":"436788","messageId":"YUuqKeXRYuXjXy1+@coredump.intra.peff.net","threadId":"56558","inReplyTo":"xmqqbl4k5lsu.fsf@gitster.g","subject":"[PATCH v2] http: match headers case-insensitively when redacting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-22T22:11:53Z","receivedAt":"2021-09-22T22:11:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 22, 2021 at 02:42:57PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > No, I think we are on the same page now. Do you want me to take\n> > another stab at writing the commit message to clarify things (i.e., do\n> > we think it's badly written, or was it just mis-interpreted)?\n> \n> I am not sure which, but perhaps more of the latter.  So I would not\n> insist.\n\nWell, I did it anyway. :) Here's the updated patch. I think it explains\nthings more clearly by showing the example output from our discussion\n(and reframes the text around it to explain it more). I'll send a\nrange-diff in a moment.\n\n(It also fixes the s/insensitive/sensitive/ typo).\n\n-- >8 --\nSubject: http: match headers case-insensitively when redacting\n\nWhen HTTP/2 is in use, we fail to correctly redact \"Authorization\" (and\nother) headers in our GIT_TRACE_CURL output.\n\nWe get the headers in our CURLOPT_DEBUGFUNCTION callback, curl_trace().\nIt passes them along to curl_dump_header(), which in turn checks\nredact_sensitive_header(). We see the headers as a text buffer like:\n\n  Host: ...\n  Authorization: Basic ...\n\nAfter breaking it into lines, we match each header using skip_prefix().\nThis is case-sensitive, even though HTTP headers are case-insensitive.\nThis has worked reliably in the past because these headers are generated\nby curl itself, which is predictable in what it sends.\n\nBut when HTTP/2 is in use, instead we get a lower-case \"authorization:\"\nheader, and we fail to match it. The fix is simple: we should match with\nskip_iprefix().\n\nTesting is more complicated, though. We do have a test for the redacting\nfeature, but we don't hit the problem case because our test Apache setup\ndoes not understand HTTP/2. You can reproduce the issue by applying this\non top of the test change in this patch:\n\n\tdiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\n\tindex afa91e38b0..19267c7107 100644\n\t--- a/t/lib-httpd/apache.conf\n\t+++ b/t/lib-httpd/apache.conf\n\t@@ -29,6 +29,9 @@ ErrorLog error.log\n\t \tLoadModule setenvif_module modules/mod_setenvif.so\n\t </IfModule>\n\n\t+LoadModule http2_module modules/mod_http2.so\n\t+Protocols h2c\n\t+\n\t <IfVersion < 2.4>\n\t LockFile accept.lock\n\t </IfVersion>\n\t@@ -64,8 +67,8 @@ LockFile accept.lock\n\t <IfModule !mod_access_compat.c>\n\t \tLoadModule access_compat_module modules/mod_access_compat.so\n\t </IfModule>\n\t-<IfModule !mod_mpm_prefork.c>\n\t-\tLoadModule mpm_prefork_module modules/mod_mpm_prefork.so\n\t+<IfModule !mod_mpm_event.c>\n\t+\tLoadModule mpm_event_module modules/mod_mpm_event.so\n\t </IfModule>\n\t <IfModule !mod_unixd.c>\n\t \tLoadModule unixd_module modules/mod_unixd.so\n\tdiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\n\tindex 1c2a444ae7..ff74f0ae8a 100755\n\t--- a/t/t5551-http-fetch-smart.sh\n\t+++ b/t/t5551-http-fetch-smart.sh\n\t@@ -24,6 +24,10 @@ test_expect_success 'create http-accessible bare repository' '\n\t \tgit push public main:main\n\t '\n\n\t+test_expect_success 'prefer http/2' '\n\t+\tgit config --global http.version HTTP/2\n\t+'\n\t+\n\t setup_askpass_helper\n\n\t test_expect_success 'clone http repository' '\n\nbut this has a few issues:\n\n  - it's not necessarily portable. The http2 apache module might not be\n    available on all systems. Further, the http2 module isn't compatible\n    with the prefork mpm, so we have to switch to something else. But we\n    don't necessarily know what's available. It would be nice if we\n    could have conditional config, but IfModule only tells us if a\n    module is already loaded, not whether it is available at all.\n\n    This might be a non-issue. The http tests are already optional, and\n    modern-enough systems may just have both of these. But...\n\n  - if we do this, then we'd no longer be testing HTTP/1.1 at all. I'm\n    not sure how much that matters since it's all handled by curl under\n    the hood, but I'd worry that some detail leaks through. We'd\n    probably want two scripts running similar tests, one with HTTP/2 and\n    one with HTTP/1.1.\n\n  - speaking of which, a later test fails with the patch above! The\n    problem is that it is making sure we used a chunked\n    transfer-encoding by looking for that header in the trace. But\n    HTTP/2 doesn't support that, as it has its own streaming mechanisms\n    (the overall operation works fine; we just don't see the header in\n    the trace).\n\nFurthermore, even with the changes above, this test still does not\ndetect the current failure, because we see _both_ HTTP/1.1 and HTTP/2\nrequests, which confuse it. Quoting only the interesting bits from the\nresulting trace file, we first see:\n\n  => Send header: GET /auth/smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1\n  => Send header: Connection: Upgrade, HTTP2-Settings\n  => Send header: Upgrade: h2c\n  => Send header: HTTP2-Settings: AAMAAABkAAQCAAAAAAIAAAAA\n\n  <= Recv header: HTTP/1.1 401 Unauthorized\n  <= Recv header: Date: Wed, 22 Sep 2021 20:03:32 GMT\n  <= Recv header: Server: Apache/2.4.49 (Debian)\n  <= Recv header: WWW-Authenticate: Basic realm=\"git-auth\"\n\nSo the client asks for HTTP/2, but Apache does not do the upgrade for\nthe 401 response. Then the client repeats with credentials:\n\n  => Send header: GET /auth/smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1\n  => Send header: Authorization: Basic <redacted>\n  => Send header: Connection: Upgrade, HTTP2-Settings\n  => Send header: Upgrade: h2c\n  => Send header: HTTP2-Settings: AAMAAABkAAQCAAAAAAIAAAAA\n\n  <= Recv header: HTTP/1.1 101 Switching Protocols\n  <= Recv header: Upgrade: h2c\n  <= Recv header: Connection: Upgrade\n  <= Recv header: HTTP/2 200\n  <= Recv header: content-type: application/x-git-upload-pack-advertisement\n\nSo the client does properly redact there, because we're speaking\nHTTP/1.1, and the server indicates it can do the upgrade. And then the\nclient will make further requests using HTTP/2:\n\n  => Send header: POST /auth/smart/repo.git/git-upload-pack HTTP/2\n  => Send header: authorization: Basic dXNlckBob3N0OnBhc3NAaG9zdA==\n  => Send header: content-type: application/x-git-upload-pack-request\n\nAnd there we can see that the credential is _not_ redacted. This part of\nthe test is what gets confused:\n\n\t# Ensure that there is no \"Basic\" followed by a base64 string, but that\n\t# the auth details are redacted\n\t! grep \"Authorization: Basic [0-9a-zA-Z+/]\" trace &&\n\tgrep \"Authorization: Basic <redacted>\" trace\n\nThe first grep does not match the un-redacted HTTP/2 header, because\nit insists on an uppercase \"A\". And the second one does find the\nHTTP/1.1 header. So as far as the test is concerned, everything is OK,\nbut it failed to notice the un-redacted lines.\n\nWe can make this test (and the other related ones) more robust by adding\n\"-i\" to grep case-insensitively. This isn't really doing anything for\nnow, since we're not actually speaking HTTP/2, but it future-proofs the\ntests for a day when we do (either we add explicit HTTP/2 test support,\nor it's eventually enabled by default by our Apache+curl test setup).\nAnd it doesn't hurt in the meantime for the tests to be more careful.\n\nThe change to use \"grep -i\", coupled with the changes to use HTTP/2\nshown above, causes the test to fail with the current code, and pass\nafter this patch is applied.\n\nAnd finally, there's one other way to demonstrate the issue (and how I\nactually found it originally). Looking at GIT_TRACE_CURL output against\ngithub.com, you'll see the unredacted output, even if you didn't set\nhttp.version. That's because setting it is only necessary for curl to\nsend the extra headers in its HTTP/1.1 request that say \"Hey, I speak\nHTTP/2; upgrade if you do, too\". But for a production site speaking\nhttps, the server advertises via ALPN, a TLS extension, that it supports\nHTTP/2, and the client can immediately start using it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n http.c                      |  6 +++---\n t/t5551-http-fetch-smart.sh | 24 ++++++++++++------------\n 2 files changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex a0f169d2fe..4f6a32165f 100644\n--- a/http.c\n+++ b/http.c\n@@ -550,8 +550,8 @@ static void redact_sensitive_header(struct strbuf *header)\n \tconst char *sensitive_header;\n \n \tif (trace_curl_redact &&\n-\t    (skip_prefix(header->buf, \"Authorization:\", &sensitive_header) ||\n-\t     skip_prefix(header->buf, \"Proxy-Authorization:\", &sensitive_header))) {\n+\t    (skip_iprefix(header->buf, \"Authorization:\", &sensitive_header) ||\n+\t     skip_iprefix(header->buf, \"Proxy-Authorization:\", &sensitive_header))) {\n \t\t/* The first token is the type, which is OK to log */\n \t\twhile (isspace(*sensitive_header))\n \t\t\tsensitive_header++;\n@@ -561,7 +561,7 @@ static void redact_sensitive_header(struct strbuf *header)\n \t\tstrbuf_setlen(header,  sensitive_header - header->buf);\n \t\tstrbuf_addstr(header, \" <redacted>\");\n \t} else if (trace_curl_redact &&\n-\t\t   skip_prefix(header->buf, \"Cookie:\", &sensitive_header)) {\n+\t\t   skip_iprefix(header->buf, \"Cookie:\", &sensitive_header)) {\n \t\tstruct strbuf redacted_header = STRBUF_INIT;\n \t\tconst char *cookie;\n \ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 4f87d90c5b..4e54226162 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -196,8 +196,8 @@ test_expect_success 'GIT_TRACE_CURL redacts auth details' '\n \n \t# Ensure that there is no \"Basic\" followed by a base64 string, but that\n \t# the auth details are redacted\n-\t! grep \"Authorization: Basic [0-9a-zA-Z+/]\" trace &&\n-\tgrep \"Authorization: Basic <redacted>\" trace\n+\t! grep -i \"Authorization: Basic [0-9a-zA-Z+/]\" trace &&\n+\tgrep -i \"Authorization: Basic <redacted>\" trace\n '\n \n test_expect_success 'GIT_CURL_VERBOSE redacts auth details' '\n@@ -208,8 +208,8 @@ test_expect_success 'GIT_CURL_VERBOSE redacts auth details' '\n \n \t# Ensure that there is no \"Basic\" followed by a base64 string, but that\n \t# the auth details are redacted\n-\t! grep \"Authorization: Basic [0-9a-zA-Z+/]\" trace &&\n-\tgrep \"Authorization: Basic <redacted>\" trace\n+\t! grep -i \"Authorization: Basic [0-9a-zA-Z+/]\" trace &&\n+\tgrep -i \"Authorization: Basic <redacted>\" trace\n '\n \n test_expect_success 'GIT_TRACE_CURL does not redact auth details if GIT_TRACE_REDACT=0' '\n@@ -219,7 +219,7 @@ test_expect_success 'GIT_TRACE_CURL does not redact auth details if GIT_TRACE_RE\n \t\tgit clone --bare \"$HTTPD_URL/auth/smart/repo.git\" redact-auth &&\n \texpect_askpass both user@host &&\n \n-\tgrep \"Authorization: Basic [0-9a-zA-Z+/]\" trace\n+\tgrep -i \"Authorization: Basic [0-9a-zA-Z+/]\" trace\n '\n \n test_expect_success 'disable dumb http on server' '\n@@ -474,10 +474,10 @@ test_expect_success 'cookies are redacted by default' '\n \tGIT_TRACE_CURL=true \\\n \t\tgit -c \"http.cookieFile=$(pwd)/cookies\" clone \\\n \t\t$HTTPD_URL/smart/repo.git clone 2>err &&\n-\tgrep \"Cookie:.*Foo=<redacted>\" err &&\n-\tgrep \"Cookie:.*Bar=<redacted>\" err &&\n-\t! grep \"Cookie:.*Foo=1\" err &&\n-\t! grep \"Cookie:.*Bar=2\" err\n+\tgrep -i \"Cookie:.*Foo=<redacted>\" err &&\n+\tgrep -i \"Cookie:.*Bar=<redacted>\" err &&\n+\t! grep -i \"Cookie:.*Foo=1\" err &&\n+\t! grep -i \"Cookie:.*Bar=2\" err\n '\n \n test_expect_success 'empty values of cookies are also redacted' '\n@@ -486,7 +486,7 @@ test_expect_success 'empty values of cookies are also redacted' '\n \tGIT_TRACE_CURL=true \\\n \t\tgit -c \"http.cookieFile=$(pwd)/cookies\" clone \\\n \t\t$HTTPD_URL/smart/repo.git clone 2>err &&\n-\tgrep \"Cookie:.*Foo=<redacted>\" err\n+\tgrep -i \"Cookie:.*Foo=<redacted>\" err\n '\n \n test_expect_success 'GIT_TRACE_REDACT=0 disables cookie redaction' '\n@@ -496,8 +496,8 @@ test_expect_success 'GIT_TRACE_REDACT=0 disables cookie redaction' '\n \tGIT_TRACE_REDACT=0 GIT_TRACE_CURL=true \\\n \t\tgit -c \"http.cookieFile=$(pwd)/cookies\" clone \\\n \t\t$HTTPD_URL/smart/repo.git clone 2>err &&\n-\tgrep \"Cookie:.*Foo=1\" err &&\n-\tgrep \"Cookie:.*Bar=2\" err\n+\tgrep -i \"Cookie:.*Foo=1\" err &&\n+\tgrep -i \"Cookie:.*Bar=2\" err\n '\n \n test_expect_success 'GIT_TRACE_CURL_NO_DATA prevents data from being traced' '\n-- \n2.33.0.1031.gb334554566\n\n"},{"id":"436790","messageId":"YUuqrNXdgUR5thl5@coredump.intra.peff.net","threadId":"56558","inReplyTo":"YUuqKeXRYuXjXy1+@coredump.intra.peff.net","subject":"Re: [PATCH v2] http: match headers case-insensitively when redacting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-22T22:14:04Z","receivedAt":"2021-09-22T22:14:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 22, 2021 at 06:11:54PM -0400, Jeff King wrote:\n\n> Well, I did it anyway. :) Here's the updated patch. I think it explains\n> things more clearly by showing the example output from our discussion\n> (and reframes the text around it to explain it more). I'll send a\n> range-diff in a moment.\n\nHere's the range-diff. I split it out because the commit message is\nalready so long and full of sample diffs and output that I thought it\nwould get hard to tell what was range-diff and what was actual diff. :)\n\n1:  faa6e6d28e ! 1:  ea064beb32 http: match headers case-insensitively when redacting\n    @@ Commit message\n           Authorization: Basic ...\n     \n         After breaking it into lines, we match each header using skip_prefix().\n    -    This is case-insensitive, even though HTTP headers are case-insensitive.\n    +    This is case-sensitive, even though HTTP headers are case-insensitive.\n         This has worked reliably in the past because these headers are generated\n         by curl itself, which is predictable in what it sends.\n     \n    @@ Commit message\n             (the overall operation works fine; we just don't see the header in\n             the trace).\n     \n    -    On top of that, we also need the test change that this patch _does_ do:\n    -    grepping the trace file case-insensitively. Otherwise the test continues\n    -    to pass even over HTTP/2, because it sees _both_ forms of the header\n    -    (redacted and unredacted), as we upgrade from HTTP/1.1 to HTTP/2. So our\n    -    double grep:\n    +    Furthermore, even with the changes above, this test still does not\n    +    detect the current failure, because we see _both_ HTTP/1.1 and HTTP/2\n    +    requests, which confuse it. Quoting only the interesting bits from the\n    +    resulting trace file, we first see:\n    +\n    +      => Send header: GET /auth/smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1\n    +      => Send header: Connection: Upgrade, HTTP2-Settings\n    +      => Send header: Upgrade: h2c\n    +      => Send header: HTTP2-Settings: AAMAAABkAAQCAAAAAAIAAAAA\n    +\n    +      <= Recv header: HTTP/1.1 401 Unauthorized\n    +      <= Recv header: Date: Wed, 22 Sep 2021 20:03:32 GMT\n    +      <= Recv header: Server: Apache/2.4.49 (Debian)\n    +      <= Recv header: WWW-Authenticate: Basic realm=\"git-auth\"\n    +\n    +    So the client asks for HTTP/2, but Apache does not do the upgrade for\n    +    the 401 response. Then the client repeats with credentials:\n    +\n    +      => Send header: GET /auth/smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1\n    +      => Send header: Authorization: Basic <redacted>\n    +      => Send header: Connection: Upgrade, HTTP2-Settings\n    +      => Send header: Upgrade: h2c\n    +      => Send header: HTTP2-Settings: AAMAAABkAAQCAAAAAAIAAAAA\n    +\n    +      <= Recv header: HTTP/1.1 101 Switching Protocols\n    +      <= Recv header: Upgrade: h2c\n    +      <= Recv header: Connection: Upgrade\n    +      <= Recv header: HTTP/2 200\n    +      <= Recv header: content-type: application/x-git-upload-pack-advertisement\n    +\n    +    So the client does properly redact there, because we're speaking\n    +    HTTP/1.1, and the server indicates it can do the upgrade. And then the\n    +    client will make further requests using HTTP/2:\n    +\n    +      => Send header: POST /auth/smart/repo.git/git-upload-pack HTTP/2\n    +      => Send header: authorization: Basic dXNlckBob3N0OnBhc3NAaG9zdA==\n    +      => Send header: content-type: application/x-git-upload-pack-request\n    +\n    +    And there we can see that the credential is _not_ redacted. This part of\n    +    the test is what gets confused:\n     \n                 # Ensure that there is no \"Basic\" followed by a base64 string, but that\n                 # the auth details are redacted\n                 ! grep \"Authorization: Basic [0-9a-zA-Z+/]\" trace &&\n                 grep \"Authorization: Basic <redacted>\" trace\n     \n    -    gets confused. It sees the \"<redacted>\" one from the pre-upgrade\n    -    HTTP/1.1 request, but fails to see the unredacted HTTP/2 one, because it\n    -    does not match the lower-case \"authorization\". Even without the rest of\n    -    the test changes, we can still make this test more robust by matching\n    -    case-insensitively. That will future-proof the test for a day when\n    -    HTTP/2 is finally enabled by default, and doesn't hurt in the meantime.\n    +    The first grep does not match the un-redacted HTTP/2 header, because\n    +    it insists on an uppercase \"A\". And the second one does find the\n    +    HTTP/1.1 header. So as far as the test is concerned, everything is OK,\n    +    but it failed to notice the un-redacted lines.\n    +\n    +    We can make this test (and the other related ones) more robust by adding\n    +    \"-i\" to grep case-insensitively. This isn't really doing anything for\n    +    now, since we're not actually speaking HTTP/2, but it future-proofs the\n    +    tests for a day when we do (either we add explicit HTTP/2 test support,\n    +    or it's eventually enabled by default by our Apache+curl test setup).\n    +    And it doesn't hurt in the meantime for the tests to be more careful.\n    +\n    +    The change to use \"grep -i\", coupled with the changes to use HTTP/2\n    +    shown above, causes the test to fail with the current code, and pass\n    +    after this patch is applied.\n     \n         And finally, there's one other way to demonstrate the issue (and how I\n         actually found it originally). Looking at GIT_TRACE_CURL output against\n"},{"id":"436812","messageId":"87lf3o5bdz.fsf@evledraar.gmail.com","threadId":"56558","inReplyTo":"YUuN+KguN0WetC49@coredump.intra.peff.net","subject":"Re: [PATCH] http: match headers case-insensitively when redacting","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-23T01:22:04Z","receivedAt":"2021-09-23T01:27:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 22 2021, Jeff King wrote:\n\n> On Wed, Sep 22, 2021 at 09:32:41AM +0700, Bagas Sanjaya wrote:\n>\n>> > but this has a few issues:\n>> > \n>> >    - it's not necessarily portable. The http2 apache module might not be\n>> >      available on all systems. Further, the http2 module isn't compatible\n>> >      with the prefork mpm, so we have to switch to something else. But we\n>> >      don't necessarily know what's available. It would be nice if we\n>> >      could have conditional config, but IfModule only tells us if a\n>> >      module is already loaded, not whether it is available at all.\n>> > \n>> >      This might be a non-issue. The http tests are already optional, and\n>> >      modern-enough systems may just have both of these. But...\n>> > \n>> >    - if we do this, then we'd no longer be testing HTTP/1.1 at all. I'm\n>> >      not sure how much that matters since it's all handled by curl under\n>> >      the hood, but I'd worry that some detail leaks through. We'd\n>> >      probably want two scripts running similar tests, one with HTTP/2 and\n>> >      one with HTTP/1.1.\n>> \n>> Maybe for httpd config we can say that if mpm_prefork isn't loaded, load\n>> mpm_event and mod_http2.\n>\n> That doesn't work. We can say \"is mpm_prefork\" loaded, and indeed we\n> already do, in order to load mpm_prefork! That's because the module may\n> or may not be built-in, and if not, we have to load it (or some mpm\n> module). See 296f0b3ea9 (t/lib-httpd/apache.conf: configure an MPM\n> module for apache 2.4, 2013-06-09).\n>\n> But we have no way of knowing _which_ modules are available. It may just\n> be that \"event\" or \"worker\" (both of which support mod_http2) are\n> available close enough to everywhere that we can just guess.\n>\n>> And for testing both HTTP/2 and HTTP/1.1 did you mean sharing the same test\n>> code (with adjustments for each protocol)?\n>\n> Yes. I'd literally run the same battery of tests against both protocols\n> (see my other response to Taylor with a sketched-out example). I'm still\n> not sure it's entirely worth the effort, though. The underlying\n> transport should be pretty transparent to Git, with the exception of\n> things like debugging output.\n\nMaybe I'm missing something, but it seems to me that trying to figure\nout if we support http v2 or not beforehand is the wrong thing to do in\nthis case. Why don't we simply try to start the server, and fail and\nskip_all=\"sorry, no httpv2\" if it fails?\n\nThen have 2 test files:\n\nt1234-http-v1.sh\nt1235-http-v2.sh\n\nWhere the latter includes the former (or is a symlink with a $0 check),\nor both include a library. Doing it this way also means you'll get a\nmessage you notice via \"prove\", since you won't run all v1 tests in one\nfile, then skip some v2.\n\nIt also means we could add \"ssl\" in that mix and have 4x files, and\nunlike a GIT_TEST_* mode or shoving it all in one test we can run these\nin parallel and test all combinations in one test run.\n\n"},{"id":"436903","messageId":"YUz4Gr3o/Kobj10r@coredump.intra.peff.net","threadId":"56558","inReplyTo":"87lf3o5bdz.fsf@evledraar.gmail.com","subject":"Re: [PATCH] http: match headers case-insensitively when redacting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-23T21:56:42Z","receivedAt":"2021-09-23T21:56:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 23, 2021 at 03:22:04AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> >> Maybe for httpd config we can say that if mpm_prefork isn't loaded, load\n> >> mpm_event and mod_http2.\n> >\n> > That doesn't work. We can say \"is mpm_prefork\" loaded, and indeed we\n> > already do, in order to load mpm_prefork! That's because the module may\n> > or may not be built-in, and if not, we have to load it (or some mpm\n> > module). See 296f0b3ea9 (t/lib-httpd/apache.conf: configure an MPM\n> > module for apache 2.4, 2013-06-09).\n> >\n> > But we have no way of knowing _which_ modules are available. It may just\n> > be that \"event\" or \"worker\" (both of which support mod_http2) are\n> > available close enough to everywhere that we can just guess.\n> >\n> >> And for testing both HTTP/2 and HTTP/1.1 did you mean sharing the same test\n> >> code (with adjustments for each protocol)?\n> >\n> > Yes. I'd literally run the same battery of tests against both protocols\n> > (see my other response to Taylor with a sketched-out example). I'm still\n> > not sure it's entirely worth the effort, though. The underlying\n> > transport should be pretty transparent to Git, with the exception of\n> > things like debugging output.\n> \n> Maybe I'm missing something, but it seems to me that trying to figure\n> out if we support http v2 or not beforehand is the wrong thing to do in\n> this case. Why don't we simply try to start the server, and fail and\n> skip_all=\"sorry, no httpv2\" if it fails?\n> \n> Then have 2 test files:\n> \n> t1234-http-v1.sh\n> t1235-http-v2.sh\n\nSure. I was assuming we'd just have one server config (which _does_\nwork), but if we are spinning up two servers anyway for the separate\nscripts, it would be easy enough to customize them. And I do think it\nwould make sense to do it in separate scripts.\n\nAnd this dual-script thing might need to be repeated for others besides\nt5551. I didn't look at which other ones might potentially benefit (or\nif it's diminishing returns as we just add more basically-identical\ntests that spend a bunch of CPU). This is why I say \"it might not be\nworth the effort\".\n\n> Where the latter includes the former (or is a symlink with a $0 check),\n> or both include a library. Doing it this way also means you'll get a\n> message you notice via \"prove\", since you won't run all v1 tests in one\n> file, then skip some v2.\n\nThis does work oddly with GIT_TEST_HTTPD=Yes, which complains about\nskipping (intentionally; it's how we notice when http setup code\nbreaks).  That might be acceptable, though, if the folks setting that\noption (like me, or the linux CI jobs) are likely to have http2 support.\n\n-Peff\n"}]}