From 827e7d4da470e8b9b222b2cf3b4a3b7f8c3c671f Mon Sep 17 00:00:00 2001 From: Jonathan Tan Date: Fri, 5 Jun 2020 14:21:36 -0700 Subject: [PATCH] http: redact all cookies, teach GIT_TRACE_REDACT=0 In trace output (when GIT_TRACE_CURL is true), redact the values of all HTTP cookies by default. Now that auth headers (since the implementation of GIT_TRACE_CURL in 74c682d3c6 ("http.c: implement the GIT_TRACE_CURL environment variable", 2016-05-24)) and cookie values (since this commit) are redacted by default in these traces, also allow the user to inhibit these redactions through an environment variable. Since values of all cookies are now redacted by default, GIT_REDACT_COOKIES (which previously allowed users to select individual cookies to redact) now has no effect. Signed-off-by: Jonathan Tan Signed-off-by: Junio C Hamano --- Documentation/git.txt | 9 ++++---- http.c | 35 ++++++++++------------------- t/t5551-http-fetch-smart.sh | 44 +++++++++++++++++++++++-------------- 3 files changed, 42 insertions(+), 46 deletions(-) diff --git a/Documentation/git.txt b/Documentation/git.txt index 427ea70701..d451f465dc 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -775,11 +775,10 @@ for full details. See `GIT_TRACE2` for available trace output options and link:technical/api-trace2.html[Trace2 documentation] for full details. -`GIT_REDACT_COOKIES`:: - This can be set to a comma-separated list of strings. When a curl trace - is enabled (see `GIT_TRACE_CURL` above), whenever a "Cookies:" header - sent by the client is dumped, values of cookies whose key is in that - list (case-sensitive) are redacted. +`GIT_TRACE_REDACT`:: + By default, when tracing is activated, Git redacts the values of + cookies, the "Authorization:" header, and the "Proxy-Authorization:" + header. Set this variable to `0` to prevent this redaction. `GIT_LITERAL_PATHSPECS`:: Setting this variable to `1` will cause Git to treat all diff --git a/http.c b/http.c index 4882c9f5b2..0eb1931a15 100644 --- a/http.c +++ b/http.c @@ -18,7 +18,7 @@ static struct trace_key trace_curl = TRACE_KEY_INIT(CURL); static int trace_curl_data = 1; -static struct string_list cookies_to_redact = STRING_LIST_INIT_DUP; +static int trace_curl_redact = 1; #if LIBCURL_VERSION_NUM >= 0x070a08 long int git_curl_ipresolve = CURL_IPRESOLVE_WHATEVER; #else @@ -642,8 +642,9 @@ static void redact_sensitive_header(struct strbuf *header) { const char *sensitive_header; - if (skip_prefix(header->buf, "Authorization:", &sensitive_header) || - skip_prefix(header->buf, "Proxy-Authorization:", &sensitive_header)) { + if (trace_curl_redact && + (skip_prefix(header->buf, "Authorization:", &sensitive_header) || + skip_prefix(header->buf, "Proxy-Authorization:", &sensitive_header))) { /* The first token is the type, which is OK to log */ while (isspace(*sensitive_header)) sensitive_header++; @@ -652,20 +653,15 @@ static void redact_sensitive_header(struct strbuf *header) /* Everything else is opaque and possibly sensitive */ strbuf_setlen(header, sensitive_header - header->buf); strbuf_addstr(header, " "); - } else if (cookies_to_redact.nr && + } else if (trace_curl_redact && skip_prefix(header->buf, "Cookie:", &sensitive_header)) { struct strbuf redacted_header = STRBUF_INIT; - char *cookie; + const char *cookie; while (isspace(*sensitive_header)) sensitive_header++; - /* - * The contents of header starting from sensitive_header will - * subsequently be overridden, so it is fine to mutate this - * string (hence the assignment to "char *"). - */ - cookie = (char *) sensitive_header; + cookie = sensitive_header; while (cookie) { char *equals; @@ -678,14 +674,8 @@ static void redact_sensitive_header(struct strbuf *header) strbuf_addstr(&redacted_header, cookie); continue; } - *equals = 0; /* temporarily set to NUL for lookup */ - if (string_list_lookup(&cookies_to_redact, cookie)) { - strbuf_addstr(&redacted_header, cookie); - strbuf_addstr(&redacted_header, "="); - } else { - *equals = '='; - strbuf_addstr(&redacted_header, cookie); - } + strbuf_add(&redacted_header, cookie, equals - cookie); + strbuf_addstr(&redacted_header, "="); if (semicolon) { /* * There are more cookies. (Or, for some @@ -1003,11 +993,8 @@ static CURL *get_curl_handle(void) setup_curl_trace(result); if (getenv("GIT_TRACE_CURL_NO_DATA")) trace_curl_data = 0; - if (getenv("GIT_REDACT_COOKIES")) { - string_list_split(&cookies_to_redact, - getenv("GIT_REDACT_COOKIES"), ',', -1); - string_list_sort(&cookies_to_redact); - } + if (!git_env_bool("GIT_TRACE_REDACT", 1)) + trace_curl_redact = 0; curl_easy_setopt(result, CURLOPT_USERAGENT, user_agent ? user_agent : git_user_agent()); diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh index be01cf7bb2..e40e9ed52f 100755 --- a/t/t5551-http-fetch-smart.sh +++ b/t/t5551-http-fetch-smart.sh @@ -209,6 +209,16 @@ test_expect_success 'GIT_CURL_VERBOSE redacts auth details' ' grep "Authorization: Basic " trace ' +test_expect_success 'GIT_TRACE_CURL does not redact auth details if GIT_TRACE_REDACT=0' ' + rm -rf redact-auth trace && + set_askpass user@host pass@host && + GIT_TRACE_REDACT=0 GIT_TRACE_CURL="$(pwd)/trace" \ + git clone --bare "$HTTPD_URL/auth/smart/repo.git" redact-auth && + expect_askpass both user@host && + + grep "Authorization: Basic [0-9a-zA-Z+/]" trace +' + test_expect_success 'disable dumb http on server' ' git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \ config http.getanyfile false @@ -454,39 +464,39 @@ test_expect_success 'fetch by SHA-1 without tag following' ' --no-tags origin $(cat bar_hash) ' -test_expect_success 'GIT_REDACT_COOKIES redacts cookies' ' +test_expect_success 'cookies are redacted by default' ' rm -rf clone && echo "Set-Cookie: Foo=1" >cookies && echo "Set-Cookie: Bar=2" >>cookies && - GIT_TRACE_CURL=true GIT_REDACT_COOKIES=Bar,Baz \ + GIT_TRACE_CURL=true \ git -c "http.cookieFile=$(pwd)/cookies" clone \ $HTTPD_URL/smart/repo.git clone 2>err && - grep "Cookie:.*Foo=1" err && + grep "Cookie:.*Foo=" err && grep "Cookie:.*Bar=" err && + ! grep "Cookie:.*Foo=1" err && ! grep "Cookie:.*Bar=2" err ' -test_expect_success 'GIT_REDACT_COOKIES redacts cookies when GIT_CURL_VERBOSE=1' ' - rm -rf clone && - echo "Set-Cookie: Foo=1" >cookies && - echo "Set-Cookie: Bar=2" >>cookies && - GIT_CURL_VERBOSE=1 GIT_REDACT_COOKIES=Bar,Baz \ - git -c "http.cookieFile=$(pwd)/cookies" clone \ - $HTTPD_URL/smart/repo.git clone 2>err && - grep "Cookie:.*Foo=1" err && - grep "Cookie:.*Bar=" err && - ! grep "Cookie:.*Bar=2" err -' - -test_expect_success 'GIT_REDACT_COOKIES handles empty values' ' +test_expect_success 'empty values of cookies are also redacted' ' rm -rf clone && echo "Set-Cookie: Foo=" >cookies && - GIT_TRACE_CURL=true GIT_REDACT_COOKIES=Foo \ + GIT_TRACE_CURL=true \ git -c "http.cookieFile=$(pwd)/cookies" clone \ $HTTPD_URL/smart/repo.git clone 2>err && grep "Cookie:.*Foo=" err ' +test_expect_success 'GIT_TRACE_REDACT=0 disables cookie redaction' ' + rm -rf clone && + echo "Set-Cookie: Foo=1" >cookies && + echo "Set-Cookie: Bar=2" >>cookies && + GIT_TRACE_REDACT=0 GIT_TRACE_CURL=true \ + git -c "http.cookieFile=$(pwd)/cookies" clone \ + $HTTPD_URL/smart/repo.git clone 2>err && + grep "Cookie:.*Foo=1" err && + grep "Cookie:.*Bar=2" err +' + test_expect_success 'GIT_TRACE_CURL_NO_DATA prevents data from being traced' ' rm -rf clone && GIT_TRACE_CURL=true \