trace2: redact signatures from presigned blob storage URLs

t/t0212-trace2-event.sh

@@ -322,44 +322,106 @@ test_expect_success 'discard traces when there are too many files' '
 	head -n2 trace_target_dir/git-trace2-discard | tail -n1 | grep \"event\":\"too_many_files\"
 '
 
-# In the following "...redact..." tests, skip testing the GIT_TRACE2_REDACT=0
-# case because we would need to exactly model the full JSON event stream like
-# we did in the basic tests above and I do not think it is worth it.
+url="HtTpS://user:pwd@example.com/?sig=secret&expires=123"
+redacted_url="HtTpS://user:<REDACTED>@example.com/?sig=<REDACTED>&expires=123"
 
 test_expect_success 'unsafe URLs are redacted by default in cmd_start events' '
 	test_when_finished \
 		"rm -r trace.event" &&
 
 	GIT_TRACE2_EVENT="$(pwd)/trace.event" \
-		test-tool trace2 300redact_start git clone https://user:pwd@example.com/ clone2 &&
-	test_grep ! user:pwd trace.event
+		test-tool trace2 300redact_start git clone "$url" clone2 &&
+	test_grep ! -E "user:pwd|secret" trace.event &&
+	test_grep -F "$redacted_url" trace.event
 '
 
 test_expect_success 'unsafe URLs are redacted by default in child_start events' '
 	test_when_finished \
 		"rm -r trace.event" &&
 
 	GIT_TRACE2_EVENT="$(pwd)/trace.event" \
-		test-tool trace2 301redact_child_start git clone https://user:pwd@example.com/ clone2 &&
-	test_grep ! user:pwd trace.event
+		test-tool trace2 301redact_child_start git clone "$url" clone2 &&
+	test_grep ! -E "user:pwd|secret" trace.event &&
+	test_grep -F "$redacted_url" trace.event
 '
 
 test_expect_success 'unsafe URLs are redacted by default in exec events' '
 	test_when_finished \
 		"rm -r trace.event" &&
 
 	GIT_TRACE2_EVENT="$(pwd)/trace.event" \
-		test-tool trace2 302redact_exec git clone https://user:pwd@example.com/ clone2 &&
-	test_grep ! user:pwd trace.event
+		test-tool trace2 302redact_exec git clone "$url" clone2 &&
+	test_grep ! -E "user:pwd|secret" trace.event &&
+	test_grep -F "$redacted_url" trace.event
 '
 
 test_expect_success 'unsafe URLs are redacted by default in def_param events' '
 	test_when_finished \
 		"rm -r trace.event" &&
 
 	GIT_TRACE2_EVENT="$(pwd)/trace.event" \
-		test-tool trace2 303redact_def_param url https://user:pwd@example.com/ &&
-	test_grep ! user:pwd trace.event
+		test-tool trace2 303redact_def_param url "$url" &&
+	test_grep ! -E "user:pwd|secret" trace.event &&
+	test_grep -F "$redacted_url" trace.event
+'
+
+test_expect_success 'redact signature query values without changing the rest of the URL' '
+	test_when_finished "rm -f trace.event actual cases" &&
+	cat >cases <<-\EOF &&
+	http ?sig=secret ?sig=<REDACTED>
+	HTTPS ?expires=123&Signature=secret ?expires=123&Signature=<REDACTED>
+	hTtP ?X-Blob-SIGNATURE=secret&expires=123 ?X-Blob-SIGNATURE=<REDACTED>&expires=123
+	https ?%73ig=secret%2Bvalue%3D&sig=other ?%73ig=<REDACTED>&sig=<REDACTED>
+	HTTP ?SIG=secret#fragment ?SIG=<REDACTED>#fragment
+	https ?sig=&signature ?sig=&signature
+	https ?sig=sig=secret&expires=123 ?sig=<REDACTED>&expires=123
+	https ?design=value&signature-extra=value ?design=value&signature-extra=value
+	https ?value=sig=public ?value=sig=public
+	https #fragment?sig=public #fragment?sig=public
+	EOF
+	while read scheme input expect
+	do
+		: >trace.event &&
+		GIT_TRACE2_EVENT="$(pwd)/trace.event" \
+			test-tool trace2 303redact_def_param url "$scheme://example.com/pack$input" &&
+		grep "\"event\":\"def_param\"" trace.event >actual &&
+		test_grep -F "\"value\":\"$scheme://example.com/pack$expect\"" actual || return 1
+	done <cases
+'
+
+test_expect_success 'URL signature redaction can be disabled' '
+	test_when_finished "rm trace.event" &&
+	GIT_TRACE2_REDACT=0 GIT_TRACE2_EVENT="$(pwd)/trace.event" \
+		test-tool trace2 303redact_def_param url "$url" &&
+	test_grep -F "$url" trace.event
+'
+
+test_expect_success 'redact URLs in error events without changing stderr' '
+	test_when_finished "rm trace.normal trace.perf trace.event errors stderr" &&
+	message="unable to access $SQ$url$SQ: try ${SQ}HTTP://example.com/o${SQ}brien?signature=other$SQ." &&
+	expected="unable to access $SQ$redacted_url$SQ: try ${SQ}HTTP://example.com/o${SQ}brien?signature=<REDACTED>$SQ." &&
+	GIT_TRACE2="$(pwd)/trace.normal" \
+	GIT_TRACE2_PERF="$(pwd)/trace.perf" \
+	GIT_TRACE2_EVENT="$(pwd)/trace.event" \
+		test-tool trace2 003error "$message" 2>stderr &&
+	grep " error " trace.normal >errors &&
+	grep "| error " trace.perf >>errors &&
+	grep "\"event\":\"error\"" trace.event >>errors &&
+	test_grep ! -E "user:pwd|secret|signature=other" errors &&
+	test_grep -F "$expected" trace.normal &&
+	test_grep -F "$expected" trace.perf &&
+	test_grep -F "$expected" trace.event &&
+	test_grep -F "\"fmt\":\"%s\"" errors &&
+	test_grep -F "$message" stderr
+'
+
+test_expect_success 'URL signature redaction can be disabled in error events' '
+	test_when_finished "rm trace.event errors stderr" &&
+	message="unable to access $SQ$url$SQ" &&
+	GIT_TRACE2_REDACT=0 GIT_TRACE2_EVENT="$(pwd)/trace.event" \
+		test-tool trace2 003error "$message" 2>stderr &&
+	grep "\"event\":\"error\"" trace.event >errors &&
+	test_grep -F "$message" errors
 '
 
 test_done

trace2.c

@@ -17,6 +17,7 @@
 #include "trace2/tr2_tgt.h"
 #include "trace2/tr2_tls.h"
 #include "trace2/tr2_tmr.h"
+#include "url.h"
 
 static int trace2_enabled;
 static int trace2_redact = 1;
@@ -249,32 +250,103 @@ int trace2_is_enabled(void)
 	return trace2_enabled;
 }
 
+static int is_signature_parameter(const char *parameter, size_t len)
+{
+	char *name = url_decode_mem(parameter, len);
+	const char *suffix = strrchr(name, '-');
+	int ret = !strcasecmp(name, "sig") ||
+		!strcasecmp(suffix ? suffix + 1 : name, "signature");
+
+	free(name);
+	return ret;
+}
+
 /*
- * Redacts an argument, i.e. ensures that no password in
- * https://user:password@host/-style URLs is logged.
+ * Redact passwords and signature query parameters in HTTP(S) URLs.
  *
  * Returns the original if nothing needed to be redacted.
  * Returns a pointer that needs to be `free()`d otherwise.
  */
 static const char *redact_arg(const char *arg)
 {
 	const char *p, *colon;
+	const char *unredacted = arg;
+	struct strbuf buf = STRBUF_INIT;
 	size_t at;
 
 	if (!trace2_redact ||
-	    (!skip_prefix(arg, "https://", &p) &&
-	     !skip_prefix(arg, "http://", &p)))
+	    (!skip_iprefix(arg, "https://", &p) &&
+	     !skip_iprefix(arg, "http://", &p)))
 		return arg;
 
-	at = strcspn(p, "@/");
-	if (p[at] != '@')
-		return arg;
+	at = strcspn(p, "@/?#");
+	if (p[at] == '@' && (colon = memchr(p, ':', at))) {
+		strbuf_add(&buf, arg, colon + 1 - arg);
+		strbuf_addstr(&buf, "<REDACTED>");
+		unredacted = p + at;
+	}
+
+	p += strcspn(p, "?#");
+	if (*p == '?') {
+		p++;
+		while (*p && *p != '#') {
+			const char *end = p + strcspn(p, "&#");
+			const char *equals = memchr(p, '=', end - p);
+
+			if (equals && equals + 1 < end &&
+			    is_signature_parameter(p, equals - p)) {
+				strbuf_add(&buf, unredacted,
+					   equals + 1 - unredacted);
+				strbuf_addstr(&buf, "<REDACTED>");
+				unredacted = end;
+			}
+			p = *end == '&' ? end + 1 : end;
+		}
+	}
 
-	colon = memchr(p, ':', at);
-	if (!colon)
+	if (!buf.len)
 		return arg;
+	strbuf_addstr(&buf, unredacted);
+	return strbuf_detach(&buf, NULL);
+}
 
-	return xstrfmt("%.*s:<REDACTED>%s", (int)(colon - arg), arg, p + at);
+static const char *redact_error_message(const char *message)
+{
+	struct strbuf buf = STRBUF_INIT;
+	const char *p = message, *unredacted = message;
+
+	if (!trace2_redact || !message)
+		return message;
+
+	while ((p = strcasestr(p, "http"))) {
+		const char *end;
+		const char *redacted;
+		char *url;
+
+		if (!skip_iprefix(p, "http://", &end) &&
+		    !skip_iprefix(p, "https://", &end)) {
+			p += 4;
+			continue;
+		}
+		end = p + strcspn(p, " \t\r\n\v\f");
+		/* Keep punctuation surrounding URLs in diagnostics. */
+		while (end > p && strchr("'\".,:;)]}>", end[-1]))
+			end--;
+		url = xmemdupz(p, end - p);
+		redacted = redact_arg(url);
+		if (redacted != url) {
+			strbuf_add(&buf, unredacted, p - unredacted);
+			strbuf_addstr(&buf, redacted);
+			unredacted = end;
+			free((char *)redacted);
+		}
+		free(url);
+		p = end;
+	}
+	if (!buf.len)
+		return message;
+	strbuf_addstr(&buf, unredacted);
+	return strbuf_detach(&buf, NULL);
 }
 
 /*
@@ -380,18 +452,33 @@ void trace2_cmd_error_va_fl(const char *file, int line, const char *fmt,
 			    va_list ap)
 {
 	struct tr2_tgt *tgt_j;
+	struct strbuf message = STRBUF_INIT;
+	const char *redacted_fmt, *redacted_message;
 	int j;
 
 	if (!trace2_enabled)
 		return;
 
-	/*
-	 * We expect each target function to treat 'ap' as constant
-	 * and use va_copy (because an 'ap' can only be walked once).
-	 */
+	if (fmt && *fmt) {
+		va_list copy_ap;
+
+		va_copy(copy_ap, ap);
+		strbuf_vaddf(&message, fmt, copy_ap);
+		va_end(copy_ap);
+	}
+	redacted_fmt = redact_error_message(fmt);
+	redacted_message = redact_error_message(message.buf);
+
 	for_each_wanted_builtin (j, tgt_j)
-		if (tgt_j->pfn_error_va_fl)
-			tgt_j->pfn_error_va_fl(file, line, fmt, ap);
+		if (tgt_j->pfn_error_fl)
+			tgt_j->pfn_error_fl(file, line, redacted_fmt,
+					    redacted_message);
+
+	if (redacted_fmt != fmt)
+		free((char *)redacted_fmt);
+	if (redacted_message != message.buf)
+		free((char *)redacted_message);
+	strbuf_release(&message);
 }
 
 void trace2_cmd_path_fl(const char *file, int line, const char *pathname)

trace2/tr2_tgt.h

@@ -28,8 +28,9 @@ typedef void(tr2_tgt_evt_exit_fl_t)(const char *file, int line,
 typedef void(tr2_tgt_evt_signal_t)(uint64_t us_elapsed_absolute, int signo);
 typedef void(tr2_tgt_evt_atexit_t)(uint64_t us_elapsed_absolute, int code);
 
-typedef void(tr2_tgt_evt_error_va_fl_t)(const char *file, int line,
-					const char *fmt, va_list ap);
+/* Error messages and format strings are redacted before dispatch. */
+typedef void(tr2_tgt_evt_error_fl_t)(const char *file, int line,
+				     const char *fmt, const char *message);
 
 typedef void(tr2_tgt_evt_command_path_fl_t)(const char *file, int line,
 					    const char *command_path);
@@ -128,7 +129,7 @@ struct tr2_tgt {
 	tr2_tgt_evt_exit_fl_t                   *pfn_exit_fl;
 	tr2_tgt_evt_signal_t                    *pfn_signal;
 	tr2_tgt_evt_atexit_t                    *pfn_atexit;
-	tr2_tgt_evt_error_va_fl_t               *pfn_error_va_fl;
+	tr2_tgt_evt_error_fl_t                  *pfn_error_fl;
 	tr2_tgt_evt_command_path_fl_t           *pfn_command_path_fl;
 	tr2_tgt_evt_command_ancestry_fl_t	*pfn_command_ancestry_fl;
 	tr2_tgt_evt_command_name_fl_t           *pfn_command_name_fl;

trace2/tr2_tgt_event.c

@@ -230,15 +230,16 @@ static void maybe_add_string_va(struct json_writer *jw, const char *field_name,
 	}
 }
 
-static void fn_error_va_fl(const char *file, int line, const char *fmt,
-			   va_list ap)
+static void fn_error_fl(const char *file, int line, const char *fmt,
+			const char *message)
 {
 	const char *event_name = "error";
 	struct json_writer jw = JSON_WRITER_INIT;
 
 	jw_object_begin(&jw, 0);
 	event_fmt_prepare(event_name, file, line, NULL, &jw);
-	maybe_add_string_va(&jw, "msg", fmt, ap);
+	if (fmt && *fmt)
+		jw_object_string(&jw, "msg", message);
 	/*
 	 * Also emit the format string as a field in case
 	 * post-processors want to aggregate common error
@@ -696,7 +697,7 @@ struct tr2_tgt tr2_tgt_event = {
 	.pfn_exit_fl = fn_exit_fl,
 	.pfn_signal = fn_signal,
 	.pfn_atexit = fn_atexit,
-	.pfn_error_va_fl = fn_error_va_fl,
+	.pfn_error_fl = fn_error_fl,
 	.pfn_command_path_fl = fn_command_path_fl,
 	.pfn_command_ancestry_fl = fn_command_ancestry_fl,
 	.pfn_command_name_fl = fn_command_name_fl,

trace2/tr2_tgt_normal.c

@@ -142,15 +142,15 @@ static void maybe_append_string_va(struct strbuf *buf, const char *fmt,
 	}
 }
 
-static void fn_error_va_fl(const char *file, int line, const char *fmt,
-			   va_list ap)
+static void fn_error_fl(const char *file, int line, const char *fmt,
+			const char *message)
 {
 	struct strbuf buf_payload = STRBUF_INIT;
 
 	strbuf_addstr(&buf_payload, "error");
 	if (fmt && *fmt) {
 		strbuf_addch(&buf_payload, ' ');
-		maybe_append_string_va(&buf_payload, fmt, ap);
+		strbuf_addstr(&buf_payload, message);
 	}
 	normal_io_write_fl(file, line, &buf_payload);
 	strbuf_release(&buf_payload);
@@ -384,7 +384,7 @@ struct tr2_tgt tr2_tgt_normal = {
 	.pfn_exit_fl = fn_exit_fl,
 	.pfn_signal = fn_signal,
 	.pfn_atexit = fn_atexit,
-	.pfn_error_va_fl = fn_error_va_fl,
+	.pfn_error_fl = fn_error_fl,
 	.pfn_command_path_fl = fn_command_path_fl,
 	.pfn_command_ancestry_fl = fn_command_ancestry_fl,
 	.pfn_command_name_fl = fn_command_name_fl,

trace2/tr2_tgt_perf.c

@@ -234,13 +234,13 @@ static void maybe_append_string_va(struct strbuf *buf, const char *fmt,
 	}
 }
 
-static void fn_error_va_fl(const char *file, int line, const char *fmt,
-			   va_list ap)
+static void fn_error_fl(const char *file, int line, const char *fmt UNUSED,
+			const char *message)
 {
 	const char *event_name = "error";
 	struct strbuf buf_payload = STRBUF_INIT;
 
-	maybe_append_string_va(&buf_payload, fmt, ap);
+	strbuf_addstr(&buf_payload, message);
 
 	perf_io_write_fl(file, line, event_name, NULL, NULL, NULL, NULL,
 			 &buf_payload);
@@ -609,7 +609,7 @@ struct tr2_tgt tr2_tgt_perf = {
 	.pfn_exit_fl = fn_exit_fl,
 	.pfn_signal = fn_signal,
 	.pfn_atexit = fn_atexit,
-	.pfn_error_va_fl = fn_error_va_fl,
+	.pfn_error_fl = fn_error_fl,
 	.pfn_command_path_fl = fn_command_path_fl,
 	.pfn_command_ancestry_fl = fn_command_ancestry_fl,
 	.pfn_command_name_fl = fn_command_name_fl,