| Message ID | pull.1377.v3.git.git.1668206106.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | http: redact curl h2h3 headers in info | expand |
On Fri, Nov 11, 2022 at 10:35:04PM +0000, Glen Choo via GitGitGadget wrote: > Big thanks to Peff for the feedback last round. I've incorporated all of the > suggestions. I'm so glad that we finally have tests here :) > > Changes in v3: > > * Add the HTTP2 test from [1] to the start of the series > * Drop struct strbuf inner in favor of doing work on the original strbuf Thanks, both look good to me (unsurprisingly ;) ). -Peff
On Mon, Nov 14, 2022 at 05:33:26PM -0500, Jeff King wrote: > On Fri, Nov 11, 2022 at 10:35:04PM +0000, Glen Choo via GitGitGadget wrote: > > > Big thanks to Peff for the feedback last round. I've incorporated all of the > > suggestions. I'm so glad that we finally have tests here :) > > > > Changes in v3: > > > > * Add the HTTP2 test from [1] to the start of the series > > * Drop struct strbuf inner in favor of doing work on the original strbuf > > Thanks, both look good to me (unsurprisingly ;) ). Thanks, both. Will start merging this down in this evening's push-out. Thanks, Taylor
Big thanks to Peff for the feedback last round. I've incorporated all of the suggestions. I'm so glad that we finally have tests here :) Changes in v3: * Add the HTTP2 test from [1] to the start of the series * Drop struct strbuf inner in favor of doing work on the original strbuf Changes in v2: * Describe the redacted string in comments. * Return 1 for "redactions have happened". * Fix a leak of the "inner" strbuf. * Rename function, fix typo. [1] https://lore.kernel.org/git/Y25hDr7aHvKnxso3@coredump.intra.peff.net Glen Choo (1): http: redact curl h2h3 headers in info Jeff King (1): t: run t5551 tests with both HTTP and HTTP/2 http.c | 47 +++++++++++++++++++++++++++---- t/lib-httpd.sh | 5 ++++ t/lib-httpd/apache.conf | 19 +++++++++++-- t/t5551-http-fetch-smart.sh | 13 +++++++-- t/t5559-http-fetch-smart-http2.sh | 4 +++ 5 files changed, 77 insertions(+), 11 deletions(-) create mode 100755 t/t5559-http-fetch-smart-http2.sh base-commit: c03801e19cb8ab36e9c0d17ff3d5e0c3b0f24193 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1377%2Fchooglen%2Fhttp%2Fredact-h2h3-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1377/chooglen/http/redact-h2h3-v3 Pull-Request: https://github.com/git/git/pull/1377 Range-diff vs v2: -: ----------- > 1: 09194dba8cd t: run t5551 tests with both HTTP and HTTP/2 1: a8c35ff4ddf ! 2: bb5df1a48b9 http: redact curl h2h3 headers in info @@ Commit message With GIT_TRACE_CURL=1 or GIT_CURL_VERBOSE=1, sensitive headers like "Authorization" and "Cookie" get redacted. However, since [1], curl's - h2h3 module also prints headers in its "info", which don't get redacted. - For example, + h2h3 module (invoked when using HTTP/2) also prints headers in its + "info", which don't get redacted. For example, echo 'github.com TRUE / FALSE 1698960413304 o foo=bar' >cookiefile && GIT_TRACE_CURL=1 GIT_TRACE_CURL_NO_DATA=1 git \ @@ Commit message 23:04:16.920562 http.c:637 => Send header: cookie: o=<redacted> Teach http.c to check for h2h3 headers in info and redact them using the - existing header redaction logic. + existing header redaction logic. This fixes the broken redaction logic + that we noted in the previous commit, so mark the redaction tests as + passing under HTTP2. [1] https://github.com/curl/curl/commit/f8c3724aa90472c0e617ddbbc420aa199971eb77 + Helped-by: Jeff King <peff@peff.net> Signed-off-by: Glen Choo <chooglen@google.com> ## http.c ## @@ http.c: static void set_curl_keepalive(CURL *c) -static void redact_sensitive_header(struct strbuf *header) +/* Return 1 if redactions have been made, 0 otherwise. */ -+static int redact_sensitive_header(struct strbuf *header) ++static int redact_sensitive_header(struct strbuf *header, size_t offset) { + int ret = 0; const char *sensitive_header; if (trace_curl_redact && +- (skip_iprefix(header->buf, "Authorization:", &sensitive_header) || +- skip_iprefix(header->buf, "Proxy-Authorization:", &sensitive_header))) { ++ (skip_iprefix(header->buf + offset, "Authorization:", &sensitive_header) || ++ skip_iprefix(header->buf + offset, "Proxy-Authorization:", &sensitive_header))) { + /* The first token is the type, which is OK to log */ + while (isspace(*sensitive_header)) + sensitive_header++; @@ http.c: static void redact_sensitive_header(struct strbuf *header) /* Everything else is opaque and possibly sensitive */ strbuf_setlen(header, sensitive_header - header->buf); strbuf_addstr(header, " <redacted>"); + ret = 1; } else if (trace_curl_redact && - skip_iprefix(header->buf, "Cookie:", &sensitive_header)) { +- skip_iprefix(header->buf, "Cookie:", &sensitive_header)) { ++ skip_iprefix(header->buf + offset, "Cookie:", &sensitive_header)) { struct strbuf redacted_header = STRBUF_INIT; + const char *cookie; + @@ http.c: static void redact_sensitive_header(struct strbuf *header) strbuf_setlen(header, sensitive_header - header->buf); @@ http.c: static void redact_sensitive_header(struct strbuf *header) + */ + if (trace_curl_redact && + skip_iprefix(header->buf, "h2h3 [", &sensitive_header)) { -+ struct strbuf inner = STRBUF_INIT; -+ -+ /* Drop the trailing "]" */ -+ strbuf_add(&inner, sensitive_header, strlen(sensitive_header) - 1); -+ if (redact_sensitive_header(&inner)) { -+ strbuf_setlen(header, strlen("h2h3 [")); -+ strbuf_addbuf(header, &inner); ++ if (redact_sensitive_header(header, sensitive_header - header->buf)) { ++ /* redaction ate our closing bracket */ + strbuf_addch(header, ']'); + } -+ -+ strbuf_release(&inner); } } +@@ http.c: static void curl_dump_header(const char *text, unsigned char *ptr, size_t size, + + for (header = headers; *header; header++) { + if (hide_sensitive_header) +- redact_sensitive_header(*header); ++ redact_sensitive_header(*header, 0); + strbuf_insertstr((*header), 0, text); + strbuf_insertstr((*header), strlen(text), ": "); + strbuf_rtrim((*header)); @@ http.c: static void curl_dump_data(const char *text, unsigned char *ptr, size_t size) strbuf_release(&out); } @@ http.c: static int curl_trace(CURL *handle, curl_infotype type, char *data, size break; case CURLINFO_HEADER_OUT: text = "=> Send header"; + + ## t/t5551-http-fetch-smart.sh ## +@@ t/t5551-http-fetch-smart.sh: test_expect_success 'redirects send auth to new location' ' + expect_askpass both user@host auth/smart/repo.git + ' + +-test_expect_success !HTTP2 'GIT_TRACE_CURL redacts auth details' ' ++test_expect_success 'GIT_TRACE_CURL redacts auth details' ' + rm -rf redact-auth trace && + set_askpass user@host pass@host && + GIT_TRACE_CURL="$(pwd)/trace" git clone --bare "$HTTPD_URL/auth/smart/repo.git" redact-auth && +@@ t/t5551-http-fetch-smart.sh: test_expect_success !HTTP2 'GIT_TRACE_CURL redacts auth details' ' + grep -i "Authorization: Basic <redacted>" trace + ' + +-test_expect_success !HTTP2 'GIT_CURL_VERBOSE redacts auth details' ' ++test_expect_success 'GIT_CURL_VERBOSE redacts auth details' ' + rm -rf redact-auth trace && + set_askpass user@host pass@host && + GIT_CURL_VERBOSE=1 git clone --bare "$HTTPD_URL/auth/smart/repo.git" redact-auth 2>trace && +@@ t/t5551-http-fetch-smart.sh: test_expect_success 'fetch by SHA-1 without tag following' ' + --no-tags origin $(cat bar_hash) + ' + +-test_expect_success !HTTP2 'cookies are redacted by default' ' ++test_expect_success 'cookies are redacted by default' ' + rm -rf clone && + echo "Set-Cookie: Foo=1" >cookies && + echo "Set-Cookie: Bar=2" >>cookies &&