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

From: Taylor Blau <me@ttaylorr.com>
Date: Sep 21, 2021, 21:20 UTC
Message-ID: <YUpMreNwBDSygFSf@nand.local>
In-Reply-To: <YUonS1uoZlZEt+Yd@coredump.intra.peff.net>

On Tue, Sep 21, 2021 at 02:41:15PM -0400, Jeff King wrote:

Show 27 quoted lines

> 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 ;).

Show 6 quoted lines

>   - 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.

Show 17 quoted lines

> 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