{"thread":{"id":"53446","subject":"[PATCH 0/2] Safer GIT_CURL_VERBOSE","startedAt":"2020-05-11T17:43:17Z","lastAt":"2020-05-15T20:47:33Z","messageCount":21,"participants":["Jonathan Tan","Jeff King","brian m. carlson","Junio C Hamano","Daniel Stenberg"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"397558","messageId":"cover.1589218693.git.jonathantanmy@google.com","threadId":"53446","inReplyTo":null,"subject":"[PATCH 0/2] Safer GIT_CURL_VERBOSE","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-05-11T17:43:08Z","receivedAt":"2020-05-11T17:43:17Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"These are patches that unify GIT_CURL_VERBOSE and GIT_TRACE_CURL. This\nhelps avoid GIT_CURL_VERBOSE being a source of confusion, especially\nwith the presence of GIT_REDACT_COOKIES. The motivation is further\nexplained in the commit message of patch 2.\n\nI noticed some commits that refer to GIT_CURL_VERBOSE as deprecated\n(see what 930b67ebd7 (\"Merge branch 'ep/use-git-trace-curl-in-tests'\",\n2016-09-12) merged in), although it seems that they are just referring\nto the fact that GIT_TRACE_CURL has more features than GIT_CURL_VERBOSE\nand should be preferred (74c682d3c6 (\"http.c: implement the\nGIT_TRACE_CURL environment variable\", 2016-05-24)). I haven't made any\nreferences to deprecating anything in any of the commit messages, but\ndeprecating GIT_CURL_VERBOSE (so that we have only one way of doing\nthings) seems good to me - but this is a matter for another patch, I\nthink.\n\nJonathan Tan (2):\n  t5551: test that GIT_TRACE_CURL redacts password\n  http, imap-send: stop using CURLOPT_VERBOSE\n\n Documentation/git.txt        |  2 --\n http.c                       |  8 +++++++-\n http.h                       |  7 +++++++\n imap-send.c                  |  2 +-\n t/t5551-http-fetch-smart.sh  | 36 ++++++++++++++++++++++++++++++++++++\n t/t5581-http-curl-verbose.sh |  2 +-\n trace.c                      | 20 ++++++++++++++++----\n trace.h                      |  6 ++++++\n 8 files changed, 74 insertions(+), 9 deletions(-)\n\n-- \n2.26.2.645.ge9eca65c58-goog\n\n"},{"id":"397559","messageId":"bee4fc7241596a6b4206fa8b52dfdaea88180f37.1589218693.git.jonathantanmy@google.com","threadId":"53446","inReplyTo":"cover.1589218693.git.jonathantanmy@google.com","subject":"[PATCH 1/2] t5551: test that GIT_TRACE_CURL redacts password","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-05-11T17:43:09Z","receivedAt":"2020-05-11T17:43:17Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Verify that when GIT_TRACE_CURL is set, Git prints out \"Authorization:\nBasic <redacted>\" instead of the base64-encoded authorization details.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n t/t5551-http-fetch-smart.sh | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 6788aeface..acc8473a72 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -185,6 +185,18 @@ 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+\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+\texpect_askpass both user@host &&\n+\n+\t# Ensure that there is no \"Basic\" followed by a base64 string, but that\n+\t# the auth details are redacted\n+\t! grep \"Authorization: Basic [0-9a-zA-Z+/]\" trace &&\n+\tgrep \"Authorization: Basic <redacted>\" 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-- \n2.26.2.645.ge9eca65c58-goog\n\n"},{"id":"397560","messageId":"1df9e9deb7831b32694ea453759bf5d21952e165.1589218693.git.jonathantanmy@google.com","threadId":"53446","inReplyTo":"cover.1589218693.git.jonathantanmy@google.com","subject":"[PATCH 2/2] http, imap-send: stop using CURLOPT_VERBOSE","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-05-11T17:43:10Z","receivedAt":"2020-05-11T17:43:21Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Whenever GIT_CURL_VERBOSE is set, teach Git to behave as if\nGIT_TRACE_CURL=1 and GIT_TRACE_CURL_NO_DATA=1 is set, instead of setting\nCURLOPT_VERBOSE.\n\nThis is to prevent inadvertent revelation of sensitive data. In\nparticular, GIT_CURL_VERBOSE redacts neither the \"Authorization\" header\nnor any cookies specified by GIT_REDACT_COOKIES.\n\nUnifying the tracing mechanism also has the future benefit that any\nimprovements to the tracing mechanism will benefit both users of\nGIT_CURL_VERBOSE and GIT_TRACE_CURL, and we do not need to remember to\nimplement any improvement twice.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n Documentation/git.txt        |  2 --\n http.c                       |  8 +++++++-\n http.h                       |  7 +++++++\n imap-send.c                  |  2 +-\n t/t5551-http-fetch-smart.sh  | 24 ++++++++++++++++++++++++\n t/t5581-http-curl-verbose.sh |  2 +-\n trace.c                      | 20 ++++++++++++++++----\n trace.h                      |  6 ++++++\n 8 files changed, 62 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex 9d6769e95a..427ea70701 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -721,8 +721,6 @@ of clones and fetches.\n \tEnables a curl full trace dump of all incoming and outgoing data,\n \tincluding descriptive information, of the git transport protocol.\n \tThis is similar to doing curl `--trace-ascii` on the command line.\n-\tThis option overrides setting the `GIT_CURL_VERBOSE` environment\n-\tvariable.\n \tSee `GIT_TRACE` for available trace output options.\n \n `GIT_TRACE_CURL_NO_DATA`::\ndiff --git a/http.c b/http.c\nindex 62aa995245..4882c9f5b2 100644\n--- a/http.c\n+++ b/http.c\n@@ -804,6 +804,12 @@ static int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size,\n \treturn 0;\n }\n \n+void http_trace_curl_no_data(void)\n+{\n+\ttrace_override_envvar(&trace_curl, \"1\");\n+\ttrace_curl_data = 0;\n+}\n+\n void setup_curl_trace(CURL *handle)\n {\n \tif (!trace_want(&trace_curl))\n@@ -993,7 +999,7 @@ static CURL *get_curl_handle(void)\n \twarning(_(\"Protocol restrictions not supported with cURL < 7.19.4\"));\n #endif\n \tif (getenv(\"GIT_CURL_VERBOSE\"))\n-\t\tcurl_easy_setopt(result, CURLOPT_VERBOSE, 1L);\n+\t\thttp_trace_curl_no_data();\n \tsetup_curl_trace(result);\n \tif (getenv(\"GIT_TRACE_CURL_NO_DATA\"))\n \t\ttrace_curl_data = 0;\ndiff --git a/http.h b/http.h\nindex 5e0ad724f9..faf8cbb0d1 100644\n--- a/http.h\n+++ b/http.h\n@@ -252,6 +252,13 @@ int finish_http_object_request(struct http_object_request *freq);\n void abort_http_object_request(struct http_object_request *freq);\n void release_http_object_request(struct http_object_request *freq);\n \n+/*\n+ * Instead of using environment variables to determine if curl tracing happens,\n+ * behave as if GIT_TRACE_CURL=1 and GIT_TRACE_CURL_NO_DATA=1 is set. Call this\n+ * before calling setup_curl_trace().\n+ */\n+void http_trace_curl_no_data(void);\n+\n /* setup routine for curl_easy_setopt CURLOPT_DEBUGFUNCTION */\n void setup_curl_trace(CURL *handle);\n #endif /* HTTP_H */\ndiff --git a/imap-send.c b/imap-send.c\nindex 6c54d8c29d..52737546f3 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -1464,7 +1464,7 @@ static CURL *setup_curl(struct imap_server_conf *srvc, struct credential *cred)\n \tcurl_easy_setopt(curl, CURLOPT_UPLOAD, 1L);\n \n \tif (0 < verbosity || getenv(\"GIT_CURL_VERBOSE\"))\n-\t\tcurl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);\n+\t\thttp_trace_curl_no_data();\n \tsetup_curl_trace(curl);\n \n \treturn curl;\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex acc8473a72..be01cf7bb2 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -197,6 +197,18 @@ test_expect_success 'GIT_TRACE_CURL redacts auth details' '\n \tgrep \"Authorization: Basic <redacted>\" trace\n '\n \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+\texpect_askpass both user@host &&\n+\n+\t# Ensure that there is no \"Basic\" followed by a base64 string, but that\n+\t# the auth details are redacted\n+\t! grep \"Authorization: Basic [0-9a-zA-Z+/]\" trace &&\n+\tgrep \"Authorization: Basic <redacted>\" 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,6 +466,18 @@ test_expect_success 'GIT_REDACT_COOKIES redacts cookies' '\n \t! grep \"Cookie:.*Bar=2\" err\n '\n \n+test_expect_success 'GIT_REDACT_COOKIES redacts cookies when GIT_CURL_VERBOSE=1' '\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+\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+'\n+\n test_expect_success 'GIT_REDACT_COOKIES handles empty values' '\n \trm -rf clone &&\n \techo \"Set-Cookie: Foo=\" >cookies &&\ndiff --git a/t/t5581-http-curl-verbose.sh b/t/t5581-http-curl-verbose.sh\nindex 5129b0724f..927aad0820 100755\n--- a/t/t5581-http-curl-verbose.sh\n+++ b/t/t5581-http-curl-verbose.sh\n@@ -20,7 +20,7 @@ test_expect_success 'failure in git-upload-pack is shown' '\n \ttest_might_fail env GIT_CURL_VERBOSE=1 \\\n \t\tgit clone \"$HTTPD_URL/error_git_upload_pack/smart/repo.git\" \\\n \t\t2>curl_log &&\n-\tgrep \"< HTTP/1.1 500 Intentional Breakage\" curl_log\n+\tgrep \"<= Recv header: HTTP/1.1 500 Intentional Breakage\" curl_log\n '\n \n test_done\ndiff --git a/trace.c b/trace.c\nindex b3ef0e627f..f726686fd9 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -29,7 +29,7 @@ struct trace_key trace_perf_key = TRACE_KEY_INIT(PERFORMANCE);\n struct trace_key trace_setup_key = TRACE_KEY_INIT(SETUP);\n \n /* Get a trace file descriptor from \"key\" env variable. */\n-static int get_trace_fd(struct trace_key *key)\n+static int get_trace_fd(struct trace_key *key, const char *override_envvar)\n {\n \tconst char *trace;\n \n@@ -37,7 +37,7 @@ static int get_trace_fd(struct trace_key *key)\n \tif (key->initialized)\n \t\treturn key->fd;\n \n-\ttrace = getenv(key->key);\n+\ttrace = override_envvar ? override_envvar : getenv(key->key);\n \n \tif (!trace || !strcmp(trace, \"\") ||\n \t    !strcmp(trace, \"0\") || !strcasecmp(trace, \"false\"))\n@@ -68,6 +68,18 @@ static int get_trace_fd(struct trace_key *key)\n \treturn key->fd;\n }\n \n+void trace_override_envvar(struct trace_key *key, const char *value)\n+{\n+\ttrace_disable(key);\n+\tkey->initialized = 0;\n+\n+\t/*\n+\t * Invoke get_trace_fd() to initialize key using the given value\n+\t * instead of the value of the environment variable.\n+\t */\n+\tget_trace_fd(key, value);\n+}\n+\n void trace_disable(struct trace_key *key)\n {\n \tif (key->need_close)\n@@ -112,7 +124,7 @@ static int prepare_trace_line(const char *file, int line,\n \n static void trace_write(struct trace_key *key, const void *buf, unsigned len)\n {\n-\tif (write_in_full(get_trace_fd(key), buf, len) < 0) {\n+\tif (write_in_full(get_trace_fd(key, NULL), buf, len) < 0) {\n \t\twarning(\"unable to write trace for %s: %s\",\n \t\t\tkey->key, strerror(errno));\n \t\ttrace_disable(key);\n@@ -383,7 +395,7 @@ void trace_repo_setup(const char *prefix)\n \n int trace_want(struct trace_key *key)\n {\n-\treturn !!get_trace_fd(key);\n+\treturn !!get_trace_fd(key, NULL);\n }\n \n #if defined(HAVE_CLOCK_GETTIME) && defined(HAVE_CLOCK_MONOTONIC)\ndiff --git a/trace.h b/trace.h\nindex 9826618b33..0dbbad0e41 100644\n--- a/trace.h\n+++ b/trace.h\n@@ -101,6 +101,12 @@ void trace_repo_setup(const char *prefix);\n  */\n int trace_want(struct trace_key *key);\n \n+/**\n+ * Enables or disables tracing for the specified key, as if the environment\n+ * variable was set to the given value.\n+ */\n+void trace_override_envvar(struct trace_key *key, const char *value);\n+\n /**\n  * Disables tracing for the specified key, even if the environment variable\n  * was set.\n-- \n2.26.2.645.ge9eca65c58-goog\n\n"},{"id":"397618","messageId":"20200512190831.GA54565@coredump.intra.peff.net","threadId":"53446","inReplyTo":"bee4fc7241596a6b4206fa8b52dfdaea88180f37.1589218693.git.jonathantanmy@google.com","subject":"Re: [PATCH 1/2] t5551: test that GIT_TRACE_CURL redacts password","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-12T19:08:31Z","receivedAt":"2020-05-12T19:08:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 11, 2020 at 10:43:09AM -0700, Jonathan Tan wrote:\n\n> Verify that when GIT_TRACE_CURL is set, Git prints out \"Authorization:\n> Basic <redacted>\" instead of the base64-encoded authorization details.\n\nYeah, it's definitely worth testing this. The patch looks good to me.\n\n-Peff\n"},{"id":"397619","messageId":"20200512191610.GB54565@coredump.intra.peff.net","threadId":"53446","inReplyTo":"1df9e9deb7831b32694ea453759bf5d21952e165.1589218693.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/2] http, imap-send: stop using CURLOPT_VERBOSE","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-12T19:16:10Z","receivedAt":"2020-05-12T19:16:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 11, 2020 at 10:43:10AM -0700, Jonathan Tan wrote:\n\n> Whenever GIT_CURL_VERBOSE is set, teach Git to behave as if\n> GIT_TRACE_CURL=1 and GIT_TRACE_CURL_NO_DATA=1 is set, instead of setting\n> CURLOPT_VERBOSE.\n> \n> This is to prevent inadvertent revelation of sensitive data. In\n> particular, GIT_CURL_VERBOSE redacts neither the \"Authorization\" header\n> nor any cookies specified by GIT_REDACT_COOKIES.\n> \n> Unifying the tracing mechanism also has the future benefit that any\n> improvements to the tracing mechanism will benefit both users of\n> GIT_CURL_VERBOSE and GIT_TRACE_CURL, and we do not need to remember to\n> implement any improvement twice.\n\nYeah, I think this is worth doing. The patch looks OK to me, though:\n\n> +void http_trace_curl_no_data(void)\n> +{\n> +\ttrace_override_envvar(&trace_curl, \"1\");\n> +\ttrace_curl_data = 0;\n> +}\n\nWould:\n\n  setenv(\"GIT_TRACE_CURL\", \"1\", 0);\n  setenv(\"GIT_TRACE_CURL_NO_DATA\", \"0\", 0);\n\nbe simpler? Perhaps it makes the flow more convoluted as we'd go on to\nparse those variables, but it puts us on the same paths that we'd use if\nthe user specified those things (and avoids the need for the special\n\"override\" function entirely).\n\nOther than that nit, it seems very cleanly done.\n\n-Peff\n\nPS I sometimes find the normal trace a bit verbose, but I do still want\n   to see data. Do others feel the same? Particularly I find the \"SSL\"\n   lines totally worthless (I guess maybe you could be debugging ssl\n   stuff, but that would be the exception, I'd think). Ditto the split\n   of data into two lines: one with the size and one with the actual\n   data.\n\n   I dunno. I haven't been debugging any git-over-http stuff lately, so\n   it hasn't been bothering me. But I definitely have written perl\n   scripts to extract the data to a more readable format. Maybe it would\n   be easier if it had a few more knobs.\n"},{"id":"397620","messageId":"20200512192300.203201-1-jonathantanmy@google.com","threadId":"53446","inReplyTo":"20200512191610.GB54565@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] http, imap-send: stop using CURLOPT_VERBOSE","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-05-12T19:23:00Z","receivedAt":"2020-05-12T19:23:09Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > +void http_trace_curl_no_data(void)\n> > +{\n> > +\ttrace_override_envvar(&trace_curl, \"1\");\n> > +\ttrace_curl_data = 0;\n> > +}\n> \n> Would:\n> \n>   setenv(\"GIT_TRACE_CURL\", \"1\", 0);\n>   setenv(\"GIT_TRACE_CURL_NO_DATA\", \"0\", 0);\n> \n> be simpler? Perhaps it makes the flow more convoluted as we'd go on to\n> parse those variables, but it puts us on the same paths that we'd use if\n> the user specified those things (and avoids the need for the special\n> \"override\" function entirely).\n> \n> Other than that nit, it seems very cleanly done.\n\nThanks for the review. I thought of doing that, but thought that it\nmight add some latent complications - in particular, someone inspecting\nthe environment variables of this running process might see some\nenvironment variables that they didn't set. But I'm OK either way.\n\n> PS I sometimes find the normal trace a bit verbose, but I do still want\n>    to see data. Do others feel the same? Particularly I find the \"SSL\"\n>    lines totally worthless (I guess maybe you could be debugging ssl\n>    stuff, but that would be the exception, I'd think). Ditto the split\n>    of data into two lines: one with the size and one with the actual\n>    data.\n> \n>    I dunno. I haven't been debugging any git-over-http stuff lately, so\n>    it hasn't been bothering me. But I definitely have written perl\n>    scripts to extract the data to a more readable format. Maybe it would\n>    be easier if it had a few more knobs.\n\nData can be turned on using GIT_TRACE_CURL=1 and refraining from setting\nGIT_TRACE_CURL_NO_DATA. What knobs were you thinking of?\n"},{"id":"397622","messageId":"20200512192706.GD54565@coredump.intra.peff.net","threadId":"53446","inReplyTo":"20200512192300.203201-1-jonathantanmy@google.com","subject":"Re: [PATCH 2/2] http, imap-send: stop using CURLOPT_VERBOSE","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-12T19:27:06Z","receivedAt":"2020-05-12T19:27:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 12, 2020 at 12:23:00PM -0700, Jonathan Tan wrote:\n\n> > PS I sometimes find the normal trace a bit verbose, but I do still want\n> >    to see data. Do others feel the same? Particularly I find the \"SSL\"\n> >    lines totally worthless (I guess maybe you could be debugging ssl\n> >    stuff, but that would be the exception, I'd think). Ditto the split\n> >    of data into two lines: one with the size and one with the actual\n> >    data.\n> > \n> >    I dunno. I haven't been debugging any git-over-http stuff lately, so\n> >    it hasn't been bothering me. But I definitely have written perl\n> >    scripts to extract the data to a more readable format. Maybe it would\n> >    be easier if it had a few more knobs.\n> \n> Data can be turned on using GIT_TRACE_CURL=1 and refraining from setting\n> GIT_TRACE_CURL_NO_DATA. What knobs were you thinking of?\n\nI still want to see data, but less cruft. I.e., something like\n\"GIT_TRACE_CURL_SSL\" (which I'd default to \"off\"), and probably just\nreducing:\n\n  15:24:01.169101 [pid=55191] http.c:702            <= Recv data, 0000000004 bytes (0x00000004)\n  15:24:01.169104 [pid=55191] http.c:717            <= Recv data: 3e..\n\nto just the second line. Actually, we might not need a knob at all for\nSSL data. I was thinking that people might actually be debugging SSL\nproblems with it, but since all of the non-printable characters are\nmunged to \".\" anyway, it's basically useless (you can often pick out a\nfew strings from the cert during handshake, but you'd be much better off\nto just connect with \"openssl s_client\" and ask it to dump the cert).\n\n-Peff\n"},{"id":"397637","messageId":"20200512231331.GA6605@camp.crustytoothpaste.net","threadId":"53446","inReplyTo":"1df9e9deb7831b32694ea453759bf5d21952e165.1589218693.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/2] http, imap-send: stop using CURLOPT_VERBOSE","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-05-12T23:13:31Z","receivedAt":"2020-05-12T23:13:38Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-05-11 at 17:43:10, Jonathan Tan wrote:\n> Whenever GIT_CURL_VERBOSE is set, teach Git to behave as if\n> GIT_TRACE_CURL=1 and GIT_TRACE_CURL_NO_DATA=1 is set, instead of setting\n> CURLOPT_VERBOSE.\n> \n> This is to prevent inadvertent revelation of sensitive data. In\n> particular, GIT_CURL_VERBOSE redacts neither the \"Authorization\" header\n> nor any cookies specified by GIT_REDACT_COOKIES.\n\nI actually use GIT_CURL_VERBOSE to debug authentication problems from\ntime to time, so I'd like to keep an option to produce full, unredacted\noutput.  Since everyone uses HTTPS, it's not possible to perform this\ndebugging using a tool like Wireshark unless you use a MITM CA cert,\nwhich seems excessive.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"397646","messageId":"xmqqwo5gwv7z.fsf@gitster.c.googlers.com","threadId":"53446","inReplyTo":"20200512231331.GA6605@camp.crustytoothpaste.net","subject":"Re: [PATCH 2/2] http, imap-send: stop using CURLOPT_VERBOSE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-13T00:10:24Z","receivedAt":"2020-05-13T00:10:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> On 2020-05-11 at 17:43:10, Jonathan Tan wrote:\n>> Whenever GIT_CURL_VERBOSE is set, teach Git to behave as if\n>> GIT_TRACE_CURL=1 and GIT_TRACE_CURL_NO_DATA=1 is set, instead of setting\n>> CURLOPT_VERBOSE.\n>> \n>> This is to prevent inadvertent revelation of sensitive data. In\n>> particular, GIT_CURL_VERBOSE redacts neither the \"Authorization\" header\n>> nor any cookies specified by GIT_REDACT_COOKIES.\n>\n> I actually use GIT_CURL_VERBOSE to debug authentication problems from\n> time to time, so I'd like to keep an option to produce full, unredacted\n> output.  Since everyone uses HTTPS, it's not possible to perform this\n> debugging using a tool like Wireshark unless you use a MITM CA cert,\n> which seems excessive.\n\nHmm, that is a valid concern.  Introducing yet another environment\nfeels a bit yucky, but something like GIT_NO_REDACT that disables\nany redacting, not limited to curl but in all codepaths, might turn\nout to be a useful escape hatch.\n\nOpinions?\n"},{"id":"397698","messageId":"20200513045001.GA65110@coredump.intra.peff.net","threadId":"53446","inReplyTo":"xmqqwo5gwv7z.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/2] http, imap-send: stop using CURLOPT_VERBOSE","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-13T04:50:01Z","receivedAt":"2020-05-13T04:50:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 12, 2020 at 05:10:24PM -0700, Junio C Hamano wrote:\n\n> > On 2020-05-11 at 17:43:10, Jonathan Tan wrote:\n> >> Whenever GIT_CURL_VERBOSE is set, teach Git to behave as if\n> >> GIT_TRACE_CURL=1 and GIT_TRACE_CURL_NO_DATA=1 is set, instead of setting\n> >> CURLOPT_VERBOSE.\n> >> \n> >> This is to prevent inadvertent revelation of sensitive data. In\n> >> particular, GIT_CURL_VERBOSE redacts neither the \"Authorization\" header\n> >> nor any cookies specified by GIT_REDACT_COOKIES.\n> >\n> > I actually use GIT_CURL_VERBOSE to debug authentication problems from\n> > time to time, so I'd like to keep an option to produce full, unredacted\n> > output.  Since everyone uses HTTPS, it's not possible to perform this\n> > debugging using a tool like Wireshark unless you use a MITM CA cert,\n> > which seems excessive.\n> \n> Hmm, that is a valid concern.  Introducing yet another environment\n> feels a bit yucky, but something like GIT_NO_REDACT that disables\n> any redacting, not limited to curl but in all codepaths, might turn\n> out to be a useful escape hatch.\n> \n> Opinions?\n\nHaving an environment variable was my first thought, as well. I do\nthink it's key that the default be to redact. That makes life slightly\nharder for people debugging auth problems, but prevents people from\naccidentally leaking private info.\n\nRegarding the name:\n\n  - should it be under GIT_TRACE_CURL_* to make its impact clear? Or do\n    we imagine it might eventually be applied elsewhere?\n\n  - doing GIT_TRACE_REDACT would get rid of the negative (and it could\n    just default to \"true\")\n\n-Peff\n"},{"id":"397701","messageId":"xmqqk11gwhkc.fsf@gitster.c.googlers.com","threadId":"53446","inReplyTo":"20200513045001.GA65110@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] http, imap-send: stop using CURLOPT_VERBOSE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-13T05:05:23Z","receivedAt":"2020-05-13T05:05:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> Hmm, that is a valid concern.  Introducing yet another environment\n>> feels a bit yucky, but something like GIT_NO_REDACT that disables\n>> any redacting, not limited to curl but in all codepaths, might turn\n>> out to be a useful escape hatch.\n>> \n>> Opinions?\n>\n> Having an environment variable was my first thought, as well. I do\n> think it's key that the default be to redact. That makes life slightly\n> harder for people debugging auth problems, but prevents people from\n> accidentally leaking private info.\n>\n> Regarding the name:\n>\n>   - should it be under GIT_TRACE_CURL_* to make its impact clear? Or do\n>     we imagine it might eventually be applied elsewhere?\n>\n>   - doing GIT_TRACE_REDACT would get rid of the negative (and it could\n>     just default to \"true\")\n\nI like the latter, and I do expect it to allow the person who debugs\nto temporarily disable *any* codepath that redacts its output.\n\nThanks.\n"},{"id":"397705","messageId":"alpine.DEB.2.20.2005130812500.28445@tvnag.unkk.fr","threadId":"53446","inReplyTo":"20200512231331.GA6605@camp.crustytoothpaste.net","subject":"Re: [PATCH 2/2] http, imap-send: stop using CURLOPT_VERBOSE","fromName":"Daniel Stenberg","fromEmail":"daniel@haxx.se","sentAt":"2020-05-13T06:16:59Z","receivedAt":"2020-05-13T06:18:15Z","isPatch":true,"sender":{"key":"daniel@haxx.se","avatar":"https://gravatar.com/avatar/69fdca87edd17cee21ca2e79fc2ff671d644603c3dc27167430f3cd3dbab7ba8?d=mp&s=160"},"body":"On Tue, 12 May 2020, brian m. carlson wrote:\n\nSorry for going slightly off-topic, but I figure this could help as a sort of \nPSA.\n\n> Since everyone uses HTTPS, it's not possible to perform this debugging using \n> a tool like Wireshark unless you use a MITM CA cert, which seems excessive.\n\nWhen you want to Wireshark the connection with libcurl, your friend is \nSSLKEYLOGFILE. If you set that environment variable (assuming the libcurl TLS \nbackend supports it - several do), libcurl will save the TLS secrets in the \nfile that environment variable mentions - at run-time in a format that \nWireshark understands.\n\nThen you can analyze the traffic, in real time, with Wirehark without fiddling \nwith a MITM etc.\n\n-- \n\n  / daniel.haxx.se\n"},{"id":"397714","messageId":"20200513144513.GA639902@coredump.intra.peff.net","threadId":"53446","inReplyTo":"alpine.DEB.2.20.2005130812500.28445@tvnag.unkk.fr","subject":"Re: [PATCH 2/2] http, imap-send: stop using CURLOPT_VERBOSE","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-13T14:45:13Z","receivedAt":"2020-05-13T15:05:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 13, 2020 at 08:16:59AM +0200, Daniel Stenberg wrote:\n\n> On Tue, 12 May 2020, brian m. carlson wrote:\n> \n> Sorry for going slightly off-topic, but I figure this could help as a sort\n> of PSA.\n> \n> > Since everyone uses HTTPS, it's not possible to perform this debugging\n> > using a tool like Wireshark unless you use a MITM CA cert, which seems\n> > excessive.\n> \n> When you want to Wireshark the connection with libcurl, your friend is\n> SSLKEYLOGFILE. If you set that environment variable (assuming the libcurl\n> TLS backend supports it - several do), libcurl will save the TLS secrets in\n> the file that environment variable mentions - at run-time in a format that\n> Wireshark understands.\n> \n> Then you can analyze the traffic, in real time, with Wirehark without\n> fiddling with a MITM etc.\n\nVery cool. I didn't know about that at all. Now I want to debug some\nhttps sessions... ;)\n\nThank you!\n\n-Peff\n"},{"id":"397734","messageId":"cover.1589394456.git.jonathantanmy@google.com","threadId":"53446","inReplyTo":"cover.1589218693.git.jonathantanmy@google.com","subject":"[PATCH v2 0/3] Safer GIT_CURL_VERBOSE","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-05-13T19:12:45Z","receivedAt":"2020-05-13T19:12:57Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Thanks everyone. I went ahead with GIT_REDACT_AUTHORIZATION to match\nGIT_REDACT_COOKIES, with the default being true (i.e. you need to set it\nto \"0\" to have behavior change).\n\nAn alternative is to name it as non-authorization-specific, e.g.\nGIT_TRACE_REDACT, as suggested by others. But as far as I can tell, we\ncurrently only redact auth (by default) and cookies (opt-in, since we\nneed the user to tell us exactly which cookies to redact), so it seems\nbetter to me to have auth redaction be a peer to cookie redaction,\nrather than being controlled by a flag that controls everything.\n\nJonathan Tan (3):\n  t5551: test that GIT_TRACE_CURL redacts password\n  http: make GIT_TRACE_CURL auth redaction optional\n  http, imap-send: stop using CURLOPT_VERBOSE\n\n Documentation/git.txt        |  8 +++++--\n http.c                       | 19 ++++++++++++---\n http.h                       |  7 ++++++\n imap-send.c                  |  2 +-\n t/t5551-http-fetch-smart.sh  | 46 ++++++++++++++++++++++++++++++++++++\n t/t5581-http-curl-verbose.sh |  2 +-\n trace.c                      | 20 ++++++++++++----\n trace.h                      |  6 +++++\n 8 files changed, 99 insertions(+), 11 deletions(-)\n\nRange-diff against v1:\n-:  ---------- > 1:  8c70a45b24 http: make GIT_TRACE_CURL auth redaction optional\n1:  1df9e9deb7 ! 2:  f5a29e8fa1 http, imap-send: stop using CURLOPT_VERBOSE\n    @@ imap-send.c: static CURL *setup_curl(struct imap_server_conf *srvc, struct crede\n      \treturn curl;\n     \n      ## t/t5551-http-fetch-smart.sh ##\n    -@@ t/t5551-http-fetch-smart.sh: test_expect_success 'GIT_TRACE_CURL redacts auth details' '\n    - \tgrep \"Authorization: Basic <redacted>\" trace\n    +@@ t/t5551-http-fetch-smart.sh: test_expect_success 'GIT_TRACE_CURL does not redact auth details if GIT_REDACT_A\n    + \tgrep \"Authorization: Basic [0-9a-zA-Z+/]\" trace\n      '\n      \n     +test_expect_success 'GIT_CURL_VERBOSE redacts auth details' '\n-- \n2.26.2.645.ge9eca65c58-goog\n\n"},{"id":"397735","messageId":"8c70a45b24861290b64d4487383bdfa5960332ab.1589394456.git.jonathantanmy@google.com","threadId":"53446","inReplyTo":"cover.1589394456.git.jonathantanmy@google.com","subject":"[PATCH v2 2/3] http: make GIT_TRACE_CURL auth redaction optional","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-05-13T19:12:47Z","receivedAt":"2020-05-13T19:13:00Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"By default, auth headers are redacted when a trace is output through\nGIT_TRACE_CURL. Allow the user to inhibit this redaction through an\nenvironment variable.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n Documentation/git.txt       |  6 ++++++\n http.c                      | 11 +++++++++--\n t/t5551-http-fetch-smart.sh | 10 ++++++++++\n 3 files changed, 25 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex 9d6769e95a..af98cd7dc2 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -777,6 +777,12 @@ 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_AUTHORIZATION`::\n+\tBy default, when a curl trace is enabled (see `GIT_TRACE_CURL` above),\n+\tthe values of \"Authorization:\" and \"Proxy-Authorization:\" headers in\n+\tthe trace are redacted. Set this variable to `0` to prevent this\n+\tredaction.\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\ndiff --git a/http.c b/http.c\nindex 62aa995245..77eac95d64 100644\n--- a/http.c\n+++ b/http.c\n@@ -18,6 +18,7 @@\n \n static struct trace_key trace_curl = TRACE_KEY_INIT(CURL);\n static int trace_curl_data = 1;\n+static int trace_curl_redact_authorization = 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@@ -642,8 +643,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_authorization &&\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@@ -859,6 +861,7 @@ static int get_curl_http_version_opt(const char *version_string, long *opt)\n static CURL *get_curl_handle(void)\n {\n \tCURL *result = curl_easy_init();\n+\tconst char *redact_authorization_envvar;\n \n \tif (!result)\n \t\tdie(\"curl_easy_init failed\");\n@@ -997,6 +1000,10 @@ 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+\tredact_authorization_envvar = getenv(\"GIT_REDACT_AUTHORIZATION\");\n+\tif (redact_authorization_envvar &&\n+\t    !strcmp(redact_authorization_envvar, \"0\"))\n+\t\ttrace_curl_redact_authorization = 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 acc8473a72..eeecfe01d7 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -197,6 +197,16 @@ test_expect_success 'GIT_TRACE_CURL 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_REDACT_AUTHORIZATION=0' '\n+\trm -rf redact-auth trace &&\n+\tset_askpass user@host pass@host &&\n+\tGIT_REDACT_AUTHORIZATION=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-- \n2.26.2.645.ge9eca65c58-goog\n\n"},{"id":"397736","messageId":"bee4fc7241596a6b4206fa8b52dfdaea88180f37.1589394456.git.jonathantanmy@google.com","threadId":"53446","inReplyTo":"cover.1589394456.git.jonathantanmy@google.com","subject":"[PATCH v2 1/3] t5551: test that GIT_TRACE_CURL redacts password","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-05-13T19:12:46Z","receivedAt":"2020-05-13T19:13:01Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Verify that when GIT_TRACE_CURL is set, Git prints out \"Authorization:\nBasic <redacted>\" instead of the base64-encoded authorization details.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n t/t5551-http-fetch-smart.sh | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 6788aeface..acc8473a72 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -185,6 +185,18 @@ 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+\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+\texpect_askpass both user@host &&\n+\n+\t# Ensure that there is no \"Basic\" followed by a base64 string, but that\n+\t# the auth details are redacted\n+\t! grep \"Authorization: Basic [0-9a-zA-Z+/]\" trace &&\n+\tgrep \"Authorization: Basic <redacted>\" 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-- \n2.26.2.645.ge9eca65c58-goog\n\n"},{"id":"397737","messageId":"f5a29e8fa1c97a80237b2f1414536c75a9911060.1589394456.git.jonathantanmy@google.com","threadId":"53446","inReplyTo":"cover.1589394456.git.jonathantanmy@google.com","subject":"[PATCH v2 3/3] http, imap-send: stop using CURLOPT_VERBOSE","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-05-13T19:12:48Z","receivedAt":"2020-05-13T19:13:03Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Whenever GIT_CURL_VERBOSE is set, teach Git to behave as if\nGIT_TRACE_CURL=1 and GIT_TRACE_CURL_NO_DATA=1 is set, instead of setting\nCURLOPT_VERBOSE.\n\nThis is to prevent inadvertent revelation of sensitive data. In\nparticular, GIT_CURL_VERBOSE redacts neither the \"Authorization\" header\nnor any cookies specified by GIT_REDACT_COOKIES.\n\nUnifying the tracing mechanism also has the future benefit that any\nimprovements to the tracing mechanism will benefit both users of\nGIT_CURL_VERBOSE and GIT_TRACE_CURL, and we do not need to remember to\nimplement any improvement twice.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n Documentation/git.txt        |  2 --\n http.c                       |  8 +++++++-\n http.h                       |  7 +++++++\n imap-send.c                  |  2 +-\n t/t5551-http-fetch-smart.sh  | 24 ++++++++++++++++++++++++\n t/t5581-http-curl-verbose.sh |  2 +-\n trace.c                      | 20 ++++++++++++++++----\n trace.h                      |  6 ++++++\n 8 files changed, 62 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex af98cd7dc2..43a440bae4 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -721,8 +721,6 @@ of clones and fetches.\n \tEnables a curl full trace dump of all incoming and outgoing data,\n \tincluding descriptive information, of the git transport protocol.\n \tThis is similar to doing curl `--trace-ascii` on the command line.\n-\tThis option overrides setting the `GIT_CURL_VERBOSE` environment\n-\tvariable.\n \tSee `GIT_TRACE` for available trace output options.\n \n `GIT_TRACE_CURL_NO_DATA`::\ndiff --git a/http.c b/http.c\nindex 77eac95d64..d0cea72cbd 100644\n--- a/http.c\n+++ b/http.c\n@@ -806,6 +806,12 @@ static int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size,\n \treturn 0;\n }\n \n+void http_trace_curl_no_data(void)\n+{\n+\ttrace_override_envvar(&trace_curl, \"1\");\n+\ttrace_curl_data = 0;\n+}\n+\n void setup_curl_trace(CURL *handle)\n {\n \tif (!trace_want(&trace_curl))\n@@ -996,7 +1002,7 @@ static CURL *get_curl_handle(void)\n \twarning(_(\"Protocol restrictions not supported with cURL < 7.19.4\"));\n #endif\n \tif (getenv(\"GIT_CURL_VERBOSE\"))\n-\t\tcurl_easy_setopt(result, CURLOPT_VERBOSE, 1L);\n+\t\thttp_trace_curl_no_data();\n \tsetup_curl_trace(result);\n \tif (getenv(\"GIT_TRACE_CURL_NO_DATA\"))\n \t\ttrace_curl_data = 0;\ndiff --git a/http.h b/http.h\nindex 5e0ad724f9..faf8cbb0d1 100644\n--- a/http.h\n+++ b/http.h\n@@ -252,6 +252,13 @@ int finish_http_object_request(struct http_object_request *freq);\n void abort_http_object_request(struct http_object_request *freq);\n void release_http_object_request(struct http_object_request *freq);\n \n+/*\n+ * Instead of using environment variables to determine if curl tracing happens,\n+ * behave as if GIT_TRACE_CURL=1 and GIT_TRACE_CURL_NO_DATA=1 is set. Call this\n+ * before calling setup_curl_trace().\n+ */\n+void http_trace_curl_no_data(void);\n+\n /* setup routine for curl_easy_setopt CURLOPT_DEBUGFUNCTION */\n void setup_curl_trace(CURL *handle);\n #endif /* HTTP_H */\ndiff --git a/imap-send.c b/imap-send.c\nindex 6c54d8c29d..52737546f3 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -1464,7 +1464,7 @@ static CURL *setup_curl(struct imap_server_conf *srvc, struct credential *cred)\n \tcurl_easy_setopt(curl, CURLOPT_UPLOAD, 1L);\n \n \tif (0 < verbosity || getenv(\"GIT_CURL_VERBOSE\"))\n-\t\tcurl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);\n+\t\thttp_trace_curl_no_data();\n \tsetup_curl_trace(curl);\n \n \treturn curl;\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex eeecfe01d7..209f2f8427 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -207,6 +207,18 @@ test_expect_success 'GIT_TRACE_CURL does not redact auth details if GIT_REDACT_A\n \tgrep \"Authorization: Basic [0-9a-zA-Z+/]\" trace\n '\n \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+\texpect_askpass both user@host &&\n+\n+\t# Ensure that there is no \"Basic\" followed by a base64 string, but that\n+\t# the auth details are redacted\n+\t! grep \"Authorization: Basic [0-9a-zA-Z+/]\" trace &&\n+\tgrep \"Authorization: Basic <redacted>\" 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@@ -464,6 +476,18 @@ test_expect_success 'GIT_REDACT_COOKIES redacts cookies' '\n \t! grep \"Cookie:.*Bar=2\" err\n '\n \n+test_expect_success 'GIT_REDACT_COOKIES redacts cookies when GIT_CURL_VERBOSE=1' '\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+\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+'\n+\n test_expect_success 'GIT_REDACT_COOKIES handles empty values' '\n \trm -rf clone &&\n \techo \"Set-Cookie: Foo=\" >cookies &&\ndiff --git a/t/t5581-http-curl-verbose.sh b/t/t5581-http-curl-verbose.sh\nindex 5129b0724f..927aad0820 100755\n--- a/t/t5581-http-curl-verbose.sh\n+++ b/t/t5581-http-curl-verbose.sh\n@@ -20,7 +20,7 @@ test_expect_success 'failure in git-upload-pack is shown' '\n \ttest_might_fail env GIT_CURL_VERBOSE=1 \\\n \t\tgit clone \"$HTTPD_URL/error_git_upload_pack/smart/repo.git\" \\\n \t\t2>curl_log &&\n-\tgrep \"< HTTP/1.1 500 Intentional Breakage\" curl_log\n+\tgrep \"<= Recv header: HTTP/1.1 500 Intentional Breakage\" curl_log\n '\n \n test_done\ndiff --git a/trace.c b/trace.c\nindex b3ef0e627f..f726686fd9 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -29,7 +29,7 @@ struct trace_key trace_perf_key = TRACE_KEY_INIT(PERFORMANCE);\n struct trace_key trace_setup_key = TRACE_KEY_INIT(SETUP);\n \n /* Get a trace file descriptor from \"key\" env variable. */\n-static int get_trace_fd(struct trace_key *key)\n+static int get_trace_fd(struct trace_key *key, const char *override_envvar)\n {\n \tconst char *trace;\n \n@@ -37,7 +37,7 @@ static int get_trace_fd(struct trace_key *key)\n \tif (key->initialized)\n \t\treturn key->fd;\n \n-\ttrace = getenv(key->key);\n+\ttrace = override_envvar ? override_envvar : getenv(key->key);\n \n \tif (!trace || !strcmp(trace, \"\") ||\n \t    !strcmp(trace, \"0\") || !strcasecmp(trace, \"false\"))\n@@ -68,6 +68,18 @@ static int get_trace_fd(struct trace_key *key)\n \treturn key->fd;\n }\n \n+void trace_override_envvar(struct trace_key *key, const char *value)\n+{\n+\ttrace_disable(key);\n+\tkey->initialized = 0;\n+\n+\t/*\n+\t * Invoke get_trace_fd() to initialize key using the given value\n+\t * instead of the value of the environment variable.\n+\t */\n+\tget_trace_fd(key, value);\n+}\n+\n void trace_disable(struct trace_key *key)\n {\n \tif (key->need_close)\n@@ -112,7 +124,7 @@ static int prepare_trace_line(const char *file, int line,\n \n static void trace_write(struct trace_key *key, const void *buf, unsigned len)\n {\n-\tif (write_in_full(get_trace_fd(key), buf, len) < 0) {\n+\tif (write_in_full(get_trace_fd(key, NULL), buf, len) < 0) {\n \t\twarning(\"unable to write trace for %s: %s\",\n \t\t\tkey->key, strerror(errno));\n \t\ttrace_disable(key);\n@@ -383,7 +395,7 @@ void trace_repo_setup(const char *prefix)\n \n int trace_want(struct trace_key *key)\n {\n-\treturn !!get_trace_fd(key);\n+\treturn !!get_trace_fd(key, NULL);\n }\n \n #if defined(HAVE_CLOCK_GETTIME) && defined(HAVE_CLOCK_MONOTONIC)\ndiff --git a/trace.h b/trace.h\nindex 9826618b33..0dbbad0e41 100644\n--- a/trace.h\n+++ b/trace.h\n@@ -101,6 +101,12 @@ void trace_repo_setup(const char *prefix);\n  */\n int trace_want(struct trace_key *key);\n \n+/**\n+ * Enables or disables tracing for the specified key, as if the environment\n+ * variable was set to the given value.\n+ */\n+void trace_override_envvar(struct trace_key *key, const char *value);\n+\n /**\n  * Disables tracing for the specified key, even if the environment variable\n  * was set.\n-- \n2.26.2.645.ge9eca65c58-goog\n\n"},{"id":"397738","messageId":"xmqqwo5ftz37.fsf@gitster.c.googlers.com","threadId":"53446","inReplyTo":"cover.1589394456.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 0/3] Safer GIT_CURL_VERBOSE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-13T19:27:24Z","receivedAt":"2020-05-13T19:27:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> Thanks everyone. I went ahead with GIT_REDACT_AUTHORIZATION to match\n> GIT_REDACT_COOKIES, with the default being true (i.e. you need to set it\n> to \"0\" to have behavior change).\n\nHmm, I would actually have expected us to move in the direction of\ndeprecating specific REDACT_BLAH and consolidate them into one,\ninstead of adding another one.  Especially as the primary reason why\nwe redact cookies is to protect those that are used for auth anyway.\n"},{"id":"397740","messageId":"xmqqpnb7tyz7.fsf@gitster.c.googlers.com","threadId":"53446","inReplyTo":"8c70a45b24861290b64d4487383bdfa5960332ab.1589394456.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 2/3] http: make GIT_TRACE_CURL auth redaction optional","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-13T19:29:48Z","receivedAt":"2020-05-13T19:29:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> diff --git a/http.c b/http.c\n> index 62aa995245..77eac95d64 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -18,6 +18,7 @@\n>  \n>  static struct trace_key trace_curl = TRACE_KEY_INIT(CURL);\n>  static int trace_curl_data = 1;\n> +static int trace_curl_redact_authorization = 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> @@ -642,8 +643,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_authorization &&\n> +\t    (skip_prefix(header->buf, \"Authorization:\", &sensitive_header) ||\n> +\t     skip_prefix(header->buf, \"Proxy-Authorization:\", &sensitive_header))) {\n\nNice.\n\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> @@ -859,6 +861,7 @@ static int get_curl_http_version_opt(const char *version_string, long *opt)\n>  static CURL *get_curl_handle(void)\n>  {\n>  \tCURL *result = curl_easy_init();\n> +\tconst char *redact_authorization_envvar;\n>  \n>  \tif (!result)\n>  \t\tdie(\"curl_easy_init failed\");\n> @@ -997,6 +1000,10 @@ 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> +\tredact_authorization_envvar = getenv(\"GIT_REDACT_AUTHORIZATION\");\n> +\tif (redact_authorization_envvar &&\n> +\t    !strcmp(redact_authorization_envvar, \"0\"))\n> +\t\ttrace_curl_redact_authorization = 0;\n\nWasn't git_env_bool() designed exactly for this kind of usage?\n\nThanks.\n"},{"id":"397743","messageId":"xmqqlflvtysu.fsf@gitster.c.googlers.com","threadId":"53446","inReplyTo":"xmqqwo5ftz37.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 0/3] Safer GIT_CURL_VERBOSE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-13T19:33:37Z","receivedAt":"2020-05-13T19:33:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jonathan Tan <jonathantanmy@google.com> writes:\n>\n>> Thanks everyone. I went ahead with GIT_REDACT_AUTHORIZATION to match\n>> GIT_REDACT_COOKIES, with the default being true (i.e. you need to set it\n>> to \"0\" to have behavior change).\n>\n> Hmm, I would actually have expected us to move in the direction of\n> deprecating specific REDACT_BLAH and consolidate them into one,\n> instead of adding another one.  Especially as the primary reason why\n> we redact cookies is to protect those that are used for auth anyway.\n\nAlso I had forgot to grep for anonymi.e to find transport_anonymize_url(),\nwhich I was hoping that the new environment variable would cover to help\nthose who debug.\n\nThanks.\n"},{"id":"397933","messageId":"20200515204729.GA115445@coredump.intra.peff.net","threadId":"53446","inReplyTo":"xmqqlflvtysu.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 0/3] Safer GIT_CURL_VERBOSE","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-15T20:47:29Z","receivedAt":"2020-05-15T20:47:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 13, 2020 at 12:33:37PM -0700, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > Jonathan Tan <jonathantanmy@google.com> writes:\n> >\n> >> Thanks everyone. I went ahead with GIT_REDACT_AUTHORIZATION to match\n> >> GIT_REDACT_COOKIES, with the default being true (i.e. you need to set it\n> >> to \"0\" to have behavior change).\n> >\n> > Hmm, I would actually have expected us to move in the direction of\n> > deprecating specific REDACT_BLAH and consolidate them into one,\n> > instead of adding another one.  Especially as the primary reason why\n> > we redact cookies is to protect those that are used for auth anyway.\n> \n> Also I had forgot to grep for anonymi.e to find transport_anonymize_url(),\n> which I was hoping that the new environment variable would cover to help\n> those who debug.\n\nYeah, I'd agree with both of these. If Jonathan doesn't feel like\nworking on transport_anonymize_url() now, I don't mind if we leave that\nfor later. But let's come up with a general scheme that we're aiming\nfor, so we minimize changes to things that users are exposed to.\n\nIMHO an option that only impacts the format of the human-readable trace\noutput is not something we need to deprecate. It's an internal detail,\nand people can't rely on the exact format of the trace anyway. So I'd be\nfine to just kill off GIT_REDACT_COOKIES completely (especially if the\ndefault behavior is the safer \"always redact\").\n\nThat said, a boolean GIT_REDACT doesn't quite do the same thing, because\nit's actually a list of cookies. My gut feeling is that this is a bit\nover-engineered. Any cookies that Git uses are likely to be sensitive,\nso just treating their values like auth (redacting by default, but\nallowing them to be unblinded when the user asks for it). But maybe\nJonathan had a specific tracing case in mind, as the author of the\noriginal.\n\n-Peff\n"}]}