All of lore.kernel.org
 help / color / mirror / Atom feed
* [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	[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	[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

* 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	[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	[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	[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 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

* 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.