http, imap-send: stop using CURLOPT_VERBOSE
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> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jonathan Tan committed
May 11, 2020 at 10:43 UTC
7167a62b9e2f648adc11411446f876f2458722a5
8 files changed
+62
-9
Documentation/git.txt
-2
@@ -721,8 +721,6 @@ of clones and fetches.
721
Enables a curl full trace dump of all incoming and outgoing data,
722
including descriptive information, of the git transport protocol.
723
This is similar to doing curl `--trace-ascii` on the command line.
724
- This option overrides setting the `GIT_CURL_VERBOSE` environment
725
- variable.
724
See `GIT_TRACE` for available trace output options.
725
726
`GIT_TRACE_CURL_NO_DATA`::
http.c
+7
-1
@@ -804,6 +804,12 @@ static int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size,
804
return 0;
805
}
806
807
+void http_trace_curl_no_data(void)
808
+{
809
+ trace_override_envvar(&trace_curl, "1");
810
+ trace_curl_data = 0;
811
+}
812
+
813
void setup_curl_trace(CURL *handle)
814
{
815
if (!trace_want(&trace_curl))
@@ -993,7 +999,7 @@ static CURL *get_curl_handle(void)
999
warning(_("Protocol restrictions not supported with cURL < 7.19.4"));
1000
#endif
1001
if (getenv("GIT_CURL_VERBOSE"))
996
- curl_easy_setopt(result, CURLOPT_VERBOSE, 1L);
1002
+ http_trace_curl_no_data();
1003
setup_curl_trace(result);
1004
if (getenv("GIT_TRACE_CURL_NO_DATA"))
1005
trace_curl_data = 0;
http.h
+7
@@ -252,6 +252,13 @@ int finish_http_object_request(struct http_object_request *freq);
252
void abort_http_object_request(struct http_object_request *freq);
253
void release_http_object_request(struct http_object_request *freq);
254
255
+/*
256
+ * Instead of using environment variables to determine if curl tracing happens,
257
+ * behave as if GIT_TRACE_CURL=1 and GIT_TRACE_CURL_NO_DATA=1 is set. Call this
258
+ * before calling setup_curl_trace().
259
+ */
260
+void http_trace_curl_no_data(void);
261
+
262
/* setup routine for curl_easy_setopt CURLOPT_DEBUGFUNCTION */
263
void setup_curl_trace(CURL *handle);
264
#endif /* HTTP_H */
imap-send.c
+1
-1
@@ -1464,7 +1464,7 @@ static CURL *setup_curl(struct imap_server_conf *srvc, struct credential *cred)
1464
curl_easy_setopt(curl, CURLOPT_UPLOAD, 1L);
1465
1466
if (0 < verbosity || getenv("GIT_CURL_VERBOSE"))
1467
- curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);
1467
+ http_trace_curl_no_data();
1468
setup_curl_trace(curl);
1469
1470
return curl;
t/t5551-http-fetch-smart.sh
+24
@@ -197,6 +197,18 @@ test_expect_success 'GIT_TRACE_CURL redacts auth details' '
197
grep "Authorization: Basic <redacted>" trace
198
'
199
200
+test_expect_success 'GIT_CURL_VERBOSE redacts auth details' '
201
+ rm -rf redact-auth trace &&
202
+ set_askpass user@host pass@host &&
203
+ GIT_CURL_VERBOSE=1 git clone --bare "$HTTPD_URL/auth/smart/repo.git" redact-auth 2>trace &&
204
+ expect_askpass both user@host &&
205
+
206
+ # Ensure that there is no "Basic" followed by a base64 string, but that
207
+ # the auth details are redacted
208
+ ! grep "Authorization: Basic [0-9a-zA-Z+/]" trace &&
209
+ grep "Authorization: Basic <redacted>" trace
210
+'
211
+
212
test_expect_success 'disable dumb http on server' '
213
git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \
214
config http.getanyfile false
@@ -454,6 +466,18 @@ test_expect_success 'GIT_REDACT_COOKIES redacts cookies' '
466
! grep "Cookie:.*Bar=2" err
467
'
468
469
+test_expect_success 'GIT_REDACT_COOKIES redacts cookies when GIT_CURL_VERBOSE=1' '
470
+ rm -rf clone &&
471
+ echo "Set-Cookie: Foo=1" >cookies &&
472
+ echo "Set-Cookie: Bar=2" >>cookies &&
473
+ GIT_CURL_VERBOSE=1 GIT_REDACT_COOKIES=Bar,Baz \
474
+ git -c "http.cookieFile=$(pwd)/cookies" clone \
475
+ $HTTPD_URL/smart/repo.git clone 2>err &&
476
+ grep "Cookie:.*Foo=1" err &&
477
+ grep "Cookie:.*Bar=<redacted>" err &&
478
+ ! grep "Cookie:.*Bar=2" err
479
+'
480
+
481
test_expect_success 'GIT_REDACT_COOKIES handles empty values' '
482
rm -rf clone &&
483
echo "Set-Cookie: Foo=" >cookies &&
t/t5581-http-curl-verbose.sh
+1
-1
@@ -20,7 +20,7 @@ test_expect_success 'failure in git-upload-pack is shown' '
20
test_might_fail env GIT_CURL_VERBOSE=1 \
21
git clone "$HTTPD_URL/error_git_upload_pack/smart/repo.git" \
22
2>curl_log &&
23
- grep "< HTTP/1.1 500 Intentional Breakage" curl_log
23
+ grep "<= Recv header: HTTP/1.1 500 Intentional Breakage" curl_log
24
'
25
26
test_done
trace.c
+16
-4
@@ -29,7 +29,7 @@ struct trace_key trace_perf_key = TRACE_KEY_INIT(PERFORMANCE);
29
struct trace_key trace_setup_key = TRACE_KEY_INIT(SETUP);
30
31
/* Get a trace file descriptor from "key" env variable. */
32
-static int get_trace_fd(struct trace_key *key)
32
+static int get_trace_fd(struct trace_key *key, const char *override_envvar)
33
{
34
const char *trace;
35
@@ -37,7 +37,7 @@ static int get_trace_fd(struct trace_key *key)
37
if (key->initialized)
38
return key->fd;
39
40
- trace = getenv(key->key);
40
+ trace = override_envvar ? override_envvar : getenv(key->key);
41
42
if (!trace || !strcmp(trace, "") ||
43
!strcmp(trace, "0") || !strcasecmp(trace, "false"))
@@ -68,6 +68,18 @@ static int get_trace_fd(struct trace_key *key)
68
return key->fd;
69
}
70
71
+void trace_override_envvar(struct trace_key *key, const char *value)
72
+{
73
+ trace_disable(key);
74
+ key->initialized = 0;
75
+
76
+ /*
77
+ * Invoke get_trace_fd() to initialize key using the given value
78
+ * instead of the value of the environment variable.
79
+ */
80
+ get_trace_fd(key, value);
81
+}
82
+
83
void trace_disable(struct trace_key *key)
84
{
85
if (key->need_close)
@@ -112,7 +124,7 @@ static int prepare_trace_line(const char *file, int line,
124
125
static void trace_write(struct trace_key *key, const void *buf, unsigned len)
126
{
115
- if (write_in_full(get_trace_fd(key), buf, len) < 0) {
127
+ if (write_in_full(get_trace_fd(key, NULL), buf, len) < 0) {
128
warning("unable to write trace for %s: %s",
129
key->key, strerror(errno));
130
trace_disable(key);
@@ -383,7 +395,7 @@ void trace_repo_setup(const char *prefix)
395
396
int trace_want(struct trace_key *key)
397
{
386
- return !!get_trace_fd(key);
398
+ return !!get_trace_fd(key, NULL);
399
}
400
401
#if defined(HAVE_CLOCK_GETTIME) && defined(HAVE_CLOCK_MONOTONIC)
trace.h
+6
@@ -101,6 +101,12 @@ void trace_repo_setup(const char *prefix);
101
*/
102
int trace_want(struct trace_key *key);
103
104
+/**
105
+ * Enables or disables tracing for the specified key, as if the environment
106
+ * variable was set to the given value.
107
+ */
108
+void trace_override_envvar(struct trace_key *key, const char *value);
109
+
110
/**
111
* Disables tracing for the specified key, even if the environment variable
112
* was set.