[PATCH v3 0/2] http: redact curl h2h3 headers in info

["Glen Choo via GitGitGadget" <gitgitgadget@gmail.com>]

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

-- 
gitgitgadget