Jeff King [off-list ref] 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.
On Wed, Sep 22, 2021 at 12:19:26PM -0700, Junio C Hamano wrote:
Jeff King [off-list ref] writes:
quoted
# 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