From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Daniel Stenberg <daniel@haxx.se>
Subject: Re: [PATCH v2] http: match headers case-insensitively when redacting
Date: Wed, 22 Sep 2021 18:14:04 -0400 [thread overview]
Message-ID: <YUuqrNXdgUR5thl5@coredump.intra.peff.net> (raw)
In-Reply-To: <YUuqKeXRYuXjXy1+@coredump.intra.peff.net>
On Wed, Sep 22, 2021 at 06:11:54PM -0400, Jeff King wrote:
> Well, I did it anyway. :) Here's the updated patch. I think it explains
> things more clearly by showing the example output from our discussion
> (and reframes the text around it to explain it more). I'll send a
> range-diff in a moment.
Here's the range-diff. I split it out because the commit message is
already so long and full of sample diffs and output that I thought it
would get hard to tell what was range-diff and what was actual diff. :)
1: faa6e6d28e ! 1: ea064beb32 http: match headers case-insensitively when redacting
@@ Commit message
Authorization: Basic ...
After breaking it into lines, we match each header using skip_prefix().
- This is case-insensitive, even though HTTP headers are case-insensitive.
+ This is case-sensitive, even though HTTP headers are case-insensitive.
This has worked reliably in the past because these headers are generated
by curl itself, which is predictable in what it sends.
@@ Commit message
(the overall operation works fine; we just don't see the header in
the trace).
- On top of that, we also need the test change that this patch _does_ do:
- grepping the trace file case-insensitively. Otherwise the test continues
- to pass even over HTTP/2, because it sees _both_ forms of the header
- (redacted and unredacted), as we upgrade from HTTP/1.1 to HTTP/2. So our
- double grep:
+ Furthermore, even with the changes above, this test still does not
+ detect the current failure, because we see _both_ HTTP/1.1 and HTTP/2
+ requests, which confuse it. Quoting only the interesting bits from the
+ resulting trace file, we first see:
+
+ => Send header: GET /auth/smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
+ => Send header: Connection: Upgrade, HTTP2-Settings
+ => Send header: Upgrade: h2c
+ => Send header: HTTP2-Settings: AAMAAABkAAQCAAAAAAIAAAAA
+
+ <= Recv header: HTTP/1.1 401 Unauthorized
+ <= Recv header: Date: Wed, 22 Sep 2021 20:03:32 GMT
+ <= Recv header: Server: Apache/2.4.49 (Debian)
+ <= Recv header: WWW-Authenticate: Basic realm="git-auth"
+
+ So the client asks for HTTP/2, but Apache does not do the upgrade for
+ the 401 response. Then the client repeats with credentials:
+
+ => Send header: GET /auth/smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
+ => Send header: Authorization: Basic <redacted>
+ => Send header: Connection: Upgrade, HTTP2-Settings
+ => Send header: Upgrade: h2c
+ => Send header: HTTP2-Settings: AAMAAABkAAQCAAAAAAIAAAAA
+
+ <= Recv header: HTTP/1.1 101 Switching Protocols
+ <= Recv header: Upgrade: h2c
+ <= Recv header: Connection: Upgrade
+ <= Recv header: HTTP/2 200
+ <= Recv header: content-type: application/x-git-upload-pack-advertisement
+
+ So the client does properly redact there, because we're speaking
+ HTTP/1.1, and the server indicates it can do the upgrade. And then the
+ client will make further requests using HTTP/2:
+
+ => Send header: POST /auth/smart/repo.git/git-upload-pack HTTP/2
+ => Send header: authorization: Basic dXNlckBob3N0OnBhc3NAaG9zdA==
+ => Send header: content-type: application/x-git-upload-pack-request
+
+ And there we can see that the credential is _not_ redacted. This part of
+ the test is what gets confused:
# Ensure that there is no "Basic" followed by a base64 string, but that
# the auth details are redacted
! grep "Authorization: Basic [0-9a-zA-Z+/]" trace &&
grep "Authorization: Basic <redacted>" trace
- gets confused. It sees the "<redacted>" one from the pre-upgrade
- HTTP/1.1 request, but fails to see the unredacted HTTP/2 one, because it
- does not match the lower-case "authorization". Even without the rest of
- the test changes, we can still make this test more robust by matching
- case-insensitively. That will future-proof the test for a day when
- HTTP/2 is finally enabled by default, and doesn't hurt in the meantime.
+ The first grep does not match the un-redacted HTTP/2 header, because
+ it insists on an uppercase "A". And the second one does find the
+ HTTP/1.1 header. So as far as the test is concerned, everything is OK,
+ but it failed to notice the un-redacted lines.
+
+ We can make this test (and the other related ones) more robust by adding
+ "-i" to grep case-insensitively. This isn't really doing anything for
+ now, since we're not actually speaking HTTP/2, but it future-proofs the
+ tests for a day when we do (either we add explicit HTTP/2 test support,
+ or it's eventually enabled by default by our Apache+curl test setup).
+ And it doesn't hurt in the meantime for the tests to be more careful.
+
+ The change to use "grep -i", coupled with the changes to use HTTP/2
+ shown above, causes the test to fail with the current code, and pass
+ after this patch is applied.
And finally, there's one other way to demonstrate the issue (and how I
actually found it originally). Looking at GIT_TRACE_CURL output against
prev parent reply other threads:[~2021-09-22 22:14 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-21 18:41 [PATCH] http: match headers case-insensitively when redacting Jeff King
2021-09-21 18:47 ` Jeff King
2021-09-21 20:14 ` Carlo Arenas
2021-09-21 20:40 ` Jeff King
2021-09-21 22:00 ` Daniel Stenberg
2021-09-22 2:32 ` Jeff King
2021-09-21 19:06 ` Eric Sunshine
2021-09-21 19:14 ` Jeff King
2021-09-22 19:10 ` Junio C Hamano
2021-09-21 21:20 ` Taylor Blau
2021-09-22 2:30 ` Jeff King
2021-09-22 2:32 ` Bagas Sanjaya
2021-09-22 20:11 ` Jeff King
2021-09-23 1:22 ` Ævar Arnfjörð Bjarmason
2021-09-23 21:56 ` Jeff King
2021-09-22 19:19 ` Junio C Hamano
2021-09-22 20:09 ` Jeff King
2021-09-22 20:51 ` Junio C Hamano
2021-09-22 21:18 ` Jeff King
2021-09-22 21:42 ` Junio C Hamano
2021-09-22 22:11 ` [PATCH v2] " Jeff King
2021-09-22 22:14 ` Jeff King [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YUuqrNXdgUR5thl5@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=daniel@haxx.se \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).