{"thread":{"id":"53620","subject":"[PATCH] http: redact all cookies, teach GIT_TRACE_REDACT=0","startedAt":"2020-06-05T21:21:45Z","lastAt":"2020-06-05T21:21:45Z","messageCount":1,"participants":["Jonathan Tan"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"399215","messageId":"20200605212136.227468-1-jonathantanmy@google.com","threadId":"53620","inReplyTo":null,"subject":"[PATCH] http: redact all cookies, teach GIT_TRACE_REDACT=0","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-06-05T21:21:36Z","receivedAt":"2020-06-05T21:21:45Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In trace output (when GIT_TRACE_CURL is true), redact the values of all\nHTTP cookies by default. Now that auth headers (since the implementation\nof GIT_TRACE_CURL in 74c682d3c6 (\"http.c: implement the GIT_TRACE_CURL\nenvironment variable\", 2016-05-24)) and cookie values (since this\ncommit) are redacted by default in these traces, also allow the user to\ninhibit these redactions through an environment variable.\n\nSince values of all cookies are now redacted by default,\nGIT_REDACT_COOKIES (which previously allowed users to select individual\ncookies to redact) now has no effect.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nThis is on jt/curl-verbose-on-trace-curl.\n\nThanks, Junio, for the reminder in What's Cooking.\n\nThis is a followup from this email thread:\nhttps://lore.kernel.org/git/cover.1589218693.git.jonathantanmy@google.com/\n\nI took a look at transport_anonymize_url() as requested in [1], which\nindeed appears in user-visible output (and could conceivably be used in\ndebugging). It can be done (just be careful about the different uses -\nsend-pack uses it to make a push cert, so that can't be changed, but we\ncan change it when it is used to print a message for the user). But it\ndoesn't seem useful to me (for HTTP(S), we would be better off looking\nat the Authorization header, I think, and for SSH, the password is\neither wrong or correct, so I don't think that there is much Git-related\nstuff to debug), and I can't think of a good name to give the\nenvironment variable that controls all these. (Right now we have\nGIT_TRACE_REDACT, which makes sense because we are redacting a trace.\nGIT_HTTP_REDACT would work too, because everything we're redacting is\nHTTP-related. But the URL is neither HTTP-specific nor trace-specific.)\n\nHaving said that, if we want one single environment variable to redact\neverything, it's probably best to do it now. It would be unexpected if\nan environment variable suddenly also controls redaction of the URL.\n\n[1] https://lore.kernel.org/git/xmqqlflvtysu.fsf@gitster.c.googlers.com/\n---\n Documentation/git.txt       |  9 ++++-----\n http.c                      | 35 +++++++++++-----------------------\n t/t5551-http-fetch-smart.sh | 38 +++++++++++++++++++++++--------------\n 3 files changed, 39 insertions(+), 43 deletions(-)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex 427ea70701..d451f465dc 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -775,11 +775,10 @@ for full details.\n \tSee `GIT_TRACE2` for available trace output options and\n \tlink:technical/api-trace2.html[Trace2 documentation] for full details.\n \n-`GIT_REDACT_COOKIES`::\n-\tThis can be set to a comma-separated list of strings. When a curl trace\n-\tis enabled (see `GIT_TRACE_CURL` above), whenever a \"Cookies:\" header\n-\tsent by the client is dumped, values of cookies whose key is in that\n-\tlist (case-sensitive) are redacted.\n+`GIT_TRACE_REDACT`::\n+\tBy default, when tracing is activated, Git redacts the values of\n+\tcookies, the \"Authorization:\" header, and the \"Proxy-Authorization:\"\n+\theader. Set this variable to `0` to prevent this redaction.\n \n `GIT_LITERAL_PATHSPECS`::\n \tSetting this variable to `1` will cause Git to treat all\ndiff --git a/http.c b/http.c\nindex 4882c9f5b2..0eb1931a15 100644\n--- a/http.c\n+++ b/http.c\n@@ -18,7 +18,7 @@\n \n static struct trace_key trace_curl = TRACE_KEY_INIT(CURL);\n static int trace_curl_data = 1;\n-static struct string_list cookies_to_redact = STRING_LIST_INIT_DUP;\n+static int trace_curl_redact = 1;\n #if LIBCURL_VERSION_NUM >= 0x070a08\n long int git_curl_ipresolve = CURL_IPRESOLVE_WHATEVER;\n #else\n@@ -642,8 +642,9 @@ static void redact_sensitive_header(struct strbuf *header)\n {\n \tconst char *sensitive_header;\n \n-\tif (skip_prefix(header->buf, \"Authorization:\", &sensitive_header) ||\n-\t    skip_prefix(header->buf, \"Proxy-Authorization:\", &sensitive_header)) {\n+\tif (trace_curl_redact &&\n+\t    (skip_prefix(header->buf, \"Authorization:\", &sensitive_header) ||\n+\t     skip_prefix(header->buf, \"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@@ -652,20 +653,15 @@ 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, \" <redacted>\");\n-\t} else if (cookies_to_redact.nr &&\n+\t} else if (trace_curl_redact &&\n \t\t   skip_prefix(header->buf, \"Cookie:\", &sensitive_header)) {\n \t\tstruct strbuf redacted_header = STRBUF_INIT;\n-\t\tchar *cookie;\n+\t\tconst char *cookie;\n \n \t\twhile (isspace(*sensitive_header))\n \t\t\tsensitive_header++;\n \n-\t\t/*\n-\t\t * The contents of header starting from sensitive_header will\n-\t\t * subsequently be overridden, so it is fine to mutate this\n-\t\t * string (hence the assignment to \"char *\").\n-\t\t */\n-\t\tcookie = (char *) sensitive_header;\n+\t\tcookie = sensitive_header;\n \n \t\twhile (cookie) {\n \t\t\tchar *equals;\n@@ -678,14 +674,8 @@ static void redact_sensitive_header(struct strbuf *header)\n \t\t\t\tstrbuf_addstr(&redacted_header, cookie);\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\t*equals = 0; /* temporarily set to NUL for lookup */\n-\t\t\tif (string_list_lookup(&cookies_to_redact, cookie)) {\n-\t\t\t\tstrbuf_addstr(&redacted_header, cookie);\n-\t\t\t\tstrbuf_addstr(&redacted_header, \"=<redacted>\");\n-\t\t\t} else {\n-\t\t\t\t*equals = '=';\n-\t\t\t\tstrbuf_addstr(&redacted_header, cookie);\n-\t\t\t}\n+\t\t\tstrbuf_add(&redacted_header, cookie, equals - cookie);\n+\t\t\tstrbuf_addstr(&redacted_header, \"=<redacted>\");\n \t\t\tif (semicolon) {\n \t\t\t\t/*\n \t\t\t\t * There are more cookies. (Or, for some\n@@ -1003,11 +993,8 @@ static CURL *get_curl_handle(void)\n \tsetup_curl_trace(result);\n \tif (getenv(\"GIT_TRACE_CURL_NO_DATA\"))\n \t\ttrace_curl_data = 0;\n-\tif (getenv(\"GIT_REDACT_COOKIES\")) {\n-\t\tstring_list_split(&cookies_to_redact,\n-\t\t\t\t  getenv(\"GIT_REDACT_COOKIES\"), ',', -1);\n-\t\tstring_list_sort(&cookies_to_redact);\n-\t}\n+\tif (!git_env_bool(\"GIT_TRACE_REDACT\", 1))\n+\t\ttrace_curl_redact = 0;\n \n \tcurl_easy_setopt(result, CURLOPT_USERAGENT,\n \t\tuser_agent ? user_agent : git_user_agent());\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex be01cf7bb2..e40e9ed52f 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -209,6 +209,16 @@ test_expect_success 'GIT_CURL_VERBOSE redacts auth details' '\n \tgrep \"Authorization: Basic <redacted>\" trace\n '\n \n+test_expect_success 'GIT_TRACE_CURL does not redact auth details if GIT_TRACE_REDACT=0' '\n+\trm -rf redact-auth trace &&\n+\tset_askpass user@host pass@host &&\n+\tGIT_TRACE_REDACT=0 GIT_TRACE_CURL=\"$(pwd)/trace\" \\\n+\t\tgit clone --bare \"$HTTPD_URL/auth/smart/repo.git\" redact-auth &&\n+\texpect_askpass both user@host &&\n+\n+\tgrep \"Authorization: Basic [0-9a-zA-Z+/]\" trace\n+'\n+\n test_expect_success 'disable dumb http on server' '\n \tgit --git-dir=\"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" \\\n \t\tconfig http.getanyfile false\n@@ -454,37 +464,37 @@ 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 'GIT_REDACT_COOKIES redacts cookies' '\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-\tGIT_TRACE_CURL=true GIT_REDACT_COOKIES=Bar,Baz \\\n+\tGIT_TRACE_CURL=true \\\n \t\tgit -c \"http.cookieFile=$(pwd)/cookies\" clone \\\n \t\t$HTTPD_URL/smart/repo.git clone 2>err &&\n-\tgrep \"Cookie:.*Foo=1\" err &&\n+\tgrep \"Cookie:.*Foo=<redacted>\" err &&\n \tgrep \"Cookie:.*Bar=<redacted>\" err &&\n+\t! grep \"Cookie:.*Foo=1\" err &&\n \t! grep \"Cookie:.*Bar=2\" err\n '\n \n-test_expect_success 'GIT_REDACT_COOKIES redacts cookies when GIT_CURL_VERBOSE=1' '\n+test_expect_success 'empty values of cookies are also redacted' '\n \trm -rf clone &&\n-\techo \"Set-Cookie: Foo=1\" >cookies &&\n-\techo \"Set-Cookie: Bar=2\" >>cookies &&\n-\tGIT_CURL_VERBOSE=1 GIT_REDACT_COOKIES=Bar,Baz \\\n+\techo \"Set-Cookie: Foo=\" >cookies &&\n+\tGIT_TRACE_CURL=true \\\n \t\tgit -c \"http.cookieFile=$(pwd)/cookies\" clone \\\n \t\t$HTTPD_URL/smart/repo.git clone 2>err &&\n-\tgrep \"Cookie:.*Foo=1\" err &&\n-\tgrep \"Cookie:.*Bar=<redacted>\" err &&\n-\t! grep \"Cookie:.*Bar=2\" err\n+\tgrep \"Cookie:.*Foo=<redacted>\" err\n '\n \n-test_expect_success 'GIT_REDACT_COOKIES handles empty values' '\n+test_expect_success 'GIT_TRACE_REDACT=0 disables cookie redaction' '\n \trm -rf clone &&\n-\techo \"Set-Cookie: Foo=\" >cookies &&\n-\tGIT_TRACE_CURL=true GIT_REDACT_COOKIES=Foo \\\n+\techo \"Set-Cookie: Foo=1\" >cookies &&\n+\techo \"Set-Cookie: Bar=2\" >>cookies &&\n+\tGIT_TRACE_REDACT=0 GIT_TRACE_CURL=true \\\n \t\tgit -c \"http.cookieFile=$(pwd)/cookies\" clone \\\n \t\t$HTTPD_URL/smart/repo.git clone 2>err &&\n-\tgrep \"Cookie:.*Foo=<redacted>\" err\n+\tgrep \"Cookie:.*Foo=1\" err &&\n+\tgrep \"Cookie:.*Bar=2\" err\n '\n \n test_expect_success 'GIT_TRACE_CURL_NO_DATA prevents data from being traced' '\n-- \n2.27.0.278.ge193c7cf3a9-goog\n\n"}]}