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

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

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

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

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