{"thread":{"id":"58774","subject":"[PATCH] http: redact curl h2h3 headers in info","startedAt":"2022-11-09T00:52:39Z","lastAt":"2022-11-14T22:43:50Z","messageCount":24,"participants":["Glen Choo via GitGitGadget","Taylor Blau","Glen Choo","Jeff King","Emily Shaffer","Ævar Arnfjörð Bjarmason","Jeff King via GitGitGadget"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"466928","messageId":"pull.1377.git.git.1667955151994.gitgitgadget@gmail.com","threadId":"58774","inReplyTo":null,"subject":"[PATCH] http: redact curl h2h3 headers in info","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-09T00:52:31Z","receivedAt":"2022-11-09T00:52:39Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo \n\nWith GIT_TRACE_CURL=1 or GIT_CURL_VERBOSE=1, sensitive headers like\n\"Authorization\" and \"Cookie\" get redacted. However, since [1], curl's\nh2h3 module also prints headers in its \"info\", which don't get redacted.\nFor example,\n\n echo 'github.com\tTRUE\t/\tFALSE\t1698960413304\to\tfoo=bar' >cookiefile &&\n GIT_TRACE_CURL=1 GIT_TRACE_CURL_NO_DATA=1 git \\\n -c 'http.cookiefile=cookiefile' \\\n -c 'http.version=' \\\n ls-remote https://github.com/git/git refs/heads/main 2>output &&\n grep 'cookie' output\n\nproduces output like:\n\n 23:04:16.920495 http.c:678 == Info: h2h3 [cookie: o=foo=bar]\n 23:04:16.920562 http.c:637 => Send header: cookie: o=\n\nTeach http.c to check for h2h3 headers in info and redact them using the\nexisting header redaction logic.\n\n[1] https://github.com/curl/curl/commit/f8c3724aa90472c0e617ddbbc420aa199971eb77\n\nSigned-off-by: Glen Choo \n---\n http: redact curl h2h3 headers in info\n \n I initially sent this to the security list, but the general impression\n is that this isn't sensitive enough for an embargoed fix, so this is\n better discussed in the open instead.\n \n Since this comes from curl's HTTP2.0/3.0 module, this can be mitigated\n by setting http.version to 1.X, e.g. \"git -c http.version=HTTP/1.1\".\n \n According to [1], the susceptible curl versions appear to be 7.86.0,\n 7.85.0, 7.84.0, 7.83.1, 7.83.0, 7.82.0, but I'm not sure which platforms\n are vulnerable.\n \n This patch fixes the issue on my machine running curl 7.85.0, so I think\n it is okay to merge as-is. That said, I would strongly prefer to add\n tests, but I haven't figured out how. In particular:\n \n * Do we have a way of using HTTP/2.0 in our tests? A cursory glance at\n our httpd config suggests that we only use HTTP/1.1.\n * How could we set up end-to-end tests to ensure that we're testing\n this against affected versions of curl? To avoid regressions, I'd\n also prefer to test against future versions of curl too.\n \n [1]\n https://github.com/curl/curl/commit/f8c3724aa90472c0e617ddbbc420aa199971eb77\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1377%2Fchooglen%2Fhttp%2Fredact-h2h3-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1377/chooglen/http/redact-h2h3-v1\nPull-Request: https://github.com/git/git/pull/1377\n\n http.c | 40 ++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 38 insertions(+), 2 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 5d0502f51fd..cbcc7c3f5b6 100644\n--- a/http.c\n+++ b/http.c\n@@ -560,8 +560,10 @@ static void set_curl_keepalive(CURL *c)\n }\n #endif\n \n-static void redact_sensitive_header(struct strbuf *header)\n+/* Return 0 if redactions been made, 1 otherwise. */\n+static int redact_sensitive_header(struct strbuf *header)\n {\n+\tint ret = 1;\n \tconst char *sensitive_header;\n \n \tif (trace_curl_redact &&\n@@ -575,6 +577,7 @@ static void redact_sensitive_header(struct strbuf *header)\n \t\t/* Everything else is opaque and possibly sensitive */\n \t\tstrbuf_setlen(header, sensitive_header - header->buf);\n \t\tstrbuf_addstr(header, \" \");\n+\t\tret = 0;\n \t} else if (trace_curl_redact &&\n \t\t skip_iprefix(header->buf, \"Cookie:\", &sensitive_header)) {\n \t\tstruct strbuf redacted_header = STRBUF_INIT;\n@@ -612,6 +615,27 @@ static void redact_sensitive_header(struct strbuf *header)\n \n \t\tstrbuf_setlen(header, sensitive_header - header->buf);\n \t\tstrbuf_addbuf(header, &redacted_header);\n+\t\tret = 0;\n+\t}\n+\treturn ret;\n+}\n+\n+/* Redact headers in info */\n+static void redact_sensitive_info_header(struct strbuf *header)\n+{\n+\tconst char *sensitive_header;\n+\n+\tif (trace_curl_redact &&\n+\t skip_iprefix(header->buf, \"h2h3 [\", &sensitive_header)) {\n+\t\tstruct strbuf inner = STRBUF_INIT;\n+\n+\t\t/* Drop the trailing \"]\" */\n+\t\tstrbuf_add(&inner, sensitive_header, strlen(sensitive_header) - 1);\n+\t\tif (!redact_sensitive_header(&inner)) {\n+\t\t\tstrbuf_setlen(header, strlen(\"h2h3 [\"));\n+\t\t\tstrbuf_addbuf(header, &inner);\n+\t\t\tstrbuf_addch(header, ']');\n+\t\t}\n \t}\n }\n \n@@ -668,6 +692,18 @@ static void curl_dump_data(const char *text, unsigned char *ptr, size_t size)\n \tstrbuf_release(&out);\n }\n \n+static void curl_print_info(char *data, size_t size)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tstrbuf_add(&buf, data, size);\n+\n+\tredact_sensitive_info_header(&buf);\n+\ttrace_printf_key(&trace_curl, \"== Info: %s\", buf.buf);\n+\n+\tstrbuf_release(&buf);\n+}\n+\n static int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size, void *userp)\n {\n \tconst char *text;\n@@ -675,7 +711,7 @@ static int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size,\n \n \tswitch (type) {\n \tcase CURLINFO_TEXT:\n-\t\ttrace_printf_key(&trace_curl, \"== Info: %s\", data);\n+\t\tcurl_print_info(data, size);\n \t\tbreak;\n \tcase CURLINFO_HEADER_OUT:\n \t\ttext = \"=> Send header\";\n\nbase-commit: c03801e19cb8ab36e9c0d17ff3d5e0c3b0f24193\n-- \ngitgitgadget\n"},{"id":"467030","messageId":"Y2xnUyEoCFmbPtUI@nand.local","threadId":"58774","inReplyTo":"pull.1377.git.git.1667955151994.gitgitgadget@gmail.com","subject":"Re: [PATCH] http: redact curl h2h3 headers in info","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-10T02:52:03Z","receivedAt":"2022-11-10T02:52:09Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Nov 09, 2022 at 12:52:31AM +0000, Glen Choo via GitGitGadget wrote:\n> From: Glen Choo \n>\n> With GIT_TRACE_CURL=1 or GIT_CURL_VERBOSE=1, sensitive headers like\n> \"Authorization\" and \"Cookie\" get redacted. However, since [1], curl's\n> h2h3 module also prints headers in its \"info\", which don't get redacted.\n> For example,\n>\n> echo 'github.com\tTRUE\t/\tFALSE\t1698960413304\to\tfoo=bar' >cookiefile &&\n> GIT_TRACE_CURL=1 GIT_TRACE_CURL_NO_DATA=1 git \\\n> -c 'http.cookiefile=cookiefile' \\\n> -c 'http.version=' \\\n> ls-remote https://github.com/git/git refs/heads/main 2>output &&\n> grep 'cookie' output\n>\n> produces output like:\n>\n> 23:04:16.920495 http.c:678 == Info: h2h3 [cookie: o=foo=bar]\n> 23:04:16.920562 http.c:637 => Send header: cookie: o=\n\nOops. Thanks for sharing this patch, I agree that the general approach\nmakes sense.\n\n> I initially sent this to the security list, but the general impression\n> is that this isn't sensitive enough for an embargoed fix, so this is\n> better discussed in the open instead.\n\nIndeed, this only really matters if the would-be victim is convinced to\nsomehow share the contents of their GIT_CURL_VEROBSE or GIT_TRACE_CURL\noutput. So I don't think there are any security implications here,\nthough I appreciate your caution in double checking with the\ngit-security list first.\n\n> * How could we set up end-to-end tests to ensure that we're testing\n> this against affected versions of curl? To avoid regressions, I'd\n> also prefer to test against future versions of curl too.\n\nDoes that necessarily matter? We want to make sure that we don't see\nsensitive headers from the h2h3 module with any version of cURL, no?\n>\n> +/* Redact headers in info */\n> +static void redact_sensitive_info_header(struct strbuf *header)\n> +{\n> +\tconst char *sensitive_header;\n> +\n> +\tif (trace_curl_redact &&\n> +\t skip_iprefix(header->buf, \"h2h3 [\", &sensitive_header)) {\n> +\t\tstruct strbuf inner = STRBUF_INIT;\n> +\n> +\t\t/* Drop the trailing \"]\" */\n> +\t\tstrbuf_add(&inner, sensitive_header, strlen(sensitive_header) - 1);\n\nMakes sense. If the incoming header contains the \"h2h3 [...]\" marker,\nand we are redacting sensitive headers, and there are header to redact,\nredact them.\n\n> +\t\tif (!redact_sensitive_header(&inner)) {\n> +\t\t\tstrbuf_setlen(header, strlen(\"h2h3 [\"));\n> +\t\t\tstrbuf_addbuf(header, &inner);\n\nThis leaks inner.buf, no?\n\n> +\t\t\tstrbuf_addch(header, ']');\n> +\t\t}\n> \t}\n> }\n\nThe rest looks good.\n\nThanks,\nTaylor\n"},{"id":"467074","messageId":"kl6lpmdu3dl5.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58774","inReplyTo":"Y2xnUyEoCFmbPtUI@nand.local","subject":"Re: [PATCH] http: redact curl h2h3 headers in info","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-11-10T17:48:38Z","receivedAt":"2022-11-10T17:48:51Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Taylor Blau writes:\n\n>> * How could we set up end-to-end tests to ensure that we're testing\n>> this against affected versions of curl? To avoid regressions, I'd\n>> also prefer to test against future versions of curl too.\n>\n> Does that necessarily matter? We want to make sure that we don't see\n> sensitive headers from the h2h3 module with any version of cURL, no?\n\nIt would help, but it might not be worth setting up infrastructure for\njust this use case alone. Given the various platforms running tests\nagainst the Git codebase, we probably get close to a representative\nsample of the population with enough time.\n\nI think it would be more important to have tests against HTTP/2.0. If we\ndid, we probably would have already caught this, e.g.\nt/t5551-http-fetch-smart.sh:'GIT_TRACE_CURL redacts auth details' and\nfriends.\n\n>> +\t\tif (!redact_sensitive_header(&inner)) {\n>> +\t\t\tstrbuf_setlen(header, strlen(\"h2h3 [\"));\n>> +\t\t\tstrbuf_addbuf(header, &inner);\n>\n> This leaks inner.buf, no?\n\nAh, you're right. Thanks.\n\n"},{"id":"467089","messageId":"Y21yPIDmwB53e+aj@coredump.intra.peff.net","threadId":"58774","inReplyTo":"kl6lpmdu3dl5.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH] http: redact curl h2h3 headers in info","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-10T21:50:52Z","receivedAt":"2022-11-10T21:50:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 10, 2022 at 09:48:38AM -0800, Glen Choo wrote:\n\n> Taylor Blau writes:\n> \n> >> * How could we set up end-to-end tests to ensure that we're testing\n> >> this against affected versions of curl? To avoid regressions, I'd\n> >> also prefer to test against future versions of curl too.\n> >\n> > Does that necessarily matter? We want to make sure that we don't see\n> > sensitive headers from the h2h3 module with any version of cURL, no?\n> \n> It would help, but it might not be worth setting up infrastructure for\n> just this use case alone. Given the various platforms running tests\n> against the Git codebase, we probably get close to a representative\n> sample of the population with enough time.\n> \n> I think it would be more important to have tests against HTTP/2.0. If we\n> did, we probably would have already caught this, e.g.\n> t/t5551-http-fetch-smart.sh:'GIT_TRACE_CURL redacts auth details' and\n> friends.\n\nThere's some discussion in b66c77a64e (http: match headers\ncase-insensitively when redacting, 2021-09-22) about testing with\nHTTP/2. Which ironically is basically this exact same bug in a different\nform. ;)\n\nThe short answer is that it's do-able, but probably there are some\nheadaches to make it work portably.\n\nI agree with you that trying various curl versions isn't worth doing. If\nenough people/platforms run Git's suite, one of them will eventually\nsee the problem.\n\n-Peff\n"},{"id":"467090","messageId":"Y21zz1HYXzyyfwqy@google.com","threadId":"58774","inReplyTo":"pull.1377.git.git.1667955151994.gitgitgadget@gmail.com","subject":"Re: [PATCH] http: redact curl h2h3 headers in info","fromName":"Emily Shaffer","fromEmail":"nasamuffin@google.com","sentAt":"2022-11-10T21:57:35Z","receivedAt":"2022-11-10T21:57:44Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Wed, Nov 09, 2022 at 12:52:31AM +0000, Glen Choo via GitGitGadget wrote:\n> \n> With GIT_TRACE_CURL=1 or GIT_CURL_VERBOSE=1, sensitive headers like\n> \"Authorization\" and \"Cookie\" get redacted. However, since [1], curl's\n> h2h3 module also prints headers in its \"info\", which don't get redacted.\n> For example,\n> \n> echo 'github.com\tTRUE\t/\tFALSE\t1698960413304\to\tfoo=bar' >cookiefile &&\n> GIT_TRACE_CURL=1 GIT_TRACE_CURL_NO_DATA=1 git \\\n> -c 'http.cookiefile=cookiefile' \\\n> -c 'http.version=' \\\n> ls-remote https://github.com/git/git refs/heads/main 2>output &&\n> grep 'cookie' output\n> \n> produces output like:\n> \n> 23:04:16.920495 http.c:678 == Info: h2h3 [cookie: o=foo=bar]\n> 23:04:16.920562 http.c:637 => Send header: cookie: o=\n> \n> Teach http.c to check for h2h3 headers in info and redact them using the\n> existing header redaction logic.\n> \n> [1] https://github.com/curl/curl/commit/f8c3724aa90472c0e617ddbbc420aa199971eb77\n> \n> Signed-off-by: Glen Choo \n> ---\n> http: redact curl h2h3 headers in info\n> \n> I initially sent this to the security list, but the general impression\n> is that this isn't sensitive enough for an embargoed fix, so this is\n> better discussed in the open instead.\n> \n> Since this comes from curl's HTTP2.0/3.0 module, this can be mitigated\n> by setting http.version to 1.X, e.g. \"git -c http.version=HTTP/1.1\".\n> \n> According to [1], the susceptible curl versions appear to be 7.86.0,\n> 7.85.0, 7.84.0, 7.83.1, 7.83.0, 7.82.0, but I'm not sure which platforms\n> are vulnerable.\n> \n> This patch fixes the issue on my machine running curl 7.85.0, so I think\n> it is okay to merge as-is. That said, I would strongly prefer to add\n> tests, but I haven't figured out how. In particular:\n> \n> * Do we have a way of using HTTP/2.0 in our tests? A cursory glance at\n> our httpd config suggests that we only use HTTP/1.1.\n> * How could we set up end-to-end tests to ensure that we're testing\n> this against affected versions of curl? To avoid regressions, I'd\n> also prefer to test against future versions of curl too.\n> \n> [1]\n> https://github.com/curl/curl/commit/f8c3724aa90472c0e617ddbbc420aa199971eb77\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1377%2Fchooglen%2Fhttp%2Fredact-h2h3-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1377/chooglen/http/redact-h2h3-v1\n> Pull-Request: https://github.com/git/git/pull/1377\n> \n> http.c | 40 ++++++++++++++++++++++++++++++++++++++--\n> 1 file changed, 38 insertions(+), 2 deletions(-)\n> \n> diff --git a/http.c b/http.c\n> index 5d0502f51fd..cbcc7c3f5b6 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -560,8 +560,10 @@ static void set_curl_keepalive(CURL *c)\n> }\n> #endif\n> \n> -static void redact_sensitive_header(struct strbuf *header)\n> +/* Return 0 if redactions been made, 1 otherwise. */\n\nDoes it make sense to reverse the retval here?\n\n`if (!redact_sensitive_header())` sounds like \"if not redacted, ...\" -\nbut here it means the opposite, right?\n\n> +static int redact_sensitive_header(struct strbuf *header)\n> {\n> +\tint ret = 1;\n> \tconst char *sensitive_header;\n> \n> \tif (trace_curl_redact &&\n> @@ -575,6 +577,7 @@ static void redact_sensitive_header(struct strbuf *header)\n> \t\t/* Everything else is opaque and possibly sensitive */\n> \t\tstrbuf_setlen(header, sensitive_header - header->buf);\n> \t\tstrbuf_addstr(header, \" \");\n> +\t\tret = 0;\n> \t} else if (trace_curl_redact &&\n> \t\t skip_iprefix(header->buf, \"Cookie:\", &sensitive_header)) {\n> \t\tstruct strbuf redacted_header = STRBUF_INIT;\n> @@ -612,6 +615,27 @@ static void redact_sensitive_header(struct strbuf *header)\n> \n> \t\tstrbuf_setlen(header, sensitive_header - header->buf);\n> \t\tstrbuf_addbuf(header, &redacted_header);\n> +\t\tret = 0;\n> +\t}\n> +\treturn ret;\n> +}\n> +\n> +/* Redact headers in info */\n> +static void redact_sensitive_info_header(struct strbuf *header)\n> +{\n> +\tconst char *sensitive_header;\n> +\n> +\tif (trace_curl_redact &&\n> +\t skip_iprefix(header->buf, \"h2h3 [\", &sensitive_header)) {\n> +\t\tstruct strbuf inner = STRBUF_INIT;\n> +\n> +\t\t/* Drop the trailing \"]\" */\n> +\t\tstrbuf_add(&inner, sensitive_header, strlen(sensitive_header) - 1);\n> +\t\tif (!redact_sensitive_header(&inner)) {\n> +\t\t\tstrbuf_setlen(header, strlen(\"h2h3 [\"));\n> +\t\t\tstrbuf_addbuf(header, &inner);\n> +\t\t\tstrbuf_addch(header, ']');\n\nI'd really like some more comments in this function - even just one\ndescribing the string we're trying to redact, or showing a sample line.\nNavigating string parsing is always a bit difficult.\n\n> +\t\t}\n> \t}\n> }\n> \n> @@ -668,6 +692,18 @@ static void curl_dump_data(const char *text, unsigned char *ptr, size_t size)\n> \tstrbuf_release(&out);\n> }\n> \n> +static void curl_print_info(char *data, size_t size)\n\nNit: Every other helper in this file calls it _dump_, so should this\nalso say _dump_ instead of _print_?\n\n> +{\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\n> +\tstrbuf_add(&buf, data, size);\n> +\n> +\tredact_sensitive_info_header(&buf);\n> +\ttrace_printf_key(&trace_curl, \"== Info: %s\", buf.buf);\n> +\n> +\tstrbuf_release(&buf);\n> +}\n> +\n> static int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size, void *userp)\n> {\n> \tconst char *text;\n> @@ -675,7 +711,7 @@ static int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size,\n> \n> \tswitch (type) {\n> \tcase CURLINFO_TEXT:\n> -\t\ttrace_printf_key(&trace_curl, \"== Info: %s\", data);\n> +\t\tcurl_print_info(data, size);\n> \t\tbreak;\n> \tcase CURLINFO_HEADER_OUT:\n> \t\ttext = \"=> Send header\";\n> \n> base-commit: c03801e19cb8ab36e9c0d17ff3d5e0c3b0f24193\n\nOtherwise functionally it seems fine to me. case CURLINFO_TEXT is the\none case that's not already using a curl_dump_* helper, so we're adding\none, and to that helper we're adding a call out to\nredact_sensitive_header().\n\nThanks.\n - Emily\n\n> -- \n> gitgitgadget\n"},{"id":"467091","messageId":"kl6lmt8y31ak.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58774","inReplyTo":"Y21zz1HYXzyyfwqy@google.com","subject":"Re: [PATCH] http: redact curl h2h3 headers in info","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-11-10T22:14:11Z","receivedAt":"2022-11-10T22:14:23Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Emily Shaffer writes:\n\n> On Wed, Nov 09, 2022 at 12:52:31AM +0000, Glen Choo via GitGitGadget wrote:\n>> \n>> With GIT_TRACE_CURL=1 or GIT_CURL_VERBOSE=1, sensitive headers like\n>> \"Authorization\" and \"Cookie\" get redacted. However, since [1], curl's\n>> h2h3 module also prints headers in its \"info\", which don't get redacted.\n>> For example,\n>> \n>> echo 'github.com\tTRUE\t/\tFALSE\t1698960413304\to\tfoo=bar' >cookiefile &&\n>> GIT_TRACE_CURL=1 GIT_TRACE_CURL_NO_DATA=1 git \\\n>> -c 'http.cookiefile=cookiefile' \\\n>> -c 'http.version=' \\\n>> ls-remote https://github.com/git/git refs/heads/main 2>output &&\n>> grep 'cookie' output\n>> \n>> produces output like:\n>> \n>> 23:04:16.920495 http.c:678 == Info: h2h3 [cookie: o=foo=bar]\n>> 23:04:16.920562 http.c:637 => Send header: cookie: o=\n>> \n>> Teach http.c to check for h2h3 headers in info and redact them using the\n>> existing header redaction logic.\n>> \n>> [1] https://github.com/curl/curl/commit/f8c3724aa90472c0e617ddbbc420aa199971eb77\n>> \n>> Signed-off-by: Glen Choo \n>> ---\n>> http: redact curl h2h3 headers in info\n>> \n>> I initially sent this to the security list, but the general impression\n>> is that this isn't sensitive enough for an embargoed fix, so this is\n>> better discussed in the open instead.\n>> \n>> Since this comes from curl's HTTP2.0/3.0 module, this can be mitigated\n>> by setting http.version to 1.X, e.g. \"git -c http.version=HTTP/1.1\".\n>> \n>> According to [1], the susceptible curl versions appear to be 7.86.0,\n>> 7.85.0, 7.84.0, 7.83.1, 7.83.0, 7.82.0, but I'm not sure which platforms\n>> are vulnerable.\n>> \n>> This patch fixes the issue on my machine running curl 7.85.0, so I think\n>> it is okay to merge as-is. That said, I would strongly prefer to add\n>> tests, but I haven't figured out how. In particular:\n>> \n>> * Do we have a way of using HTTP/2.0 in our tests? A cursory glance at\n>> our httpd config suggests that we only use HTTP/1.1.\n>> * How could we set up end-to-end tests to ensure that we're testing\n>> this against affected versions of curl? To avoid regressions, I'd\n>> also prefer to test against future versions of curl too.\n>> \n>> [1]\n>> https://github.com/curl/curl/commit/f8c3724aa90472c0e617ddbbc420aa199971eb77\n>> \n>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1377%2Fchooglen%2Fhttp%2Fredact-h2h3-v1\n>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1377/chooglen/http/redact-h2h3-v1\n>> Pull-Request: https://github.com/git/git/pull/1377\n>> \n>> http.c | 40 ++++++++++++++++++++++++++++++++++++++--\n>> 1 file changed, 38 insertions(+), 2 deletions(-)\n>> \n>> diff --git a/http.c b/http.c\n>> index 5d0502f51fd..cbcc7c3f5b6 100644\n>> --- a/http.c\n>> +++ b/http.c\n>> @@ -560,8 +560,10 @@ static void set_curl_keepalive(CURL *c)\n>> }\n>> #endif\n>> \n>> -static void redact_sensitive_header(struct strbuf *header)\n>> +/* Return 0 if redactions been made, 1 otherwise. */\n>\n> Does it make sense to reverse the retval here?\n>\n> `if (!redact_sensitive_header())` sounds like \"if not redacted, ...\" -\n> but here it means the opposite, right?\n\nI struggled with this for a bit since I wasn't sure what the convention\nis here. Enumerating some off the top of my head, we have:\n\n- For 'booleans', we \"0\" for false and \"1\" for true (e.g.\n starts_with()).\n- For functions that may fail with error, we have \"0\" for success and\n nonzero to signal the failure type (e.g. strbuf_getdelim()).\n- For functions that don't fail we have \"0\" for \"nothing was done\" and\n \"1\" for something was done (e.g. skip_prefix()).\n\n(Tangent: from a readability perspective, this is pretty poor. I need to\nknow beforehand whether or not the function may fail with error before I\nknow what the return value means?)\n\nThis probably falls into the last category, so for consistency, I think\nthis should return \"1\" for \"redactions have happened\" (as you\nsuggested).\n\n>> +static int redact_sensitive_header(struct strbuf *header)\n>> {\n>> +\tint ret = 1;\n>> \tconst char *sensitive_header;\n>> \n>> \tif (trace_curl_redact &&\n>> @@ -575,6 +577,7 @@ static void redact_sensitive_header(struct strbuf *header)\n>> \t\t/* Everything else is opaque and possibly sensitive */\n>> \t\tstrbuf_setlen(header, sensitive_header - header->buf);\n>> \t\tstrbuf_addstr(header, \" \");\n>> +\t\tret = 0;\n>> \t} else if (trace_curl_redact &&\n>> \t\t skip_iprefix(header->buf, \"Cookie:\", &sensitive_header)) {\n>> \t\tstruct strbuf redacted_header = STRBUF_INIT;\n>> @@ -612,6 +615,27 @@ static void redact_sensitive_header(struct strbuf *header)\n>> \n>> \t\tstrbuf_setlen(header, sensitive_header - header->buf);\n>> \t\tstrbuf_addbuf(header, &redacted_header);\n>> +\t\tret = 0;\n>> +\t}\n>> +\treturn ret;\n>> +}\n>> +\n>> +/* Redact headers in info */\n>> +static void redact_sensitive_info_header(struct strbuf *header)\n>> +{\n>> +\tconst char *sensitive_header;\n>> +\n>> +\tif (trace_curl_redact &&\n>> +\t skip_iprefix(header->buf, \"h2h3 [\", &sensitive_header)) {\n>> +\t\tstruct strbuf inner = STRBUF_INIT;\n>> +\n>> +\t\t/* Drop the trailing \"]\" */\n>> +\t\tstrbuf_add(&inner, sensitive_header, strlen(sensitive_header) - 1);\n>> +\t\tif (!redact_sensitive_header(&inner)) {\n>> +\t\t\tstrbuf_setlen(header, strlen(\"h2h3 [\"));\n>> +\t\t\tstrbuf_addbuf(header, &inner);\n>> +\t\t\tstrbuf_addch(header, ']');\n>\n> I'd really like some more comments in this function - even just one\n> describing the string we're trying to redact, or showing a sample line.\n> Navigating string parsing is always a bit difficult.\n\nAh yes, I should include a description of the string.\n\n>> +\t\t}\n>> \t}\n>> }\n>> \n>> @@ -668,6 +692,18 @@ static void curl_dump_data(const char *text, unsigned char *ptr, size_t size)\n>> \tstrbuf_release(&out);\n>> }\n>> \n>> +static void curl_print_info(char *data, size_t size)\n>\n> Nit: Every other helper in this file calls it _dump_, so should this\n> also say _dump_ instead of _print_?\n\nSure, I have no opinion here, so I'll do that.\n\n>> +{\n>> +\tstruct strbuf buf = STRBUF_INIT;\n>> +\n>> +\tstrbuf_add(&buf, data, size);\n>> +\n>> +\tredact_sensitive_info_header(&buf);\n>> +\ttrace_printf_key(&trace_curl, \"== Info: %s\", buf.buf);\n>> +\n>> +\tstrbuf_release(&buf);\n>> +}\n>> +\n>> static int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size, void *userp)\n>> {\n>> \tconst char *text;\n>> @@ -675,7 +711,7 @@ static int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size,\n>> \n>> \tswitch (type) {\n>> \tcase CURLINFO_TEXT:\n>> -\t\ttrace_printf_key(&trace_curl, \"== Info: %s\", data);\n>> +\t\tcurl_print_info(data, size);\n>> \t\tbreak;\n>> \tcase CURLINFO_HEADER_OUT:\n>> \t\ttext = \"=> Send header\";\n>> \n>> base-commit: c03801e19cb8ab36e9c0d17ff3d5e0c3b0f24193\n>\n> Otherwise functionally it seems fine to me. case CURLINFO_TEXT is the\n> one case that's not already using a curl_dump_* helper, so we're adding\n> one, and to that helper we're adding a call out to\n> redact_sensitive_header().\n>\n> Thanks.\n> - Emily\n>\n>> -- \n>> gitgitgadget\n"},{"id":"467096","messageId":"kl6lk0422zgd.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58774","inReplyTo":"Y21yPIDmwB53e+aj@coredump.intra.peff.net","subject":"Re: [PATCH] http: redact curl h2h3 headers in info","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-11-10T22:53:54Z","receivedAt":"2022-11-10T22:54:13Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Jeff King writes:\n\n> On Thu, Nov 10, 2022 at 09:48:38AM -0800, Glen Choo wrote:\n>\n>> Taylor Blau writes:\n>> \n>> >> * How could we set up end-to-end tests to ensure that we're testing\n>> >> this against affected versions of curl? To avoid regressions, I'd\n>> >> also prefer to test against future versions of curl too.\n>> >\n>> > Does that necessarily matter? We want to make sure that we don't see\n>> > sensitive headers from the h2h3 module with any version of cURL, no?\n>> \n>> It would help, but it might not be worth setting up infrastructure for\n>> just this use case alone. Given the various platforms running tests\n>> against the Git codebase, we probably get close to a representative\n>> sample of the population with enough time.\n>> \n>> I think it would be more important to have tests against HTTP/2.0. If we\n>> did, we probably would have already caught this, e.g.\n>> t/t5551-http-fetch-smart.sh:'GIT_TRACE_CURL redacts auth details' and\n>> friends.\n>\n> There's some discussion in b66c77a64e (http: match headers\n> case-insensitively when redacting, 2021-09-22) about testing with\n> HTTP/2. Which ironically is basically this exact same bug in a different\n> form. ;)\n>\n> The short answer is that it's do-able, but probably there are some\n> headaches to make it work portably.\n\nArgh, what a shame :( Okay, maybe it's not worth trying to use httpd\nthen.\n\nSome other ideas I had were:\n\n- Create a test-tool that calls the redaction logic directly (without\n involving about curl), and we pass the strings we want to redacted to\n it. Way less than ideal, since we'd never be able to proactively catch\n failures, but better than nothing I suppose.\n\n- Write our own HTTP/2 server for redaction tests. I assume this won't\n be trivial, but maybe not prohibitive, e.g. [1] implements its own\n http server for credential helper tests.\n\n[1] https://lore.kernel.org/git/4947e81546a51883365d0087ce616b6b77e24a63.1667426970.git.gitgitgadget@gmail.com/\n\n>\n> I agree with you that trying various curl versions isn't worth doing. If\n> enough people/platforms run Git's suite, one of them will eventually\n> see the problem.\n>\n> -Peff\n"},{"id":"467097","messageId":"pull.1377.v2.git.git.1668121055059.gitgitgadget@gmail.com","threadId":"58774","inReplyTo":"pull.1377.git.git.1667955151994.gitgitgadget@gmail.com","subject":"[PATCH v2] http: redact curl h2h3 headers in info","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-10T22:57:34Z","receivedAt":"2022-11-10T22:57:41Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo \n\nWith GIT_TRACE_CURL=1 or GIT_CURL_VERBOSE=1, sensitive headers like\n\"Authorization\" and \"Cookie\" get redacted. However, since [1], curl's\nh2h3 module also prints headers in its \"info\", which don't get redacted.\nFor example,\n\n echo 'github.com\tTRUE\t/\tFALSE\t1698960413304\to\tfoo=bar' >cookiefile &&\n GIT_TRACE_CURL=1 GIT_TRACE_CURL_NO_DATA=1 git \\\n -c 'http.cookiefile=cookiefile' \\\n -c 'http.version=' \\\n ls-remote https://github.com/git/git refs/heads/main 2>output &&\n grep 'cookie' output\n\nproduces output like:\n\n 23:04:16.920495 http.c:678 == Info: h2h3 [cookie: o=foo=bar]\n 23:04:16.920562 http.c:637 => Send header: cookie: o=\n\nTeach http.c to check for h2h3 headers in info and redact them using the\nexisting header redaction logic.\n\n[1] https://github.com/curl/curl/commit/f8c3724aa90472c0e617ddbbc420aa199971eb77\n\nSigned-off-by: Glen Choo \n---\n http: redact curl h2h3 headers in info\n \n Thanks for the feedback, all :) For this patch, it sounds like it would\n be too onerous to do the kinds of testing I initially envisioned, so I\n think v2 is ready as-is.\n \n Changes in v2:\n \n * Describe the redacted string in comments.\n * Return 1 for \"redactions have happened\".\n * Fix a leak of the \"inner\" strbuf.\n * Rename function, fix typo.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1377%2Fchooglen%2Fhttp%2Fredact-h2h3-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1377/chooglen/http/redact-h2h3-v2\nPull-Request: https://github.com/git/git/pull/1377\n\nRange-diff vs v1:\n\n 1: e9232396736 ! 1: a8c35ff4ddf http: redact curl h2h3 headers in info\n @@ http.c: static void set_curl_keepalive(CURL *c)\n #endif\n \n -static void redact_sensitive_header(struct strbuf *header)\n -+/* Return 0 if redactions been made, 1 otherwise. */\n ++/* Return 1 if redactions have been made, 0 otherwise. */\n +static int redact_sensitive_header(struct strbuf *header)\n {\n -+\tint ret = 1;\n ++\tint ret = 0;\n \tconst char *sensitive_header;\n \n \tif (trace_curl_redact &&\n @@ http.c: static void redact_sensitive_header(struct strbuf *header)\n \t\t/* Everything else is opaque and possibly sensitive */\n \t\tstrbuf_setlen(header, sensitive_header - header->buf);\n \t\tstrbuf_addstr(header, \" \");\n -+\t\tret = 0;\n ++\t\tret = 1;\n \t} else if (trace_curl_redact &&\n \t\t skip_iprefix(header->buf, \"Cookie:\", &sensitive_header)) {\n \t\tstruct strbuf redacted_header = STRBUF_INIT;\n @@ http.c: static void redact_sensitive_header(struct strbuf *header)\n \n \t\tstrbuf_setlen(header, sensitive_header - header->buf);\n \t\tstrbuf_addbuf(header, &redacted_header);\n -+\t\tret = 0;\n ++\t\tret = 1;\n +\t}\n +\treturn ret;\n +}\n @@ http.c: static void redact_sensitive_header(struct strbuf *header)\n +{\n +\tconst char *sensitive_header;\n +\n ++\t/*\n ++\t * curl's h2h3 prints headers in info, e.g.:\n ++\t * h2h3 [: ]\n ++\t */\n +\tif (trace_curl_redact &&\n +\t skip_iprefix(header->buf, \"h2h3 [\", &sensitive_header)) {\n +\t\tstruct strbuf inner = STRBUF_INIT;\n +\n +\t\t/* Drop the trailing \"]\" */\n +\t\tstrbuf_add(&inner, sensitive_header, strlen(sensitive_header) - 1);\n -+\t\tif (!redact_sensitive_header(&inner)) {\n ++\t\tif (redact_sensitive_header(&inner)) {\n +\t\t\tstrbuf_setlen(header, strlen(\"h2h3 [\"));\n +\t\t\tstrbuf_addbuf(header, &inner);\n +\t\t\tstrbuf_addch(header, ']');\n +\t\t}\n ++\n ++\t\tstrbuf_release(&inner);\n \t}\n }\n \n @@ http.c: static void curl_dump_data(const char *text, unsigned char *ptr, size_t\n \tstrbuf_release(&out);\n }\n \n -+static void curl_print_info(char *data, size_t size)\n ++static void curl_dump_info(char *data, size_t size)\n +{\n +\tstruct strbuf buf = STRBUF_INIT;\n +\n @@ http.c: static int curl_trace(CURL *handle, curl_infotype type, char *data, size\n \tswitch (type) {\n \tcase CURLINFO_TEXT:\n -\t\ttrace_printf_key(&trace_curl, \"== Info: %s\", data);\n -+\t\tcurl_print_info(data, size);\n ++\t\tcurl_dump_info(data, size);\n \t\tbreak;\n \tcase CURLINFO_HEADER_OUT:\n \t\ttext = \"=> Send header\";\n\n\n http.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 44 insertions(+), 2 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 5d0502f51fd..8135fac283e 100644\n--- a/http.c\n+++ b/http.c\n@@ -560,8 +560,10 @@ static void set_curl_keepalive(CURL *c)\n }\n #endif\n \n-static void redact_sensitive_header(struct strbuf *header)\n+/* Return 1 if redactions have been made, 0 otherwise. */\n+static int redact_sensitive_header(struct strbuf *header)\n {\n+\tint ret = 0;\n \tconst char *sensitive_header;\n \n \tif (trace_curl_redact &&\n@@ -575,6 +577,7 @@ static void redact_sensitive_header(struct strbuf *header)\n \t\t/* Everything else is opaque and possibly sensitive */\n \t\tstrbuf_setlen(header, sensitive_header - header->buf);\n \t\tstrbuf_addstr(header, \" \");\n+\t\tret = 1;\n \t} else if (trace_curl_redact &&\n \t\t skip_iprefix(header->buf, \"Cookie:\", &sensitive_header)) {\n \t\tstruct strbuf redacted_header = STRBUF_INIT;\n@@ -612,6 +615,33 @@ static void redact_sensitive_header(struct strbuf *header)\n \n \t\tstrbuf_setlen(header, sensitive_header - header->buf);\n \t\tstrbuf_addbuf(header, &redacted_header);\n+\t\tret = 1;\n+\t}\n+\treturn ret;\n+}\n+\n+/* Redact headers in info */\n+static void redact_sensitive_info_header(struct strbuf *header)\n+{\n+\tconst char *sensitive_header;\n+\n+\t/*\n+\t * curl's h2h3 prints headers in info, e.g.:\n+\t * h2h3 [: ]\n+\t */\n+\tif (trace_curl_redact &&\n+\t skip_iprefix(header->buf, \"h2h3 [\", &sensitive_header)) {\n+\t\tstruct strbuf inner = STRBUF_INIT;\n+\n+\t\t/* Drop the trailing \"]\" */\n+\t\tstrbuf_add(&inner, sensitive_header, strlen(sensitive_header) - 1);\n+\t\tif (redact_sensitive_header(&inner)) {\n+\t\t\tstrbuf_setlen(header, strlen(\"h2h3 [\"));\n+\t\t\tstrbuf_addbuf(header, &inner);\n+\t\t\tstrbuf_addch(header, ']');\n+\t\t}\n+\n+\t\tstrbuf_release(&inner);\n \t}\n }\n \n@@ -668,6 +698,18 @@ static void curl_dump_data(const char *text, unsigned char *ptr, size_t size)\n \tstrbuf_release(&out);\n }\n \n+static void curl_dump_info(char *data, size_t size)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tstrbuf_add(&buf, data, size);\n+\n+\tredact_sensitive_info_header(&buf);\n+\ttrace_printf_key(&trace_curl, \"== Info: %s\", buf.buf);\n+\n+\tstrbuf_release(&buf);\n+}\n+\n static int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size, void *userp)\n {\n \tconst char *text;\n@@ -675,7 +717,7 @@ static int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size,\n \n \tswitch (type) {\n \tcase CURLINFO_TEXT:\n-\t\ttrace_printf_key(&trace_curl, \"== Info: %s\", data);\n+\t\tcurl_dump_info(data, size);\n \t\tbreak;\n \tcase CURLINFO_HEADER_OUT:\n \t\ttext = \"=> Send header\";\n\nbase-commit: c03801e19cb8ab36e9c0d17ff3d5e0c3b0f24193\n-- \ngitgitgadget\n"},{"id":"467110","messageId":"Y22ze1m6ayQCv9B5@coredump.intra.peff.net","threadId":"58774","inReplyTo":"kl6lk0422zgd.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH] http: redact curl h2h3 headers in info","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-11T02:29:15Z","receivedAt":"2022-11-11T02:29:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 10, 2022 at 02:53:54PM -0800, Glen Choo wrote:\n\n> > There's some discussion in b66c77a64e (http: match headers\n> > case-insensitively when redacting, 2021-09-22) about testing with\n> > HTTP/2. Which ironically is basically this exact same bug in a different\n> > form. ;)\n> >\n> > The short answer is that it's do-able, but probably there are some\n> > headaches to make it work portably.\n> \n> Argh, what a shame :( Okay, maybe it's not worth trying to use httpd\n> then.\n> \n> Some other ideas I had were:\n> \n> - Create a test-tool that calls the redaction logic directly (without\n> involving about curl), and we pass the strings we want to redacted to\n> it. Way less than ideal, since we'd never be able to proactively catch\n> failures, but better than nothing I suppose.\n\nI don't think this is worth the effort. It's nice to exercise the code a\nbit, but it wouldn't have actually found this regression, since the\nunexpected thing here was curl changing.\n\n> - Write our own HTTP/2 server for redaction tests. I assume this won't\n> be trivial, but maybe not prohibitive, e.g. [1] implements its own\n> http server for credential helper tests.\n\nThese seems like a lot more work than just setting up HTTP/2 support for\nApache. I checked the recipe from b66c77a64e, and it still works. It\ndoes indeed find the bug (my curl is 7.86.0) and confirms your fix.\n\nI think a simple path forward could be something like:\n\n - teach lib-httpd to conditionally use the current set up versus the\n http2 one outlined in b66c77a64e\n\n - push most of t5551 into a lib-http-fetch.sh; the client-side set up\n from b66c77a64e checks for an HTTP2 prereq. The test that looks for\n chunked encoding (and only works on HTTP1) checks for !HTTP2.\n\n - t5551 tells lib-httpd to use the usual setup, and then sources\n lib-http-fetch; it behaves as before\n\n - t5559 (sadly, not contiguous without renumbering intermediate tests)\n tells lib-httpd to use http2, and sets the HTTP2 prereq. It runs the\n same tests but via http2.\n\nWe don't get the _whole_ test suite running with http2, but hopefully it\ngives us a fairly representative sample. And it does find this bug.\n\nI can try to work the above into patch form, but I may not get to it for\na day or two.\n\n-Peff\n"},{"id":"467111","messageId":"Y220CXetWek/eZEu@nand.local","threadId":"58774","inReplyTo":"Y22ze1m6ayQCv9B5@coredump.intra.peff.net","subject":"Re: [PATCH] http: redact curl h2h3 headers in info","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-11T02:31:37Z","receivedAt":"2022-11-11T02:31:47Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Nov 10, 2022 at 09:29:15PM -0500, Jeff King wrote:\n> > - Write our own HTTP/2 server for redaction tests. I assume this won't\n> > be trivial, but maybe not prohibitive, e.g. [1] implements its own\n> > http server for credential helper tests.\n>\n> These seems like a lot more work than just setting up HTTP/2 support for\n> Apache. I checked the recipe from b66c77a64e, and it still works. It\n> does indeed find the bug (my curl is 7.86.0) and confirms your fix.\n>\n> I think a simple path forward could be something like:\n>\n> [...]\n\nThanks. Your plan looks very sane and seems like it hopefully wouldn't\nbe too much effort.\n\nI'll leave it to you and Glen to figure out how to divvy up the patches,\nbut I'm sure you can figure it out ;-). Thanks in advance.\n\nThanks,\nTaylor\n"},{"id":"467112","messageId":"Y22038eJwS6sQN/y@nand.local","threadId":"58774","inReplyTo":"kl6lmt8y31ak.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH] http: redact curl h2h3 headers in info","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-11T02:35:11Z","receivedAt":"2022-11-11T02:37:03Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Nov 10, 2022 at 02:14:11PM -0800, Glen Choo wrote:\n> >> @@ -560,8 +560,10 @@ static void set_curl_keepalive(CURL *c)\n> >> }\n> >> #endif\n> >>\n> >> -static void redact_sensitive_header(struct strbuf *header)\n> >> +/* Return 0 if redactions been made, 1 otherwise. */\n> >\n> > Does it make sense to reverse the retval here?\n> >\n> > `if (!redact_sensitive_header())` sounds like \"if not redacted, ...\" -\n> > but here it means the opposite, right?\n>\n> I struggled with this for a bit since I wasn't sure what the convention\n> is here. Enumerating some off the top of my head, we have:\n\nFor what it's worth, the \"return zero if redactions were made\" is what I\nwould have expected. I think of it as returning zero if we didn't\nencounter an error (and returning a negative, non-zero value if we did).\n\n> - For functions that don't fail we have \"0\" for \"nothing was done\" and\n> \"1\" for something was done (e.g. skip_prefix()).\n>\n> (Tangent: from a readability perspective, this is pretty poor. I need to\n> know beforehand whether or not the function may fail with error before I\n> know what the return value means?)\n>\n> This probably falls into the last category, so for consistency, I think\n> this should return \"1\" for \"redactions have happened\" (as you\n> suggested).\n\n...But I don't really care that much ;-). As long as you choose\nconsistently, and document your choice where it is unclear, it is fine.\n\n> >> +/* Redact headers in info */\n> >> +static void redact_sensitive_info_header(struct strbuf *header)\n> >> +{\n> >> +\tconst char *sensitive_header;\n> >> +\n> >> +\tif (trace_curl_redact &&\n> >> +\t skip_iprefix(header->buf, \"h2h3 [\", &sensitive_header)) {\n> >> +\t\tstruct strbuf inner = STRBUF_INIT;\n> >> +\n> >> +\t\t/* Drop the trailing \"]\" */\n> >> +\t\tstrbuf_add(&inner, sensitive_header, strlen(sensitive_header) - 1);\n> >> +\t\tif (!redact_sensitive_header(&inner)) {\n> >> +\t\t\tstrbuf_setlen(header, strlen(\"h2h3 [\"));\n> >> +\t\t\tstrbuf_addbuf(header, &inner);\n> >> +\t\t\tstrbuf_addch(header, ']');\n> >\n> > I'd really like some more comments in this function - even just one\n> > describing the string we're trying to redact, or showing a sample line.\n> > Navigating string parsing is always a bit difficult.\n>\n> Ah yes, I should include a description of the string.\n\nEh. To be honest, I probably wouldn't have documented it any more than\nyou did. At most, I would add an example \"before\" and \"after\" string\nthat shows what we're trying to generate.\n\nI agree that string manipulation can end up with some fairly convoluted\ncode. But I think what is written here is straightforward, and that\nany attempt to comment it more than suggested would end up just\nrepeating what the code does in English.\n\nThanks,\nTaylor\n"},{"id":"467113","messageId":"Y221RAgmLOQp3MDs@nand.local","threadId":"58774","inReplyTo":"pull.1377.v2.git.git.1668121055059.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] http: redact curl h2h3 headers in info","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-11T02:36:52Z","receivedAt":"2022-11-11T02:41:10Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Nov 10, 2022 at 10:57:34PM +0000, Glen Choo via GitGitGadget wrote:\n> Thanks for the feedback, all :) For this patch, it sounds like it would\n> be too onerous to do the kinds of testing I initially envisioned, so I\n> think v2 is ready as-is.\n\nYeah, I'm fine to start merging this one down as-is, especially with\nsome verification from others that this does in fact fix what you say it\ndoes.\n\nI would be happy to see more tests here in the future, but I agree that\nthey can be done on top.\n\nThanks,\nTaylor\n"},{"id":"467114","messageId":"Y221yQva4WfZEE94@nand.local","threadId":"58774","inReplyTo":"Y221kGaQUfZJznO9@coredump.intra.peff.net","subject":"Re: [PATCH v2] http: redact curl h2h3 headers in info","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-11T02:39:05Z","receivedAt":"2022-11-11T02:45:10Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Nov 10, 2022 at 09:38:08PM -0500, Jeff King wrote:\n> On Thu, Nov 10, 2022 at 10:57:34PM +0000, Glen Choo via GitGitGadget wrote:\n>\n> > +/* Redact headers in info */\n> > +static void redact_sensitive_info_header(struct strbuf *header)\n> > +{\n> > +\tconst char *sensitive_header;\n> > +\n> > +\t/*\n> > +\t * curl's h2h3 prints headers in info, e.g.:\n> > +\t * h2h3 [: ]\n> > +\t */\n> > +\tif (trace_curl_redact &&\n> > +\t skip_iprefix(header->buf, \"h2h3 [\", &sensitive_header)) {\n> > +\t\tstruct strbuf inner = STRBUF_INIT;\n> > +\n> > +\t\t/* Drop the trailing \"]\" */\n> > +\t\tstrbuf_add(&inner, sensitive_header, strlen(sensitive_header) - 1);\n>\n> This will misbehave if fed the string \"h2h3 [\", because that strlen()\n> becomes 0, and the subtraction underflows.\n>\n> Unlikely, since we are being fed by curl, but possibly worth asserting\n> (though see below for an alternative which drops this line).\n\nEek. Thanks for spotting. Let's hold off on this one until Glen submits\nanother version, or you and him coordinate a combined series.\n\nThanks,\nTaylor\n"},{"id":"467118","messageId":"Y221kGaQUfZJznO9@coredump.intra.peff.net","threadId":"58774","inReplyTo":"pull.1377.v2.git.git.1668121055059.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] http: redact curl h2h3 headers in info","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-11T02:38:08Z","receivedAt":"2022-11-11T02:58:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 10, 2022 at 10:57:34PM +0000, Glen Choo via GitGitGadget wrote:\n\n> +/* Redact headers in info */\n> +static void redact_sensitive_info_header(struct strbuf *header)\n> +{\n> +\tconst char *sensitive_header;\n> +\n> +\t/*\n> +\t * curl's h2h3 prints headers in info, e.g.:\n> +\t * h2h3 [: ]\n> +\t */\n> +\tif (trace_curl_redact &&\n> +\t skip_iprefix(header->buf, \"h2h3 [\", &sensitive_header)) {\n> +\t\tstruct strbuf inner = STRBUF_INIT;\n> +\n> +\t\t/* Drop the trailing \"]\" */\n> +\t\tstrbuf_add(&inner, sensitive_header, strlen(sensitive_header) - 1);\n\nThis will misbehave if fed the string \"h2h3 [\", because that strlen()\nbecomes 0, and the subtraction underflows.\n\nUnlikely, since we are being fed by curl, but possibly worth asserting\n(though see below for an alternative which drops this line).\n\n> +\t\tif (redact_sensitive_header(&inner)) {\n> +\t\t\tstrbuf_setlen(header, strlen(\"h2h3 [\"));\n\nThis strlen may be better spelled as:\n\n sensitive_header - header->buf\n\nwhich IMHO makes it more clear that our intent is to truncate based on\nthe pointer we computed by skipping (and has no chance of getting out of\nsync with the earlier copy of the string).\n\nIt's also a little more robust, in that it doesn't depend on \"h2h3\"\nbeing at the beginning of the string (though in practice it must be,\nbecause that's where skip_iprefix() is checking). See below on that.\n\n> +\t\t\tstrbuf_addbuf(header, &inner);\n> +\t\t\tstrbuf_addch(header, ']');\n> +\t\t}\n> +\n> +\t\tstrbuf_release(&inner);\n\nThis will do a new allocation/free for each info line, even if it's not\nredacted. It's probably premature optimization to worry about it, but\nyou could do it all in the original strbuf, if we inform\nredact_sensitive_header() of the offset at which it should look for the\nheader (and because it uses \"sensitive_header - header->buf\" for the\ntruncation, it handles the extra \"h2h3\" at the beginning just fine).\nSomething like:\n\ndiff --git a/http.c b/http.c\nindex 8135fac283..8a5ba3f477 100644\n--- a/http.c\n+++ b/http.c\n@@ -561,14 +561,14 @@ static void set_curl_keepalive(CURL *c)\n #endif\n \n /* Return 1 if redactions have been made, 0 otherwise. */\n-static int redact_sensitive_header(struct strbuf *header)\n+static int redact_sensitive_header(struct strbuf *header, size_t offset)\n {\n \tint ret = 0;\n \tconst char *sensitive_header;\n \n \tif (trace_curl_redact &&\n-\t (skip_iprefix(header->buf, \"Authorization:\", &sensitive_header) ||\n-\t skip_iprefix(header->buf, \"Proxy-Authorization:\", &sensitive_header))) {\n+\t (skip_iprefix(header->buf + offset, \"Authorization:\", &sensitive_header) ||\n+\t skip_iprefix(header->buf + offset, \"Proxy-Authorization:\", &sensitive_header))) {\n \t\t/* The first token is the type, which is OK to log */\n \t\twhile (isspace(*sensitive_header))\n \t\t\tsensitive_header++;\n@@ -579,7 +579,7 @@ static int redact_sensitive_header(struct strbuf *header)\n \t\tstrbuf_addstr(header, \" \");\n \t\tret = 1;\n \t} else if (trace_curl_redact &&\n-\t\t skip_iprefix(header->buf, \"Cookie:\", &sensitive_header)) {\n+\t\t skip_iprefix(header->buf + offset, \"Cookie:\", &sensitive_header)) {\n \t\tstruct strbuf redacted_header = STRBUF_INIT;\n \t\tconst char *cookie;\n \n@@ -631,17 +631,10 @@ static void redact_sensitive_info_header(struct strbuf *header)\n \t */\n \tif (trace_curl_redact &&\n \t skip_iprefix(header->buf, \"h2h3 [\", &sensitive_header)) {\n-\t\tstruct strbuf inner = STRBUF_INIT;\n-\n-\t\t/* Drop the trailing \"]\" */\n-\t\tstrbuf_add(&inner, sensitive_header, strlen(sensitive_header) - 1);\n-\t\tif (redact_sensitive_header(&inner)) {\n-\t\t\tstrbuf_setlen(header, strlen(\"h2h3 [\"));\n-\t\t\tstrbuf_addbuf(header, &inner);\n+\t\tif (redact_sensitive_header(header, sensitive_header - header->buf)) {\n+\t\t\t/* redaction ate our closing bracket */\n \t\t\tstrbuf_addch(header, ']');\n \t\t}\n-\n-\t\tstrbuf_release(&inner);\n \t}\n }\n \n@@ -659,7 +652,7 @@ static void curl_dump_header(const char *text, unsigned char *ptr, size_t size,\n \n \tfor (header = headers; *header; header++) {\n \t\tif (hide_sensitive_header)\n-\t\t\tredact_sensitive_header(*header);\n+\t\t\tredact_sensitive_header(*header, 0);\n \t\tstrbuf_insertstr((*header), 0, text);\n \t\tstrbuf_insertstr((*header), strlen(text), \": \");\n \t\tstrbuf_rtrim((*header));\n"},{"id":"467139","messageId":"Y25hDr7aHvKnxso3@coredump.intra.peff.net","threadId":"58774","inReplyTo":"Y22ze1m6ayQCv9B5@coredump.intra.peff.net","subject":"[PATCH] t: run t5551 tests with both HTTP and HTTP/2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-11T14:49:50Z","receivedAt":"2022-11-11T14:49:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 10, 2022 at 09:29:15PM -0500, Jeff King wrote:\n\n> We don't get the _whole_ test suite running with http2, but hopefully it\n> gives us a fairly representative sample. And it does find this bug.\n> \n> I can try to work the above into patch form, but I may not get to it for\n> a day or two.\n\nSo here's a patch which finds the bug you're fixing. It's set up to be\nprepared before your patch, which helps confirm the bug (and that we're\ncorrectly using http/2 in the tests!). You'll want to unmark the !HTTP2\nprereqs as part of your patch.\n\nAlternatively, it could come after your patch to confirm the fix, in\nwhich case it would omit those !HTTP2 prereqs from the get-go.\n\nLet me know if you'd like to pick it up as part of your series.\nOtherwise, I can submit it separately in the on-top form (but my\npreference is for you to take it, since it makes testing the fix\neasier).\n\n-- >8 --\nSubject: t: run t5551 tests with both HTTP and HTTP/2\n\nWe have occasionally seen bugs that affect Git running only against an\nHTTP/2 web server, not an HTTP one. For instance, b66c77a64e (http:\nmatch headers case-insensitively when redacting, 2021-09-22). But since\nwe have no test coverage using HTTP/2, we only uncover these bugs in the\nwild.\n\nThat commit gives a recipe for converting our Apache setup to support\nHTTP/2, but:\n\n - it's not necessarily portable\n\n - we don't want to just test HTTP/2; we really want to do a variety of\n basic tests for _both_ protocols\n\nThis patch handles both problems by running a duplicate of t5551\n(labeled as t5559 here) with an alternate-universe setup that enables\nHTTP/2. So we'll continue to run t5551 as before, but run the same\nbattery of tests again with HTTP/2. If HTTP/2 isn't supported on a given\nplatform, then t5559 should bail during the webserver setup, and\ngracefully skip all tests (unless GIT_TEST_HTTPD has been changed from\n\"auto\" to \"yes\", where the point is to complain when webserver setup\nfails).\n\nIn theory other http-related test scripts could benefit from the same\nduplication, but doing t5551 should give us a reasonable check of basic\nfunctionality, and would have caught both bugs we've seen in the wild\nwith HTTP/2.\n\nA few notes on the implementation:\n\n - a script enables the server side config by calling enable_http2\n before starting the webserver. This avoids even trying to load any\n HTTP/2 config for t5551 (which is what lets it keep working with\n regular HTTP even on systems that don't support it). This also sets\n a prereq which can be used by individual tests.\n\n - As discussed in b66c77a64e, the http2 module isn't compatible with\n the \"prefork\" mpm, so we need to pick something else. I chose\n \"event\" here, which works on my Debian system, but it's possible\n there are platforms which would prefer something else. We can adjust\n that later if somebody finds such a platform.\n\n - The test \"large fetch-pack requests can be sent using chunked\n encoding\" makes sure we use a chunked transfer-encoding by looking\n for that header in the trace. But since HTTP/2 has its own streaming\n mechanisms, we won't find such a header. We could skip the test\n entirely by marking it with !HTTP2. But there's some value in making\n sure that the fetch itself succeeded. So instead, we'll confirm that\n either we're using HTTP2 _or_ we saw the expected chunked header.\n\n - the redaction tests fail under HTTP/2 with recent versions of curl.\n This is a bug! I've marked them with !HTTP2 here to skip them under\n t5559 for the moment. Using test_expect_failure would be more\n appropriate, but would require a bunch of boilerplate. Since we'll\n be fixing them momentarily, let's just skip them for now to keep the\n test suite bisectable, and we can re-enable them in the commit that\n fixes the bug.\n\n - one alternative layout would be to push most of t5551 into a\n lib-t5551.sh script, then source it from both t5551 and t5559.\n Keeping t5551 intact seemed a little simpler, as its one less level\n of indirection for people fixing bugs/regressions in the non-HTTP/2\n tests.\n\nSigned-off-by: Jeff King \n---\n t/lib-httpd.sh | 5 +++++\n t/lib-httpd/apache.conf | 19 ++++++++++++++++---\n t/t5551-http-fetch-smart.sh | 19 ++++++++++++++-----\n t/t5559-http-fetch-smart-http2.sh | 4 ++++\n 4 files changed, 39 insertions(+), 8 deletions(-)\n create mode 100755 t/t5559-http-fetch-smart-http2.sh\n\ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex 1f6b9b08d1..ba9fe36772 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -174,6 +174,11 @@ prepare_httpd() {\n \tfi\n }\n \n+enable_http2 () {\n+\tHTTPD_PARA=\"$HTTPD_PARA -DHTTP2\"\n+\ttest_set_prereq HTTP2\n+}\n+\n start_httpd() {\n \tprepare_httpd >&3 2>&4\n \ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex 706799391b..0294739a77 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -29,6 +29,11 @@ ErrorLog error.log\n \tLoadModule setenvif_module modules/mod_setenvif.so\n \n \n+\n+LoadModule http2_module modules/mod_http2.so\n+Protocols h2c\n+\n+\n \n LockFile accept.lock\n \n@@ -64,12 +69,20 @@ LockFile accept.lock\n \n \tLoadModule access_compat_module modules/mod_access_compat.so\n \n-\n-\tLoadModule mpm_prefork_module modules/mod_mpm_prefork.so\n-\n \n \tLoadModule unixd_module modules/mod_unixd.so\n \n+\n+\n+\n+\tLoadModule mpm_event_module modules/mod_mpm_event.so\n+\n+\n+\n+\n+\tLoadModule mpm_prefork_module modules/mod_mpm_prefork.so\n+\n+\n \n \n PassEnv GIT_VALGRIND\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 64c6c9f59e..9826631926 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -1,13 +1,19 @@\n #!/bin/sh\n \n-test_description='test smart fetching over http via http-backend'\n+: ${HTTP_PROTO:=HTTP}\n+test_description=\"test smart fetching over http via http-backend ($HTTP_PROTO)\"\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n+test \"$HTTP_PROTO\" = \"HTTP/2\" && enable_http2\n start_httpd\n \n+test_expect_success HTTP2 'enable client-side http/2' '\n+\tgit config --global http.version HTTP/2\n+'\n+\n test_expect_success 'setup repository' '\n \tgit config push.default matching &&\n \techo content >file &&\n@@ -194,7 +200,7 @@ test_expect_success 'redirects send auth to new location' '\n \texpect_askpass both user@host auth/smart/repo.git\n '\n \n-test_expect_success 'GIT_TRACE_CURL redacts auth details' '\n+test_expect_success !HTTP2 'GIT_TRACE_CURL redacts auth details' '\n \trm -rf redact-auth trace &&\n \tset_askpass user@host pass@host &&\n \tGIT_TRACE_CURL=\"$(pwd)/trace\" git clone --bare \"$HTTPD_URL/auth/smart/repo.git\" redact-auth &&\n@@ -206,7 +212,7 @@ test_expect_success 'GIT_TRACE_CURL redacts auth details' '\n \tgrep -i \"Authorization: Basic \" trace\n '\n \n-test_expect_success 'GIT_CURL_VERBOSE redacts auth details' '\n+test_expect_success !HTTP2 'GIT_CURL_VERBOSE redacts auth details' '\n \trm -rf redact-auth trace &&\n \tset_askpass user@host pass@host &&\n \tGIT_CURL_VERBOSE=1 git clone --bare \"$HTTPD_URL/auth/smart/repo.git\" redact-auth 2>trace &&\n@@ -347,7 +353,10 @@ test_expect_success CMDLINE_LIMIT \\\n test_expect_success 'large fetch-pack requests can be sent using chunked encoding' '\n \tGIT_TRACE_CURL=true git -c http.postbuffer=65536 \\\n \t\tclone --bare \"$HTTPD_URL/smart/repo.git\" split.git 2>err &&\n-\tgrep \"^=> Send header: Transfer-Encoding: chunked\" err\n+\t{\n+\t\ttest_have_prereq HTTP2 ||\n+\t\tgrep \"^=> Send header: Transfer-Encoding: chunked\" err\n+\t}\n '\n \n test_expect_success 'test allowreachablesha1inwant' '\n@@ -473,7 +482,7 @@ test_expect_success 'fetch by SHA-1 without tag following' '\n \t\t--no-tags origin $(cat bar_hash)\n '\n \n-test_expect_success 'cookies are redacted by default' '\n+test_expect_success !HTTP2 'cookies are redacted by default' '\n \trm -rf clone &&\n \techo \"Set-Cookie: Foo=1\" >cookies &&\n \techo \"Set-Cookie: Bar=2\" >>cookies &&\ndiff --git a/t/t5559-http-fetch-smart-http2.sh b/t/t5559-http-fetch-smart-http2.sh\nnew file mode 100755\nindex 0000000000..9eece71c2c\n--- /dev/null\n+++ b/t/t5559-http-fetch-smart-http2.sh\n@@ -0,0 +1,4 @@\n+#!/bin/sh\n+\n+HTTP_PROTO=HTTP/2\n+. ./t5551-http-fetch-smart.sh\n-- \n2.38.1.808.gb4bb4d4010\n\n"},{"id":"467143","messageId":"221111.86wn81mssk.gmgdl@evledraar.gmail.com","threadId":"58774","inReplyTo":"Y25hDr7aHvKnxso3@coredump.intra.peff.net","subject":"Re: [PATCH] t: run t5551 tests with both HTTP and HTTP/2","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-11T15:06:20Z","receivedAt":"2022-11-11T15:13:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Nov 11 2022, Jeff King wrote:\n\n> On Thu, Nov 10, 2022 at 09:29:15PM -0500, Jeff King wrote:\n>\n>> We don't get the _whole_ test suite running with http2, but hopefully it\n>> gives us a fairly representative sample. And it does find this bug.\n>> \n>> I can try to work the above into patch form, but I may not get to it for\n>> a day or two.\n>\n> So here's a patch which finds the bug you're fixing. It's set up to be\n> prepared before your patch, which helps confirm the bug (and that we're\n> correctly using http/2 in the tests!). You'll want to unmark the !HTTP2\n> prereqs as part of your patch.\n>\n> Alternatively, it could come after your patch to confirm the fix, in\n> which case it would omit those !HTTP2 prereqs from the get-go.\n>\n> Let me know if you'd like to pick it up as part of your series.\n> Otherwise, I can submit it separately in the on-top form (but my\n> preference is for you to take it, since it makes testing the fix\n> easier).\n>\n> -- >8 --\n> Subject: t: run t5551 tests with both HTTP and HTTP/2\n>\n> We have occasionally seen bugs that affect Git running only against an\n> HTTP/2 web server, not an HTTP one. For instance, b66c77a64e (http:\n> match headers case-insensitively when redacting, 2021-09-22). But since\n> we have no test coverage using HTTP/2, we only uncover these bugs in the\n> wild.\n>\n> That commit gives a recipe for converting our Apache setup to support\n> HTTP/2, but:\n>\n> - it's not necessarily portable\n>\n> - we don't want to just test HTTP/2; we really want to do a variety of\n> basic tests for _both_ protocols\n>\n> This patch handles both problems by running a duplicate of t5551\n> (labeled as t5559 here) with an alternate-universe setup that enables\n> HTTP/2. So we'll continue to run t5551 as before, but run the same\n> battery of tests again with HTTP/2. If HTTP/2 isn't supported on a given\n> platform, then t5559 should bail during the webserver setup, and\n> gracefully skip all tests (unless GIT_TEST_HTTPD has been changed from\n> \"auto\" to \"yes\", where the point is to complain when webserver setup\n> fails).\n>\n> In theory other http-related test scripts could benefit from the same\n> duplication, but doing t5551 should give us a reasonable check of basic\n> functionality, and would have caught both bugs we've seen in the wild\n> with HTTP/2.\n\nLooking at the diff below, would it be much more work to just support a\nGIT_TEST_HTTP_VERSION=2?\n\nThen:\n\n> diff --git a/t/t5559-http-fetch-smart-http2.sh b/t/t5559-http-fetch-smart-http2.sh\n> new file mode 100755\n> index 0000000000..9eece71c2c\n> --- /dev/null\n> +++ b/t/t5559-http-fetch-smart-http2.sh\n> @@ -0,0 +1,4 @@\n> +#!/bin/sh\n> +\n> +HTTP_PROTO=HTTP/2\n> +. ./t5551-http-fetch-smart.sh\n\nThis would just skip_all if it was already set to 2, as we'd know we\nwere testing it in other tests, and lib-httpd.sh would do\nGIT_TEST_HTTP_VERSION=1 by default.\n\nThere's the \"!HTTP2\" additions, which you mention you'll be \"fixing\nmomentarily\", but this is a stand-alone patch, so maybe I'm missing that\ncontext...\n"},{"id":"467145","messageId":"Y25oAj+OdC4s+jJM@coredump.intra.peff.net","threadId":"58774","inReplyTo":"221111.86wn81mssk.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] t: run t5551 tests with both HTTP and HTTP/2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-11T15:19:30Z","receivedAt":"2022-11-11T15:20:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 11, 2022 at 04:06:20PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > In theory other http-related test scripts could benefit from the same\n> > duplication, but doing t5551 should give us a reasonable check of basic\n> > functionality, and would have caught both bugs we've seen in the wild\n> > with HTTP/2.\n> \n> Looking at the diff below, would it be much more work to just support a\n> GIT_TEST_HTTP_VERSION=2?\n\nNo, but 99% of the tests would not benefit (because they don't run http\nat all), and it would consume a bunch of extra CPU.\n\n> There's the \"!HTTP2\" additions, which you mention you'll be \"fixing\n> momentarily\", but this is a stand-alone patch, so maybe I'm missing that\n> context...\n\nThe other context is Glen's patch that started this thread.\n\n-Peff\n"},{"id":"467146","messageId":"Y25oKeLGxh2S2eyU@coredump.intra.peff.net","threadId":"58774","inReplyTo":"Y25hDr7aHvKnxso3@coredump.intra.peff.net","subject":"Re: [PATCH] t: run t5551 tests with both HTTP and HTTP/2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-11T15:20:09Z","receivedAt":"2022-11-11T15:20:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 11, 2022 at 09:49:50AM -0500, Jeff King wrote:\n\n> - As discussed in b66c77a64e, the http2 module isn't compatible with\n> the \"prefork\" mpm, so we need to pick something else. I chose\n> \"event\" here, which works on my Debian system, but it's possible\n> there are platforms which would prefer something else. We can adjust\n> that later if somebody finds such a platform.\n\nBy the way, I confirmed that t5559 runs in CI (I checked linux-gcc, but\nall of the ubuntu-based builds should run it, as they all install\napache).\n\n-Peff\n"},{"id":"467151","messageId":"kl6ly1sh1ill.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58774","inReplyTo":"Y221kGaQUfZJznO9@coredump.intra.peff.net","subject":"Re: [PATCH v2] http: redact curl h2h3 headers in info","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-11-11T17:55:34Z","receivedAt":"2022-11-11T17:55:40Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Jeff King writes:\n\n> On Thu, Nov 10, 2022 at 10:57:34PM +0000, Glen Choo via GitGitGadget wrote:\n>\n>> +/* Redact headers in info */\n>> +static void redact_sensitive_info_header(struct strbuf *header)\n>> +{\n>> +\tconst char *sensitive_header;\n>> +\n>> +\t/*\n>> +\t * curl's h2h3 prints headers in info, e.g.:\n>> +\t * h2h3 [: ]\n>> +\t */\n>> +\tif (trace_curl_redact &&\n>> +\t skip_iprefix(header->buf, \"h2h3 [\", &sensitive_header)) {\n>> +\t\tstruct strbuf inner = STRBUF_INIT;\n>> +\n>> +\t\t/* Drop the trailing \"]\" */\n>> +\t\tstrbuf_add(&inner, sensitive_header, strlen(sensitive_header) - 1);\n>\n> This will misbehave if fed the string \"h2h3 [\", because that strlen()\n> becomes 0, and the subtraction underflows.\n>\n> Unlikely, since we are being fed by curl, but possibly worth asserting\n> (though see below for an alternative which drops this line).\n>\n>> +\t\tif (redact_sensitive_header(&inner)) {\n>> +\t\t\tstrbuf_setlen(header, strlen(\"h2h3 [\"));\n>\n> This strlen may be better spelled as:\n>\n> sensitive_header - header->buf\n>\n> which IMHO makes it more clear that our intent is to truncate based on\n> the pointer we computed by skipping (and has no chance of getting out of\n> sync with the earlier copy of the string).\n>\n> It's also a little more robust, in that it doesn't depend on \"h2h3\"\n> being at the beginning of the string (though in practice it must be,\n> because that's where skip_iprefix() is checking). See below on that.\n>\n>> +\t\t\tstrbuf_addbuf(header, &inner);\n>> +\t\t\tstrbuf_addch(header, ']');\n>> +\t\t}\n>> +\n>> +\t\tstrbuf_release(&inner);\n>\n> This will do a new allocation/free for each info line, even if it's not\n> redacted. It's probably premature optimization to worry about it, but\n> you could do it all in the original strbuf, if we inform\n> redact_sensitive_header() of the offset at which it should look for the\n> header (and because it uses \"sensitive_header - header->buf\" for the\n> truncation, it handles the extra \"h2h3\" at the beginning just fine).\n> Something like:\n>\n> diff --git a/http.c b/http.c\n> index 8135fac283..8a5ba3f477 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -561,14 +561,14 @@ static void set_curl_keepalive(CURL *c)\n> #endif\n> \n> /* Return 1 if redactions have been made, 0 otherwise. */\n> -static int redact_sensitive_header(struct strbuf *header)\n> +static int redact_sensitive_header(struct strbuf *header, size_t offset)\n> {\n> \tint ret = 0;\n> \tconst char *sensitive_header;\n> \n> \tif (trace_curl_redact &&\n> -\t (skip_iprefix(header->buf, \"Authorization:\", &sensitive_header) ||\n> -\t skip_iprefix(header->buf, \"Proxy-Authorization:\", &sensitive_header))) {\n> +\t (skip_iprefix(header->buf + offset, \"Authorization:\", &sensitive_header) ||\n> +\t skip_iprefix(header->buf + offset, \"Proxy-Authorization:\", &sensitive_header))) {\n> \t\t/* The first token is the type, which is OK to log */\n> \t\twhile (isspace(*sensitive_header))\n> \t\t\tsensitive_header++;\n> @@ -579,7 +579,7 @@ static int redact_sensitive_header(struct strbuf *header)\n> \t\tstrbuf_addstr(header, \" \");\n> \t\tret = 1;\n> \t} else if (trace_curl_redact &&\n> -\t\t skip_iprefix(header->buf, \"Cookie:\", &sensitive_header)) {\n> +\t\t skip_iprefix(header->buf + offset, \"Cookie:\", &sensitive_header)) {\n> \t\tstruct strbuf redacted_header = STRBUF_INIT;\n> \t\tconst char *cookie;\n> \n> @@ -631,17 +631,10 @@ static void redact_sensitive_info_header(struct strbuf *header)\n> \t */\n> \tif (trace_curl_redact &&\n> \t skip_iprefix(header->buf, \"h2h3 [\", &sensitive_header)) {\n> -\t\tstruct strbuf inner = STRBUF_INIT;\n> -\n> -\t\t/* Drop the trailing \"]\" */\n> -\t\tstrbuf_add(&inner, sensitive_header, strlen(sensitive_header) - 1);\n> -\t\tif (redact_sensitive_header(&inner)) {\n> -\t\t\tstrbuf_setlen(header, strlen(\"h2h3 [\"));\n> -\t\t\tstrbuf_addbuf(header, &inner);\n> +\t\tif (redact_sensitive_header(header, sensitive_header - header->buf)) {\n> +\t\t\t/* redaction ate our closing bracket */\n> \t\t\tstrbuf_addch(header, ']');\n> \t\t}\n> -\n> -\t\tstrbuf_release(&inner);\n> \t}\n> }\n> \n> @@ -659,7 +652,7 @@ static void curl_dump_header(const char *text, unsigned char *ptr, size_t size,\n> \n> \tfor (header = headers; *header; header++) {\n> \t\tif (hide_sensitive_header)\n> -\t\t\tredact_sensitive_header(*header);\n> +\t\t\tredact_sensitive_header(*header, 0);\n> \t\tstrbuf_insertstr((*header), 0, text);\n> \t\tstrbuf_insertstr((*header), strlen(text), \": \");\n> \t\tstrbuf_rtrim((*header));\n\nAs someone who's still trying to wrap my head around pointer\nmanipulations, these suggestions are very welcome, thanks!\n\nI'll take these suggestions along with the HTTP2 one.\n"},{"id":"467170","messageId":"pull.1377.v3.git.git.1668206106.gitgitgadget@gmail.com","threadId":"58774","inReplyTo":"pull.1377.v2.git.git.1668121055059.gitgitgadget@gmail.com","subject":"[PATCH v3 0/2] http: redact curl h2h3 headers in info","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-11T22:35:04Z","receivedAt":"2022-11-11T22:35:13Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Big thanks to Peff for the feedback last round. I've incorporated all of the\nsuggestions. I'm so glad that we finally have tests here :)\n\nChanges in v3:\n\n * Add the HTTP2 test from [1] to the start of the series\n * Drop struct strbuf inner in favor of doing work on the original strbuf\n\nChanges in v2:\n\n * Describe the redacted string in comments.\n * Return 1 for \"redactions have happened\".\n * Fix a leak of the \"inner\" strbuf.\n * Rename function, fix typo.\n\n[1] https://lore.kernel.org/git/Y25hDr7aHvKnxso3@coredump.intra.peff.net\n\nGlen Choo (1):\n http: redact curl h2h3 headers in info\n\nJeff King (1):\n t: run t5551 tests with both HTTP and HTTP/2\n\n http.c | 47 +++++++++++++++++++++++++++----\n t/lib-httpd.sh | 5 ++++\n t/lib-httpd/apache.conf | 19 +++++++++++--\n t/t5551-http-fetch-smart.sh | 13 +++++++--\n t/t5559-http-fetch-smart-http2.sh | 4 +++\n 5 files changed, 77 insertions(+), 11 deletions(-)\n create mode 100755 t/t5559-http-fetch-smart-http2.sh\n\n\nbase-commit: c03801e19cb8ab36e9c0d17ff3d5e0c3b0f24193\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1377%2Fchooglen%2Fhttp%2Fredact-h2h3-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1377/chooglen/http/redact-h2h3-v3\nPull-Request: https://github.com/git/git/pull/1377\n\nRange-diff vs v2:\n\n -: ----------- > 1: 09194dba8cd t: run t5551 tests with both HTTP and HTTP/2\n 1: a8c35ff4ddf ! 2: bb5df1a48b9 http: redact curl h2h3 headers in info\n @@ Commit message\n \n With GIT_TRACE_CURL=1 or GIT_CURL_VERBOSE=1, sensitive headers like\n \"Authorization\" and \"Cookie\" get redacted. However, since [1], curl's\n - h2h3 module also prints headers in its \"info\", which don't get redacted.\n - For example,\n + h2h3 module (invoked when using HTTP/2) also prints headers in its\n + \"info\", which don't get redacted. For example,\n \n echo 'github.com TRUE / FALSE 1698960413304 o foo=bar' >cookiefile &&\n GIT_TRACE_CURL=1 GIT_TRACE_CURL_NO_DATA=1 git \\\n @@ Commit message\n 23:04:16.920562 http.c:637 => Send header: cookie: o=\n \n Teach http.c to check for h2h3 headers in info and redact them using the\n - existing header redaction logic.\n + existing header redaction logic. This fixes the broken redaction logic\n + that we noted in the previous commit, so mark the redaction tests as\n + passing under HTTP2.\n \n [1] https://github.com/curl/curl/commit/f8c3724aa90472c0e617ddbbc420aa199971eb77\n \n + Helped-by: Jeff King \n Signed-off-by: Glen Choo \n \n ## http.c ##\n @@ http.c: static void set_curl_keepalive(CURL *c)\n \n -static void redact_sensitive_header(struct strbuf *header)\n +/* Return 1 if redactions have been made, 0 otherwise. */\n -+static int redact_sensitive_header(struct strbuf *header)\n ++static int redact_sensitive_header(struct strbuf *header, size_t offset)\n {\n +\tint ret = 0;\n \tconst char *sensitive_header;\n \n \tif (trace_curl_redact &&\n +-\t (skip_iprefix(header->buf, \"Authorization:\", &sensitive_header) ||\n +-\t skip_iprefix(header->buf, \"Proxy-Authorization:\", &sensitive_header))) {\n ++\t (skip_iprefix(header->buf + offset, \"Authorization:\", &sensitive_header) ||\n ++\t skip_iprefix(header->buf + offset, \"Proxy-Authorization:\", &sensitive_header))) {\n + \t\t/* The first token is the type, which is OK to log */\n + \t\twhile (isspace(*sensitive_header))\n + \t\t\tsensitive_header++;\n @@ http.c: static void redact_sensitive_header(struct strbuf *header)\n \t\t/* Everything else is opaque and possibly sensitive */\n \t\tstrbuf_setlen(header, sensitive_header - header->buf);\n \t\tstrbuf_addstr(header, \" \");\n +\t\tret = 1;\n \t} else if (trace_curl_redact &&\n - \t\t skip_iprefix(header->buf, \"Cookie:\", &sensitive_header)) {\n +-\t\t skip_iprefix(header->buf, \"Cookie:\", &sensitive_header)) {\n ++\t\t skip_iprefix(header->buf + offset, \"Cookie:\", &sensitive_header)) {\n \t\tstruct strbuf redacted_header = STRBUF_INIT;\n + \t\tconst char *cookie;\n + \n @@ http.c: static void redact_sensitive_header(struct strbuf *header)\n \n \t\tstrbuf_setlen(header, sensitive_header - header->buf);\n @@ http.c: static void redact_sensitive_header(struct strbuf *header)\n +\t */\n +\tif (trace_curl_redact &&\n +\t skip_iprefix(header->buf, \"h2h3 [\", &sensitive_header)) {\n -+\t\tstruct strbuf inner = STRBUF_INIT;\n -+\n -+\t\t/* Drop the trailing \"]\" */\n -+\t\tstrbuf_add(&inner, sensitive_header, strlen(sensitive_header) - 1);\n -+\t\tif (redact_sensitive_header(&inner)) {\n -+\t\t\tstrbuf_setlen(header, strlen(\"h2h3 [\"));\n -+\t\t\tstrbuf_addbuf(header, &inner);\n ++\t\tif (redact_sensitive_header(header, sensitive_header - header->buf)) {\n ++\t\t\t/* redaction ate our closing bracket */\n +\t\t\tstrbuf_addch(header, ']');\n +\t\t}\n -+\n -+\t\tstrbuf_release(&inner);\n \t}\n }\n \n +@@ http.c: static void curl_dump_header(const char *text, unsigned char *ptr, size_t size,\n + \n + \tfor (header = headers; *header; header++) {\n + \t\tif (hide_sensitive_header)\n +-\t\t\tredact_sensitive_header(*header);\n ++\t\t\tredact_sensitive_header(*header, 0);\n + \t\tstrbuf_insertstr((*header), 0, text);\n + \t\tstrbuf_insertstr((*header), strlen(text), \": \");\n + \t\tstrbuf_rtrim((*header));\n @@ http.c: static void curl_dump_data(const char *text, unsigned char *ptr, size_t size)\n \tstrbuf_release(&out);\n }\n @@ http.c: static int curl_trace(CURL *handle, curl_infotype type, char *data, size\n \t\tbreak;\n \tcase CURLINFO_HEADER_OUT:\n \t\ttext = \"=> Send header\";\n +\n + ## t/t5551-http-fetch-smart.sh ##\n +@@ t/t5551-http-fetch-smart.sh: test_expect_success 'redirects send auth to new location' '\n + \texpect_askpass both user@host auth/smart/repo.git\n + '\n + \n +-test_expect_success !HTTP2 'GIT_TRACE_CURL redacts auth details' '\n ++test_expect_success 'GIT_TRACE_CURL redacts auth details' '\n + \trm -rf redact-auth trace &&\n + \tset_askpass user@host pass@host &&\n + \tGIT_TRACE_CURL=\"$(pwd)/trace\" git clone --bare \"$HTTPD_URL/auth/smart/repo.git\" redact-auth &&\n +@@ t/t5551-http-fetch-smart.sh: test_expect_success !HTTP2 'GIT_TRACE_CURL redacts auth details' '\n + \tgrep -i \"Authorization: Basic \" trace\n + '\n + \n +-test_expect_success !HTTP2 'GIT_CURL_VERBOSE redacts auth details' '\n ++test_expect_success 'GIT_CURL_VERBOSE redacts auth details' '\n + \trm -rf redact-auth trace &&\n + \tset_askpass user@host pass@host &&\n + \tGIT_CURL_VERBOSE=1 git clone --bare \"$HTTPD_URL/auth/smart/repo.git\" redact-auth 2>trace &&\n +@@ t/t5551-http-fetch-smart.sh: test_expect_success 'fetch by SHA-1 without tag following' '\n + \t\t--no-tags origin $(cat bar_hash)\n + '\n + \n +-test_expect_success !HTTP2 'cookies are redacted by default' '\n ++test_expect_success 'cookies are redacted by default' '\n + \trm -rf clone &&\n + \techo \"Set-Cookie: Foo=1\" >cookies &&\n + \techo \"Set-Cookie: Bar=2\" >>cookies &&\n\n-- \ngitgitgadget\n"},{"id":"467171","messageId":"09194dba8cd3fac72c5a21ad29d3c7cde3efd049.1668206106.git.gitgitgadget@gmail.com","threadId":"58774","inReplyTo":"pull.1377.v3.git.git.1668206106.gitgitgadget@gmail.com","subject":"[PATCH v3 1/2] t: run t5551 tests with both HTTP and HTTP/2","fromName":"Jeff King via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-11T22:35:05Z","receivedAt":"2022-11-11T22:35:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Jeff King \n\nWe have occasionally seen bugs that affect Git running only against an\nHTTP/2 web server, not an HTTP one. For instance, b66c77a64e (http:\nmatch headers case-insensitively when redacting, 2021-09-22). But since\nwe have no test coverage using HTTP/2, we only uncover these bugs in the\nwild.\n\nThat commit gives a recipe for converting our Apache setup to support\nHTTP/2, but:\n\n - it's not necessarily portable\n\n - we don't want to just test HTTP/2; we really want to do a variety of\n basic tests for _both_ protocols\n\nThis patch handles both problems by running a duplicate of t5551\n(labeled as t5559 here) with an alternate-universe setup that enables\nHTTP/2. So we'll continue to run t5551 as before, but run the same\nbattery of tests again with HTTP/2. If HTTP/2 isn't supported on a given\nplatform, then t5559 should bail during the webserver setup, and\ngracefully skip all tests (unless GIT_TEST_HTTPD has been changed from\n\"auto\" to \"yes\", where the point is to complain when webserver setup\nfails).\n\nIn theory other http-related test scripts could benefit from the same\nduplication, but doing t5551 should give us a reasonable check of basic\nfunctionality, and would have caught both bugs we've seen in the wild\nwith HTTP/2.\n\nA few notes on the implementation:\n\n - a script enables the server side config by calling enable_http2\n before starting the webserver. This avoids even trying to load any\n HTTP/2 config for t5551 (which is what lets it keep working with\n regular HTTP even on systems that don't support it). This also sets\n a prereq which can be used by individual tests.\n\n - As discussed in b66c77a64e, the http2 module isn't compatible with\n the \"prefork\" mpm, so we need to pick something else. I chose\n \"event\" here, which works on my Debian system, but it's possible\n there are platforms which would prefer something else. We can adjust\n that later if somebody finds such a platform.\n\n - The test \"large fetch-pack requests can be sent using chunked\n encoding\" makes sure we use a chunked transfer-encoding by looking\n for that header in the trace. But since HTTP/2 has its own streaming\n mechanisms, we won't find such a header. We could skip the test\n entirely by marking it with !HTTP2. But there's some value in making\n sure that the fetch itself succeeded. So instead, we'll confirm that\n either we're using HTTP2 _or_ we saw the expected chunked header.\n\n - the redaction tests fail under HTTP/2 with recent versions of curl.\n This is a bug! I've marked them with !HTTP2 here to skip them under\n t5559 for the moment. Using test_expect_failure would be more\n appropriate, but would require a bunch of boilerplate. Since we'll\n be fixing them momentarily, let's just skip them for now to keep the\n test suite bisectable, and we can re-enable them in the commit that\n fixes the bug.\n\n - one alternative layout would be to push most of t5551 into a\n lib-t5551.sh script, then source it from both t5551 and t5559.\n Keeping t5551 intact seemed a little simpler, as its one less level\n of indirection for people fixing bugs/regressions in the non-HTTP/2\n tests.\n\nSigned-off-by: Jeff King \n---\n t/lib-httpd.sh | 5 +++++\n t/lib-httpd/apache.conf | 19 ++++++++++++++++---\n t/t5551-http-fetch-smart.sh | 19 ++++++++++++++-----\n t/t5559-http-fetch-smart-http2.sh | 4 ++++\n 4 files changed, 39 insertions(+), 8 deletions(-)\n create mode 100755 t/t5559-http-fetch-smart-http2.sh\n\ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex 1f6b9b08d1d..ba9fe36772a 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -174,6 +174,11 @@ prepare_httpd() {\n \tfi\n }\n \n+enable_http2 () {\n+\tHTTPD_PARA=\"$HTTPD_PARA -DHTTP2\"\n+\ttest_set_prereq HTTP2\n+}\n+\n start_httpd() {\n \tprepare_httpd >&3 2>&4\n \ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex 706799391bd..0294739a77a 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -29,6 +29,11 @@ ErrorLog error.log\n \tLoadModule setenvif_module modules/mod_setenvif.so\n \n \n+\n+LoadModule http2_module modules/mod_http2.so\n+Protocols h2c\n+\n+\n \n LockFile accept.lock\n \n@@ -64,12 +69,20 @@ LockFile accept.lock\n \n \tLoadModule access_compat_module modules/mod_access_compat.so\n \n-\n-\tLoadModule mpm_prefork_module modules/mod_mpm_prefork.so\n-\n \n \tLoadModule unixd_module modules/mod_unixd.so\n \n+\n+\n+\n+\tLoadModule mpm_event_module modules/mod_mpm_event.so\n+\n+\n+\n+\n+\tLoadModule mpm_prefork_module modules/mod_mpm_prefork.so\n+\n+\n \n \n PassEnv GIT_VALGRIND\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 6a38294a476..ad0a0639e6b 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -1,13 +1,19 @@\n #!/bin/sh\n \n-test_description='test smart fetching over http via http-backend'\n+: ${HTTP_PROTO:=HTTP}\n+test_description=\"test smart fetching over http via http-backend ($HTTP_PROTO)\"\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n+test \"$HTTP_PROTO\" = \"HTTP/2\" && enable_http2\n start_httpd\n \n+test_expect_success HTTP2 'enable client-side http/2' '\n+\tgit config --global http.version HTTP/2\n+'\n+\n test_expect_success 'setup repository' '\n \tgit config push.default matching &&\n \techo content >file &&\n@@ -194,7 +200,7 @@ test_expect_success 'redirects send auth to new location' '\n \texpect_askpass both user@host auth/smart/repo.git\n '\n \n-test_expect_success 'GIT_TRACE_CURL redacts auth details' '\n+test_expect_success !HTTP2 'GIT_TRACE_CURL redacts auth details' '\n \trm -rf redact-auth trace &&\n \tset_askpass user@host pass@host &&\n \tGIT_TRACE_CURL=\"$(pwd)/trace\" git clone --bare \"$HTTPD_URL/auth/smart/repo.git\" redact-auth &&\n@@ -206,7 +212,7 @@ test_expect_success 'GIT_TRACE_CURL redacts auth details' '\n \tgrep -i \"Authorization: Basic \" trace\n '\n \n-test_expect_success 'GIT_CURL_VERBOSE redacts auth details' '\n+test_expect_success !HTTP2 'GIT_CURL_VERBOSE redacts auth details' '\n \trm -rf redact-auth trace &&\n \tset_askpass user@host pass@host &&\n \tGIT_CURL_VERBOSE=1 git clone --bare \"$HTTPD_URL/auth/smart/repo.git\" redact-auth 2>trace &&\n@@ -347,7 +353,10 @@ test_expect_success CMDLINE_LIMIT \\\n test_expect_success 'large fetch-pack requests can be sent using chunked encoding' '\n \tGIT_TRACE_CURL=true git -c http.postbuffer=65536 \\\n \t\tclone --bare \"$HTTPD_URL/smart/repo.git\" split.git 2>err &&\n-\tgrep \"^=> Send header: Transfer-Encoding: chunked\" err\n+\t{\n+\t\ttest_have_prereq HTTP2 ||\n+\t\tgrep \"^=> Send header: Transfer-Encoding: chunked\" err\n+\t}\n '\n \n test_expect_success 'test allowreachablesha1inwant' '\n@@ -473,7 +482,7 @@ test_expect_success 'fetch by SHA-1 without tag following' '\n \t\t--no-tags origin $(cat bar_hash)\n '\n \n-test_expect_success 'cookies are redacted by default' '\n+test_expect_success !HTTP2 'cookies are redacted by default' '\n \trm -rf clone &&\n \techo \"Set-Cookie: Foo=1\" >cookies &&\n \techo \"Set-Cookie: Bar=2\" >>cookies &&\ndiff --git a/t/t5559-http-fetch-smart-http2.sh b/t/t5559-http-fetch-smart-http2.sh\nnew file mode 100755\nindex 00000000000..9eece71c2c2\n--- /dev/null\n+++ b/t/t5559-http-fetch-smart-http2.sh\n@@ -0,0 +1,4 @@\n+#!/bin/sh\n+\n+HTTP_PROTO=HTTP/2\n+. ./t5551-http-fetch-smart.sh\n-- \ngitgitgadget\n\n"},{"id":"467172","messageId":"bb5df1a48b98cb71826d53acb6c8f3b25d140b83.1668206106.git.gitgitgadget@gmail.com","threadId":"58774","inReplyTo":"pull.1377.v3.git.git.1668206106.gitgitgadget@gmail.com","subject":"[PATCH v3 2/2] http: redact curl h2h3 headers in info","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-11T22:35:06Z","receivedAt":"2022-11-11T22:35:20Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo \n\nWith GIT_TRACE_CURL=1 or GIT_CURL_VERBOSE=1, sensitive headers like\n\"Authorization\" and \"Cookie\" get redacted. However, since [1], curl's\nh2h3 module (invoked when using HTTP/2) also prints headers in its\n\"info\", which don't get redacted. For example,\n\n echo 'github.com\tTRUE\t/\tFALSE\t1698960413304\to\tfoo=bar' >cookiefile &&\n GIT_TRACE_CURL=1 GIT_TRACE_CURL_NO_DATA=1 git \\\n -c 'http.cookiefile=cookiefile' \\\n -c 'http.version=' \\\n ls-remote https://github.com/git/git refs/heads/main 2>output &&\n grep 'cookie' output\n\nproduces output like:\n\n 23:04:16.920495 http.c:678 == Info: h2h3 [cookie: o=foo=bar]\n 23:04:16.920562 http.c:637 => Send header: cookie: o=\n\nTeach http.c to check for h2h3 headers in info and redact them using the\nexisting header redaction logic. This fixes the broken redaction logic\nthat we noted in the previous commit, so mark the redaction tests as\npassing under HTTP2.\n\n[1] https://github.com/curl/curl/commit/f8c3724aa90472c0e617ddbbc420aa199971eb77\n\nHelped-by: Jeff King \nSigned-off-by: Glen Choo \n---\n http.c | 47 ++++++++++++++++++++++++++++++++-----\n t/t5551-http-fetch-smart.sh | 6 ++---\n 2 files changed, 44 insertions(+), 9 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 5d0502f51fd..8a5ba3f4776 100644\n--- a/http.c\n+++ b/http.c\n@@ -560,13 +560,15 @@ static void set_curl_keepalive(CURL *c)\n }\n #endif\n \n-static void redact_sensitive_header(struct strbuf *header)\n+/* Return 1 if redactions have been made, 0 otherwise. */\n+static int redact_sensitive_header(struct strbuf *header, size_t offset)\n {\n+\tint ret = 0;\n \tconst char *sensitive_header;\n \n \tif (trace_curl_redact &&\n-\t (skip_iprefix(header->buf, \"Authorization:\", &sensitive_header) ||\n-\t skip_iprefix(header->buf, \"Proxy-Authorization:\", &sensitive_header))) {\n+\t (skip_iprefix(header->buf + offset, \"Authorization:\", &sensitive_header) ||\n+\t skip_iprefix(header->buf + offset, \"Proxy-Authorization:\", &sensitive_header))) {\n \t\t/* The first token is the type, which is OK to log */\n \t\twhile (isspace(*sensitive_header))\n \t\t\tsensitive_header++;\n@@ -575,8 +577,9 @@ static void redact_sensitive_header(struct strbuf *header)\n \t\t/* Everything else is opaque and possibly sensitive */\n \t\tstrbuf_setlen(header, sensitive_header - header->buf);\n \t\tstrbuf_addstr(header, \" \");\n+\t\tret = 1;\n \t} else if (trace_curl_redact &&\n-\t\t skip_iprefix(header->buf, \"Cookie:\", &sensitive_header)) {\n+\t\t skip_iprefix(header->buf + offset, \"Cookie:\", &sensitive_header)) {\n \t\tstruct strbuf redacted_header = STRBUF_INIT;\n \t\tconst char *cookie;\n \n@@ -612,6 +615,26 @@ static void redact_sensitive_header(struct strbuf *header)\n \n \t\tstrbuf_setlen(header, sensitive_header - header->buf);\n \t\tstrbuf_addbuf(header, &redacted_header);\n+\t\tret = 1;\n+\t}\n+\treturn ret;\n+}\n+\n+/* Redact headers in info */\n+static void redact_sensitive_info_header(struct strbuf *header)\n+{\n+\tconst char *sensitive_header;\n+\n+\t/*\n+\t * curl's h2h3 prints headers in info, e.g.:\n+\t * h2h3 [: ]\n+\t */\n+\tif (trace_curl_redact &&\n+\t skip_iprefix(header->buf, \"h2h3 [\", &sensitive_header)) {\n+\t\tif (redact_sensitive_header(header, sensitive_header - header->buf)) {\n+\t\t\t/* redaction ate our closing bracket */\n+\t\t\tstrbuf_addch(header, ']');\n+\t\t}\n \t}\n }\n \n@@ -629,7 +652,7 @@ static void curl_dump_header(const char *text, unsigned char *ptr, size_t size,\n \n \tfor (header = headers; *header; header++) {\n \t\tif (hide_sensitive_header)\n-\t\t\tredact_sensitive_header(*header);\n+\t\t\tredact_sensitive_header(*header, 0);\n \t\tstrbuf_insertstr((*header), 0, text);\n \t\tstrbuf_insertstr((*header), strlen(text), \": \");\n \t\tstrbuf_rtrim((*header));\n@@ -668,6 +691,18 @@ static void curl_dump_data(const char *text, unsigned char *ptr, size_t size)\n \tstrbuf_release(&out);\n }\n \n+static void curl_dump_info(char *data, size_t size)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tstrbuf_add(&buf, data, size);\n+\n+\tredact_sensitive_info_header(&buf);\n+\ttrace_printf_key(&trace_curl, \"== Info: %s\", buf.buf);\n+\n+\tstrbuf_release(&buf);\n+}\n+\n static int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size, void *userp)\n {\n \tconst char *text;\n@@ -675,7 +710,7 @@ static int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size,\n \n \tswitch (type) {\n \tcase CURLINFO_TEXT:\n-\t\ttrace_printf_key(&trace_curl, \"== Info: %s\", data);\n+\t\tcurl_dump_info(data, size);\n \t\tbreak;\n \tcase CURLINFO_HEADER_OUT:\n \t\ttext = \"=> Send header\";\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex ad0a0639e6b..a2ebce1787b 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -200,7 +200,7 @@ test_expect_success 'redirects send auth to new location' '\n \texpect_askpass both user@host auth/smart/repo.git\n '\n \n-test_expect_success !HTTP2 'GIT_TRACE_CURL redacts auth details' '\n+test_expect_success 'GIT_TRACE_CURL redacts auth details' '\n \trm -rf redact-auth trace &&\n \tset_askpass user@host pass@host &&\n \tGIT_TRACE_CURL=\"$(pwd)/trace\" git clone --bare \"$HTTPD_URL/auth/smart/repo.git\" redact-auth &&\n@@ -212,7 +212,7 @@ test_expect_success !HTTP2 'GIT_TRACE_CURL redacts auth details' '\n \tgrep -i \"Authorization: Basic \" trace\n '\n \n-test_expect_success !HTTP2 'GIT_CURL_VERBOSE redacts auth details' '\n+test_expect_success 'GIT_CURL_VERBOSE redacts auth details' '\n \trm -rf redact-auth trace &&\n \tset_askpass user@host pass@host &&\n \tGIT_CURL_VERBOSE=1 git clone --bare \"$HTTPD_URL/auth/smart/repo.git\" redact-auth 2>trace &&\n@@ -482,7 +482,7 @@ test_expect_success 'fetch by SHA-1 without tag following' '\n \t\t--no-tags origin $(cat bar_hash)\n '\n \n-test_expect_success !HTTP2 'cookies are redacted by default' '\n+test_expect_success 'cookies are redacted by default' '\n \trm -rf clone &&\n \techo \"Set-Cookie: Foo=1\" >cookies &&\n \techo \"Set-Cookie: Bar=2\" >>cookies &&\n-- \ngitgitgadget\n"},{"id":"467276","messageId":"Y3LCNmsqfHDtG33u@coredump.intra.peff.net","threadId":"58774","inReplyTo":"pull.1377.v3.git.git.1668206106.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/2] http: redact curl h2h3 headers in info","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-14T22:33:26Z","receivedAt":"2022-11-14T22:33:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 11, 2022 at 10:35:04PM +0000, Glen Choo via GitGitGadget wrote:\n\n> Big thanks to Peff for the feedback last round. I've incorporated all of the\n> suggestions. I'm so glad that we finally have tests here :)\n> \n> Changes in v3:\n> \n> * Add the HTTP2 test from [1] to the start of the series\n> * Drop struct strbuf inner in favor of doing work on the original strbuf\n\nThanks, both look good to me (unsurprisingly ;) ).\n\n-Peff\n"},{"id":"467279","messageId":"Y3LEi0maFyh3Qwwy@nand.local","threadId":"58774","inReplyTo":"Y3LCNmsqfHDtG33u@coredump.intra.peff.net","subject":"Re: [PATCH v3 0/2] http: redact curl h2h3 headers in info","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-14T22:43:23Z","receivedAt":"2022-11-14T22:43:50Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Nov 14, 2022 at 05:33:26PM -0500, Jeff King wrote:\n> On Fri, Nov 11, 2022 at 10:35:04PM +0000, Glen Choo via GitGitGadget wrote:\n>\n> > Big thanks to Peff for the feedback last round. I've incorporated all of the\n> > suggestions. I'm so glad that we finally have tests here :)\n> >\n> > Changes in v3:\n> >\n> > * Add the HTTP2 test from [1] to the start of the series\n> > * Drop struct strbuf inner in favor of doing work on the original strbuf\n>\n> Thanks, both look good to me (unsurprisingly ;) ).\n\nThanks, both. Will start merging this down in this evening's push-out.\n\nThanks,\nTaylor\n"}]}