{"thread":{"id":"47628","subject":"[RFC PATCH 0/2] Cookie redaction during GIT_TRACE_CURL","startedAt":"2018-01-18T00:34:46Z","lastAt":"2018-01-18T18:26:01Z","messageCount":5,"participants":["Jonathan Tan","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"336744","messageId":"cover.1516235197.git.jonathantanmy@google.com","threadId":"47628","inReplyTo":null,"subject":"[RFC PATCH 0/2] Cookie redaction during GIT_TRACE_CURL","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-01-18T00:34:29Z","receivedAt":"2018-01-18T00:34:46Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Sometimes authentication information is sent over HTTP through cookies,\nbut when using GIT_TRACE_CURL, that information appears in logs. There\nare some HTTP headers already redacted (\"Authorization:\" and\n\"Proxy-Authorization:\") - the first patch extends such redaction to a\nuser-specified list.\n\nI've also included another patch to allow omission of data transmission\ninformation from being logged when using GIT_TRACE_CURL. This reduces\nthe information logged to that similar to GIT_CURL_VERBOSE.\n(As for why not use GIT_CURL_VERBOSE instead - that is because\nGIT_CURL_VERBOSE does not perform any redaction, merely using Curl's\ndefault logging mechanism.)\n\nThe patches are ready for merging, but I marked this as \"RFC\" just in\ncase there is a better way to accomplish this.\n\nJonathan Tan (2):\n  http: support cookie redaction when tracing\n  http: support omitting data from traces\n\n http.c                      | 82 ++++++++++++++++++++++++++++++++++++++++-----\n t/t5551-http-fetch-smart.sh | 24 +++++++++++++\n 2 files changed, 98 insertions(+), 8 deletions(-)\n\n-- \n2.16.0.rc1.238.g530d649a79-goog\n\n"},{"id":"336745","messageId":"970dbd7a52de6ab084df8e9ce083bb76ec1889c0.1516235197.git.jonathantanmy@google.com","threadId":"47628","inReplyTo":"cover.1516235197.git.jonathantanmy@google.com","subject":"[RFC PATCH 2/2] http: support omitting data from traces","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-01-18T00:34:31Z","receivedAt":"2018-01-18T00:34:48Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"GIT_TRACE_CURL provides a way to debug what is being sent and received\nover HTTP, with automatic redaction of sensitive information. But it\nalso logs data transmissions, which significantly increases the log file\nsize, sometimes unnecessarily. Add an option \"GIT_TRACE_CURL_NO_DATA\" to\nallow the user to omit such data transmissions.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n http.c                      | 27 +++++++++++++++++++--------\n t/t5551-http-fetch-smart.sh | 12 ++++++++++++\n 2 files changed, 31 insertions(+), 8 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 088cf70bf..32a823895 100644\n--- a/http.c\n+++ b/http.c\n@@ -16,6 +16,7 @@\n #include \"string-list.h\"\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 #if LIBCURL_VERSION_NUM >= 0x070a08\n long int git_curl_ipresolve = CURL_IPRESOLVE_WHATEVER;\n@@ -695,24 +696,32 @@ static int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size,\n \t\tcurl_dump_header(text, (unsigned char *)data, size, DO_FILTER);\n \t\tbreak;\n \tcase CURLINFO_DATA_OUT:\n-\t\ttext = \"=> Send data\";\n-\t\tcurl_dump_data(text, (unsigned char *)data, size);\n+\t\tif (trace_curl_data) {\n+\t\t\ttext = \"=> Send data\";\n+\t\t\tcurl_dump_data(text, (unsigned char *)data, size);\n+\t\t}\n \t\tbreak;\n \tcase CURLINFO_SSL_DATA_OUT:\n-\t\ttext = \"=> Send SSL data\";\n-\t\tcurl_dump_data(text, (unsigned char *)data, size);\n+\t\tif (trace_curl_data) {\n+\t\t\ttext = \"=> Send SSL data\";\n+\t\t\tcurl_dump_data(text, (unsigned char *)data, size);\n+\t\t}\n \t\tbreak;\n \tcase CURLINFO_HEADER_IN:\n \t\ttext = \"<= Recv header\";\n \t\tcurl_dump_header(text, (unsigned char *)data, size, NO_FILTER);\n \t\tbreak;\n \tcase CURLINFO_DATA_IN:\n-\t\ttext = \"<= Recv data\";\n-\t\tcurl_dump_data(text, (unsigned char *)data, size);\n+\t\tif (trace_curl_data) {\n+\t\t\ttext = \"<= Recv data\";\n+\t\t\tcurl_dump_data(text, (unsigned char *)data, size);\n+\t\t}\n \t\tbreak;\n \tcase CURLINFO_SSL_DATA_IN:\n-\t\ttext = \"<= Recv SSL data\";\n-\t\tcurl_dump_data(text, (unsigned char *)data, size);\n+\t\tif (trace_curl_data) {\n+\t\t\ttext = \"<= Recv SSL data\";\n+\t\t\tcurl_dump_data(text, (unsigned char *)data, size);\n+\t\t}\n \t\tbreak;\n \n \tdefault:\t\t/* we ignore unknown types by default */\n@@ -857,6 +866,8 @@ static CURL *get_curl_handle(void)\n \tif (getenv(\"GIT_CURL_VERBOSE\"))\n \t\tcurl_easy_setopt(result, CURLOPT_VERBOSE, 1L);\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);\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 88bd9c094..0c62d00af 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -376,5 +376,17 @@ test_expect_success 'GIT_REDACT_COOKIES redacts cookies' '\n \t! grep \"Cookie:.*Secret=Bar\" err\n '\n \n+test_expect_success 'GIT_TRACE_CURL_NO_DATA prevents data from being traced' '\n+\trm -rf clone &&\n+\tGIT_TRACE_CURL=true \\\n+\t\tgit clone $HTTPD_URL/smart/repo.git clone 2>err &&\n+\tgrep \"=> Send data\" err &&\n+\n+\trm -rf clone &&\n+\tGIT_TRACE_CURL=true GIT_TRACE_CURL_NO_DATA=1 \\\n+\t\tgit clone $HTTPD_URL/smart/repo.git clone 2>err &&\n+\t! grep \"=> Send data\" err\n+'\n+\n stop_httpd\n test_done\n-- \n2.16.0.rc1.238.g530d649a79-goog\n\n"},{"id":"336746","messageId":"b0d7c8cbdd1c6e32f05f95ebde4612eb8bd16fdf.1516235197.git.jonathantanmy@google.com","threadId":"47628","inReplyTo":"cover.1516235197.git.jonathantanmy@google.com","subject":"[RFC PATCH 1/2] http: support cookie redaction when tracing","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-01-18T00:34:30Z","receivedAt":"2018-01-18T00:34:50Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When using GIT_TRACE_CURL, Git already redacts the \"Authorization:\" and\n\"Proxy-Authorization:\" HTTP headers. Extend this redaction to a\nuser-specified list of cookies, specified through the\n\"GIT_REDACT_COOKIES\" environment variable.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n http.c                      | 55 +++++++++++++++++++++++++++++++++++++++++++++\n t/t5551-http-fetch-smart.sh | 12 ++++++++++\n 2 files changed, 67 insertions(+)\n\ndiff --git a/http.c b/http.c\nindex 597771271..088cf70bf 100644\n--- a/http.c\n+++ b/http.c\n@@ -13,8 +13,10 @@\n #include \"transport.h\"\n #include \"packfile.h\"\n #include \"protocol.h\"\n+#include \"string-list.h\"\n \n static struct trace_key trace_curl = TRACE_KEY_INIT(CURL);\n+static struct string_list cookies_to_redact = STRING_LIST_INIT_DUP;\n #if LIBCURL_VERSION_NUM >= 0x070a08\n long int git_curl_ipresolve = CURL_IPRESOLVE_WHATEVER;\n #else\n@@ -575,6 +577,54 @@ 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\t   skip_prefix(header->buf, \"Cookie:\", &sensitive_header)) {\n+\t\tstruct strbuf redacted_header = STRBUF_INIT;\n+\t\tchar *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+\n+\t\twhile (cookie) {\n+\t\t\tchar *equals;\n+\t\t\tchar *semicolon = strstr(cookie, \"; \");\n+\t\t\tif (semicolon)\n+\t\t\t\t*semicolon = 0;\n+\t\t\tequals = strchrnul(cookie, '=');\n+\t\t\tif (!equals) {\n+\t\t\t\t/* invalid cookie, just append and continue */\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\tif (semicolon) {\n+\t\t\t\t/*\n+\t\t\t\t * There are more cookies. (Or, for some\n+\t\t\t\t * reason, the input string ends in \"; \".)\n+\t\t\t\t */\n+\t\t\t\tstrbuf_addstr(&redacted_header, \"; \");\n+\t\t\t\tcookie = semicolon + strlen(\"; \");\n+\t\t\t} else {\n+\t\t\t\tcookie = NULL;\n+\t\t\t}\n+\t\t}\n+\n+\t\tstrbuf_setlen(header, sensitive_header - header->buf);\n+\t\tstrbuf_addbuf(header, &redacted_header);\n \t}\n }\n \n@@ -807,6 +857,11 @@ static CURL *get_curl_handle(void)\n \tif (getenv(\"GIT_CURL_VERBOSE\"))\n \t\tcurl_easy_setopt(result, CURLOPT_VERBOSE, 1L);\n \tsetup_curl_trace(result);\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 \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 a51b7e20d..88bd9c094 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -364,5 +364,17 @@ test_expect_success 'custom http headers' '\n \t\tsubmodule update sub\n '\n \n+test_expect_success 'GIT_REDACT_COOKIES redacts cookies' '\n+\trm -rf clone &&\n+\techo \"Set-Cookie: NotSecret=Foo\" >cookies &&\n+\techo \"Set-Cookie: Secret=Bar\" >>cookies &&\n+\tGIT_TRACE_CURL=true GIT_REDACT_COOKIES=Secret,AnotherSecret \\\n+\t\tgit -c \"http.cookieFile=$(pwd)/cookies\" clone \\\n+\t\t$HTTPD_URL/smart/repo.git clone 2>err &&\n+\tgrep \"Cookie:.*NotSecret=Foo\" err &&\n+\tgrep \"Cookie:.*Secret=<redacted>\" err &&\n+\t! grep \"Cookie:.*Secret=Bar\" err\n+'\n+\n stop_httpd\n test_done\n-- \n2.16.0.rc1.238.g530d649a79-goog\n\n"},{"id":"336808","messageId":"CAPig+cRyTL6LXk0JXTcotP8yco1-TxRVZxOWEu-YOnUfEhjr=g@mail.gmail.com","threadId":"47628","inReplyTo":"b0d7c8cbdd1c6e32f05f95ebde4612eb8bd16fdf.1516235197.git.jonathantanmy@google.com","subject":"Re: [RFC PATCH 1/2] http: support cookie redaction when tracing","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-01-18T18:25:12Z","receivedAt":"2018-01-18T18:25:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jan 17, 2018 at 7:34 PM, Jonathan Tan <jonathantanmy@google.com> wrote:\n> When using GIT_TRACE_CURL, Git already redacts the \"Authorization:\" and\n> \"Proxy-Authorization:\" HTTP headers. Extend this redaction to a\n> user-specified list of cookies, specified through the\n> \"GIT_REDACT_COOKIES\" environment variable.\n>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n>  http.c                      | 55 +++++++++++++++++++++++++++++++++++++++++++++\n>  t/t5551-http-fetch-smart.sh | 12 ++++++++++\n>  2 files changed, 67 insertions(+)\n\nPlease document GIT_REDACT_COOKIES in Documentation/git.txt.\n\n> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\n> @@ -364,5 +364,17 @@ test_expect_success 'custom http headers' '\n> +test_expect_success 'GIT_REDACT_COOKIES redacts cookies' '\n> +       rm -rf clone &&\n> +       echo \"Set-Cookie: NotSecret=Foo\" >cookies &&\n> +       echo \"Set-Cookie: Secret=Bar\" >>cookies &&\n> +       GIT_TRACE_CURL=true GIT_REDACT_COOKIES=Secret,AnotherSecret \\\n> +               git -c \"http.cookieFile=$(pwd)/cookies\" clone \\\n> +               $HTTPD_URL/smart/repo.git clone 2>err &&\n> +       grep \"Cookie:.*NotSecret=Foo\" err &&\n> +       grep \"Cookie:.*Secret=<redacted>\" err &&\n> +       ! grep \"Cookie:.*Secret=Bar\" err\n> +'\n\nThe looseness of these grep expressions (/Cookie:.*Secret/ also\nmatches \"Cookie: NotSecret\", for instance) requires extra\nconcentration on the part of the reader to see that you do indeed\ncover all cases. I wonder, therefore, if it would be better to tighten\nthem to instead match the exact string.\n\nAlso, after reading the implementation, I had expected to see testing\nof the \"Cookie: foo=bar; cow=moo\" case, as well as the handled corner\ncases, such as missing missing \"=\" and missing value after \"=\".\n"},{"id":"336809","messageId":"CAPig+cQBysXjoLKRg=tqJnHPdW9jx+TH0=PV-yTyxMqEOtuiMQ@mail.gmail.com","threadId":"47628","inReplyTo":"970dbd7a52de6ab084df8e9ce083bb76ec1889c0.1516235197.git.jonathantanmy@google.com","subject":"Re: [RFC PATCH 2/2] http: support omitting data from traces","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-01-18T18:25:56Z","receivedAt":"2018-01-18T18:26:01Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jan 17, 2018 at 7:34 PM, Jonathan Tan <jonathantanmy@google.com> wrote:\n> GIT_TRACE_CURL provides a way to debug what is being sent and received\n> over HTTP, with automatic redaction of sensitive information. But it\n> also logs data transmissions, which significantly increases the log file\n> size, sometimes unnecessarily. Add an option \"GIT_TRACE_CURL_NO_DATA\" to\n> allow the user to omit such data transmissions.\n>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n>  http.c                      | 27 +++++++++++++++++++--------\n>  t/t5551-http-fetch-smart.sh | 12 ++++++++++++\n>  2 files changed, 31 insertions(+), 8 deletions(-)\n\nPlease document GIT_TRACE_CURL_NO_DATA in Documentation/git.txt.\n"}]}