* [PATCH 0/2] Safer GIT_CURL_VERBOSE @ 2020-05-11 17:43 Jonathan Tan 2020-05-11 17:43 ` [PATCH 1/2] t5551: test that GIT_TRACE_CURL redacts password Jonathan Tan ` (2 more replies) 0 siblings, 3 replies; 21+ messages in thread From: Jonathan Tan @ 2020-05-11 17:43 UTC (permalink / raw) To: git; +Cc: Jonathan Tan These are patches that unify GIT_CURL_VERBOSE and GIT_TRACE_CURL. This helps avoid GIT_CURL_VERBOSE being a source of confusion, especially with the presence of GIT_REDACT_COOKIES. The motivation is further explained in the commit message of patch 2. I noticed some commits that refer to GIT_CURL_VERBOSE as deprecated (see what 930b67ebd7 ("Merge branch 'ep/use-git-trace-curl-in-tests'", 2016-09-12) merged in), although it seems that they are just referring to the fact that GIT_TRACE_CURL has more features than GIT_CURL_VERBOSE and should be preferred (74c682d3c6 ("http.c: implement the GIT_TRACE_CURL environment variable", 2016-05-24)). I haven't made any references to deprecating anything in any of the commit messages, but deprecating GIT_CURL_VERBOSE (so that we have only one way of doing things) seems good to me - but this is a matter for another patch, I think. Jonathan Tan (2): t5551: test that GIT_TRACE_CURL redacts password http, imap-send: stop using CURLOPT_VERBOSE Documentation/git.txt | 2 -- http.c | 8 +++++++- http.h | 7 +++++++ imap-send.c | 2 +- t/t5551-http-fetch-smart.sh | 36 ++++++++++++++++++++++++++++++++++++ t/t5581-http-curl-verbose.sh | 2 +- trace.c | 20 ++++++++++++++++---- trace.h | 6 ++++++ 8 files changed, 74 insertions(+), 9 deletions(-) -- 2.26.2.645.ge9eca65c58-goog ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 1/2] t5551: test that GIT_TRACE_CURL redacts password 2020-05-11 17:43 [PATCH 0/2] Safer GIT_CURL_VERBOSE Jonathan Tan @ 2020-05-11 17:43 ` Jonathan Tan 2020-05-12 19:08 ` Jeff King 2020-05-11 17:43 ` [PATCH 2/2] http, imap-send: stop using CURLOPT_VERBOSE Jonathan Tan 2020-05-13 19:12 ` [PATCH v2 0/3] Safer GIT_CURL_VERBOSE Jonathan Tan 2 siblings, 1 reply; 21+ messages in thread From: Jonathan Tan @ 2020-05-11 17:43 UTC (permalink / raw) To: git; +Cc: Jonathan Tan Verify that when GIT_TRACE_CURL is set, Git prints out "Authorization: Basic <redacted>" instead of the base64-encoded authorization details. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- t/t5551-http-fetch-smart.sh | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh index 6788aeface..acc8473a72 100755 --- a/t/t5551-http-fetch-smart.sh +++ b/t/t5551-http-fetch-smart.sh @@ -185,6 +185,18 @@ test_expect_success 'redirects send auth to new location' ' expect_askpass both user@host auth/smart/repo.git ' +test_expect_success 'GIT_TRACE_CURL redacts auth details' ' + rm -rf redact-auth trace && + set_askpass user@host pass@host && + GIT_TRACE_CURL="$(pwd)/trace" git clone --bare "$HTTPD_URL/auth/smart/repo.git" redact-auth && + expect_askpass both user@host && + + # Ensure that there is no "Basic" followed by a base64 string, but that + # the auth details are redacted + ! grep "Authorization: Basic [0-9a-zA-Z+/]" trace && + grep "Authorization: Basic <redacted>" trace +' + test_expect_success 'disable dumb http on server' ' git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \ config http.getanyfile false -- 2.26.2.645.ge9eca65c58-goog ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] t5551: test that GIT_TRACE_CURL redacts password 2020-05-11 17:43 ` [PATCH 1/2] t5551: test that GIT_TRACE_CURL redacts password Jonathan Tan @ 2020-05-12 19:08 ` Jeff King 0 siblings, 0 replies; 21+ messages in thread From: Jeff King @ 2020-05-12 19:08 UTC (permalink / raw) To: Jonathan Tan; +Cc: git On Mon, May 11, 2020 at 10:43:09AM -0700, Jonathan Tan wrote: > Verify that when GIT_TRACE_CURL is set, Git prints out "Authorization: > Basic <redacted>" instead of the base64-encoded authorization details. Yeah, it's definitely worth testing this. The patch looks good to me. -Peff ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/2] http, imap-send: stop using CURLOPT_VERBOSE 2020-05-11 17:43 [PATCH 0/2] Safer GIT_CURL_VERBOSE Jonathan Tan 2020-05-11 17:43 ` [PATCH 1/2] t5551: test that GIT_TRACE_CURL redacts password Jonathan Tan @ 2020-05-11 17:43 ` Jonathan Tan 2020-05-12 19:16 ` Jeff King 2020-05-12 23:13 ` brian m. carlson 2020-05-13 19:12 ` [PATCH v2 0/3] Safer GIT_CURL_VERBOSE Jonathan Tan 2 siblings, 2 replies; 21+ messages in thread From: Jonathan Tan @ 2020-05-11 17:43 UTC (permalink / raw) To: git; +Cc: Jonathan Tan Whenever GIT_CURL_VERBOSE is set, teach Git to behave as if GIT_TRACE_CURL=1 and GIT_TRACE_CURL_NO_DATA=1 is set, instead of setting CURLOPT_VERBOSE. This is to prevent inadvertent revelation of sensitive data. In particular, GIT_CURL_VERBOSE redacts neither the "Authorization" header nor any cookies specified by GIT_REDACT_COOKIES. Unifying the tracing mechanism also has the future benefit that any improvements to the tracing mechanism will benefit both users of GIT_CURL_VERBOSE and GIT_TRACE_CURL, and we do not need to remember to implement any improvement twice. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- Documentation/git.txt | 2 -- http.c | 8 +++++++- http.h | 7 +++++++ imap-send.c | 2 +- t/t5551-http-fetch-smart.sh | 24 ++++++++++++++++++++++++ t/t5581-http-curl-verbose.sh | 2 +- trace.c | 20 ++++++++++++++++---- trace.h | 6 ++++++ 8 files changed, 62 insertions(+), 9 deletions(-) diff --git a/Documentation/git.txt b/Documentation/git.txt index 9d6769e95a..427ea70701 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -721,8 +721,6 @@ of clones and fetches. Enables a curl full trace dump of all incoming and outgoing data, including descriptive information, of the git transport protocol. This is similar to doing curl `--trace-ascii` on the command line. - This option overrides setting the `GIT_CURL_VERBOSE` environment - variable. See `GIT_TRACE` for available trace output options. `GIT_TRACE_CURL_NO_DATA`:: diff --git a/http.c b/http.c index 62aa995245..4882c9f5b2 100644 --- a/http.c +++ b/http.c @@ -804,6 +804,12 @@ static int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size, return 0; } +void http_trace_curl_no_data(void) +{ + trace_override_envvar(&trace_curl, "1"); + trace_curl_data = 0; +} + void setup_curl_trace(CURL *handle) { if (!trace_want(&trace_curl)) @@ -993,7 +999,7 @@ static CURL *get_curl_handle(void) warning(_("Protocol restrictions not supported with cURL < 7.19.4")); #endif if (getenv("GIT_CURL_VERBOSE")) - curl_easy_setopt(result, CURLOPT_VERBOSE, 1L); + http_trace_curl_no_data(); setup_curl_trace(result); if (getenv("GIT_TRACE_CURL_NO_DATA")) trace_curl_data = 0; diff --git a/http.h b/http.h index 5e0ad724f9..faf8cbb0d1 100644 --- a/http.h +++ b/http.h @@ -252,6 +252,13 @@ int finish_http_object_request(struct http_object_request *freq); void abort_http_object_request(struct http_object_request *freq); void release_http_object_request(struct http_object_request *freq); +/* + * Instead of using environment variables to determine if curl tracing happens, + * behave as if GIT_TRACE_CURL=1 and GIT_TRACE_CURL_NO_DATA=1 is set. Call this + * before calling setup_curl_trace(). + */ +void http_trace_curl_no_data(void); + /* setup routine for curl_easy_setopt CURLOPT_DEBUGFUNCTION */ void setup_curl_trace(CURL *handle); #endif /* HTTP_H */ diff --git a/imap-send.c b/imap-send.c index 6c54d8c29d..52737546f3 100644 --- a/imap-send.c +++ b/imap-send.c @@ -1464,7 +1464,7 @@ static CURL *setup_curl(struct imap_server_conf *srvc, struct credential *cred) curl_easy_setopt(curl, CURLOPT_UPLOAD, 1L); if (0 < verbosity || getenv("GIT_CURL_VERBOSE")) - curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L); + http_trace_curl_no_data(); setup_curl_trace(curl); return curl; diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh index acc8473a72..be01cf7bb2 100755 --- a/t/t5551-http-fetch-smart.sh +++ b/t/t5551-http-fetch-smart.sh @@ -197,6 +197,18 @@ test_expect_success 'GIT_TRACE_CURL redacts auth details' ' grep "Authorization: Basic <redacted>" trace ' +test_expect_success 'GIT_CURL_VERBOSE redacts auth details' ' + rm -rf redact-auth trace && + set_askpass user@host pass@host && + GIT_CURL_VERBOSE=1 git clone --bare "$HTTPD_URL/auth/smart/repo.git" redact-auth 2>trace && + expect_askpass both user@host && + + # Ensure that there is no "Basic" followed by a base64 string, but that + # the auth details are redacted + ! grep "Authorization: Basic [0-9a-zA-Z+/]" trace && + grep "Authorization: Basic <redacted>" trace +' + test_expect_success 'disable dumb http on server' ' git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \ config http.getanyfile false @@ -454,6 +466,18 @@ test_expect_success 'GIT_REDACT_COOKIES redacts cookies' ' ! 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=<redacted>" err && + ! grep "Cookie:.*Bar=2" err +' + test_expect_success 'GIT_REDACT_COOKIES handles empty values' ' rm -rf clone && echo "Set-Cookie: Foo=" >cookies && diff --git a/t/t5581-http-curl-verbose.sh b/t/t5581-http-curl-verbose.sh index 5129b0724f..927aad0820 100755 --- a/t/t5581-http-curl-verbose.sh +++ b/t/t5581-http-curl-verbose.sh @@ -20,7 +20,7 @@ test_expect_success 'failure in git-upload-pack is shown' ' test_might_fail env GIT_CURL_VERBOSE=1 \ git clone "$HTTPD_URL/error_git_upload_pack/smart/repo.git" \ 2>curl_log && - grep "< HTTP/1.1 500 Intentional Breakage" curl_log + grep "<= Recv header: HTTP/1.1 500 Intentional Breakage" curl_log ' test_done diff --git a/trace.c b/trace.c index b3ef0e627f..f726686fd9 100644 --- a/trace.c +++ b/trace.c @@ -29,7 +29,7 @@ struct trace_key trace_perf_key = TRACE_KEY_INIT(PERFORMANCE); struct trace_key trace_setup_key = TRACE_KEY_INIT(SETUP); /* Get a trace file descriptor from "key" env variable. */ -static int get_trace_fd(struct trace_key *key) +static int get_trace_fd(struct trace_key *key, const char *override_envvar) { const char *trace; @@ -37,7 +37,7 @@ static int get_trace_fd(struct trace_key *key) if (key->initialized) return key->fd; - trace = getenv(key->key); + trace = override_envvar ? override_envvar : getenv(key->key); if (!trace || !strcmp(trace, "") || !strcmp(trace, "0") || !strcasecmp(trace, "false")) @@ -68,6 +68,18 @@ static int get_trace_fd(struct trace_key *key) return key->fd; } +void trace_override_envvar(struct trace_key *key, const char *value) +{ + trace_disable(key); + key->initialized = 0; + + /* + * Invoke get_trace_fd() to initialize key using the given value + * instead of the value of the environment variable. + */ + get_trace_fd(key, value); +} + void trace_disable(struct trace_key *key) { if (key->need_close) @@ -112,7 +124,7 @@ static int prepare_trace_line(const char *file, int line, static void trace_write(struct trace_key *key, const void *buf, unsigned len) { - if (write_in_full(get_trace_fd(key), buf, len) < 0) { + if (write_in_full(get_trace_fd(key, NULL), buf, len) < 0) { warning("unable to write trace for %s: %s", key->key, strerror(errno)); trace_disable(key); @@ -383,7 +395,7 @@ void trace_repo_setup(const char *prefix) int trace_want(struct trace_key *key) { - return !!get_trace_fd(key); + return !!get_trace_fd(key, NULL); } #if defined(HAVE_CLOCK_GETTIME) && defined(HAVE_CLOCK_MONOTONIC) diff --git a/trace.h b/trace.h index 9826618b33..0dbbad0e41 100644 --- a/trace.h +++ b/trace.h @@ -101,6 +101,12 @@ void trace_repo_setup(const char *prefix); */ int trace_want(struct trace_key *key); +/** + * Enables or disables tracing for the specified key, as if the environment + * variable was set to the given value. + */ +void trace_override_envvar(struct trace_key *key, const char *value); + /** * Disables tracing for the specified key, even if the environment variable * was set. -- 2.26.2.645.ge9eca65c58-goog ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 2/2] http, imap-send: stop using CURLOPT_VERBOSE 2020-05-11 17:43 ` [PATCH 2/2] http, imap-send: stop using CURLOPT_VERBOSE Jonathan Tan @ 2020-05-12 19:16 ` Jeff King 2020-05-12 19:23 ` Jonathan Tan 2020-05-12 23:13 ` brian m. carlson 1 sibling, 1 reply; 21+ messages in thread From: Jeff King @ 2020-05-12 19:16 UTC (permalink / raw) To: Jonathan Tan; +Cc: git On Mon, May 11, 2020 at 10:43:10AM -0700, Jonathan Tan wrote: > Whenever GIT_CURL_VERBOSE is set, teach Git to behave as if > GIT_TRACE_CURL=1 and GIT_TRACE_CURL_NO_DATA=1 is set, instead of setting > CURLOPT_VERBOSE. > > This is to prevent inadvertent revelation of sensitive data. In > particular, GIT_CURL_VERBOSE redacts neither the "Authorization" header > nor any cookies specified by GIT_REDACT_COOKIES. > > Unifying the tracing mechanism also has the future benefit that any > improvements to the tracing mechanism will benefit both users of > GIT_CURL_VERBOSE and GIT_TRACE_CURL, and we do not need to remember to > implement any improvement twice. Yeah, I think this is worth doing. The patch looks OK to me, though: > +void http_trace_curl_no_data(void) > +{ > + trace_override_envvar(&trace_curl, "1"); > + trace_curl_data = 0; > +} Would: setenv("GIT_TRACE_CURL", "1", 0); setenv("GIT_TRACE_CURL_NO_DATA", "0", 0); be simpler? Perhaps it makes the flow more convoluted as we'd go on to parse those variables, but it puts us on the same paths that we'd use if the user specified those things (and avoids the need for the special "override" function entirely). Other than that nit, it seems very cleanly done. -Peff PS I sometimes find the normal trace a bit verbose, but I do still want to see data. Do others feel the same? Particularly I find the "SSL" lines totally worthless (I guess maybe you could be debugging ssl stuff, but that would be the exception, I'd think). Ditto the split of data into two lines: one with the size and one with the actual data. I dunno. I haven't been debugging any git-over-http stuff lately, so it hasn't been bothering me. But I definitely have written perl scripts to extract the data to a more readable format. Maybe it would be easier if it had a few more knobs. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/2] http, imap-send: stop using CURLOPT_VERBOSE 2020-05-12 19:16 ` Jeff King @ 2020-05-12 19:23 ` Jonathan Tan 2020-05-12 19:27 ` Jeff King 0 siblings, 1 reply; 21+ messages in thread From: Jonathan Tan @ 2020-05-12 19:23 UTC (permalink / raw) To: peff; +Cc: jonathantanmy, git > > +void http_trace_curl_no_data(void) > > +{ > > + trace_override_envvar(&trace_curl, "1"); > > + trace_curl_data = 0; > > +} > > Would: > > setenv("GIT_TRACE_CURL", "1", 0); > setenv("GIT_TRACE_CURL_NO_DATA", "0", 0); > > be simpler? Perhaps it makes the flow more convoluted as we'd go on to > parse those variables, but it puts us on the same paths that we'd use if > the user specified those things (and avoids the need for the special > "override" function entirely). > > Other than that nit, it seems very cleanly done. Thanks for the review. I thought of doing that, but thought that it might add some latent complications - in particular, someone inspecting the environment variables of this running process might see some environment variables that they didn't set. But I'm OK either way. > PS I sometimes find the normal trace a bit verbose, but I do still want > to see data. Do others feel the same? Particularly I find the "SSL" > lines totally worthless (I guess maybe you could be debugging ssl > stuff, but that would be the exception, I'd think). Ditto the split > of data into two lines: one with the size and one with the actual > data. > > I dunno. I haven't been debugging any git-over-http stuff lately, so > it hasn't been bothering me. But I definitely have written perl > scripts to extract the data to a more readable format. Maybe it would > be easier if it had a few more knobs. Data can be turned on using GIT_TRACE_CURL=1 and refraining from setting GIT_TRACE_CURL_NO_DATA. What knobs were you thinking of? ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/2] http, imap-send: stop using CURLOPT_VERBOSE 2020-05-12 19:23 ` Jonathan Tan @ 2020-05-12 19:27 ` Jeff King 0 siblings, 0 replies; 21+ messages in thread From: Jeff King @ 2020-05-12 19:27 UTC (permalink / raw) To: Jonathan Tan; +Cc: git On Tue, May 12, 2020 at 12:23:00PM -0700, Jonathan Tan wrote: > > PS I sometimes find the normal trace a bit verbose, but I do still want > > to see data. Do others feel the same? Particularly I find the "SSL" > > lines totally worthless (I guess maybe you could be debugging ssl > > stuff, but that would be the exception, I'd think). Ditto the split > > of data into two lines: one with the size and one with the actual > > data. > > > > I dunno. I haven't been debugging any git-over-http stuff lately, so > > it hasn't been bothering me. But I definitely have written perl > > scripts to extract the data to a more readable format. Maybe it would > > be easier if it had a few more knobs. > > Data can be turned on using GIT_TRACE_CURL=1 and refraining from setting > GIT_TRACE_CURL_NO_DATA. What knobs were you thinking of? I still want to see data, but less cruft. I.e., something like "GIT_TRACE_CURL_SSL" (which I'd default to "off"), and probably just reducing: 15:24:01.169101 [pid=55191] http.c:702 <= Recv data, 0000000004 bytes (0x00000004) 15:24:01.169104 [pid=55191] http.c:717 <= Recv data: 3e.. to just the second line. Actually, we might not need a knob at all for SSL data. I was thinking that people might actually be debugging SSL problems with it, but since all of the non-printable characters are munged to "." anyway, it's basically useless (you can often pick out a few strings from the cert during handshake, but you'd be much better off to just connect with "openssl s_client" and ask it to dump the cert). -Peff ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/2] http, imap-send: stop using CURLOPT_VERBOSE 2020-05-11 17:43 ` [PATCH 2/2] http, imap-send: stop using CURLOPT_VERBOSE Jonathan Tan 2020-05-12 19:16 ` Jeff King @ 2020-05-12 23:13 ` brian m. carlson 2020-05-13 0:10 ` Junio C Hamano 2020-05-13 6:16 ` Daniel Stenberg 1 sibling, 2 replies; 21+ messages in thread From: brian m. carlson @ 2020-05-12 23:13 UTC (permalink / raw) To: Jonathan Tan; +Cc: git [-- Attachment #1: Type: text/plain, Size: 795 bytes --] On 2020-05-11 at 17:43:10, Jonathan Tan wrote: > Whenever GIT_CURL_VERBOSE is set, teach Git to behave as if > GIT_TRACE_CURL=1 and GIT_TRACE_CURL_NO_DATA=1 is set, instead of setting > CURLOPT_VERBOSE. > > This is to prevent inadvertent revelation of sensitive data. In > particular, GIT_CURL_VERBOSE redacts neither the "Authorization" header > nor any cookies specified by GIT_REDACT_COOKIES. I actually use GIT_CURL_VERBOSE to debug authentication problems from time to time, so I'd like to keep an option to produce full, unredacted output. Since everyone uses HTTPS, it's not possible to perform this debugging using a tool like Wireshark unless you use a MITM CA cert, which seems excessive. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 263 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/2] http, imap-send: stop using CURLOPT_VERBOSE 2020-05-12 23:13 ` brian m. carlson @ 2020-05-13 0:10 ` Junio C Hamano 2020-05-13 4:50 ` Jeff King 2020-05-13 6:16 ` Daniel Stenberg 1 sibling, 1 reply; 21+ messages in thread From: Junio C Hamano @ 2020-05-13 0:10 UTC (permalink / raw) To: brian m. carlson; +Cc: Jonathan Tan, git "brian m. carlson" <sandals@crustytoothpaste.net> writes: > On 2020-05-11 at 17:43:10, Jonathan Tan wrote: >> Whenever GIT_CURL_VERBOSE is set, teach Git to behave as if >> GIT_TRACE_CURL=1 and GIT_TRACE_CURL_NO_DATA=1 is set, instead of setting >> CURLOPT_VERBOSE. >> >> This is to prevent inadvertent revelation of sensitive data. In >> particular, GIT_CURL_VERBOSE redacts neither the "Authorization" header >> nor any cookies specified by GIT_REDACT_COOKIES. > > I actually use GIT_CURL_VERBOSE to debug authentication problems from > time to time, so I'd like to keep an option to produce full, unredacted > output. Since everyone uses HTTPS, it's not possible to perform this > debugging using a tool like Wireshark unless you use a MITM CA cert, > which seems excessive. Hmm, that is a valid concern. Introducing yet another environment feels a bit yucky, but something like GIT_NO_REDACT that disables any redacting, not limited to curl but in all codepaths, might turn out to be a useful escape hatch. Opinions? ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/2] http, imap-send: stop using CURLOPT_VERBOSE 2020-05-13 0:10 ` Junio C Hamano @ 2020-05-13 4:50 ` Jeff King 2020-05-13 5:05 ` Junio C Hamano 0 siblings, 1 reply; 21+ messages in thread From: Jeff King @ 2020-05-13 4:50 UTC (permalink / raw) To: Junio C Hamano; +Cc: brian m. carlson, Jonathan Tan, git On Tue, May 12, 2020 at 05:10:24PM -0700, Junio C Hamano wrote: > > On 2020-05-11 at 17:43:10, Jonathan Tan wrote: > >> Whenever GIT_CURL_VERBOSE is set, teach Git to behave as if > >> GIT_TRACE_CURL=1 and GIT_TRACE_CURL_NO_DATA=1 is set, instead of setting > >> CURLOPT_VERBOSE. > >> > >> This is to prevent inadvertent revelation of sensitive data. In > >> particular, GIT_CURL_VERBOSE redacts neither the "Authorization" header > >> nor any cookies specified by GIT_REDACT_COOKIES. > > > > I actually use GIT_CURL_VERBOSE to debug authentication problems from > > time to time, so I'd like to keep an option to produce full, unredacted > > output. Since everyone uses HTTPS, it's not possible to perform this > > debugging using a tool like Wireshark unless you use a MITM CA cert, > > which seems excessive. > > Hmm, that is a valid concern. Introducing yet another environment > feels a bit yucky, but something like GIT_NO_REDACT that disables > any redacting, not limited to curl but in all codepaths, might turn > out to be a useful escape hatch. > > Opinions? Having an environment variable was my first thought, as well. I do think it's key that the default be to redact. That makes life slightly harder for people debugging auth problems, but prevents people from accidentally leaking private info. Regarding the name: - should it be under GIT_TRACE_CURL_* to make its impact clear? Or do we imagine it might eventually be applied elsewhere? - doing GIT_TRACE_REDACT would get rid of the negative (and it could just default to "true") -Peff ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/2] http, imap-send: stop using CURLOPT_VERBOSE 2020-05-13 4:50 ` Jeff King @ 2020-05-13 5:05 ` Junio C Hamano 0 siblings, 0 replies; 21+ messages in thread From: Junio C Hamano @ 2020-05-13 5:05 UTC (permalink / raw) To: Jeff King; +Cc: brian m. carlson, Jonathan Tan, git Jeff King <peff@peff.net> writes: >> Hmm, that is a valid concern. Introducing yet another environment >> feels a bit yucky, but something like GIT_NO_REDACT that disables >> any redacting, not limited to curl but in all codepaths, might turn >> out to be a useful escape hatch. >> >> Opinions? > > Having an environment variable was my first thought, as well. I do > think it's key that the default be to redact. That makes life slightly > harder for people debugging auth problems, but prevents people from > accidentally leaking private info. > > Regarding the name: > > - should it be under GIT_TRACE_CURL_* to make its impact clear? Or do > we imagine it might eventually be applied elsewhere? > > - doing GIT_TRACE_REDACT would get rid of the negative (and it could > just default to "true") I like the latter, and I do expect it to allow the person who debugs to temporarily disable *any* codepath that redacts its output. Thanks. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/2] http, imap-send: stop using CURLOPT_VERBOSE 2020-05-12 23:13 ` brian m. carlson 2020-05-13 0:10 ` Junio C Hamano @ 2020-05-13 6:16 ` Daniel Stenberg 2020-05-13 14:45 ` Jeff King 1 sibling, 1 reply; 21+ messages in thread From: Daniel Stenberg @ 2020-05-13 6:16 UTC (permalink / raw) To: brian m. carlson; +Cc: Jonathan Tan, git On Tue, 12 May 2020, brian m. carlson wrote: Sorry for going slightly off-topic, but I figure this could help as a sort of PSA. > Since everyone uses HTTPS, it's not possible to perform this debugging using > a tool like Wireshark unless you use a MITM CA cert, which seems excessive. When you want to Wireshark the connection with libcurl, your friend is SSLKEYLOGFILE. If you set that environment variable (assuming the libcurl TLS backend supports it - several do), libcurl will save the TLS secrets in the file that environment variable mentions - at run-time in a format that Wireshark understands. Then you can analyze the traffic, in real time, with Wirehark without fiddling with a MITM etc. -- / daniel.haxx.se ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/2] http, imap-send: stop using CURLOPT_VERBOSE 2020-05-13 6:16 ` Daniel Stenberg @ 2020-05-13 14:45 ` Jeff King 0 siblings, 0 replies; 21+ messages in thread From: Jeff King @ 2020-05-13 14:45 UTC (permalink / raw) To: Daniel Stenberg; +Cc: brian m. carlson, Jonathan Tan, git On Wed, May 13, 2020 at 08:16:59AM +0200, Daniel Stenberg wrote: > On Tue, 12 May 2020, brian m. carlson wrote: > > Sorry for going slightly off-topic, but I figure this could help as a sort > of PSA. > > > Since everyone uses HTTPS, it's not possible to perform this debugging > > using a tool like Wireshark unless you use a MITM CA cert, which seems > > excessive. > > When you want to Wireshark the connection with libcurl, your friend is > SSLKEYLOGFILE. If you set that environment variable (assuming the libcurl > TLS backend supports it - several do), libcurl will save the TLS secrets in > the file that environment variable mentions - at run-time in a format that > Wireshark understands. > > Then you can analyze the traffic, in real time, with Wirehark without > fiddling with a MITM etc. Very cool. I didn't know about that at all. Now I want to debug some https sessions... ;) Thank you! -Peff ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 0/3] Safer GIT_CURL_VERBOSE 2020-05-11 17:43 [PATCH 0/2] Safer GIT_CURL_VERBOSE Jonathan Tan 2020-05-11 17:43 ` [PATCH 1/2] t5551: test that GIT_TRACE_CURL redacts password Jonathan Tan 2020-05-11 17:43 ` [PATCH 2/2] http, imap-send: stop using CURLOPT_VERBOSE Jonathan Tan @ 2020-05-13 19:12 ` Jonathan Tan 2020-05-13 19:12 ` [PATCH v2 1/3] t5551: test that GIT_TRACE_CURL redacts password Jonathan Tan ` (3 more replies) 2 siblings, 4 replies; 21+ messages in thread From: Jonathan Tan @ 2020-05-13 19:12 UTC (permalink / raw) To: git; +Cc: Jonathan Tan, sandals, gitster, peff Thanks everyone. I went ahead with GIT_REDACT_AUTHORIZATION to match GIT_REDACT_COOKIES, with the default being true (i.e. you need to set it to "0" to have behavior change). An alternative is to name it as non-authorization-specific, e.g. GIT_TRACE_REDACT, as suggested by others. But as far as I can tell, we currently only redact auth (by default) and cookies (opt-in, since we need the user to tell us exactly which cookies to redact), so it seems better to me to have auth redaction be a peer to cookie redaction, rather than being controlled by a flag that controls everything. Jonathan Tan (3): t5551: test that GIT_TRACE_CURL redacts password http: make GIT_TRACE_CURL auth redaction optional http, imap-send: stop using CURLOPT_VERBOSE Documentation/git.txt | 8 +++++-- http.c | 19 ++++++++++++--- http.h | 7 ++++++ imap-send.c | 2 +- t/t5551-http-fetch-smart.sh | 46 ++++++++++++++++++++++++++++++++++++ t/t5581-http-curl-verbose.sh | 2 +- trace.c | 20 ++++++++++++---- trace.h | 6 +++++ 8 files changed, 99 insertions(+), 11 deletions(-) Range-diff against v1: -: ---------- > 1: 8c70a45b24 http: make GIT_TRACE_CURL auth redaction optional 1: 1df9e9deb7 ! 2: f5a29e8fa1 http, imap-send: stop using CURLOPT_VERBOSE @@ imap-send.c: static CURL *setup_curl(struct imap_server_conf *srvc, struct crede return curl; ## t/t5551-http-fetch-smart.sh ## -@@ t/t5551-http-fetch-smart.sh: test_expect_success 'GIT_TRACE_CURL redacts auth details' ' - grep "Authorization: Basic <redacted>" trace +@@ t/t5551-http-fetch-smart.sh: test_expect_success 'GIT_TRACE_CURL does not redact auth details if GIT_REDACT_A + grep "Authorization: Basic [0-9a-zA-Z+/]" trace ' +test_expect_success 'GIT_CURL_VERBOSE redacts auth details' ' -- 2.26.2.645.ge9eca65c58-goog ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 1/3] t5551: test that GIT_TRACE_CURL redacts password 2020-05-13 19:12 ` [PATCH v2 0/3] Safer GIT_CURL_VERBOSE Jonathan Tan @ 2020-05-13 19:12 ` Jonathan Tan 2020-05-13 19:12 ` [PATCH v2 2/3] http: make GIT_TRACE_CURL auth redaction optional Jonathan Tan ` (2 subsequent siblings) 3 siblings, 0 replies; 21+ messages in thread From: Jonathan Tan @ 2020-05-13 19:12 UTC (permalink / raw) To: git; +Cc: Jonathan Tan, sandals, gitster, peff Verify that when GIT_TRACE_CURL is set, Git prints out "Authorization: Basic <redacted>" instead of the base64-encoded authorization details. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- t/t5551-http-fetch-smart.sh | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh index 6788aeface..acc8473a72 100755 --- a/t/t5551-http-fetch-smart.sh +++ b/t/t5551-http-fetch-smart.sh @@ -185,6 +185,18 @@ test_expect_success 'redirects send auth to new location' ' expect_askpass both user@host auth/smart/repo.git ' +test_expect_success 'GIT_TRACE_CURL redacts auth details' ' + rm -rf redact-auth trace && + set_askpass user@host pass@host && + GIT_TRACE_CURL="$(pwd)/trace" git clone --bare "$HTTPD_URL/auth/smart/repo.git" redact-auth && + expect_askpass both user@host && + + # Ensure that there is no "Basic" followed by a base64 string, but that + # the auth details are redacted + ! grep "Authorization: Basic [0-9a-zA-Z+/]" trace && + grep "Authorization: Basic <redacted>" trace +' + test_expect_success 'disable dumb http on server' ' git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \ config http.getanyfile false -- 2.26.2.645.ge9eca65c58-goog ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 2/3] http: make GIT_TRACE_CURL auth redaction optional 2020-05-13 19:12 ` [PATCH v2 0/3] Safer GIT_CURL_VERBOSE Jonathan Tan 2020-05-13 19:12 ` [PATCH v2 1/3] t5551: test that GIT_TRACE_CURL redacts password Jonathan Tan @ 2020-05-13 19:12 ` Jonathan Tan 2020-05-13 19:29 ` Junio C Hamano 2020-05-13 19:12 ` [PATCH v2 3/3] http, imap-send: stop using CURLOPT_VERBOSE Jonathan Tan 2020-05-13 19:27 ` [PATCH v2 0/3] Safer GIT_CURL_VERBOSE Junio C Hamano 3 siblings, 1 reply; 21+ messages in thread From: Jonathan Tan @ 2020-05-13 19:12 UTC (permalink / raw) To: git; +Cc: Jonathan Tan, sandals, gitster, peff By default, auth headers are redacted when a trace is output through GIT_TRACE_CURL. Allow the user to inhibit this redaction through an environment variable. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- Documentation/git.txt | 6 ++++++ http.c | 11 +++++++++-- t/t5551-http-fetch-smart.sh | 10 ++++++++++ 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/Documentation/git.txt b/Documentation/git.txt index 9d6769e95a..af98cd7dc2 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -777,6 +777,12 @@ for full details. See `GIT_TRACE2` for available trace output options and link:technical/api-trace2.html[Trace2 documentation] for full details. +`GIT_REDACT_AUTHORIZATION`:: + By default, when a curl trace is enabled (see `GIT_TRACE_CURL` above), + the values of "Authorization:" and "Proxy-Authorization:" headers in + the trace are redacted. Set this variable to `0` to prevent this + redaction. + `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 diff --git a/http.c b/http.c index 62aa995245..77eac95d64 100644 --- a/http.c +++ b/http.c @@ -18,6 +18,7 @@ static struct trace_key trace_curl = TRACE_KEY_INIT(CURL); static int trace_curl_data = 1; +static int trace_curl_redact_authorization = 1; static struct string_list cookies_to_redact = STRING_LIST_INIT_DUP; #if LIBCURL_VERSION_NUM >= 0x070a08 long int git_curl_ipresolve = CURL_IPRESOLVE_WHATEVER; @@ -642,8 +643,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_authorization && + (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++; @@ -859,6 +861,7 @@ static int get_curl_http_version_opt(const char *version_string, long *opt) static CURL *get_curl_handle(void) { CURL *result = curl_easy_init(); + const char *redact_authorization_envvar; if (!result) die("curl_easy_init failed"); @@ -997,6 +1000,10 @@ static CURL *get_curl_handle(void) setup_curl_trace(result); if (getenv("GIT_TRACE_CURL_NO_DATA")) trace_curl_data = 0; + redact_authorization_envvar = getenv("GIT_REDACT_AUTHORIZATION"); + if (redact_authorization_envvar && + !strcmp(redact_authorization_envvar, "0")) + trace_curl_redact_authorization = 0; if (getenv("GIT_REDACT_COOKIES")) { string_list_split(&cookies_to_redact, getenv("GIT_REDACT_COOKIES"), ',', -1); diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh index acc8473a72..eeecfe01d7 100755 --- a/t/t5551-http-fetch-smart.sh +++ b/t/t5551-http-fetch-smart.sh @@ -197,6 +197,16 @@ test_expect_success 'GIT_TRACE_CURL redacts auth details' ' grep "Authorization: Basic <redacted>" trace ' +test_expect_success 'GIT_TRACE_CURL does not redact auth details if GIT_REDACT_AUTHORIZATION=0' ' + rm -rf redact-auth trace && + set_askpass user@host pass@host && + GIT_REDACT_AUTHORIZATION=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 -- 2.26.2.645.ge9eca65c58-goog ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/3] http: make GIT_TRACE_CURL auth redaction optional 2020-05-13 19:12 ` [PATCH v2 2/3] http: make GIT_TRACE_CURL auth redaction optional Jonathan Tan @ 2020-05-13 19:29 ` Junio C Hamano 0 siblings, 0 replies; 21+ messages in thread From: Junio C Hamano @ 2020-05-13 19:29 UTC (permalink / raw) To: Jonathan Tan; +Cc: git, sandals, peff Jonathan Tan <jonathantanmy@google.com> writes: > diff --git a/http.c b/http.c > index 62aa995245..77eac95d64 100644 > --- a/http.c > +++ b/http.c > @@ -18,6 +18,7 @@ > > static struct trace_key trace_curl = TRACE_KEY_INIT(CURL); > static int trace_curl_data = 1; > +static int trace_curl_redact_authorization = 1; > static struct string_list cookies_to_redact = STRING_LIST_INIT_DUP; > #if LIBCURL_VERSION_NUM >= 0x070a08 > long int git_curl_ipresolve = CURL_IPRESOLVE_WHATEVER; > @@ -642,8 +643,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_authorization && > + (skip_prefix(header->buf, "Authorization:", &sensitive_header) || > + skip_prefix(header->buf, "Proxy-Authorization:", &sensitive_header))) { Nice. > /* The first token is the type, which is OK to log */ > while (isspace(*sensitive_header)) > sensitive_header++; > @@ -859,6 +861,7 @@ static int get_curl_http_version_opt(const char *version_string, long *opt) > static CURL *get_curl_handle(void) > { > CURL *result = curl_easy_init(); > + const char *redact_authorization_envvar; > > if (!result) > die("curl_easy_init failed"); > @@ -997,6 +1000,10 @@ static CURL *get_curl_handle(void) > setup_curl_trace(result); > if (getenv("GIT_TRACE_CURL_NO_DATA")) > trace_curl_data = 0; > + redact_authorization_envvar = getenv("GIT_REDACT_AUTHORIZATION"); > + if (redact_authorization_envvar && > + !strcmp(redact_authorization_envvar, "0")) > + trace_curl_redact_authorization = 0; Wasn't git_env_bool() designed exactly for this kind of usage? Thanks. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 3/3] http, imap-send: stop using CURLOPT_VERBOSE 2020-05-13 19:12 ` [PATCH v2 0/3] Safer GIT_CURL_VERBOSE Jonathan Tan 2020-05-13 19:12 ` [PATCH v2 1/3] t5551: test that GIT_TRACE_CURL redacts password Jonathan Tan 2020-05-13 19:12 ` [PATCH v2 2/3] http: make GIT_TRACE_CURL auth redaction optional Jonathan Tan @ 2020-05-13 19:12 ` Jonathan Tan 2020-05-13 19:27 ` [PATCH v2 0/3] Safer GIT_CURL_VERBOSE Junio C Hamano 3 siblings, 0 replies; 21+ messages in thread From: Jonathan Tan @ 2020-05-13 19:12 UTC (permalink / raw) To: git; +Cc: Jonathan Tan, sandals, gitster, peff Whenever GIT_CURL_VERBOSE is set, teach Git to behave as if GIT_TRACE_CURL=1 and GIT_TRACE_CURL_NO_DATA=1 is set, instead of setting CURLOPT_VERBOSE. This is to prevent inadvertent revelation of sensitive data. In particular, GIT_CURL_VERBOSE redacts neither the "Authorization" header nor any cookies specified by GIT_REDACT_COOKIES. Unifying the tracing mechanism also has the future benefit that any improvements to the tracing mechanism will benefit both users of GIT_CURL_VERBOSE and GIT_TRACE_CURL, and we do not need to remember to implement any improvement twice. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- Documentation/git.txt | 2 -- http.c | 8 +++++++- http.h | 7 +++++++ imap-send.c | 2 +- t/t5551-http-fetch-smart.sh | 24 ++++++++++++++++++++++++ t/t5581-http-curl-verbose.sh | 2 +- trace.c | 20 ++++++++++++++++---- trace.h | 6 ++++++ 8 files changed, 62 insertions(+), 9 deletions(-) diff --git a/Documentation/git.txt b/Documentation/git.txt index af98cd7dc2..43a440bae4 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -721,8 +721,6 @@ of clones and fetches. Enables a curl full trace dump of all incoming and outgoing data, including descriptive information, of the git transport protocol. This is similar to doing curl `--trace-ascii` on the command line. - This option overrides setting the `GIT_CURL_VERBOSE` environment - variable. See `GIT_TRACE` for available trace output options. `GIT_TRACE_CURL_NO_DATA`:: diff --git a/http.c b/http.c index 77eac95d64..d0cea72cbd 100644 --- a/http.c +++ b/http.c @@ -806,6 +806,12 @@ static int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size, return 0; } +void http_trace_curl_no_data(void) +{ + trace_override_envvar(&trace_curl, "1"); + trace_curl_data = 0; +} + void setup_curl_trace(CURL *handle) { if (!trace_want(&trace_curl)) @@ -996,7 +1002,7 @@ static CURL *get_curl_handle(void) warning(_("Protocol restrictions not supported with cURL < 7.19.4")); #endif if (getenv("GIT_CURL_VERBOSE")) - curl_easy_setopt(result, CURLOPT_VERBOSE, 1L); + http_trace_curl_no_data(); setup_curl_trace(result); if (getenv("GIT_TRACE_CURL_NO_DATA")) trace_curl_data = 0; diff --git a/http.h b/http.h index 5e0ad724f9..faf8cbb0d1 100644 --- a/http.h +++ b/http.h @@ -252,6 +252,13 @@ int finish_http_object_request(struct http_object_request *freq); void abort_http_object_request(struct http_object_request *freq); void release_http_object_request(struct http_object_request *freq); +/* + * Instead of using environment variables to determine if curl tracing happens, + * behave as if GIT_TRACE_CURL=1 and GIT_TRACE_CURL_NO_DATA=1 is set. Call this + * before calling setup_curl_trace(). + */ +void http_trace_curl_no_data(void); + /* setup routine for curl_easy_setopt CURLOPT_DEBUGFUNCTION */ void setup_curl_trace(CURL *handle); #endif /* HTTP_H */ diff --git a/imap-send.c b/imap-send.c index 6c54d8c29d..52737546f3 100644 --- a/imap-send.c +++ b/imap-send.c @@ -1464,7 +1464,7 @@ static CURL *setup_curl(struct imap_server_conf *srvc, struct credential *cred) curl_easy_setopt(curl, CURLOPT_UPLOAD, 1L); if (0 < verbosity || getenv("GIT_CURL_VERBOSE")) - curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L); + http_trace_curl_no_data(); setup_curl_trace(curl); return curl; diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh index eeecfe01d7..209f2f8427 100755 --- a/t/t5551-http-fetch-smart.sh +++ b/t/t5551-http-fetch-smart.sh @@ -207,6 +207,18 @@ test_expect_success 'GIT_TRACE_CURL does not redact auth details if GIT_REDACT_A grep "Authorization: Basic [0-9a-zA-Z+/]" trace ' +test_expect_success 'GIT_CURL_VERBOSE redacts auth details' ' + rm -rf redact-auth trace && + set_askpass user@host pass@host && + GIT_CURL_VERBOSE=1 git clone --bare "$HTTPD_URL/auth/smart/repo.git" redact-auth 2>trace && + expect_askpass both user@host && + + # Ensure that there is no "Basic" followed by a base64 string, but that + # the auth details are redacted + ! grep "Authorization: Basic [0-9a-zA-Z+/]" trace && + grep "Authorization: Basic <redacted>" trace +' + test_expect_success 'disable dumb http on server' ' git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \ config http.getanyfile false @@ -464,6 +476,18 @@ test_expect_success 'GIT_REDACT_COOKIES redacts cookies' ' ! 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=<redacted>" err && + ! grep "Cookie:.*Bar=2" err +' + test_expect_success 'GIT_REDACT_COOKIES handles empty values' ' rm -rf clone && echo "Set-Cookie: Foo=" >cookies && diff --git a/t/t5581-http-curl-verbose.sh b/t/t5581-http-curl-verbose.sh index 5129b0724f..927aad0820 100755 --- a/t/t5581-http-curl-verbose.sh +++ b/t/t5581-http-curl-verbose.sh @@ -20,7 +20,7 @@ test_expect_success 'failure in git-upload-pack is shown' ' test_might_fail env GIT_CURL_VERBOSE=1 \ git clone "$HTTPD_URL/error_git_upload_pack/smart/repo.git" \ 2>curl_log && - grep "< HTTP/1.1 500 Intentional Breakage" curl_log + grep "<= Recv header: HTTP/1.1 500 Intentional Breakage" curl_log ' test_done diff --git a/trace.c b/trace.c index b3ef0e627f..f726686fd9 100644 --- a/trace.c +++ b/trace.c @@ -29,7 +29,7 @@ struct trace_key trace_perf_key = TRACE_KEY_INIT(PERFORMANCE); struct trace_key trace_setup_key = TRACE_KEY_INIT(SETUP); /* Get a trace file descriptor from "key" env variable. */ -static int get_trace_fd(struct trace_key *key) +static int get_trace_fd(struct trace_key *key, const char *override_envvar) { const char *trace; @@ -37,7 +37,7 @@ static int get_trace_fd(struct trace_key *key) if (key->initialized) return key->fd; - trace = getenv(key->key); + trace = override_envvar ? override_envvar : getenv(key->key); if (!trace || !strcmp(trace, "") || !strcmp(trace, "0") || !strcasecmp(trace, "false")) @@ -68,6 +68,18 @@ static int get_trace_fd(struct trace_key *key) return key->fd; } +void trace_override_envvar(struct trace_key *key, const char *value) +{ + trace_disable(key); + key->initialized = 0; + + /* + * Invoke get_trace_fd() to initialize key using the given value + * instead of the value of the environment variable. + */ + get_trace_fd(key, value); +} + void trace_disable(struct trace_key *key) { if (key->need_close) @@ -112,7 +124,7 @@ static int prepare_trace_line(const char *file, int line, static void trace_write(struct trace_key *key, const void *buf, unsigned len) { - if (write_in_full(get_trace_fd(key), buf, len) < 0) { + if (write_in_full(get_trace_fd(key, NULL), buf, len) < 0) { warning("unable to write trace for %s: %s", key->key, strerror(errno)); trace_disable(key); @@ -383,7 +395,7 @@ void trace_repo_setup(const char *prefix) int trace_want(struct trace_key *key) { - return !!get_trace_fd(key); + return !!get_trace_fd(key, NULL); } #if defined(HAVE_CLOCK_GETTIME) && defined(HAVE_CLOCK_MONOTONIC) diff --git a/trace.h b/trace.h index 9826618b33..0dbbad0e41 100644 --- a/trace.h +++ b/trace.h @@ -101,6 +101,12 @@ void trace_repo_setup(const char *prefix); */ int trace_want(struct trace_key *key); +/** + * Enables or disables tracing for the specified key, as if the environment + * variable was set to the given value. + */ +void trace_override_envvar(struct trace_key *key, const char *value); + /** * Disables tracing for the specified key, even if the environment variable * was set. -- 2.26.2.645.ge9eca65c58-goog ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 0/3] Safer GIT_CURL_VERBOSE 2020-05-13 19:12 ` [PATCH v2 0/3] Safer GIT_CURL_VERBOSE Jonathan Tan ` (2 preceding siblings ...) 2020-05-13 19:12 ` [PATCH v2 3/3] http, imap-send: stop using CURLOPT_VERBOSE Jonathan Tan @ 2020-05-13 19:27 ` Junio C Hamano 2020-05-13 19:33 ` Junio C Hamano 3 siblings, 1 reply; 21+ messages in thread From: Junio C Hamano @ 2020-05-13 19:27 UTC (permalink / raw) To: Jonathan Tan; +Cc: git, sandals, peff Jonathan Tan <jonathantanmy@google.com> writes: > Thanks everyone. I went ahead with GIT_REDACT_AUTHORIZATION to match > GIT_REDACT_COOKIES, with the default being true (i.e. you need to set it > to "0" to have behavior change). Hmm, I would actually have expected us to move in the direction of deprecating specific REDACT_BLAH and consolidate them into one, instead of adding another one. Especially as the primary reason why we redact cookies is to protect those that are used for auth anyway. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 0/3] Safer GIT_CURL_VERBOSE 2020-05-13 19:27 ` [PATCH v2 0/3] Safer GIT_CURL_VERBOSE Junio C Hamano @ 2020-05-13 19:33 ` Junio C Hamano 2020-05-15 20:47 ` Jeff King 0 siblings, 1 reply; 21+ messages in thread From: Junio C Hamano @ 2020-05-13 19:33 UTC (permalink / raw) To: Jonathan Tan; +Cc: git, sandals, peff Junio C Hamano <gitster@pobox.com> writes: > Jonathan Tan <jonathantanmy@google.com> writes: > >> Thanks everyone. I went ahead with GIT_REDACT_AUTHORIZATION to match >> GIT_REDACT_COOKIES, with the default being true (i.e. you need to set it >> to "0" to have behavior change). > > Hmm, I would actually have expected us to move in the direction of > deprecating specific REDACT_BLAH and consolidate them into one, > instead of adding another one. Especially as the primary reason why > we redact cookies is to protect those that are used for auth anyway. Also I had forgot to grep for anonymi.e to find transport_anonymize_url(), which I was hoping that the new environment variable would cover to help those who debug. Thanks. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 0/3] Safer GIT_CURL_VERBOSE 2020-05-13 19:33 ` Junio C Hamano @ 2020-05-15 20:47 ` Jeff King 0 siblings, 0 replies; 21+ messages in thread From: Jeff King @ 2020-05-15 20:47 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Tan, git, sandals On Wed, May 13, 2020 at 12:33:37PM -0700, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > Jonathan Tan <jonathantanmy@google.com> writes: > > > >> Thanks everyone. I went ahead with GIT_REDACT_AUTHORIZATION to match > >> GIT_REDACT_COOKIES, with the default being true (i.e. you need to set it > >> to "0" to have behavior change). > > > > Hmm, I would actually have expected us to move in the direction of > > deprecating specific REDACT_BLAH and consolidate them into one, > > instead of adding another one. Especially as the primary reason why > > we redact cookies is to protect those that are used for auth anyway. > > Also I had forgot to grep for anonymi.e to find transport_anonymize_url(), > which I was hoping that the new environment variable would cover to help > those who debug. Yeah, I'd agree with both of these. If Jonathan doesn't feel like working on transport_anonymize_url() now, I don't mind if we leave that for later. But let's come up with a general scheme that we're aiming for, so we minimize changes to things that users are exposed to. IMHO an option that only impacts the format of the human-readable trace output is not something we need to deprecate. It's an internal detail, and people can't rely on the exact format of the trace anyway. So I'd be fine to just kill off GIT_REDACT_COOKIES completely (especially if the default behavior is the safer "always redact"). That said, a boolean GIT_REDACT doesn't quite do the same thing, because it's actually a list of cookies. My gut feeling is that this is a bit over-engineered. Any cookies that Git uses are likely to be sensitive, so just treating their values like auth (redacting by default, but allowing them to be unblinded when the user asks for it). But maybe Jonathan had a specific tracing case in mind, as the author of the original. -Peff ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2020-05-15 20:47 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-11 17:43 [PATCH 0/2] Safer GIT_CURL_VERBOSE Jonathan Tan 2020-05-11 17:43 ` [PATCH 1/2] t5551: test that GIT_TRACE_CURL redacts password Jonathan Tan 2020-05-12 19:08 ` Jeff King 2020-05-11 17:43 ` [PATCH 2/2] http, imap-send: stop using CURLOPT_VERBOSE Jonathan Tan 2020-05-12 19:16 ` Jeff King 2020-05-12 19:23 ` Jonathan Tan 2020-05-12 19:27 ` Jeff King 2020-05-12 23:13 ` brian m. carlson 2020-05-13 0:10 ` Junio C Hamano 2020-05-13 4:50 ` Jeff King 2020-05-13 5:05 ` Junio C Hamano 2020-05-13 6:16 ` Daniel Stenberg 2020-05-13 14:45 ` Jeff King 2020-05-13 19:12 ` [PATCH v2 0/3] Safer GIT_CURL_VERBOSE Jonathan Tan 2020-05-13 19:12 ` [PATCH v2 1/3] t5551: test that GIT_TRACE_CURL redacts password Jonathan Tan 2020-05-13 19:12 ` [PATCH v2 2/3] http: make GIT_TRACE_CURL auth redaction optional Jonathan Tan 2020-05-13 19:29 ` Junio C Hamano 2020-05-13 19:12 ` [PATCH v2 3/3] http, imap-send: stop using CURLOPT_VERBOSE Jonathan Tan 2020-05-13 19:27 ` [PATCH v2 0/3] Safer GIT_CURL_VERBOSE Junio C Hamano 2020-05-13 19:33 ` Junio C Hamano 2020-05-15 20:47 ` Jeff King
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.