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

From: Jeff King <peff@peff.net>
Date: Sep 22, 2021, 20:09 UTC
Message-ID: <YUuNXOb5blV7iN6P@coredump.intra.peff.net>
In-Reply-To: <xmqq8rzo770h.fsf@gitster.g>

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

Show 18 quoted lines

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

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

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

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

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

I get (with some extraneous headers omitted):

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

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

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

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

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

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

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

-Peff