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

[Jeff King <peff@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