fetch-pack: redact packfile urls in traces

In some setups, packfile uris act as bearer token. It is not recommended to expose them plainly in logs, although in special circunstances (e.g. debug) it makes sense to write them.

Redact the packfile-uri lines by default, unless the GIT_TRACE_REDACT variable is set to false. This mimics the redacting of the Authorization header in HTTP.

Ivan Frade (2):
fetch-pack: redact packfile urls in traces
Documentation: packfile-uri hash can be longer than 40 hex chars

 Documentation/technical/protocol-v2.txt |  8 ++---
 fetch-pack.c                            | 11 +++++++
 http-fetch.c                            |  4 ++-
 pkt-line.c                              |  7 +++-
 pkt-line.h                              |  1 +
 t/t5702-protocol-v2.sh                  | 43 +++++++++++++++++++++++++
 6 files changed, 68 insertions(+), 6 deletions(-)

base-commit: 0785eb769886ae81e346df10e88bc49ffc0ac64e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1052%2Fifradeo%2Fredact-packfile-uri-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1052/ifradeo/redact-packfile-uri-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1052

gitgitgadget

From: Ivan Frade <[email protected]>

In some setups, packfile uris act as bearer token. It is not recommended to expose them plainly in logs, although in special circunstances (e.g. debug) it makes sense to write them.

Redact the packfile-uri lines by default, unless the GIT_TRACE_REDACT variable is set to false. This mimics the redacting of the Authorization header in HTTP.

---
 fetch-pack.c           | 11 +++++++++++
 http-fetch.c           |  4 +++-
 pkt-line.c             |  7 ++++++-
 pkt-line.h             |  1 +
 t/t5702-protocol-v2.sh | 43 ++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 64 insertions(+), 2 deletions(-)

index a9604f35a3e..05c85eeafa1 100644
@@ -1518,7 +1518,16 @@ static void receive_wanted_refs(struct packet_reader *reader,
 static void receive_packfile_uris(struct packet_reader *reader,
 				  struct string_list *uris)
 {
+	int original_options;
 	process_section_header(reader, "packfile-uris", 0);
+	/*
+	 * In some setups, packfile-uris act as bearer tokens,
+	 * redact them by default.
+	 */
+	original_options = reader->options;
+	if (git_env_bool("GIT_TRACE_REDACT", 1))
+		reader->options |= PACKET_READ_REDACT_ON_TRACE;
+
 	while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
 		if (reader->pktlen < the_hash_algo->hexsz ||
 		    reader->line[the_hash_algo->hexsz] != ' ')

@@ -1526,6 +1535,8 @@ static void receive_packfile_uris(struct packet_reader *reader,
 
 		string_list_append(uris, reader->line);
 	}
+	reader->options = original_options;
+
 	if (reader->status != PACKET_READ_DELIM)
 		die("expected DELIM");
 }

index fa642462a9e..d35e33e4f65 100644
@@ -63,7 +63,9 @@ static void fetch_single_packfile(struct object_id *packfile_hash,
 	if (start_active_slot(preq->slot)) {
 		run_active_slot(preq->slot);
 		if (results.curl_result != CURLE_OK) {
-			die("Unable to get pack file %s\n%s", preq->url,
+			int showUrl = git_env_bool("GIT_TRACE_REDACT", 1);
+			die("Unable to get offloaded pack file %s\n%s",
+			    showUrl ? preq->url : "<redacted>",
 			    curl_errorstr);
 		}
 	} else {

index de4a94b437e..8da8ed88ccf 100644
@@ -443,7 +443,12 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
 		len--;
 
 	buffer[len] = 0;
-	packet_trace(buffer, len, 0);
+	if (options & PACKET_READ_REDACT_ON_TRACE) {
+		const char *redacted = "<redacted>";
+		packet_trace(redacted, strlen(redacted), 0);
+	} else {
+		packet_trace(buffer, len, 0);
+	}
 
 	if ((options & PACKET_READ_DIE_ON_ERR_PACKET) &&
 	    starts_with(buffer, "ERR "))

index 82b95e4bdd3..44c02f3bc6e 100644
@@ -88,6 +88,7 @@ void packet_fflush(FILE *f);
 #define PACKET_READ_CHOMP_NEWLINE        (1u<<1)
 #define PACKET_READ_DIE_ON_ERR_PACKET    (1u<<2)
 #define PACKET_READ_GENTLE_ON_READ_ERROR (1u<<3)
+#define PACKET_READ_REDACT_ON_TRACE      (1u<<4)
 int packet_read(int fd, char **src_buffer, size_t *src_len, char
 		*buffer, unsigned size, int options);
 

gitgitgadget

From: Ivan Frade <[email protected]>

Packfile-uri line specifies a hash of 40 hex character, but with SHA256 this hash size is 64. There are already tests using SHA256 (e.g. in ubuntu-latest/linux-clang).

Update protocol-v2 documentation to indicate that the hash size depends on the hash algorithm in use.

---
 Documentation/technical/protocol-v2.txt | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index 21e8258ccf3..a23f12d6c2b 100644
@@ -393,7 +393,7 @@ header. Most sections are sent only when the packfile is sent.
     wanted-ref = obj-id SP refname
 
     packfile-uris = PKT-LINE("packfile-uris" LF) *packfile-uri
-    packfile-uri = PKT-LINE(40*(HEXDIGIT) SP *%x20-ff LF)
+    packfile-uri = PKT-LINE((40|64)*(HEXDIGIT) SP *%x20-ff LF)
 
     packfile = PKT-LINE("packfile" LF)
 	       *PKT-LINE(%x01-03 *%x00-ff)

@@ -476,9 +476,9 @@ header. Most sections are sent only when the packfile is sent.
 	* For each URI the server sends, it sends a hash of the pack's
 	  contents (as output by git index-pack) followed by the URI.
 
-	* The hashes are 40 hex characters long. When Git upgrades to a new
-	  hash algorithm, this might need to be updated. (It should match
-	  whatever index-pack outputs after "pack\t" or "keep\t".
+	* The hashes length is defined by the hash algorithm (40 hex
+	  characters in SHA-1, 64 in SHA-256). It should match whatever
+	  index-pack outputs after "pack\t" or "keep\t".
 
     packfile section
 	* This section is only included if the client has sent 'want'

gitgitgadget

For a regular user the URL could be confusing (should they click on it? try to download it by themselves?). I also got a suggestion to print e.g. only the domain and maybe the packname.

In any case, I agree it is a different thing than trace logging. I removed it from this patch.

Updated the grep expressions to look only for the relevant lines and removed the escaping of the space char.

I was trying to limit the grep to the "packfile-uri" section, not to match something else by accident, but I think "obj-id http://"
shouldn't match anything else in the clone response (no ref can start with http://).

Thanks for the quick review!

Ivan

From: Ivan Frade <[email protected]>

In some setups, packfile uris act as bearer token. It is not recommended to expose them plainly in logs, although in special circunstances (e.g. debug) it makes sense to write them.

Redact the packfile-uri lines by default, unless the GIT_TRACE_REDACT variable is set to false. This mimics the redacting of the Authorization header in HTTP.

Changes since v1:
- Removed non-POSIX flags in tests
- More accurate regex for the non-encrypted packfile line
- Dropped documentation change
- Dropped redacting the die message in http-fetch

---
fetch-pack: redact packfile urls in traces

In some setups, packfile uris act as bearer token. It is not recommended to expose them plainly in logs, although in special circunstances (e.g. debug) it makes sense to write them.

Redact the packfile-uri lines by default, unless the GIT_TRACE_REDACT variable is set to false. This mimics the redacting of the Authorization header in HTTP.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1052%2Fifradeo%2Fredact-packfile-uri-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1052/ifradeo/redact-packfile-uri-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1052

Range-diff vs v1:

1: b473f145a87 ! 1: 701cb7a6ab9 fetch-pack: redact packfile urls in traces

     @@ Commit message
          variable is set to false. This mimics the redacting of the
          Authorization header in HTTP.
      
     +    Changes since v1:
     +    - Removed non-POSIX flags in tests
     +    - More accurate regex for the non-encrypted packfile line
     +    - Dropped documentation change
     +    - Dropped redacting the die message in http-fetch
     +
          Signed-off-by: Ivan Frade <[email protected]>
      
       ## fetch-pack.c ##
     @@ fetch-pack.c: static void receive_packfile_uris(struct packet_reader *reader,
       		die("expected DELIM");
       }
      
     - ## http-fetch.c ##
     -@@ http-fetch.c: static void fetch_single_packfile(struct object_id *packfile_hash,
     - 	if (start_active_slot(preq->slot)) {
     - 		run_active_slot(preq->slot);
     - 		if (results.curl_result != CURLE_OK) {
     --			die("Unable to get pack file %s\n%s", preq->url,
     -+			int showUrl = git_env_bool("GIT_TRACE_REDACT", 1);
     -+			die("Unable to get offloaded pack file %s\n%s",
     -+			    showUrl ? preq->url : "<redacted>",
     - 			    curl_errorstr);
     - 		}
     - 	} else {
     -
       ## pkt-line.c ##
      @@ pkt-line.c: enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
       		len--;
     @@ t/t5702-protocol-v2.sh: test_expect_success 'packfile-uri with transfer.fsckobje
      +		-c fetch.uriprotocols=http,https \
      +		clone "$HTTPD_URL/smart/http_parent" http_child &&
      +
     -+	grep -A1 "clone<\ ..packfile-uris" log | grep "clone<\ <redacted>"
     ++	grep "clone< <redacted>" log
      +'
      +
      +test_expect_success 'packfile-uri not redacted in trace when GIT_TRACE_REDACT=0' '
     @@ t/t5702-protocol-v2.sh: test_expect_success 'packfile-uri with transfer.fsckobje
      +		-c fetch.uriprotocols=http,https \
      +		clone "$HTTPD_URL/smart/http_parent" http_child &&
      +
     -+	grep -A1 "clone<\ ..packfile-uris" log  | grep -E "clone<\ ..[[:alnum:]]{40,64}\ http"
     ++	grep -E "clone< ..[0-9a-f]{40,64} http://" log
      +'
      +
       test_expect_success 'http:// --negotiate-only' '

2: 497c5fd18d7 < -: ----------- Documentation: packfile-uri hash can be longer than 40 hex chars

 fetch-pack.c           | 11 +++++++++++
 pkt-line.c             |  7 ++++++-
 pkt-line.h             |  1 +
 t/t5702-protocol-v2.sh | 43 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 61 insertions(+), 1 deletion(-)

index a9604f35a3e..05c85eeafa1 100644
@@ -1518,7 +1518,16 @@ static void receive_wanted_refs(struct packet_reader *reader,
 static void receive_packfile_uris(struct packet_reader *reader,
 				  struct string_list *uris)
 {
+	int original_options;
 	process_section_header(reader, "packfile-uris", 0);
+	/*
+	 * In some setups, packfile-uris act as bearer tokens,
+	 * redact them by default.
+	 */
+	original_options = reader->options;
+	if (git_env_bool("GIT_TRACE_REDACT", 1))
+		reader->options |= PACKET_READ_REDACT_ON_TRACE;
+
 	while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
 		if (reader->pktlen < the_hash_algo->hexsz ||
 		    reader->line[the_hash_algo->hexsz] != ' ')

@@ -1526,6 +1535,8 @@ static void receive_packfile_uris(struct packet_reader *reader,
 
 		string_list_append(uris, reader->line);
 	}
+	reader->options = original_options;
+
 	if (reader->status != PACKET_READ_DELIM)
 		die("expected DELIM");
 }

index de4a94b437e..8da8ed88ccf 100644
@@ -443,7 +443,12 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
 		len--;
 
 	buffer[len] = 0;
-	packet_trace(buffer, len, 0);
+	if (options & PACKET_READ_REDACT_ON_TRACE) {
+		const char *redacted = "<redacted>";
+		packet_trace(redacted, strlen(redacted), 0);
+	} else {
+		packet_trace(buffer, len, 0);
+	}
 
 	if ((options & PACKET_READ_DIE_ON_ERR_PACKET) &&
 	    starts_with(buffer, "ERR "))

index 82b95e4bdd3..44c02f3bc6e 100644
@@ -88,6 +88,7 @@ void packet_fflush(FILE *f);
 #define PACKET_READ_CHOMP_NEWLINE        (1u<<1)
 #define PACKET_READ_DIE_ON_ERR_PACKET    (1u<<2)
 #define PACKET_READ_GENTLE_ON_READ_ERROR (1u<<3)
+#define PACKET_READ_REDACT_ON_TRACE      (1u<<4)
 int packet_read(int fd, char **src_buffer, size_t *src_len, char
 		*buffer, unsigned size, int options);
 

base-commit: 0785eb769886ae81e346df10e88bc49ffc0ac64e

gitgitgadget

Well explained.

It of course is a different matter if the explained idea is agreeable, though ;-). Hiding the entire packet, based on the "it might be in some setups" seems a bit too much.

Is it often the case that the whole URI is sensitive, or perhaps leading "<scheme>://<host>/pack-<abc>.pack" part is not sensitive at all, and what follows after that "public" part has some "nonce" material that makes it sensitive?

These are not for those who read "git log" in 3 months, as they may not even have seen the "v1". But these are very helpful for those who read the "v1" to see how good this round is. Please write such material below the three-dash line.

i.e. here.

And there is no need to duplicate the log message here ;-)

From: Ivan Frade <[email protected]>

In some setups, packfile uris act as bearer token. It is not recommended to expose them plainly in logs, although in special circunstances (e.g. debug) it makes sense to write them.

Redact the packfile URL paths by default, unless the GIT_TRACE_REDACT variable is set to false. This mimics the redacting of the Authorization header in HTTP.

---
fetch-pack: redact packfile urls in traces

Changes since v1:

* Redact only the path of the URL
* Test are now strict, validating the exact line expected in the log

Changes since v1:

* Removed non-POSIX flags in tests
* More accurate regex for the non-encrypted packfile line
* Dropped documentation change
* Dropped redacting the die message in http-fetch

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1052%2Fifradeo%2Fredact-packfile-uri-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1052/ifradeo/redact-packfile-uri-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1052

Range-diff vs v2:

1: 701cb7a6ab9 ! 1: 9afe0093af4 fetch-pack: redact packfile urls in traces

     @@ Commit message
          recommended to expose them plainly in logs, although in special
          circunstances (e.g. debug) it makes sense to write them.
      
     -    Redact the packfile-uri lines by default, unless the GIT_TRACE_REDACT
     -    variable is set to false. This mimics the redacting of the
     -    Authorization header in HTTP.
     -
     -    Changes since v1:
     -    - Removed non-POSIX flags in tests
     -    - More accurate regex for the non-encrypted packfile line
     -    - Dropped documentation change
     -    - Dropped redacting the die message in http-fetch
     +    Redact the packfile URL paths by default, unless the GIT_TRACE_REDACT
     +    variable is set to false. This mimics the redacting of the Authorization
     +    header in HTTP.
      
          Signed-off-by: Ivan Frade <[email protected]>
      
     @@ fetch-pack.c: static void receive_wanted_refs(struct packet_reader *reader,
       static void receive_packfile_uris(struct packet_reader *reader,
       				  struct string_list *uris)
       {
     -+	int original_options;
     ++	int saved_options;
       	process_section_header(reader, "packfile-uris", 0);
      +	/*
      +	 * In some setups, packfile-uris act as bearer tokens,
      +	 * redact them by default.
      +	 */
     -+	original_options = reader->options;
     ++	saved_options = reader->options;
      +	if (git_env_bool("GIT_TRACE_REDACT", 1))
     -+		reader->options |= PACKET_READ_REDACT_ON_TRACE;
     ++		reader->options |= PACKET_READ_REDACT_URL_PATH;
      +
       	while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
       		if (reader->pktlen < the_hash_algo->hexsz ||
     @@ fetch-pack.c: static void receive_packfile_uris(struct packet_reader *reader,
       
       		string_list_append(uris, reader->line);
       	}
     -+	reader->options = original_options;
     ++	reader->options = saved_options;
      +
       	if (reader->status != PACKET_READ_DELIM)
       		die("expected DELIM");
       }
      
       ## pkt-line.c ##
     +@@ pkt-line.c: int packet_length(const char lenbuf_hex[4])
     + 	return (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);
     + }
     + 
     ++static int find_url_path_start(const char* buffer)
     ++{
     ++	const char *URL_MARK = "://";
     ++	char *p = strstr(buffer, URL_MARK);
     ++	if (!p) {
     ++		return -1;
     ++	}
     ++
     ++	p += strlen(URL_MARK);
     ++	while (*p && *p != '/')
     ++		p++;
     ++
     ++	// Position after '/'
     ++	if (*p && *(p + 1))
     ++		return (p + 1) - buffer;
     ++
     ++	return -1;
     ++}
     ++
     + enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
     + 						size_t *src_len, char *buffer,
     + 						unsigned size, int *pktlen,
     +@@ pkt-line.c: enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
     + {
     + 	int len;
     + 	char linelen[4];
     ++	int url_path_start;
     + 
     + 	if (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0) {
     + 		*pktlen = -1;
      @@ pkt-line.c: enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
       		len--;
       
       	buffer[len] = 0;
      -	packet_trace(buffer, len, 0);
     -+	if (options & PACKET_READ_REDACT_ON_TRACE) {
     ++	if (options & PACKET_READ_REDACT_URL_PATH &&
     ++	    (url_path_start = find_url_path_start(buffer)) != -1) {
      +		const char *redacted = "<redacted>";
     -+		packet_trace(redacted, strlen(redacted), 0);
     ++		struct strbuf tracebuf = STRBUF_INIT;
     ++		strbuf_insert(&tracebuf, 0, buffer, len);
     ++		strbuf_splice(&tracebuf, url_path_start,
     ++			      len - url_path_start, redacted, strlen(redacted));
     ++		packet_trace(tracebuf.buf, tracebuf.len, 0);
     ++		strbuf_release(&tracebuf);
      +	} else {
      +		packet_trace(buffer, len, 0);
      +	}
     @@ pkt-line.h: void packet_fflush(FILE *f);
       #define PACKET_READ_CHOMP_NEWLINE        (1u<<1)
       #define PACKET_READ_DIE_ON_ERR_PACKET    (1u<<2)
       #define PACKET_READ_GENTLE_ON_READ_ERROR (1u<<3)
     -+#define PACKET_READ_REDACT_ON_TRACE      (1u<<4)
     ++#define PACKET_READ_REDACT_URL_PATH      (1u<<4)
       int packet_read(int fd, char **src_buffer, size_t *src_len, char
       		*buffer, unsigned size, int options);
       
     @@ t/t5702-protocol-v2.sh: test_expect_success 'packfile-uri with transfer.fsckobje
       	test_i18ngrep "disallowed submodule name" err
       '
       
     -+test_expect_success 'packfile-uri redacted in trace' '
     ++test_expect_success 'packfile-uri path redacted in trace' '
      +	P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
      +	rm -rf "$P" http_child log &&
      +
     @@ t/t5702-protocol-v2.sh: test_expect_success 'packfile-uri with transfer.fsckobje
      +	git -C "$P" add my-blob &&
      +	git -C "$P" commit -m x &&
      +
     -+	configure_exclusion "$P" my-blob >h &&
     ++	git -C "$P" hash-object my-blob >objh &&
     ++	git -C "$P" pack-objects "$HTTPD_DOCUMENT_ROOT_PATH/mypack" <objh >packh &&
     ++	git -C "$P" config --add \
     ++		"uploadpack.blobpackfileuri" \
     ++		"$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack" &&
      +
      +	GIT_TRACE=1 GIT_TRACE_PACKET="$(pwd)/log" GIT_TEST_SIDEBAND_ALL=1 \
      +	git -c protocol.version=2 \
      +		-c fetch.uriprotocols=http,https \
      +		clone "$HTTPD_URL/smart/http_parent" http_child &&
      +
     -+	grep "clone< <redacted>" log
     ++	grep -F "clone< \\1$(cat packh) $HTTPD_URL/<redacted>" log
      +'
      +
     -+test_expect_success 'packfile-uri not redacted in trace when GIT_TRACE_REDACT=0' '
     ++test_expect_success 'packfile-uri path not redacted in trace when GIT_TRACE_REDACT=0' '
      +	P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
      +	rm -rf "$P" http_child log &&
      +
     @@ t/t5702-protocol-v2.sh: test_expect_success 'packfile-uri with transfer.fsckobje
      +	git -C "$P" add my-blob &&
      +	git -C "$P" commit -m x &&
      +
     -+	configure_exclusion "$P" my-blob >h &&
     ++	git -C "$P" hash-object my-blob >objh &&
     ++	git -C "$P" pack-objects "$HTTPD_DOCUMENT_ROOT_PATH/mypack" <objh >packh &&
     ++	git -C "$P" config --add \
     ++		"uploadpack.blobpackfileuri" \
     ++		"$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack" &&
      +
      +	GIT_TRACE=1 GIT_TRACE_PACKET="$(pwd)/log" GIT_TEST_SIDEBAND_ALL=1 \
      +	GIT_TRACE_REDACT=0 \
     @@ t/t5702-protocol-v2.sh: test_expect_success 'packfile-uri with transfer.fsckobje
      +		-c fetch.uriprotocols=http,https \
      +		clone "$HTTPD_URL/smart/http_parent" http_child &&
      +
     -+	grep -E "clone< ..[0-9a-f]{40,64} http://" log
     ++	grep -F "clone< \\1$(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack" log
      +'
      +
       test_expect_success 'http:// --negotiate-only' '

 fetch-pack.c           | 11 +++++++++
 pkt-line.c             | 33 ++++++++++++++++++++++++++-
 pkt-line.h             |  1 +
 t/t5702-protocol-v2.sh | 51 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 95 insertions(+), 1 deletion(-)

index a9604f35a3e..1587b9ae662 100644
@@ -1518,7 +1518,16 @@ static void receive_wanted_refs(struct packet_reader *reader,
 static void receive_packfile_uris(struct packet_reader *reader,
 				  struct string_list *uris)
 {
+	int saved_options;
 	process_section_header(reader, "packfile-uris", 0);
+	/*
+	 * In some setups, packfile-uris act as bearer tokens,
+	 * redact them by default.
+	 */
+	saved_options = reader->options;
+	if (git_env_bool("GIT_TRACE_REDACT", 1))
+		reader->options |= PACKET_READ_REDACT_URL_PATH;
+
 	while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
 		if (reader->pktlen < the_hash_algo->hexsz ||
 		    reader->line[the_hash_algo->hexsz] != ' ')

@@ -1526,6 +1535,8 @@ static void receive_packfile_uris(struct packet_reader *reader,
 
 		string_list_append(uris, reader->line);
 	}
+	reader->options = saved_options;
+
 	if (reader->status != PACKET_READ_DELIM)
 		die("expected DELIM");
 }

index de4a94b437e..1a9e6870559 100644
@@ -386,6 +386,25 @@ int packet_length(const char lenbuf_hex[4])
 	return (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);
 }
 
+static int find_url_path_start(const char* buffer)
+{
+	const char *URL_MARK = "://";
+	char *p = strstr(buffer, URL_MARK);
+	if (!p) {
+		return -1;
+	}
+
+	p += strlen(URL_MARK);
+	while (*p && *p != '/')
+		p++;
+
+	// Position after '/'
+	if (*p && *(p + 1))
+		return (p + 1) - buffer;
+
+	return -1;
+}
+
 enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
 						size_t *src_len, char *buffer,
 						unsigned size, int *pktlen,

@@ -393,6 +412,7 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
 {
 	int len;
 	char linelen[4];
+	int url_path_start;
 
 	if (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0) {
 		*pktlen = -1;

@@ -443,7 +463,18 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
 		len--;
 
 	buffer[len] = 0;
-	packet_trace(buffer, len, 0);
+	if (options & PACKET_READ_REDACT_URL_PATH &&
+	    (url_path_start = find_url_path_start(buffer)) != -1) {
+		const char *redacted = "<redacted>";
+		struct strbuf tracebuf = STRBUF_INIT;
+		strbuf_insert(&tracebuf, 0, buffer, len);
+		strbuf_splice(&tracebuf, url_path_start,
+			      len - url_path_start, redacted, strlen(redacted));
+		packet_trace(tracebuf.buf, tracebuf.len, 0);
+		strbuf_release(&tracebuf);
+	} else {
+		packet_trace(buffer, len, 0);
+	}
 
 	if ((options & PACKET_READ_DIE_ON_ERR_PACKET) &&
 	    starts_with(buffer, "ERR "))

index 82b95e4bdd3..853d20688c8 100644
@@ -88,6 +88,7 @@ void packet_fflush(FILE *f);
 #define PACKET_READ_CHOMP_NEWLINE        (1u<<1)
 #define PACKET_READ_DIE_ON_ERR_PACKET    (1u<<2)
 #define PACKET_READ_GENTLE_ON_READ_ERROR (1u<<3)
+#define PACKET_READ_REDACT_URL_PATH      (1u<<4)
 int packet_read(int fd, char **src_buffer, size_t *src_len, char
 		*buffer, unsigned size, int options);
 

base-commit: 9d530dc0024503ab4218fe6c4395b8a0aa245478

gitgitgadget

Just context for other reviewers:

s/Changes since v1/Changes since v2/ I see, from the context of
https://lore.kernel.org/git/[email protected]/

And this was changed in v2...

[...]

Since both of those were done I think in response to my feedback I just want to clarify (if needed):

* That the documentation change is still good to have, although I had feedback on fixing that more generally in the protocol v2 docs. It would be great if you could still pursue it (and that I didn't discourage you from doing so).

* I think having this redaction in die() could still be valuable, e.g. your packfile-uri's start failing, and now users are copy/pasting "private" URLs that contain their passwords or whatever to try to get help, that would be bad.

But perhaps if you don't have private URLs redacting them unconditionally would slow down debugging for some, i.e. you have 10x pasted URLs, and all the errors are from one set of servers (although your current redaction includes the hostname, which I think would address most cases of say one CDN node failing).

It was really just a comment that your v1's commit message didn't mention or justify it, but just having it make a mention of it would also be an OK solution, or fold that into another patch or whatever...

Documentation/git.txt-  By default, when tracing is activated, Git redacts the values of
Documentation/git.txt-  cookies, the "Authorization:" header, and the "Proxy-Authorization:"
Documentation/git.txt-  header. Set this variable to `0` to prevent this redaction.

It seems I sent my original reply only to the github PR. Sorry for the confusion:

...

In the specific case I am working on, the path of the URL is an encrypted string that shouldn't be completely exposed (exposing part of it would be fine). In general, I think we can assume that <scheme>://<host>/ are always "public" but the path could be sensitive.

We could redact only the path (<scheme>://<host>/REDACTED), or even a fixed length of the URL? (<scheme>://<host>/pack-<xxREDACTED).

In the next patch version I go with redacting the path.

...

Done

Done

In the latest iteration, the option is enabled for all sections and there is no need to set/unset the flag.

Now the tests verify the expected packfile-uri full line is in the log.

Thanks,

Ivan

Changes since v3:

* Enable redacting URLs for all sections
* Redact only URL path (it was until the end of line)
* Redact URL in die() with more friendly message
* Update doc to mention that packfile URIs are also redacted.

Changes since v2:

* Redact only the path of the URL
* Test are now strict, validating the exact line expected in the log

Changes since v1:

* Removed non-POSIX flags in tests
* More accurate regex for the non-encrypted packfile line
* Dropped documentation change
* Dropped redacting the die message in http-fetch

Ivan Frade (2):
fetch-pack: redact packfile urls in traces
http-fetch: redact url on die() message

 Documentation/git.txt  |  5 +++--
 fetch-pack.c           |  3 +++
 http-fetch.c           | 15 +++++++++++--
 pkt-line.c             | 40 ++++++++++++++++++++++++++++++++-
 pkt-line.h             |  1 +
 t/t5702-protocol-v2.sh | 51 ++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 110 insertions(+), 5 deletions(-)

base-commit: e9e5ba39a78c8f5057262d49e261b42a8660d5b9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1052%2Fifradeo%2Fredact-packfile-uri-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1052/ifradeo/redact-packfile-uri-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1052

Range-diff vs v3:

1: 9afe0093af4 ! 1: 973a250752c fetch-pack: redact packfile urls in traces

     @@ Commit message
      
          Signed-off-by: Ivan Frade <[email protected]>
      
     - ## fetch-pack.c ##
     -@@ fetch-pack.c: static void receive_wanted_refs(struct packet_reader *reader,
     - static void receive_packfile_uris(struct packet_reader *reader,
     - 				  struct string_list *uris)
     - {
     -+	int saved_options;
     - 	process_section_header(reader, "packfile-uris", 0);
     -+	/*
     -+	 * In some setups, packfile-uris act as bearer tokens,
     -+	 * redact them by default.
     -+	 */
     -+	saved_options = reader->options;
     -+	if (git_env_bool("GIT_TRACE_REDACT", 1))
     -+		reader->options |= PACKET_READ_REDACT_URL_PATH;
     -+
     - 	while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
     - 		if (reader->pktlen < the_hash_algo->hexsz ||
     - 		    reader->line[the_hash_algo->hexsz] != ' ')
     -@@ fetch-pack.c: static void receive_packfile_uris(struct packet_reader *reader,
     + ## Documentation/git.txt ##
     +@@ Documentation/git.txt: for full details.
       
     - 		string_list_append(uris, reader->line);
     + `GIT_TRACE_REDACT`::
     + 	By default, when tracing is activated, Git redacts the values of
     +-	cookies, the "Authorization:" header, and the "Proxy-Authorization:"
     +-	header. Set this variable to `0` to prevent this redaction.
     ++	cookies, the "Authorization:" header, the "Proxy-Authorization:"
     ++	header and packfile URLs. Set this variable to `0` to prevent this
     ++	redaction.
     + 
     + `GIT_LITERAL_PATHSPECS`::
     + 	Setting this variable to `1` will cause Git to treat all
     +
     + ## fetch-pack.c ##
     +@@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
     + 		reader.me = "fetch-pack";
       	}
     -+	reader->options = saved_options;
     + 
     ++	if (git_env_bool("GIT_TRACE_REDACT", 1))
     ++		reader.options |= PACKET_READ_REDACT_URL_PATH;
      +
     - 	if (reader->status != PACKET_READ_DELIM)
     - 		die("expected DELIM");
     - }
     + 	while (state != FETCH_DONE) {
     + 		switch (state) {
     + 		case FETCH_CHECK_LOCAL:
      
       ## pkt-line.c ##
      @@ pkt-line.c: int packet_length(const char lenbuf_hex[4])
       	return (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);
       }
       
     -+static int find_url_path_start(const char* buffer)
     ++static char *find_url_path(const char* buffer, int *path_len)
      +{
      +	const char *URL_MARK = "://";
     -+	char *p = strstr(buffer, URL_MARK);
     -+	if (!p) {
     -+		return -1;
     -+	}
     ++	char *path = strstr(buffer, URL_MARK);
     ++	if (!path)
     ++		return NULL;
      +
     -+	p += strlen(URL_MARK);
     -+	while (*p && *p != '/')
     -+		p++;
     ++	path += strlen(URL_MARK);
     ++	while (*path && *path != '/')
     ++		path++;
      +
     -+	// Position after '/'
     -+	if (*p && *(p + 1))
     -+		return (p + 1) - buffer;
     ++	if (!*path || !*(path + 1))
     ++		return NULL;
     ++
     ++	// position after '/'
     ++	path++;
     ++
     ++	if (path_len) {
     ++		char *url_end = strchrnul(path, ' ');
     ++		*path_len = url_end - path;
     ++	}
      +
     -+	return -1;
     ++	return path;
      +}
      +
       enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
     @@ pkt-line.c: enum packet_read_status packet_read_with_status(int fd, char **src_b
       {
       	int len;
       	char linelen[4];
     -+	int url_path_start;
     ++	char *url_path_start;
     ++	int url_path_len;
       
       	if (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0) {
       		*pktlen = -1;
     @@ pkt-line.c: enum packet_read_status packet_read_with_status(int fd, char **src_b
       	buffer[len] = 0;
      -	packet_trace(buffer, len, 0);
      +	if (options & PACKET_READ_REDACT_URL_PATH &&
     -+	    (url_path_start = find_url_path_start(buffer)) != -1) {
     ++	    (url_path_start = find_url_path(buffer, &url_path_len))) {
      +		const char *redacted = "<redacted>";
      +		struct strbuf tracebuf = STRBUF_INIT;
      +		strbuf_insert(&tracebuf, 0, buffer, len);
     -+		strbuf_splice(&tracebuf, url_path_start,
     -+			      len - url_path_start, redacted, strlen(redacted));
     ++		strbuf_splice(&tracebuf, url_path_start - buffer,
     ++			      url_path_len, redacted, strlen(redacted));
      +		packet_trace(tracebuf.buf, tracebuf.len, 0);
      +		strbuf_release(&tracebuf);
      +	} else {
     @@ pkt-line.h: void packet_fflush(FILE *f);
       #define PACKET_READ_DIE_ON_ERR_PACKET    (1u<<2)
       #define PACKET_READ_GENTLE_ON_READ_ERROR (1u<<3)
      +#define PACKET_READ_REDACT_URL_PATH      (1u<<4)
     - int packet_read(int fd, char **src_buffer, size_t *src_len, char
     - 		*buffer, unsigned size, int options);
     + int packet_read(int fd, char *buffer, unsigned size, int options);
       
     + /*
      
       ## t/t5702-protocol-v2.sh ##
      @@ t/t5702-protocol-v2.sh: test_expect_success 'packfile-uri with transfer.fsckobjects fails when .gitmodul

-: ----------- > 2: c7f0977cabd http-fetch: redact url on die() message

gitgitgadget

From: Ivan Frade <[email protected]>

In some setups, packfile uris act as bearer token. It is not recommended to expose them plainly in logs, although in special circunstances (e.g. debug) it makes sense to write them.

Redact the packfile URL paths by default, unless the GIT_TRACE_REDACT variable is set to false. This mimics the redacting of the Authorization header in HTTP.

---
 Documentation/git.txt  |  5 +++--
 fetch-pack.c           |  3 +++
 pkt-line.c             | 40 ++++++++++++++++++++++++++++++++-
 pkt-line.h             |  1 +
 t/t5702-protocol-v2.sh | 51 ++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 97 insertions(+), 3 deletions(-)

index d63c65e67d8..f64c8ce5183 100644
@@ -832,8 +832,9 @@ for full details.
 
 `GIT_TRACE_REDACT`::
 	By default, when tracing is activated, Git redacts the values of
-	cookies, the "Authorization:" header, and the "Proxy-Authorization:"
-	header. Set this variable to `0` to prevent this redaction.
+	cookies, the "Authorization:" header, the "Proxy-Authorization:"
+	header and packfile URLs. Set this variable to `0` to prevent this
+	redaction.
 
 `GIT_LITERAL_PATHSPECS`::
 	Setting this variable to `1` will cause Git to treat all

index a9604f35a3e..ad8ac49ca50 100644
@@ -1581,6 +1581,9 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 		reader.me = "fetch-pack";
 	}
 
+	if (git_env_bool("GIT_TRACE_REDACT", 1))
+		reader.options |= PACKET_READ_REDACT_URL_PATH;
+
 	while (state != FETCH_DONE) {
 		switch (state) {
 		case FETCH_CHECK_LOCAL:

index 2dc8ac274bd..ba0a2d65f0c 100644
@@ -370,6 +370,31 @@ int packet_length(const char lenbuf_hex[4])
 	return (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);
 }
 
+static char *find_url_path(const char* buffer, int *path_len)
+{
+	const char *URL_MARK = "://";
+	char *path = strstr(buffer, URL_MARK);
+	if (!path)
+		return NULL;
+
+	path += strlen(URL_MARK);
+	while (*path && *path != '/')
+		path++;
+
+	if (!*path || !*(path + 1))
+		return NULL;
+
+	// position after '/'
+	path++;
+
+	if (path_len) {
+		char *url_end = strchrnul(path, ' ');
+		*path_len = url_end - path;
+	}
+
+	return path;
+}
+
 enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
 						size_t *src_len, char *buffer,
 						unsigned size, int *pktlen,

@@ -377,6 +402,8 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
 {
 	int len;
 	char linelen[4];
+	char *url_path_start;
+	int url_path_len;
 
 	if (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0) {
 		*pktlen = -1;

@@ -427,7 +454,18 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
 		len--;
 
 	buffer[len] = 0;
-	packet_trace(buffer, len, 0);
+	if (options & PACKET_READ_REDACT_URL_PATH &&
+	    (url_path_start = find_url_path(buffer, &url_path_len))) {
+		const char *redacted = "<redacted>";
+		struct strbuf tracebuf = STRBUF_INIT;
+		strbuf_insert(&tracebuf, 0, buffer, len);
+		strbuf_splice(&tracebuf, url_path_start - buffer,
+			      url_path_len, redacted, strlen(redacted));
+		packet_trace(tracebuf.buf, tracebuf.len, 0);
+		strbuf_release(&tracebuf);
+	} else {
+		packet_trace(buffer, len, 0);
+	}
 
 	if ((options & PACKET_READ_DIE_ON_ERR_PACKET) &&
 	    starts_with(buffer, "ERR "))

index 467ae013573..a610ecb88e8 100644
@@ -87,6 +87,7 @@ void packet_fflush(FILE *f);
 #define PACKET_READ_CHOMP_NEWLINE        (1u<<1)
 #define PACKET_READ_DIE_ON_ERR_PACKET    (1u<<2)
 #define PACKET_READ_GENTLE_ON_READ_ERROR (1u<<3)
+#define PACKET_READ_REDACT_URL_PATH      (1u<<4)
 int packet_read(int fd, char *buffer, unsigned size, int options);
 
 /*

gitgitgadget

From: Ivan Frade <[email protected]>

http-fetch prints the URL after failing to fetch it. This can be confusing to users (they cannot really do anything with it) but even worse, they can share by accident a sensitive URL (e.g. with credentials) while looking for help.

Redact the URL unless the GIT_TRACE_REDACT variable is set to false. This mimics the redaction of other sensitive information in git, like the Authorization header in HTTP.

---
 http-fetch.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

index fa642462a9e..bbe09a6ad9f 100644
@@ -4,6 +4,7 @@
 #include "http.h"
 #include "walker.h"
 #include "strvec.h"
+#include "urlmatch.h"
 
 static const char http_fetch_usage[] = "git http-fetch "
 "[-c] [-t] [-a] [-v] [--recover] [-w ref] [--stdin | --packfile=hash | commit-id] url";

@@ -63,8 +64,18 @@ static void fetch_single_packfile(struct object_id *packfile_hash,
 	if (start_active_slot(preq->slot)) {
 		run_active_slot(preq->slot);
 		if (results.curl_result != CURLE_OK) {
-			die("Unable to get pack file %s\n%s", preq->url,
-			    curl_errorstr);
+			struct url_info url;
+			char *nurl = url_normalize(preq->url, &url);
+			if (!git_env_bool("GIT_TRACE_REDACT", 1) || !nurl) {
+				die("Unable to get pack file %s\n%s", preq->url,
+				    curl_errorstr);
+			} else {
+				char *schema = xstrndup(url.url, url.scheme_len);
+				char *host = xstrndup(&url.url[url.host_off], url.host_len);
+				die("failed to get '%s' url from '%s' "
+				    "(full URL redacted due to GIT_TRACE_REDACT setting)\n%s",
+				    schema, host, curl_errorstr);
+			}
 		}
 	} else {
 		die("Unable to start request");

gitgitgadget

I had the same passing thought when glancing over this code (although this appears to be an error patch, thus not performance critical, so not terribly important).

Note that this is not a new die() call; it just got indented as-is by this patch, so the changes you suggest to the message string are potentially outside the scope of this patch. Possibilities: (1) make the changes in this patch but mention them in the commit message; (2) make the changes in a preparatory patch; (3) punt on the changes for now.

I wondered the same when reading the patch. Thanks for mentioning it.

The feature is "packfile URI" (and the section is called so in the protocol). I changed all "url" to "uri".

I was trying to find a uri in a packet in general, not counting on the packfile-uri line format. That is probably an overgeneralization.

Next patch version follows these suggestions to look for a packfile-uri line.

Yes, now that we count on the packfile-uri line format, we can redact everything to the end and there is no need to return the length.

We only allocate and redact if it looks like a packfile-uri line, so it shouldn't happen too frequently.

I move the set/unset of the redacting flag to the FETCH_GET_PACK around the "packfile-uris" section.
There is no need to check every incoming packet for a packfile-uri line, we know when they should come.

Thanks,

Ivan

...

Done

Done

Indeed, "%.*s" did the trick. Thanks!

Ivan

I went for option (1). It is a minimal change and we are moving that line in this commit.

Ivan

Ah, I see. This is merely a tracing, so we might benefit from a generalized version of redactor, and from that point of view, the use of strstr and stopping at the whitespace do make sort-of sense to me, but then we lack any attempt to redact more than one instance of URL in a packet, so the generalization may have quite a limited usefulness.

Yeah, I think that is a good way to go, at least for now. When we want a more general one, we can revisit it, but not now.

I was mostly wondering about the cost of determining "if it looks like?". But we do this only for the protocol header part, so we won't have thousands of attempts to match, I guess. Oh, or if we also do this for the ref advertisement packets, then we might have quite a many. Hmph.

Yeah, that is quite a wise design decision, I would think.

Thanks.

Changes since v4:

* Use "uri" instead of "url"
* Look specifically for a line with packfile-uri format (instead of for a URL in general)
* Limit the redacting to the packfile-uri section in do_fetch_pack_v2
* Use "%.*s" instead of duplicating parts of the string to print

Changes since v3:

* Enable redacting URLs for all sections
* Redact only URL path (it was until the end of line)
* Redact URL in die() with more friendly message
* Update doc to mention that packfile URIs are also redacted.

Changes since v2:

* Redact only the path of the URL
* Test are now strict, validating the exact line expected in the log

Changes since v1:

* Removed non-POSIX flags in tests
* More accurate regex for the non-encrypted packfile line
* Dropped documentation change
* Dropped redacting the die message in http-fetch

Ivan Frade (2):
fetch-pack: redact packfile urls in traces
http-fetch: redact url on die() message

 Documentation/git.txt  |  5 +++--
 fetch-pack.c           |  4 ++++
 http-fetch.c           | 14 ++++++++++--
 pkt-line.c             | 39 +++++++++++++++++++++++++++++++-
 pkt-line.h             |  1 +
 t/t5702-protocol-v2.sh | 51 ++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 109 insertions(+), 5 deletions(-)

base-commit: e9e5ba39a78c8f5057262d49e261b42a8660d5b9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1052%2Fifradeo%2Fredact-packfile-uri-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1052/ifradeo/redact-packfile-uri-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/1052

Range-diff vs v4:

1: 973a250752c ! 1: c95b3cafcd6 fetch-pack: redact packfile urls in traces

     @@ Documentation/git.txt: for full details.
      -	cookies, the "Authorization:" header, and the "Proxy-Authorization:"
      -	header. Set this variable to `0` to prevent this redaction.
      +	cookies, the "Authorization:" header, the "Proxy-Authorization:"
     -+	header and packfile URLs. Set this variable to `0` to prevent this
     ++	header and packfile URIs. Set this variable to `0` to prevent this
      +	redaction.
       
       `GIT_LITERAL_PATHSPECS`::
     @@ Documentation/git.txt: for full details.
      
       ## fetch-pack.c ##
      @@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
     - 		reader.me = "fetch-pack";
     - 	}
     + 				receive_wanted_refs(&reader, sought, nr_sought);
       
     -+	if (git_env_bool("GIT_TRACE_REDACT", 1))
     -+		reader.options |= PACKET_READ_REDACT_URL_PATH;
     -+
     - 	while (state != FETCH_DONE) {
     - 		switch (state) {
     - 		case FETCH_CHECK_LOCAL:
     + 			/* get the pack(s) */
     ++			if (git_env_bool("GIT_TRACE_REDACT", 1))
     ++				reader.options |= PACKET_READ_REDACT_URI_PATH;
     + 			if (process_section_header(&reader, "packfile-uris", 1))
     + 				receive_packfile_uris(&reader, &packfile_uris);
     ++			reader.options &= ~PACKET_READ_REDACT_URI_PATH;
     ++
     + 			process_section_header(&reader, "packfile", 0);
     + 
     + 			/*
      
       ## pkt-line.c ##
      @@ pkt-line.c: int packet_length(const char lenbuf_hex[4])
       	return (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);
       }
       
     -+static char *find_url_path(const char* buffer, int *path_len)
     ++static char *find_packfile_uri_path(const char *buffer)
      +{
     -+	const char *URL_MARK = "://";
     -+	char *path = strstr(buffer, URL_MARK);
     -+	if (!path)
     -+		return NULL;
     ++	const char *URI_MARK = "://";
     ++	char *path;
     ++	int len;
      +
     -+	path += strlen(URL_MARK);
     -+	while (*path && *path != '/')
     -+		path++;
     ++	/* First char is sideband mark */
     ++	buffer += 1;
      +
     -+	if (!*path || !*(path + 1))
     -+		return NULL;
     ++	len = strspn(buffer, "0123456789abcdefABCDEF");
     ++	if (!(len == 40 || len == 64) || buffer[len] != ' ')
     ++		return NULL; /* required "<hash>SP" not seen */
      +
     -+	// position after '/'
     -+	path++;
     ++	path = strstr(buffer + len + 1, URI_MARK);
     ++	if (!path)
     ++		return NULL;
      +
     -+	if (path_len) {
     -+		char *url_end = strchrnul(path, ' ');
     -+		*path_len = url_end - path;
     -+	}
     ++	path = strchr(path + strlen(URI_MARK), '/');
     ++	if (!path || !*(path + 1))
     ++		return NULL;
      +
     -+	return path;
     ++	/* position after '/' */
     ++	return ++path;
      +}
      +
       enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
     @@ pkt-line.c: enum packet_read_status packet_read_with_status(int fd, char **src_b
       {
       	int len;
       	char linelen[4];
     -+	char *url_path_start;
     -+	int url_path_len;
     ++	char *uri_path_start;
       
       	if (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0) {
       		*pktlen = -1;
     @@ pkt-line.c: enum packet_read_status packet_read_with_status(int fd, char **src_b
       
       	buffer[len] = 0;
      -	packet_trace(buffer, len, 0);
     -+	if (options & PACKET_READ_REDACT_URL_PATH &&
     -+	    (url_path_start = find_url_path(buffer, &url_path_len))) {
     ++	if (options & PACKET_READ_REDACT_URI_PATH &&
     ++	    (uri_path_start = find_packfile_uri_path(buffer))) {
      +		const char *redacted = "<redacted>";
      +		struct strbuf tracebuf = STRBUF_INIT;
      +		strbuf_insert(&tracebuf, 0, buffer, len);
     -+		strbuf_splice(&tracebuf, url_path_start - buffer,
     -+			      url_path_len, redacted, strlen(redacted));
     ++		strbuf_splice(&tracebuf, uri_path_start - buffer,
     ++			      strlen(uri_path_start), redacted, strlen(redacted));
      +		packet_trace(tracebuf.buf, tracebuf.len, 0);
      +		strbuf_release(&tracebuf);
      +	} else {
     @@ pkt-line.h: void packet_fflush(FILE *f);
       #define PACKET_READ_CHOMP_NEWLINE        (1u<<1)
       #define PACKET_READ_DIE_ON_ERR_PACKET    (1u<<2)
       #define PACKET_READ_GENTLE_ON_READ_ERROR (1u<<3)
     -+#define PACKET_READ_REDACT_URL_PATH      (1u<<4)
     ++#define PACKET_READ_REDACT_URI_PATH      (1u<<4)
       int packet_read(int fd, char *buffer, unsigned size, int options);
       
       /*
 2:  c7f0977cabd ! 2:  6912a690197 http-fetch: redact url on die() message
     @@ Commit message
          http-fetch: redact url on die() message
      
          http-fetch prints the URL after failing to fetch it. This can be
     -    confusing to users (they cannot really do anything with it) but even
     -    worse, they can share by accident a sensitive URL (e.g. with
     -    credentials) while looking for help.
     +    confusing to users (they cannot really do anything with it), and they
     +    can share by accident a sensitive URL (e.g. with credentials) while
     +    looking for help.
      
          Redact the URL unless the GIT_TRACE_REDACT variable is set to false. This
          mimics the redaction of other sensitive information in git, like the
          Authorization header in HTTP.
      
     +    Fix also capitalization of previous die() message (must start in
     +    lowercase).
     +
          Signed-off-by: Ivan Frade <[email protected]>
      
       ## http-fetch.c ##
     @@ http-fetch.c: static void fetch_single_packfile(struct object_id *packfile_hash,
      -			    curl_errorstr);
      +			struct url_info url;
      +			char *nurl = url_normalize(preq->url, &url);
     -+			if (!git_env_bool("GIT_TRACE_REDACT", 1) || !nurl) {
     -+				die("Unable to get pack file %s\n%s", preq->url,
     ++			if (!nurl || !git_env_bool("GIT_TRACE_REDACT", 1)) {
     ++				die("unable to get pack file '%s'\n%s", preq->url,
      +				    curl_errorstr);
      +			} else {
     -+				char *schema = xstrndup(url.url, url.scheme_len);
     -+				char *host = xstrndup(&url.url[url.host_off], url.host_len);
     -+				die("failed to get '%s' url from '%s' "
     ++				die("failed to get '%.*s' url from '%.*s' "
      +				    "(full URL redacted due to GIT_TRACE_REDACT setting)\n%s",
     -+				    schema, host, curl_errorstr);
     ++				    (int)url.scheme_len, url.url,
     ++				    (int)url.host_len, &url.url[url.host_off], curl_errorstr);
      +			}
       		}
       	} else {

gitgitgadget

From: Ivan Frade <[email protected]>

In some setups, packfile uris act as bearer token. It is not recommended to expose them plainly in logs, although in special circunstances (e.g. debug) it makes sense to write them.

Redact the packfile URL paths by default, unless the GIT_TRACE_REDACT variable is set to false. This mimics the redacting of the Authorization header in HTTP.

---
 Documentation/git.txt  |  5 +++--
 fetch-pack.c           |  4 ++++
 pkt-line.c             | 39 +++++++++++++++++++++++++++++++-
 pkt-line.h             |  1 +
 t/t5702-protocol-v2.sh | 51 ++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 97 insertions(+), 3 deletions(-)

index d63c65e67d8..c91aa2737f0 100644
@@ -832,8 +832,9 @@ for full details.
 
 `GIT_TRACE_REDACT`::
 	By default, when tracing is activated, Git redacts the values of
-	cookies, the "Authorization:" header, and the "Proxy-Authorization:"
-	header. Set this variable to `0` to prevent this redaction.
+	cookies, the "Authorization:" header, the "Proxy-Authorization:"
+	header and packfile URIs. Set this variable to `0` to prevent this
+	redaction.
 
 `GIT_LITERAL_PATHSPECS`::
 	Setting this variable to `1` will cause Git to treat all

index a9604f35a3e..62ea90541c5 100644
@@ -1653,8 +1653,12 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 				receive_wanted_refs(&reader, sought, nr_sought);
 
 			/* get the pack(s) */
+			if (git_env_bool("GIT_TRACE_REDACT", 1))
+				reader.options |= PACKET_READ_REDACT_URI_PATH;
 			if (process_section_header(&reader, "packfile-uris", 1))
 				receive_packfile_uris(&reader, &packfile_uris);
+			reader.options &= ~PACKET_READ_REDACT_URI_PATH;
+
 			process_section_header(&reader, "packfile", 0);
 
 			/*

index 2dc8ac274bd..06013d2a54a 100644
@@ -370,6 +370,31 @@ int packet_length(const char lenbuf_hex[4])
 	return (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);
 }
 
+static char *find_packfile_uri_path(const char *buffer)
+{
+	const char *URI_MARK = "://";
+	char *path;
+	int len;
+
+	/* First char is sideband mark */
+	buffer += 1;
+
+	len = strspn(buffer, "0123456789abcdefABCDEF");
+	if (!(len == 40 || len == 64) || buffer[len] != ' ')
+		return NULL; /* required "<hash>SP" not seen */
+
+	path = strstr(buffer + len + 1, URI_MARK);
+	if (!path)
+		return NULL;
+
+	path = strchr(path + strlen(URI_MARK), '/');
+	if (!path || !*(path + 1))
+		return NULL;
+
+	/* position after '/' */
+	return ++path;
+}
+
 enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
 						size_t *src_len, char *buffer,
 						unsigned size, int *pktlen,

@@ -377,6 +402,7 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
 {
 	int len;
 	char linelen[4];
+	char *uri_path_start;
 
 	if (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0) {
 		*pktlen = -1;

@@ -427,7 +453,18 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
 		len--;
 
 	buffer[len] = 0;
-	packet_trace(buffer, len, 0);
+	if (options & PACKET_READ_REDACT_URI_PATH &&
+	    (uri_path_start = find_packfile_uri_path(buffer))) {
+		const char *redacted = "<redacted>";
+		struct strbuf tracebuf = STRBUF_INIT;
+		strbuf_insert(&tracebuf, 0, buffer, len);
+		strbuf_splice(&tracebuf, uri_path_start - buffer,
+			      strlen(uri_path_start), redacted, strlen(redacted));
+		packet_trace(tracebuf.buf, tracebuf.len, 0);
+		strbuf_release(&tracebuf);
+	} else {
+		packet_trace(buffer, len, 0);
+	}
 
 	if ((options & PACKET_READ_DIE_ON_ERR_PACKET) &&
 	    starts_with(buffer, "ERR "))

index 467ae013573..6d2a63db238 100644
@@ -87,6 +87,7 @@ void packet_fflush(FILE *f);
 #define PACKET_READ_CHOMP_NEWLINE        (1u<<1)
 #define PACKET_READ_DIE_ON_ERR_PACKET    (1u<<2)
 #define PACKET_READ_GENTLE_ON_READ_ERROR (1u<<3)
+#define PACKET_READ_REDACT_URI_PATH      (1u<<4)
 int packet_read(int fd, char *buffer, unsigned size, int options);
 
 /*

gitgitgadget

From: Ivan Frade <[email protected]>

http-fetch prints the URL after failing to fetch it. This can be confusing to users (they cannot really do anything with it), and they can share by accident a sensitive URL (e.g. with credentials) while looking for help.

Redact the URL unless the GIT_TRACE_REDACT variable is set to false. This mimics the redaction of other sensitive information in git, like the Authorization header in HTTP.

Fix also capitalization of previous die() message (must start in lowercase).

---
 http-fetch.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

index fa642462a9e..c7c7d391ac5 100644
@@ -4,6 +4,7 @@
 #include "http.h"
 #include "walker.h"
 #include "strvec.h"
+#include "urlmatch.h"
 
 static const char http_fetch_usage[] = "git http-fetch "
 "[-c] [-t] [-a] [-v] [--recover] [-w ref] [--stdin | --packfile=hash | commit-id] url";

@@ -63,8 +64,17 @@ static void fetch_single_packfile(struct object_id *packfile_hash,
 	if (start_active_slot(preq->slot)) {
 		run_active_slot(preq->slot);
 		if (results.curl_result != CURLE_OK) {
-			die("Unable to get pack file %s\n%s", preq->url,
-			    curl_errorstr);
+			struct url_info url;
+			char *nurl = url_normalize(preq->url, &url);
+			if (!nurl || !git_env_bool("GIT_TRACE_REDACT", 1)) {
+				die("unable to get pack file '%s'\n%s", preq->url,
+				    curl_errorstr);
+			} else {
+				die("failed to get '%.*s' url from '%.*s' "
+				    "(full URL redacted due to GIT_TRACE_REDACT setting)\n%s",
+				    (int)url.scheme_len, url.url,
+				    (int)url.host_len, &url.url[url.host_off], curl_errorstr);
+			}
 		}
 	} else {
 		die("Unable to start request");

gitgitgadget

Latest version uses the_hash_algo->hexsz:

+ if (len != (int)the_hash_algo->hexsz || buffer[len] != ' ')
+ return NULL; /* required "<hash>SP" not seen */

Thanks!

Changes since v5:

* Use hexsz instead of hardcoded hash sizes

Changes since v4:

* Use "uri" instead of "url"
* Look specifically for a line with packfile-uri format (instead of for a URL in general)
* Limit the redacting to the packfile-uri section in do_fetch_pack_v2
* Use "%.*s" instead of duplicating parts of the string to print

Changes since v3:

* Enable redacting URLs for all sections
* Redact only URL path (it was until the end of line)
* Redact URL in die() with more friendly message
* Update doc to mention that packfile URIs are also redacted.

Changes since v2:

* Redact only the path of the URL
* Test are now strict, validating the exact line expected in the log

Changes since v1:

* Removed non-POSIX flags in tests
* More accurate regex for the non-encrypted packfile line
* Dropped documentation change
* Dropped redacting the die message in http-fetch

Ivan Frade (2):
fetch-pack: redact packfile urls in traces
http-fetch: redact url on die() message

 Documentation/git.txt  |  5 +++--
 fetch-pack.c           |  4 ++++
 http-fetch.c           | 14 ++++++++++--
 pkt-line.c             | 39 +++++++++++++++++++++++++++++++-
 pkt-line.h             |  1 +
 t/t5702-protocol-v2.sh | 51 ++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 109 insertions(+), 5 deletions(-)

base-commit: e9e5ba39a78c8f5057262d49e261b42a8660d5b9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1052%2Fifradeo%2Fredact-packfile-uri-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1052/ifradeo/redact-packfile-uri-v6
Pull-Request: https://github.com/gitgitgadget/git/pull/1052

Range-diff vs v5:

1: c95b3cafcd6 ! 1: a6098f98946 fetch-pack: redact packfile urls in traces

     @@ pkt-line.c: int packet_length(const char lenbuf_hex[4])
      +	buffer += 1;
      +
      +	len = strspn(buffer, "0123456789abcdefABCDEF");
     -+	if (!(len == 40 || len == 64) || buffer[len] != ' ')
     ++	if (len != (int)the_hash_algo->hexsz || buffer[len] != ' ')
      +		return NULL; /* required "<hash>SP" not seen */
      +
      +	path = strstr(buffer + len + 1, URI_MARK);

2: 6912a690197 = 2: 38859ae7b7d http-fetch: redact url on die() message

gitgitgadget

From: Ivan Frade <[email protected]>

In some setups, packfile uris act as bearer token. It is not recommended to expose them plainly in logs, although in special circunstances (e.g. debug) it makes sense to write them.

Redact the packfile URL paths by default, unless the GIT_TRACE_REDACT variable is set to false. This mimics the redacting of the Authorization header in HTTP.

---
 Documentation/git.txt  |  5 +++--
 fetch-pack.c           |  4 ++++
 pkt-line.c             | 39 +++++++++++++++++++++++++++++++-
 pkt-line.h             |  1 +
 t/t5702-protocol-v2.sh | 51 ++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 97 insertions(+), 3 deletions(-)

index d63c65e67d8..c91aa2737f0 100644
@@ -832,8 +832,9 @@ for full details.
 
 `GIT_TRACE_REDACT`::
 	By default, when tracing is activated, Git redacts the values of
-	cookies, the "Authorization:" header, and the "Proxy-Authorization:"
-	header. Set this variable to `0` to prevent this redaction.
+	cookies, the "Authorization:" header, the "Proxy-Authorization:"
+	header and packfile URIs. Set this variable to `0` to prevent this
+	redaction.
 
 `GIT_LITERAL_PATHSPECS`::
 	Setting this variable to `1` will cause Git to treat all

index a9604f35a3e..62ea90541c5 100644
@@ -1653,8 +1653,12 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 				receive_wanted_refs(&reader, sought, nr_sought);
 
 			/* get the pack(s) */
+			if (git_env_bool("GIT_TRACE_REDACT", 1))
+				reader.options |= PACKET_READ_REDACT_URI_PATH;
 			if (process_section_header(&reader, "packfile-uris", 1))
 				receive_packfile_uris(&reader, &packfile_uris);
+			reader.options &= ~PACKET_READ_REDACT_URI_PATH;
+
 			process_section_header(&reader, "packfile", 0);
 
 			/*

index 2dc8ac274bd..5a69ddc2e77 100644
@@ -370,6 +370,31 @@ int packet_length(const char lenbuf_hex[4])
 	return (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);
 }
 
+static char *find_packfile_uri_path(const char *buffer)
+{
+	const char *URI_MARK = "://";
+	char *path;
+	int len;
+
+	/* First char is sideband mark */
+	buffer += 1;
+
+	len = strspn(buffer, "0123456789abcdefABCDEF");
+	if (len != (int)the_hash_algo->hexsz || buffer[len] != ' ')
+		return NULL; /* required "<hash>SP" not seen */
+
+	path = strstr(buffer + len + 1, URI_MARK);
+	if (!path)
+		return NULL;
+
+	path = strchr(path + strlen(URI_MARK), '/');
+	if (!path || !*(path + 1))
+		return NULL;
+
+	/* position after '/' */
+	return ++path;
+}
+
 enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
 						size_t *src_len, char *buffer,
 						unsigned size, int *pktlen,

@@ -377,6 +402,7 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
 {
 	int len;
 	char linelen[4];
+	char *uri_path_start;
 
 	if (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0) {
 		*pktlen = -1;

@@ -427,7 +453,18 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
 		len--;
 
 	buffer[len] = 0;
-	packet_trace(buffer, len, 0);
+	if (options & PACKET_READ_REDACT_URI_PATH &&
+	    (uri_path_start = find_packfile_uri_path(buffer))) {
+		const char *redacted = "<redacted>";
+		struct strbuf tracebuf = STRBUF_INIT;
+		strbuf_insert(&tracebuf, 0, buffer, len);
+		strbuf_splice(&tracebuf, uri_path_start - buffer,
+			      strlen(uri_path_start), redacted, strlen(redacted));
+		packet_trace(tracebuf.buf, tracebuf.len, 0);
+		strbuf_release(&tracebuf);
+	} else {
+		packet_trace(buffer, len, 0);
+	}
 
 	if ((options & PACKET_READ_DIE_ON_ERR_PACKET) &&
 	    starts_with(buffer, "ERR "))

index 467ae013573..6d2a63db238 100644
@@ -87,6 +87,7 @@ void packet_fflush(FILE *f);
 #define PACKET_READ_CHOMP_NEWLINE        (1u<<1)
 #define PACKET_READ_DIE_ON_ERR_PACKET    (1u<<2)
 #define PACKET_READ_GENTLE_ON_READ_ERROR (1u<<3)
+#define PACKET_READ_REDACT_URI_PATH      (1u<<4)
 int packet_read(int fd, char *buffer, unsigned size, int options);
 
 /*

gitgitgadget

From: Ivan Frade <[email protected]>

http-fetch prints the URL after failing to fetch it. This can be confusing to users (they cannot really do anything with it), and they can share by accident a sensitive URL (e.g. with credentials) while looking for help.

Redact the URL unless the GIT_TRACE_REDACT variable is set to false. This mimics the redaction of other sensitive information in git, like the Authorization header in HTTP.

Fix also capitalization of previous die() message (must start in lowercase).

---
 http-fetch.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

index fa642462a9e..c7c7d391ac5 100644
@@ -4,6 +4,7 @@
 #include "http.h"
 #include "walker.h"
 #include "strvec.h"
+#include "urlmatch.h"
 
 static const char http_fetch_usage[] = "git http-fetch "
 "[-c] [-t] [-a] [-v] [--recover] [-w ref] [--stdin | --packfile=hash | commit-id] url";

@@ -63,8 +64,17 @@ static void fetch_single_packfile(struct object_id *packfile_hash,
 	if (start_active_slot(preq->slot)) {
 		run_active_slot(preq->slot);
 		if (results.curl_result != CURLE_OK) {
-			die("Unable to get pack file %s\n%s", preq->url,
-			    curl_errorstr);
+			struct url_info url;
+			char *nurl = url_normalize(preq->url, &url);
+			if (!nurl || !git_env_bool("GIT_TRACE_REDACT", 1)) {
+				die("unable to get pack file '%s'\n%s", preq->url,
+				    curl_errorstr);
+			} else {
+				die("failed to get '%.*s' url from '%.*s' "
+				    "(full URL redacted due to GIT_TRACE_REDACT setting)\n%s",
+				    (int)url.scheme_len, url.url,
+				    (int)url.host_len, &url.url[url.host_off], curl_errorstr);
+			}
 		}
 	} else {
 		die("Unable to start request");

gitgitgadget

OK. If the <hash> is given by this side (as opposed to "you started to talk to a remote, and it turns out that you are still talking SHA-1 but the other side talks SHA-256 and their <hash> size that is 64 does not match your 40" case), then checking against the_hash_algo->hexsz should be sufficient. The original suggestion was tried both because I didn't know where <hash> originates, and we would want to redact even in such a hash type mismatch case.

Thanks. Will take a look at the updated one.

They typically do not see what happens inside git_env_bool() while compling this compilation unit, and cannot tell if the programmer wanted to call it first for its side effects, hence they cannot swap them safely.

This is when reading from the remote, so <hash> comes from the other side. I don't think that the remote sending the wrong hash size (and then needing to redact) is a big concern, but there is definitely no harm in checking for both (and commenting that these are the SHA-1 and SHA-256 hash sizes).

...yeah...

Correct me if I'm wrong, but I find it really strange that we're trying to parse things in pkt-line.c like this.

We'll only get these from a client in code that's invoked in fetch-pack.c, specifically the process_section_header() quoted above, no?

From there we'll call packet_reader_read(), which will call packet_read_with_status(), and from there we'll call packet_trace().

Then right after all this happens we've got a loop that parses out these packfile URIs, including this being-done-first-here parsing of the hex value just for logging, except in pkt-line.c we've lost the information about what hash algorithm length we should be using, which fetch-pack.c of course needs to know.

Why can't that process_section_header() in fetch-pack.c just be made to call some pkt-line.c API saying "don't log yet", i.e. something like this pseudocode:

index a9604f35a3e..31f5ee7fc6b 100644
@@ -1518,14 +1518,18 @@ static void receive_wanted_refs(struct packet_reader *reader,
 static void receive_packfile_uris(struct packet_reader *reader,
                                  struct string_list *uris)
 {
+       struct string_list log = STRING_LIST_INIT_DUP;
+
        process_section_header(reader, "packfile-uris", 0);
-       while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
+       while (packet_reader_read_log_to(reader, &log) == PACKET_READ_NORMAL) {
                if (reader->pktlen < the_hash_algo->hexsz ||
                    reader->line[the_hash_algo->hexsz] != ' ')
                        die("expected '<hash> <uri>', got: %s\n", reader->line);
 
+               /* move the parsing of the URLs here */
                string_list_append(uris, reader->line);
        }
+       log_stuff(&log);
        if (reader->status != PACKET_READ_DELIM)
                die("expected DELIM");
 }

I.e. we'll eventually call trace_strbuf() in pkt-line.c, and we know that we're doing these packfile-uris, and we know that we're just about to parse them. Let's just:

1. Start reading the section
2. Turn off tracing
3. Parse the URIs as we go
3. When done (or on the fly), scrub URIs, log any backlog suppressed trace, and turn on tracing again

Instead of:

1. Set a flag to scrub stuff
2. Because of the disconnect between fetch-pack.c and pkt-line.c, effectively implement a new parser for data we're already going to be parsing some microseconds later during the course of the request.

That "turn off the trace" could be passing down a string_list/strbuf, or even doing the same via a nev member in "struct packet_reader", both would be simpler than needing to re-do the parse. Probably simplest is just:

    struct string_list log = STRING_LIST_INIT_DUP;

    reader.deferred_trace = &log;
    /* packet_reader_read() etc. code, unchanged from now */
    /* parse URIs (just move the existing code around a bit) */
    packet_reader.deferred_trace = NULL;
    for_each...(item, &log)
        trace_strbuf(...);

*nod*, but since that function is just:

    int git_env_bool(const char *k, int def)
    {
            const char *v = getenv(k);
            return v ? git_config_bool(k, v) : def;
    }

I was hedging and pondering if some compilers were smart enough these days to optimize things like that.

I.e. in this case getenv() is a simple C library function, the env variable is constant, and we do a boolean test of it before calling git_config_bool().

So a sufficiently smart compiler could turn that into:

     /* global, probably something iterated over env already */
    static int __have_seen_GIT_TRACE_REDACT = 0;
    ...

    if ((!__have_seen_GIT_TRACE_REDACT || !nurl) ||
        (__have_seen_GIT_TRACE_REDACT && git_env_bool_without_v_bool_check(...)))

But probably not, since it wolud need quite a bit of C library cooperation/hooks...

Done

...

Done (with a comment indicating they are the hash sizes of SHA1 and SHA256)

Done in both tests.

Thanks,

...

This is a more generic redacting mechanism, but I understood that there is no need for it. Previous comments went in the direction of removing generality (e.g. not looking for a URI anywhere in the packet, but specifically for the packfile line format) and now this patch is very specific to redact packfile-uri lines in the protocol.

pkt-line is only looking for the "<n-hex-chars>SP" shape. True that it encodes some protocol knowledge, but it is hardly a new parser.

Saving the lines and delaying the tracing could also produce weird outputs, no? e.g. 3 lines received, the second doesn't validate, the program aborts and the trace doesn't show any of the lines that caused the problem. Or we would need to iterate in parallel through lines and saved-log-lines assuming they match 1:1. Nothing unsolvable, but I am not sure it is worthy the effort now.

Thanks,

Changes since v6:

* Use specific hash sizes instead of hexsz
* Remove unnecessary env vars in tests
* Added comment on bit toggle

Changes since v5:

* Use hexsz instead of hardcoded hash sizes

Changes since v4:

* Use "uri" instead of "url"
* Look specifically for a line with packfile-uri format (instead of for a URL in general)
* Limit the redacting to the packfile-uri section in do_fetch_pack_v2
* Use "%.*s" instead of duplicating parts of the string to print

Changes since v3:

* Enable redacting URLs for all sections
* Redact only URL path (it was until the end of line)
* Redact URL in die() with more friendly message
* Update doc to mention that packfile URIs are also redacted.

Changes since v2:

* Redact only the path of the URL
* Test are now strict, validating the exact line expected in the log

Changes since v1:

* Removed non-POSIX flags in tests
* More accurate regex for the non-encrypted packfile line
* Dropped documentation change
* Dropped redacting the die message in http-fetch

Ivan Frade (2):
fetch-pack: redact packfile urls in traces
http-fetch: redact url on die() message

 Documentation/git.txt  |  5 +++--
 fetch-pack.c           |  5 +++++
 http-fetch.c           | 14 ++++++++++--
 pkt-line.c             | 40 ++++++++++++++++++++++++++++++++-
 pkt-line.h             |  1 +
 t/t5702-protocol-v2.sh | 51 ++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 111 insertions(+), 5 deletions(-)

base-commit: 88d915a634b449147855041d44875322de2b286d
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1052%2Fifradeo%2Fredact-packfile-uri-v7
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1052/ifradeo/redact-packfile-uri-v7
Pull-Request: https://github.com/gitgitgadget/git/pull/1052

Range-diff vs v6:

1: a6098f98946 ! 1: bbfdc346ede fetch-pack: redact packfile urls in traces

     @@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
      +				reader.options |= PACKET_READ_REDACT_URI_PATH;
       			if (process_section_header(&reader, "packfile-uris", 1))
       				receive_packfile_uris(&reader, &packfile_uris);
     ++			/* We don't expect more URIs. Reset to avoid expensive URI check. */
      +			reader.options &= ~PACKET_READ_REDACT_URI_PATH;
      +
       			process_section_header(&reader, "packfile", 0);
     @@ pkt-line.c: int packet_length(const char lenbuf_hex[4])
      +	buffer += 1;
      +
      +	len = strspn(buffer, "0123456789abcdefABCDEF");
     -+	if (len != (int)the_hash_algo->hexsz || buffer[len] != ' ')
     ++	/* size of SHA1 and SHA256 hash */
     ++	if (!(len == 40 || len == 64) || buffer[len] != ' ')
      +		return NULL; /* required "<hash>SP" not seen */
      +
      +	path = strstr(buffer + len + 1, URI_MARK);
     @@ t/t5702-protocol-v2.sh: test_expect_success 'packfile-uri with transfer.fsckobje
      +		"uploadpack.blobpackfileuri" \
      +		"$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack" &&
      +
     -+	GIT_TRACE=1 GIT_TRACE_PACKET="$(pwd)/log" GIT_TEST_SIDEBAND_ALL=1 \
     ++	GIT_TRACE_PACKET="$(pwd)/log" \
      +	git -c protocol.version=2 \
      +		-c fetch.uriprotocols=http,https \
      +		clone "$HTTPD_URL/smart/http_parent" http_child &&
     @@ t/t5702-protocol-v2.sh: test_expect_success 'packfile-uri with transfer.fsckobje
      +		"uploadpack.blobpackfileuri" \
      +		"$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack" &&
      +
     -+	GIT_TRACE=1 GIT_TRACE_PACKET="$(pwd)/log" GIT_TEST_SIDEBAND_ALL=1 \
     ++	GIT_TRACE_PACKET="$(pwd)/log" \
      +	GIT_TRACE_REDACT=0 \
      +	git -c protocol.version=2 \
      +		-c fetch.uriprotocols=http,https \

2: 38859ae7b7d = 2: 3b210735bc8 http-fetch: redact url on die() message

gitgitgadget

From: Ivan Frade <[email protected]>

In some setups, packfile uris act as bearer token. It is not recommended to expose them plainly in logs, although in special circunstances (e.g. debug) it makes sense to write them.

Redact the packfile URL paths by default, unless the GIT_TRACE_REDACT variable is set to false. This mimics the redacting of the Authorization header in HTTP.

---
 Documentation/git.txt  |  5 +++--
 fetch-pack.c           |  5 +++++
 pkt-line.c             | 40 ++++++++++++++++++++++++++++++++-
 pkt-line.h             |  1 +
 t/t5702-protocol-v2.sh | 51 ++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 99 insertions(+), 3 deletions(-)

index 281c5f8caef..13f83a2a3a1 100644
@@ -832,8 +832,9 @@ for full details.
 
 `GIT_TRACE_REDACT`::
 	By default, when tracing is activated, Git redacts the values of
-	cookies, the "Authorization:" header, and the "Proxy-Authorization:"
-	header. Set this variable to `0` to prevent this redaction.
+	cookies, the "Authorization:" header, the "Proxy-Authorization:"
+	header and packfile URIs. Set this variable to `0` to prevent this
+	redaction.
 
 `GIT_LITERAL_PATHSPECS`::
 	Setting this variable to `1` will cause Git to treat all

index a9604f35a3e..8b8c75f33aa 100644
@@ -1653,8 +1653,13 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 				receive_wanted_refs(&reader, sought, nr_sought);
 
 			/* get the pack(s) */
+			if (git_env_bool("GIT_TRACE_REDACT", 1))
+				reader.options |= PACKET_READ_REDACT_URI_PATH;
 			if (process_section_header(&reader, "packfile-uris", 1))
 				receive_packfile_uris(&reader, &packfile_uris);
+			/* We don't expect more URIs. Reset to avoid expensive URI check. */
+			reader.options &= ~PACKET_READ_REDACT_URI_PATH;
+
 			process_section_header(&reader, "packfile", 0);
 
 			/*

index 2dc8ac274bd..8e43c2def4c 100644
@@ -370,6 +370,32 @@ int packet_length(const char lenbuf_hex[4])
 	return (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);
 }
 
+static char *find_packfile_uri_path(const char *buffer)
+{
+	const char *URI_MARK = "://";
+	char *path;
+	int len;
+
+	/* First char is sideband mark */
+	buffer += 1;
+
+	len = strspn(buffer, "0123456789abcdefABCDEF");
+	/* size of SHA1 and SHA256 hash */
+	if (!(len == 40 || len == 64) || buffer[len] != ' ')
+		return NULL; /* required "<hash>SP" not seen */
+
+	path = strstr(buffer + len + 1, URI_MARK);
+	if (!path)
+		return NULL;
+
+	path = strchr(path + strlen(URI_MARK), '/');
+	if (!path || !*(path + 1))
+		return NULL;
+
+	/* position after '/' */
+	return ++path;
+}
+
 enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
 						size_t *src_len, char *buffer,
 						unsigned size, int *pktlen,

@@ -377,6 +403,7 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
 {
 	int len;
 	char linelen[4];
+	char *uri_path_start;
 
 	if (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0) {
 		*pktlen = -1;

@@ -427,7 +454,18 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
 		len--;
 
 	buffer[len] = 0;
-	packet_trace(buffer, len, 0);
+	if (options & PACKET_READ_REDACT_URI_PATH &&
+	    (uri_path_start = find_packfile_uri_path(buffer))) {
+		const char *redacted = "<redacted>";
+		struct strbuf tracebuf = STRBUF_INIT;
+		strbuf_insert(&tracebuf, 0, buffer, len);
+		strbuf_splice(&tracebuf, uri_path_start - buffer,
+			      strlen(uri_path_start), redacted, strlen(redacted));
+		packet_trace(tracebuf.buf, tracebuf.len, 0);
+		strbuf_release(&tracebuf);
+	} else {
+		packet_trace(buffer, len, 0);
+	}
 
 	if ((options & PACKET_READ_DIE_ON_ERR_PACKET) &&
 	    starts_with(buffer, "ERR "))

index 467ae013573..6d2a63db238 100644
@@ -87,6 +87,7 @@ void packet_fflush(FILE *f);
 #define PACKET_READ_CHOMP_NEWLINE        (1u<<1)
 #define PACKET_READ_DIE_ON_ERR_PACKET    (1u<<2)
 #define PACKET_READ_GENTLE_ON_READ_ERROR (1u<<3)
+#define PACKET_READ_REDACT_URI_PATH      (1u<<4)
 int packet_read(int fd, char *buffer, unsigned size, int options);
 
 /*

gitgitgadget

From: Ivan Frade <[email protected]>

http-fetch prints the URL after failing to fetch it. This can be confusing to users (they cannot really do anything with it), and they can share by accident a sensitive URL (e.g. with credentials) while looking for help.

Redact the URL unless the GIT_TRACE_REDACT variable is set to false. This mimics the redaction of other sensitive information in git, like the Authorization header in HTTP.

Fix also capitalization of previous die() message (must start in lowercase).

---
 http-fetch.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

index fa642462a9e..c7c7d391ac5 100644
@@ -4,6 +4,7 @@
 #include "http.h"
 #include "walker.h"
 #include "strvec.h"
+#include "urlmatch.h"
 
 static const char http_fetch_usage[] = "git http-fetch "
 "[-c] [-t] [-a] [-v] [--recover] [-w ref] [--stdin | --packfile=hash | commit-id] url";

@@ -63,8 +64,17 @@ static void fetch_single_packfile(struct object_id *packfile_hash,
 	if (start_active_slot(preq->slot)) {
 		run_active_slot(preq->slot);
 		if (results.curl_result != CURLE_OK) {
-			die("Unable to get pack file %s\n%s", preq->url,
-			    curl_errorstr);
+			struct url_info url;
+			char *nurl = url_normalize(preq->url, &url);
+			if (!nurl || !git_env_bool("GIT_TRACE_REDACT", 1)) {
+				die("unable to get pack file '%s'\n%s", preq->url,
+				    curl_errorstr);
+			} else {
+				die("failed to get '%.*s' url from '%.*s' "
+				    "(full URL redacted due to GIT_TRACE_REDACT setting)\n%s",
+				    (int)url.scheme_len, url.url,
+				    (int)url.host_len, &url.url[url.host_off], curl_errorstr);
+			}
 		}
 	} else {
 		die("Unable to start request");

gitgitgadget

It's less generic, because it would live in the loop that consumes the lines.

Yeah, but why have find_packfile_uri_path() at all instead of just moving the parsing code around?

We've already got the code that parses these lines, it's just a few lines removed from the code you're adding...

It would only be weird if you do :

    download_later =
    while (consume lines)
        download_later += buffer_lines;
    log lines;

I'm suggesting:

download_later =
while (consume lines)
raw, to_log = parse line
log line(to_log)
download_later += raw

Sure, you'll need to do something in the case where the line doesn't validate, should you redact it still, or log it as is? Anyway, that's also a caveat you've got now.

That's not iterating in parallel, having one for-loop instead of two.

I see now that that approach would also solve at least one bug/misfeature in the packfile-uri handling, i.e.:

        for (i = 0; i < packfile_uris.nr; i++) {
            [...]
            start_command(...) [... to download the URI ...]
            [...]
            die("fetch-pack: pack downloaded from %s does not match expected hash %.*s",
        }

I.e. we've already received all the URIs, but then do validation on them one at a time, so we might only notice that the server has sent us bad data for the Nth URI after first downloading the first N-1 URIs.

Thanks, will queue.