{"thread":{"id":"56667","subject":"[PATCH 0/2] fetch-pack: redact packfile urls in traces","startedAt":"2021-10-08T16:03:12Z","lastAt":"2021-11-12T04:43:57Z","messageCount":43,"participants":["Ivan Frade via GitGitGadget","Ævar Arnfjörð Bjarmason","Ivan Frade","Junio C Hamano","Eric Sunshine","Jonathan Tan"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"438294","messageId":"pull.1052.git.1633708986.gitgitgadget@gmail.com","threadId":"56667","inReplyTo":null,"subject":"[PATCH 0/2] fetch-pack: redact packfile urls in traces","fromName":"Ivan Frade via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-08T16:03:04Z","receivedAt":"2021-10-08T16:03:12Z","isPatch":true,"sender":{"key":"ifrade@google.com","avatar":"https://avatars.githubusercontent.com/u/58185630?v=4"},"body":"In some setups, packfile uris act as bearer token. It is not recommended to\nexpose them plainly in logs, although in special circunstances (e.g. debug)\nit makes sense to write them.\n\nRedact the packfile-uri lines by default, unless the GIT_TRACE_REDACT\nvariable is set to false. This mimics the redacting of the Authorization\nheader in HTTP.\n\nSigned-off-by: Ivan Frade ifrade@google.com\n\nIvan Frade (2):\n fetch-pack: redact packfile urls in traces\n Documentation: packfile-uri hash can be longer than 40 hex chars\n\n Documentation/technical/protocol-v2.txt | 8 ++---\n fetch-pack.c | 11 +++++++\n http-fetch.c | 4 ++-\n pkt-line.c | 7 +++-\n pkt-line.h | 1 +\n t/t5702-protocol-v2.sh | 43 +++++++++++++++++++++++++\n 6 files changed, 68 insertions(+), 6 deletions(-)\n\n\nbase-commit: 0785eb769886ae81e346df10e88bc49ffc0ac64e\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1052%2Fifradeo%2Fredact-packfile-uri-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1052/ifradeo/redact-packfile-uri-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1052\n-- \ngitgitgadget\n"},{"id":"438295","messageId":"b473f145a87a22db99734c6a21395f0d24c3da3c.1633708986.git.gitgitgadget@gmail.com","threadId":"56667","inReplyTo":"pull.1052.git.1633708986.gitgitgadget@gmail.com","subject":"[PATCH 1/2] fetch-pack: redact packfile urls in traces","fromName":"Ivan Frade via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-08T16:03:05Z","receivedAt":"2021-10-08T16:03:13Z","isPatch":true,"sender":{"key":"ifrade@google.com","avatar":"https://avatars.githubusercontent.com/u/58185630?v=4"},"body":"From: Ivan Frade \n\nIn some setups, packfile uris act as bearer token. It is not\nrecommended to expose them plainly in logs, although in special\ncircunstances (e.g. debug) it makes sense to write them.\n\nRedact the packfile-uri lines by default, unless the GIT_TRACE_REDACT\nvariable is set to false. This mimics the redacting of the\nAuthorization header in HTTP.\n\nSigned-off-by: Ivan Frade \n---\n fetch-pack.c | 11 +++++++++++\n http-fetch.c | 4 +++-\n pkt-line.c | 7 ++++++-\n pkt-line.h | 1 +\n t/t5702-protocol-v2.sh | 43 ++++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 64 insertions(+), 2 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex a9604f35a3e..05c85eeafa1 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1518,7 +1518,16 @@ static void receive_wanted_refs(struct packet_reader *reader,\n static void receive_packfile_uris(struct packet_reader *reader,\n \t\t\t\t struct string_list *uris)\n {\n+\tint original_options;\n \tprocess_section_header(reader, \"packfile-uris\", 0);\n+\t/*\n+\t * In some setups, packfile-uris act as bearer tokens,\n+\t * redact them by default.\n+\t */\n+\toriginal_options = reader->options;\n+\tif (git_env_bool(\"GIT_TRACE_REDACT\", 1))\n+\t\treader->options |= PACKET_READ_REDACT_ON_TRACE;\n+\n \twhile (packet_reader_read(reader) == PACKET_READ_NORMAL) {\n \t\tif (reader->pktlen < the_hash_algo->hexsz ||\n \t\t reader->line[the_hash_algo->hexsz] != ' ')\n@@ -1526,6 +1535,8 @@ static void receive_packfile_uris(struct packet_reader *reader,\n \n \t\tstring_list_append(uris, reader->line);\n \t}\n+\treader->options = original_options;\n+\n \tif (reader->status != PACKET_READ_DELIM)\n \t\tdie(\"expected DELIM\");\n }\ndiff --git a/http-fetch.c b/http-fetch.c\nindex fa642462a9e..d35e33e4f65 100644\n--- a/http-fetch.c\n+++ b/http-fetch.c\n@@ -63,7 +63,9 @@ static void fetch_single_packfile(struct object_id *packfile_hash,\n \tif (start_active_slot(preq->slot)) {\n \t\trun_active_slot(preq->slot);\n \t\tif (results.curl_result != CURLE_OK) {\n-\t\t\tdie(\"Unable to get pack file %s\\n%s\", preq->url,\n+\t\t\tint showUrl = git_env_bool(\"GIT_TRACE_REDACT\", 1);\n+\t\t\tdie(\"Unable to get offloaded pack file %s\\n%s\",\n+\t\t\t showUrl ? preq->url : \"\",\n \t\t\t curl_errorstr);\n \t\t}\n \t} else {\ndiff --git a/pkt-line.c b/pkt-line.c\nindex de4a94b437e..8da8ed88ccf 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -443,7 +443,12 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t\tlen--;\n \n \tbuffer[len] = 0;\n-\tpacket_trace(buffer, len, 0);\n+\tif (options & PACKET_READ_REDACT_ON_TRACE) {\n+\t\tconst char *redacted = \"\";\n+\t\tpacket_trace(redacted, strlen(redacted), 0);\n+\t} else {\n+\t\tpacket_trace(buffer, len, 0);\n+\t}\n \n \tif ((options & PACKET_READ_DIE_ON_ERR_PACKET) &&\n \t starts_with(buffer, \"ERR \"))\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 82b95e4bdd3..44c02f3bc6e 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -88,6 +88,7 @@ void packet_fflush(FILE *f);\n #define PACKET_READ_CHOMP_NEWLINE (1u<<1)\n #define PACKET_READ_DIE_ON_ERR_PACKET (1u<<2)\n #define PACKET_READ_GENTLE_ON_READ_ERROR (1u<<3)\n+#define PACKET_READ_REDACT_ON_TRACE (1u<<4)\n int packet_read(int fd, char **src_buffer, size_t *src_len, char\n \t\t*buffer, unsigned size, int options);\n \ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex d527cf6c49f..a620a678a56 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -1107,6 +1107,49 @@ test_expect_success 'packfile-uri with transfer.fsckobjects fails when .gitmodul\n \ttest_i18ngrep \"disallowed submodule name\" err\n '\n \n+test_expect_success 'packfile-uri redacted in trace' '\n+\tP=\"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n+\trm -rf \"$P\" http_child log &&\n+\n+\tgit init \"$P\" &&\n+\tgit -C \"$P\" config \"uploadpack.allowsidebandall\" \"true\" &&\n+\n+\techo my-blob >\"$P/my-blob\" &&\n+\tgit -C \"$P\" add my-blob &&\n+\tgit -C \"$P\" commit -m x &&\n+\n+\tconfigure_exclusion \"$P\" my-blob >h &&\n+\n+\tGIT_TRACE=1 GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TEST_SIDEBAND_ALL=1 \\\n+\tgit -c protocol.version=2 \\\n+\t\t-c fetch.uriprotocols=http,https \\\n+\t\tclone \"$HTTPD_URL/smart/http_parent\" http_child &&\n+\n+\tgrep -A1 \"clone<\\ ..packfile-uris\" log | grep \"clone<\\ \"\n+'\n+\n+test_expect_success 'packfile-uri not redacted in trace when GIT_TRACE_REDACT=0' '\n+\tP=\"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n+\trm -rf \"$P\" http_child log &&\n+\n+\tgit init \"$P\" &&\n+\tgit -C \"$P\" config \"uploadpack.allowsidebandall\" \"true\" &&\n+\n+\techo my-blob >\"$P/my-blob\" &&\n+\tgit -C \"$P\" add my-blob &&\n+\tgit -C \"$P\" commit -m x &&\n+\n+\tconfigure_exclusion \"$P\" my-blob >h &&\n+\n+\tGIT_TRACE=1 GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TEST_SIDEBAND_ALL=1 \\\n+\tGIT_TRACE_REDACT=0 \\\n+\tgit -c protocol.version=2 \\\n+\t\t-c fetch.uriprotocols=http,https \\\n+\t\tclone \"$HTTPD_URL/smart/http_parent\" http_child &&\n+\n+\tgrep -A1 \"clone<\\ ..packfile-uris\" log | grep -E \"clone<\\ ..[[:alnum:]]{40,64}\\ http\"\n+'\n+\n test_expect_success 'http:// --negotiate-only' '\n \tSERVER=\"$HTTPD_DOCUMENT_ROOT_PATH/server\" &&\n \tURI=\"$HTTPD_URL/smart/server\" &&\n-- \ngitgitgadget\n\n"},{"id":"438296","messageId":"497c5fd18d7206c137d8a62d229d2f295c9fe4fa.1633708986.git.gitgitgadget@gmail.com","threadId":"56667","inReplyTo":"pull.1052.git.1633708986.gitgitgadget@gmail.com","subject":"[PATCH 2/2] Documentation: packfile-uri hash can be longer than 40 hex chars","fromName":"Ivan Frade via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-08T16:03:06Z","receivedAt":"2021-10-08T16:03:14Z","isPatch":true,"sender":{"key":"ifrade@google.com","avatar":"https://avatars.githubusercontent.com/u/58185630?v=4"},"body":"From: Ivan Frade \n\nPackfile-uri line specifies a hash of 40 hex character, but with SHA256\nthis hash size is 64. There are already tests using SHA256 (e.g. in\nubuntu-latest/linux-clang).\n\nUpdate protocol-v2 documentation to indicate that the hash size depends\non the hash algorithm in use.\n\nSigned-off-by: Ivan Frade \n---\n Documentation/technical/protocol-v2.txt | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\nindex 21e8258ccf3..a23f12d6c2b 100644\n--- a/Documentation/technical/protocol-v2.txt\n+++ b/Documentation/technical/protocol-v2.txt\n@@ -393,7 +393,7 @@ header. Most sections are sent only when the packfile is sent.\n wanted-ref = obj-id SP refname\n \n packfile-uris = PKT-LINE(\"packfile-uris\" LF) *packfile-uri\n- packfile-uri = PKT-LINE(40*(HEXDIGIT) SP *%x20-ff LF)\n+ packfile-uri = PKT-LINE((40|64)*(HEXDIGIT) SP *%x20-ff LF)\n \n packfile = PKT-LINE(\"packfile\" LF)\n \t *PKT-LINE(%x01-03 *%x00-ff)\n@@ -476,9 +476,9 @@ header. Most sections are sent only when the packfile is sent.\n \t* For each URI the server sends, it sends a hash of the pack's\n \t contents (as output by git index-pack) followed by the URI.\n \n-\t* The hashes are 40 hex characters long. When Git upgrades to a new\n-\t hash algorithm, this might need to be updated. (It should match\n-\t whatever index-pack outputs after \"pack\\t\" or \"keep\\t\".\n+\t* The hashes length is defined by the hash algorithm (40 hex\n+\t characters in SHA-1, 64 in SHA-256). It should match whatever\n+\t index-pack outputs after \"pack\\t\" or \"keep\\t\".\n \n packfile section\n \t* This section is only included if the client has sent 'want'\n-- \ngitgitgadget\n"},{"id":"438331","messageId":"87zgrjmhgd.fsf@evledraar.gmail.com","threadId":"56667","inReplyTo":"b473f145a87a22db99734c6a21395f0d24c3da3c.1633708986.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] fetch-pack: redact packfile urls in traces","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-08T19:36:37Z","receivedAt":"2021-10-08T19:42:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Oct 08 2021, Ivan Frade via GitGitGadget wrote:\n\n> diff --git a/http-fetch.c b/http-fetch.c\n> index fa642462a9e..d35e33e4f65 100644\n> --- a/http-fetch.c\n> +++ b/http-fetch.c\n> @@ -63,7 +63,9 @@ static void fetch_single_packfile(struct object_id *packfile_hash,\n> \tif (start_active_slot(preq->slot)) {\n> \t\trun_active_slot(preq->slot);\n> \t\tif (results.curl_result != CURLE_OK) {\n> -\t\t\tdie(\"Unable to get pack file %s\\n%s\", preq->url,\n> +\t\t\tint showUrl = git_env_bool(\"GIT_TRACE_REDACT\", 1);\n> +\t\t\tdie(\"Unable to get offloaded pack file %s\\n%s\",\n> +\t\t\t showUrl ? preq->url : \"\",\n> \t\t\t curl_errorstr);\n> \t\t}\n> \t} else {\n\nYour CL and commit message just talk about traes, but this is a die()\nmessage.\n\nPerhaps it makes sense to redact it there too for some reason, but that\nseems to be a thing to separately argue for.\n\nThis message is shown interactively to users, and I could see it be\nannoying to not have the URL that failed in your terminal output, even\nif it has some one-time token.\n\nWhich is presumably different from the use-cases you're thinking of, I'm\nassuming some logging of detached processes, or central logging of user\nactions.\n\n> +test_expect_success 'packfile-uri redacted in trace' '\n> +\tP=\"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n> +\trm -rf \"$P\" http_child log &&\n> +\n> +\tgit init \"$P\" &&\n> +\tgit -C \"$P\" config \"uploadpack.allowsidebandall\" \"true\" &&\n> +\n> +\techo my-blob >\"$P/my-blob\" &&\n> +\tgit -C \"$P\" add my-blob &&\n> +\tgit -C \"$P\" commit -m x &&\n> +\n> +\tconfigure_exclusion \"$P\" my-blob >h &&\n> +\n> +\tGIT_TRACE=1 GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TEST_SIDEBAND_ALL=1 \\\n> +\tgit -c protocol.version=2 \\\n> +\t\t-c fetch.uriprotocols=http,https \\\n> +\t\tclone \"$HTTPD_URL/smart/http_parent\" http_child &&\n> +\n> +\tgrep -A1 \"clone<\\ ..packfile-uris\" log | grep \"clone<\\ \"\n\nWe don't rely on GNU options like those for the test suite, it'll break\non various supported platformrs.\n\nIn this case the whole LHS of the pipe looks like it could be dropped,\nwhy not grep for \"^clone< \"?\n\nAlso you don't need to quote the space character in regexes, it's not a\nmetacharacter.\n"},{"id":"438332","messageId":"87v927mgw9.fsf@evledraar.gmail.com","threadId":"56667","inReplyTo":"497c5fd18d7206c137d8a62d229d2f295c9fe4fa.1633708986.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] Documentation: packfile-uri hash can be longer than 40 hex chars","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-08T19:43:24Z","receivedAt":"2021-10-08T19:54:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Oct 08 2021, Ivan Frade via GitGitGadget wrote:\n\n> From: Ivan Frade \n>\n> Packfile-uri line specifies a hash of 40 hex character, but with SHA256\n> this hash size is 64. There are already tests using SHA256 (e.g. in\n> ubuntu-latest/linux-clang).\n>\n> Update protocol-v2 documentation to indicate that the hash size depends\n> on the hash algorithm in use.\n>\n> Signed-off-by: Ivan Frade \n> ---\n> Documentation/technical/protocol-v2.txt | 8 ++++----\n> 1 file changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\n> index 21e8258ccf3..a23f12d6c2b 100644\n> --- a/Documentation/technical/protocol-v2.txt\n> +++ b/Documentation/technical/protocol-v2.txt\n> @@ -393,7 +393,7 @@ header. Most sections are sent only when the packfile is sent.\n> wanted-ref = obj-id SP refname\n> \n> packfile-uris = PKT-LINE(\"packfile-uris\" LF) *packfile-uri\n> - packfile-uri = PKT-LINE(40*(HEXDIGIT) SP *%x20-ff LF)\n> + packfile-uri = PKT-LINE((40|64)*(HEXDIGIT) SP *%x20-ff LF)\n> \n> packfile = PKT-LINE(\"packfile\" LF)\n> \t *PKT-LINE(%x01-03 *%x00-ff)\n> @@ -476,9 +476,9 @@ header. Most sections are sent only when the packfile is sent.\n> \t* For each URI the server sends, it sends a hash of the pack's\n> \t contents (as output by git index-pack) followed by the URI.\n> \n> -\t* The hashes are 40 hex characters long. When Git upgrades to a new\n> -\t hash algorithm, this might need to be updated. (It should match\n> -\t whatever index-pack outputs after \"pack\\t\" or \"keep\\t\".\n> +\t* The hashes length is defined by the hash algorithm (40 hex\n> +\t characters in SHA-1, 64 in SHA-256). It should match whatever\n> +\t index-pack outputs after \"pack\\t\" or \"keep\\t\".\n> \n> packfile section\n> \t* This section is only included if the client has sent 'want'\n\n(I forgot to say in my first reply, but welcome to the Git Mailing\nList!)\n\nThis is well spotted, but it seems even better to simply drop this\nexhaustive listing of 40 or 64 hex digits here.\n\nIn protocol-common.txt we talk about \"obj-id\", and that's then used\nelsewhere in protocol-v2.txt matter-of-factly, e.g. (quoting from a\nhandy part that happens to use \"obj-id\"):\n\n [...]\n obj-id-or-unborn = (obj-id | \"unborn\")\n ref = PKT-LINE(obj-id-or-unborn SP refname *(SP ref-attribute) LF)\n [...]\n\nSo let's just have packfile-uri do the same.\n\nNow, the thing that *does* need to be updated then is\nprotocol-common.txt, or this part:\n\n zero-id = 40*\"0\"\n obj-id = 40*(HEXDIGIT)\n\nBecause now if you use obj-id that'll just refer back to that, but\nthat's also a problem with all the rest of the protocol docs.\n\nIt would seem that all our SHA-256 tests and client/servers are in\nviolation of the documentation, and should truncate their OIDs to 40\nchars, or we could fix the docs :)\n\nAnyway, whatever we do here this improvement is unrelated to whatever\nwe're doing with log redaction in your 1/2, I think it would be better\nto submit as its own 1 or 2 patch series.\n"},{"id":"438383","messageId":"CANQMx9Wd36xdMS5xyu759=aw1gVVnuGqsWzGsDVXqYLO_wuh1A@mail.gmail.com","threadId":"56667","inReplyTo":"87zgrjmhgd.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/2] fetch-pack: redact packfile urls in traces","fromName":"Ivan Frade","fromEmail":"ifrade@google.com","sentAt":"2021-10-08T23:15:10Z","receivedAt":"2021-10-08T23:15:24Z","isPatch":true,"sender":{"key":"ifrade@google.com","avatar":"https://avatars.githubusercontent.com/u/58185630?v=4"},"body":"On Fri, Oct 8, 2021 at 12:42 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n>\n> On Fri, Oct 08 2021, Ivan Frade via GitGitGadget wrote:\n>\n> > diff --git a/http-fetch.c b/http-fetch.c\n> > index fa642462a9e..d35e33e4f65 100644\n> > --- a/http-fetch.c\n> > +++ b/http-fetch.c\n> > @@ -63,7 +63,9 @@ static void fetch_single_packfile(struct object_id *packfile_hash,\n> > if (start_active_slot(preq->slot)) {\n> > run_active_slot(preq->slot);\n> > if (results.curl_result != CURLE_OK) {\n> > - die(\"Unable to get pack file %s\\n%s\", preq->url,\n> > + int showUrl = git_env_bool(\"GIT_TRACE_REDACT\", 1);\n> > + die(\"Unable to get offloaded pack file %s\\n%s\",\n> > + showUrl ? preq->url : \"\",\n> > curl_errorstr);\n> > }\n> > } else {\n>\n> Your CL and commit message just talk about traes, but this is a die()\n> message.\n>\n> Perhaps it makes sense to redact it there too for some reason, but that\n> seems to be a thing to separately argue for.\n>\n> This message is shown interactively to users, and I could see it be\n> annoying to not have the URL that failed in your terminal output, even\n> if it has some one-time token.\n\n\nFor a regular user the URL could be confusing (should they click on\nit? try to download it by themselves?). I also got a suggestion to\nprint e.g. only the domain and maybe the packname.\n\nIn any case, I agree it is a different thing than trace logging. I\nremoved it from this patch.\n\n>\n> > +\n> > + grep -A1 \"clone<\\ ..packfile-uris\" log | grep \"clone<\\ \"\n>\n> We don't rely on GNU options like those for the test suite, it'll break\n> on various supported platformrs.\n>\n> In this case the whole LHS of the pipe looks like it could be dropped,\n> why not grep for \"^clone< \"?\n>\n>\n> Also you don't need to quote the space character in regexes, it's not a\n> metacharacter.\n\nUpdated the grep expressions to look only for the relevant lines and\nremoved the escaping of the space char.\n\nI was trying to limit the grep to the \"packfile-uri\" section, not to\nmatch something else by accident, but I think \"obj-id http://\"\nshouldn't match anything else in the clone response (no ref can start\nwith http://).\n\nThanks for the quick review!\n\nIvan\n"},{"id":"438389","messageId":"pull.1052.v2.git.1633746024175.gitgitgadget@gmail.com","threadId":"56667","inReplyTo":"pull.1052.git.1633708986.gitgitgadget@gmail.com","subject":"[PATCH v2] fetch-pack: redact packfile urls in traces","fromName":"Ivan Frade via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-09T02:20:24Z","receivedAt":"2021-10-09T02:20:30Z","isPatch":true,"sender":{"key":"ifrade@google.com","avatar":"https://avatars.githubusercontent.com/u/58185630?v=4"},"body":"From: Ivan Frade \n\nIn some setups, packfile uris act as bearer token. It is not\nrecommended to expose them plainly in logs, although in special\ncircunstances (e.g. debug) it makes sense to write them.\n\nRedact the packfile-uri lines by default, unless the GIT_TRACE_REDACT\nvariable is set to false. This mimics the redacting of the\nAuthorization header in HTTP.\n\nChanges since v1:\n- Removed non-POSIX flags in tests\n- More accurate regex for the non-encrypted packfile line\n- Dropped documentation change\n- Dropped redacting the die message in http-fetch\n\nSigned-off-by: Ivan Frade \n---\n fetch-pack: redact packfile urls in traces\n \n In some setups, packfile uris act as bearer token. It is not recommended\n to expose them plainly in logs, although in special circunstances (e.g.\n debug) it makes sense to write them.\n \n Redact the packfile-uri lines by default, unless the GIT_TRACE_REDACT\n variable is set to false. This mimics the redacting of the Authorization\n header in HTTP.\n \n Signed-off-by: Ivan Frade ifrade@google.com\n \n cc: Ævar Arnfjörð Bjarmason avarab@gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1052%2Fifradeo%2Fredact-packfile-uri-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1052/ifradeo/redact-packfile-uri-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1052\n\nRange-diff vs v1:\n\n 1: b473f145a87 ! 1: 701cb7a6ab9 fetch-pack: redact packfile urls in traces\n @@ Commit message\n variable is set to false. This mimics the redacting of the\n Authorization header in HTTP.\n \n + Changes since v1:\n + - Removed non-POSIX flags in tests\n + - More accurate regex for the non-encrypted packfile line\n + - Dropped documentation change\n + - Dropped redacting the die message in http-fetch\n +\n Signed-off-by: Ivan Frade \n \n ## fetch-pack.c ##\n @@ fetch-pack.c: static void receive_packfile_uris(struct packet_reader *reader,\n \t\tdie(\"expected DELIM\");\n }\n \n - ## http-fetch.c ##\n -@@ http-fetch.c: static void fetch_single_packfile(struct object_id *packfile_hash,\n - \tif (start_active_slot(preq->slot)) {\n - \t\trun_active_slot(preq->slot);\n - \t\tif (results.curl_result != CURLE_OK) {\n --\t\t\tdie(\"Unable to get pack file %s\\n%s\", preq->url,\n -+\t\t\tint showUrl = git_env_bool(\"GIT_TRACE_REDACT\", 1);\n -+\t\t\tdie(\"Unable to get offloaded pack file %s\\n%s\",\n -+\t\t\t showUrl ? preq->url : \"\",\n - \t\t\t curl_errorstr);\n - \t\t}\n - \t} else {\n -\n ## pkt-line.c ##\n @@ pkt-line.c: enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t\tlen--;\n @@ t/t5702-protocol-v2.sh: test_expect_success 'packfile-uri with transfer.fsckobje\n +\t\t-c fetch.uriprotocols=http,https \\\n +\t\tclone \"$HTTPD_URL/smart/http_parent\" http_child &&\n +\n -+\tgrep -A1 \"clone<\\ ..packfile-uris\" log | grep \"clone<\\ \"\n ++\tgrep \"clone< \" log\n +'\n +\n +test_expect_success 'packfile-uri not redacted in trace when GIT_TRACE_REDACT=0' '\n @@ t/t5702-protocol-v2.sh: test_expect_success 'packfile-uri with transfer.fsckobje\n +\t\t-c fetch.uriprotocols=http,https \\\n +\t\tclone \"$HTTPD_URL/smart/http_parent\" http_child &&\n +\n -+\tgrep -A1 \"clone<\\ ..packfile-uris\" log | grep -E \"clone<\\ ..[[:alnum:]]{40,64}\\ http\"\n ++\tgrep -E \"clone< ..[0-9a-f]{40,64} http://\" log\n +'\n +\n test_expect_success 'http:// --negotiate-only' '\n 2: 497c5fd18d7 < -: ----------- Documentation: packfile-uri hash can be longer than 40 hex chars\n\n\n fetch-pack.c | 11 +++++++++++\n pkt-line.c | 7 ++++++-\n pkt-line.h | 1 +\n t/t5702-protocol-v2.sh | 43 ++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 61 insertions(+), 1 deletion(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex a9604f35a3e..05c85eeafa1 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1518,7 +1518,16 @@ static void receive_wanted_refs(struct packet_reader *reader,\n static void receive_packfile_uris(struct packet_reader *reader,\n \t\t\t\t struct string_list *uris)\n {\n+\tint original_options;\n \tprocess_section_header(reader, \"packfile-uris\", 0);\n+\t/*\n+\t * In some setups, packfile-uris act as bearer tokens,\n+\t * redact them by default.\n+\t */\n+\toriginal_options = reader->options;\n+\tif (git_env_bool(\"GIT_TRACE_REDACT\", 1))\n+\t\treader->options |= PACKET_READ_REDACT_ON_TRACE;\n+\n \twhile (packet_reader_read(reader) == PACKET_READ_NORMAL) {\n \t\tif (reader->pktlen < the_hash_algo->hexsz ||\n \t\t reader->line[the_hash_algo->hexsz] != ' ')\n@@ -1526,6 +1535,8 @@ static void receive_packfile_uris(struct packet_reader *reader,\n \n \t\tstring_list_append(uris, reader->line);\n \t}\n+\treader->options = original_options;\n+\n \tif (reader->status != PACKET_READ_DELIM)\n \t\tdie(\"expected DELIM\");\n }\ndiff --git a/pkt-line.c b/pkt-line.c\nindex de4a94b437e..8da8ed88ccf 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -443,7 +443,12 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t\tlen--;\n \n \tbuffer[len] = 0;\n-\tpacket_trace(buffer, len, 0);\n+\tif (options & PACKET_READ_REDACT_ON_TRACE) {\n+\t\tconst char *redacted = \"\";\n+\t\tpacket_trace(redacted, strlen(redacted), 0);\n+\t} else {\n+\t\tpacket_trace(buffer, len, 0);\n+\t}\n \n \tif ((options & PACKET_READ_DIE_ON_ERR_PACKET) &&\n \t starts_with(buffer, \"ERR \"))\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 82b95e4bdd3..44c02f3bc6e 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -88,6 +88,7 @@ void packet_fflush(FILE *f);\n #define PACKET_READ_CHOMP_NEWLINE (1u<<1)\n #define PACKET_READ_DIE_ON_ERR_PACKET (1u<<2)\n #define PACKET_READ_GENTLE_ON_READ_ERROR (1u<<3)\n+#define PACKET_READ_REDACT_ON_TRACE (1u<<4)\n int packet_read(int fd, char **src_buffer, size_t *src_len, char\n \t\t*buffer, unsigned size, int options);\n \ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex d527cf6c49f..f0273317861 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -1107,6 +1107,49 @@ test_expect_success 'packfile-uri with transfer.fsckobjects fails when .gitmodul\n \ttest_i18ngrep \"disallowed submodule name\" err\n '\n \n+test_expect_success 'packfile-uri redacted in trace' '\n+\tP=\"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n+\trm -rf \"$P\" http_child log &&\n+\n+\tgit init \"$P\" &&\n+\tgit -C \"$P\" config \"uploadpack.allowsidebandall\" \"true\" &&\n+\n+\techo my-blob >\"$P/my-blob\" &&\n+\tgit -C \"$P\" add my-blob &&\n+\tgit -C \"$P\" commit -m x &&\n+\n+\tconfigure_exclusion \"$P\" my-blob >h &&\n+\n+\tGIT_TRACE=1 GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TEST_SIDEBAND_ALL=1 \\\n+\tgit -c protocol.version=2 \\\n+\t\t-c fetch.uriprotocols=http,https \\\n+\t\tclone \"$HTTPD_URL/smart/http_parent\" http_child &&\n+\n+\tgrep \"clone< \" log\n+'\n+\n+test_expect_success 'packfile-uri not redacted in trace when GIT_TRACE_REDACT=0' '\n+\tP=\"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n+\trm -rf \"$P\" http_child log &&\n+\n+\tgit init \"$P\" &&\n+\tgit -C \"$P\" config \"uploadpack.allowsidebandall\" \"true\" &&\n+\n+\techo my-blob >\"$P/my-blob\" &&\n+\tgit -C \"$P\" add my-blob &&\n+\tgit -C \"$P\" commit -m x &&\n+\n+\tconfigure_exclusion \"$P\" my-blob >h &&\n+\n+\tGIT_TRACE=1 GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TEST_SIDEBAND_ALL=1 \\\n+\tGIT_TRACE_REDACT=0 \\\n+\tgit -c protocol.version=2 \\\n+\t\t-c fetch.uriprotocols=http,https \\\n+\t\tclone \"$HTTPD_URL/smart/http_parent\" http_child &&\n+\n+\tgrep -E \"clone< ..[0-9a-f]{40,64} http://\" log\n+'\n+\n test_expect_success 'http:// --negotiate-only' '\n \tSERVER=\"$HTTPD_DOCUMENT_ROOT_PATH/server\" &&\n \tURI=\"$HTTPD_URL/smart/server\" &&\n\nbase-commit: 0785eb769886ae81e346df10e88bc49ffc0ac64e\n-- \ngitgitgadget\n"},{"id":"438502","messageId":"xmqqczobb8jd.fsf@gitster.g","threadId":"56667","inReplyTo":"pull.1052.v2.git.1633746024175.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] fetch-pack: redact packfile urls in traces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-11T20:39:34Z","receivedAt":"2021-10-11T20:39:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Ivan Frade via GitGitGadget\" writes:\n\n> From: Ivan Frade \n>\n> In some setups, packfile uris act as bearer token. It is not\n> recommended to expose them plainly in logs, although in special\n> circunstances (e.g. debug) it makes sense to write them.\n>\n> Redact the packfile-uri lines by default, unless the GIT_TRACE_REDACT\n> variable is set to false. This mimics the redacting of the\n> Authorization header in HTTP.\n\nWell explained.\n\nIt of course is a different matter if the explained idea is\nagreeable, though ;-). Hiding the entire packet, based on the \"it\nmight be in some setups\" seems a bit too much.\n\nIs it often the case that the whole URI is sensitive, or perhaps\nleading \":///pack-.pack\" part is not sensitive at\nall, and what follows after that \"public\" part has some \"nonce\"\nmaterial that makes it sensitive?\n\n> Changes since v1:\n> - Removed non-POSIX flags in tests\n> - More accurate regex for the non-encrypted packfile line\n> - Dropped documentation change\n> - Dropped redacting the die message in http-fetch\n\nThese are not for those who read \"git log\" in 3 months, as they may\nnot even have seen the \"v1\". But these are very helpful for those\nwho read the \"v1\" to see how good this round is. Please write such\nmaterial below the three-dash line.\n\n> Signed-off-by: Ivan Frade \n> ---\n\ni.e. here.\n\n> fetch-pack: redact packfile urls in traces\n> \n> In some setups, packfile uris act as bearer token. It is not recommended\n> to expose them plainly in logs, although in special circunstances (e.g.\n> debug) it makes sense to write them.\n> \n> Redact the packfile-uri lines by default, unless the GIT_TRACE_REDACT\n> variable is set to false. This mimics the redacting of the Authorization\n> header in HTTP.\n> \n> Signed-off-by: Ivan Frade ifrade@google.com\n> \n> cc: Ævar Arnfjörð Bjarmason avarab@gmail.com\n\nAnd there is no need to duplicate the log message here ;-)\n\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index a9604f35a3e..05c85eeafa1 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -1518,7 +1518,16 @@ static void receive_wanted_refs(struct packet_reader *reader,\n> static void receive_packfile_uris(struct packet_reader *reader,\n> \t\t\t\t struct string_list *uris)\n> {\n> +\tint original_options;\n> \tprocess_section_header(reader, \"packfile-uris\", 0);\n> +\t/*\n> +\t * In some setups, packfile-uris act as bearer tokens,\n> +\t * redact them by default.\n> +\t */\n> +\toriginal_options = reader->options;\n> +\tif (git_env_bool(\"GIT_TRACE_REDACT\", 1))\n> +\t\treader->options |= PACKET_READ_REDACT_ON_TRACE;\n> +\n> \twhile (packet_reader_read(reader) == PACKET_READ_NORMAL) {\n> \t\tif (reader->pktlen < the_hash_algo->hexsz ||\n> \t\t reader->line[the_hash_algo->hexsz] != ' ')\n> @@ -1526,6 +1535,8 @@ static void receive_packfile_uris(struct packet_reader *reader,\n> \n> \t\tstring_list_append(uris, reader->line);\n> \t}\n> +\treader->options = original_options;\n\nSo \"original_options\" is used to save away the reader->options so\nthat it can be restored before returning to our caller? \n\nOK (it may be more common in this codebase to call such a variable\n\"saved_X\", though).\n\n> diff --git a/pkt-line.c b/pkt-line.c\n> index de4a94b437e..8da8ed88ccf 100644\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -443,7 +443,12 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n> \t\tlen--;\n> \n> \tbuffer[len] = 0;\n> -\tpacket_trace(buffer, len, 0);\n> +\tif (options & PACKET_READ_REDACT_ON_TRACE) {\n> +\t\tconst char *redacted = \"\";\n> +\t\tpacket_trace(redacted, strlen(redacted), 0);\n> +\t} else {\n> +\t\tpacket_trace(buffer, len, 0);\n> +\t}\n> ...\n> +\tGIT_TRACE=1 GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TEST_SIDEBAND_ALL=1 \\\n> +\tgit -c protocol.version=2 \\\n> +\t\t-c fetch.uriprotocols=http,https \\\n> +\t\tclone \"$HTTPD_URL/smart/http_parent\" http_child &&\n> +\n> +\tgrep \"clone< \" log\n\nThis checks only that \"redacted\" string appears, but what the theme\nof the change really cares about is different, no? You want to\nensure that no sensitive substring of the URI appears in the log.\n\nImagine somebody breaking the redact logic by making it prepend that\nstring to the payload, instead of replacing the payload with that\nstring---this test will not catch such a regression.\n\nThanks.\n"},{"id":"439055","messageId":"pull.1052.v3.git.1634684260142.gitgitgadget@gmail.com","threadId":"56667","inReplyTo":"pull.1052.v2.git.1633746024175.gitgitgadget@gmail.com","subject":"[PATCH v3] fetch-pack: redact packfile urls in traces","fromName":"Ivan Frade via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-19T22:57:39Z","receivedAt":"2021-10-19T22:57:46Z","isPatch":true,"sender":{"key":"ifrade@google.com","avatar":"https://avatars.githubusercontent.com/u/58185630?v=4"},"body":"From: Ivan Frade \n\nIn some setups, packfile uris act as bearer token. It is not\nrecommended to expose them plainly in logs, although in special\ncircunstances (e.g. debug) it makes sense to write them.\n\nRedact the packfile URL paths by default, unless the GIT_TRACE_REDACT\nvariable is set to false. This mimics the redacting of the Authorization\nheader in HTTP.\n\nSigned-off-by: Ivan Frade \n---\n fetch-pack: redact packfile urls in traces\n \n Changes since v1:\n \n * Redact only the path of the URL\n * Test are now strict, validating the exact line expected in the log\n \n Changes since v1:\n \n * Removed non-POSIX flags in tests\n * More accurate regex for the non-encrypted packfile line\n * Dropped documentation change\n * Dropped redacting the die message in http-fetch\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1052%2Fifradeo%2Fredact-packfile-uri-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1052/ifradeo/redact-packfile-uri-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1052\n\nRange-diff vs v2:\n\n 1: 701cb7a6ab9 ! 1: 9afe0093af4 fetch-pack: redact packfile urls in traces\n @@ Commit message\n recommended to expose them plainly in logs, although in special\n circunstances (e.g. debug) it makes sense to write them.\n \n - Redact the packfile-uri lines by default, unless the GIT_TRACE_REDACT\n - variable is set to false. This mimics the redacting of the\n - Authorization header in HTTP.\n -\n - Changes since v1:\n - - Removed non-POSIX flags in tests\n - - More accurate regex for the non-encrypted packfile line\n - - Dropped documentation change\n - - Dropped redacting the die message in http-fetch\n + Redact the packfile URL paths by default, unless the GIT_TRACE_REDACT\n + variable is set to false. This mimics the redacting of the Authorization\n + header in HTTP.\n \n Signed-off-by: Ivan Frade \n \n @@ fetch-pack.c: static void receive_wanted_refs(struct packet_reader *reader,\n static void receive_packfile_uris(struct packet_reader *reader,\n \t\t\t\t struct string_list *uris)\n {\n -+\tint original_options;\n ++\tint saved_options;\n \tprocess_section_header(reader, \"packfile-uris\", 0);\n +\t/*\n +\t * In some setups, packfile-uris act as bearer tokens,\n +\t * redact them by default.\n +\t */\n -+\toriginal_options = reader->options;\n ++\tsaved_options = reader->options;\n +\tif (git_env_bool(\"GIT_TRACE_REDACT\", 1))\n -+\t\treader->options |= PACKET_READ_REDACT_ON_TRACE;\n ++\t\treader->options |= PACKET_READ_REDACT_URL_PATH;\n +\n \twhile (packet_reader_read(reader) == PACKET_READ_NORMAL) {\n \t\tif (reader->pktlen < the_hash_algo->hexsz ||\n @@ fetch-pack.c: static void receive_packfile_uris(struct packet_reader *reader,\n \n \t\tstring_list_append(uris, reader->line);\n \t}\n -+\treader->options = original_options;\n ++\treader->options = saved_options;\n +\n \tif (reader->status != PACKET_READ_DELIM)\n \t\tdie(\"expected DELIM\");\n }\n \n ## pkt-line.c ##\n +@@ pkt-line.c: int packet_length(const char lenbuf_hex[4])\n + \treturn (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);\n + }\n + \n ++static int find_url_path_start(const char* buffer)\n ++{\n ++\tconst char *URL_MARK = \"://\";\n ++\tchar *p = strstr(buffer, URL_MARK);\n ++\tif (!p) {\n ++\t\treturn -1;\n ++\t}\n ++\n ++\tp += strlen(URL_MARK);\n ++\twhile (*p && *p != '/')\n ++\t\tp++;\n ++\n ++\t// Position after '/'\n ++\tif (*p && *(p + 1))\n ++\t\treturn (p + 1) - buffer;\n ++\n ++\treturn -1;\n ++}\n ++\n + enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n + \t\t\t\t\t\tsize_t *src_len, char *buffer,\n + \t\t\t\t\t\tunsigned size, int *pktlen,\n +@@ pkt-line.c: enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n + {\n + \tint len;\n + \tchar linelen[4];\n ++\tint url_path_start;\n + \n + \tif (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0) {\n + \t\t*pktlen = -1;\n @@ pkt-line.c: enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t\tlen--;\n \n \tbuffer[len] = 0;\n -\tpacket_trace(buffer, len, 0);\n -+\tif (options & PACKET_READ_REDACT_ON_TRACE) {\n ++\tif (options & PACKET_READ_REDACT_URL_PATH &&\n ++\t (url_path_start = find_url_path_start(buffer)) != -1) {\n +\t\tconst char *redacted = \"\";\n -+\t\tpacket_trace(redacted, strlen(redacted), 0);\n ++\t\tstruct strbuf tracebuf = STRBUF_INIT;\n ++\t\tstrbuf_insert(&tracebuf, 0, buffer, len);\n ++\t\tstrbuf_splice(&tracebuf, url_path_start,\n ++\t\t\t len - url_path_start, redacted, strlen(redacted));\n ++\t\tpacket_trace(tracebuf.buf, tracebuf.len, 0);\n ++\t\tstrbuf_release(&tracebuf);\n +\t} else {\n +\t\tpacket_trace(buffer, len, 0);\n +\t}\n @@ pkt-line.h: void packet_fflush(FILE *f);\n #define PACKET_READ_CHOMP_NEWLINE (1u<<1)\n #define PACKET_READ_DIE_ON_ERR_PACKET (1u<<2)\n #define PACKET_READ_GENTLE_ON_READ_ERROR (1u<<3)\n -+#define PACKET_READ_REDACT_ON_TRACE (1u<<4)\n ++#define PACKET_READ_REDACT_URL_PATH (1u<<4)\n int packet_read(int fd, char **src_buffer, size_t *src_len, char\n \t\t*buffer, unsigned size, int options);\n \n @@ t/t5702-protocol-v2.sh: test_expect_success 'packfile-uri with transfer.fsckobje\n \ttest_i18ngrep \"disallowed submodule name\" err\n '\n \n -+test_expect_success 'packfile-uri redacted in trace' '\n ++test_expect_success 'packfile-uri path redacted in trace' '\n +\tP=\"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n +\trm -rf \"$P\" http_child log &&\n +\n @@ t/t5702-protocol-v2.sh: test_expect_success 'packfile-uri with transfer.fsckobje\n +\tgit -C \"$P\" add my-blob &&\n +\tgit -C \"$P\" commit -m x &&\n +\n -+\tconfigure_exclusion \"$P\" my-blob >h &&\n ++\tgit -C \"$P\" hash-object my-blob >objh &&\n ++\tgit -C \"$P\" pack-objects \"$HTTPD_DOCUMENT_ROOT_PATH/mypack\" packh &&\n ++\tgit -C \"$P\" config --add \\\n ++\t\t\"uploadpack.blobpackfileuri\" \\\n ++\t\t\"$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack\" &&\n +\n +\tGIT_TRACE=1 GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TEST_SIDEBAND_ALL=1 \\\n +\tgit -c protocol.version=2 \\\n +\t\t-c fetch.uriprotocols=http,https \\\n +\t\tclone \"$HTTPD_URL/smart/http_parent\" http_child &&\n +\n -+\tgrep \"clone< \" log\n ++\tgrep -F \"clone< \\\\1$(cat packh) $HTTPD_URL/\" log\n +'\n +\n -+test_expect_success 'packfile-uri not redacted in trace when GIT_TRACE_REDACT=0' '\n ++test_expect_success 'packfile-uri path not redacted in trace when GIT_TRACE_REDACT=0' '\n +\tP=\"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n +\trm -rf \"$P\" http_child log &&\n +\n @@ t/t5702-protocol-v2.sh: test_expect_success 'packfile-uri with transfer.fsckobje\n +\tgit -C \"$P\" add my-blob &&\n +\tgit -C \"$P\" commit -m x &&\n +\n -+\tconfigure_exclusion \"$P\" my-blob >h &&\n ++\tgit -C \"$P\" hash-object my-blob >objh &&\n ++\tgit -C \"$P\" pack-objects \"$HTTPD_DOCUMENT_ROOT_PATH/mypack\" packh &&\n ++\tgit -C \"$P\" config --add \\\n ++\t\t\"uploadpack.blobpackfileuri\" \\\n ++\t\t\"$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack\" &&\n +\n +\tGIT_TRACE=1 GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TEST_SIDEBAND_ALL=1 \\\n +\tGIT_TRACE_REDACT=0 \\\n @@ t/t5702-protocol-v2.sh: test_expect_success 'packfile-uri with transfer.fsckobje\n +\t\t-c fetch.uriprotocols=http,https \\\n +\t\tclone \"$HTTPD_URL/smart/http_parent\" http_child &&\n +\n -+\tgrep -E \"clone< ..[0-9a-f]{40,64} http://\" log\n ++\tgrep -F \"clone< \\\\1$(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack\" log\n +'\n +\n test_expect_success 'http:// --negotiate-only' '\n\n\n fetch-pack.c | 11 +++++++++\n pkt-line.c | 33 ++++++++++++++++++++++++++-\n pkt-line.h | 1 +\n t/t5702-protocol-v2.sh | 51 ++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 95 insertions(+), 1 deletion(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex a9604f35a3e..1587b9ae662 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1518,7 +1518,16 @@ static void receive_wanted_refs(struct packet_reader *reader,\n static void receive_packfile_uris(struct packet_reader *reader,\n \t\t\t\t struct string_list *uris)\n {\n+\tint saved_options;\n \tprocess_section_header(reader, \"packfile-uris\", 0);\n+\t/*\n+\t * In some setups, packfile-uris act as bearer tokens,\n+\t * redact them by default.\n+\t */\n+\tsaved_options = reader->options;\n+\tif (git_env_bool(\"GIT_TRACE_REDACT\", 1))\n+\t\treader->options |= PACKET_READ_REDACT_URL_PATH;\n+\n \twhile (packet_reader_read(reader) == PACKET_READ_NORMAL) {\n \t\tif (reader->pktlen < the_hash_algo->hexsz ||\n \t\t reader->line[the_hash_algo->hexsz] != ' ')\n@@ -1526,6 +1535,8 @@ static void receive_packfile_uris(struct packet_reader *reader,\n \n \t\tstring_list_append(uris, reader->line);\n \t}\n+\treader->options = saved_options;\n+\n \tif (reader->status != PACKET_READ_DELIM)\n \t\tdie(\"expected DELIM\");\n }\ndiff --git a/pkt-line.c b/pkt-line.c\nindex de4a94b437e..1a9e6870559 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -386,6 +386,25 @@ int packet_length(const char lenbuf_hex[4])\n \treturn (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);\n }\n \n+static int find_url_path_start(const char* buffer)\n+{\n+\tconst char *URL_MARK = \"://\";\n+\tchar *p = strstr(buffer, URL_MARK);\n+\tif (!p) {\n+\t\treturn -1;\n+\t}\n+\n+\tp += strlen(URL_MARK);\n+\twhile (*p && *p != '/')\n+\t\tp++;\n+\n+\t// Position after '/'\n+\tif (*p && *(p + 1))\n+\t\treturn (p + 1) - buffer;\n+\n+\treturn -1;\n+}\n+\n enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t\t\t\t\t\tsize_t *src_len, char *buffer,\n \t\t\t\t\t\tunsigned size, int *pktlen,\n@@ -393,6 +412,7 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n {\n \tint len;\n \tchar linelen[4];\n+\tint url_path_start;\n \n \tif (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0) {\n \t\t*pktlen = -1;\n@@ -443,7 +463,18 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t\tlen--;\n \n \tbuffer[len] = 0;\n-\tpacket_trace(buffer, len, 0);\n+\tif (options & PACKET_READ_REDACT_URL_PATH &&\n+\t (url_path_start = find_url_path_start(buffer)) != -1) {\n+\t\tconst char *redacted = \"\";\n+\t\tstruct strbuf tracebuf = STRBUF_INIT;\n+\t\tstrbuf_insert(&tracebuf, 0, buffer, len);\n+\t\tstrbuf_splice(&tracebuf, url_path_start,\n+\t\t\t len - url_path_start, redacted, strlen(redacted));\n+\t\tpacket_trace(tracebuf.buf, tracebuf.len, 0);\n+\t\tstrbuf_release(&tracebuf);\n+\t} else {\n+\t\tpacket_trace(buffer, len, 0);\n+\t}\n \n \tif ((options & PACKET_READ_DIE_ON_ERR_PACKET) &&\n \t starts_with(buffer, \"ERR \"))\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 82b95e4bdd3..853d20688c8 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -88,6 +88,7 @@ void packet_fflush(FILE *f);\n #define PACKET_READ_CHOMP_NEWLINE (1u<<1)\n #define PACKET_READ_DIE_ON_ERR_PACKET (1u<<2)\n #define PACKET_READ_GENTLE_ON_READ_ERROR (1u<<3)\n+#define PACKET_READ_REDACT_URL_PATH (1u<<4)\n int packet_read(int fd, char **src_buffer, size_t *src_len, char\n \t\t*buffer, unsigned size, int options);\n \ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex d527cf6c49f..f01af2f2ed3 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -1107,6 +1107,57 @@ test_expect_success 'packfile-uri with transfer.fsckobjects fails when .gitmodul\n \ttest_i18ngrep \"disallowed submodule name\" err\n '\n \n+test_expect_success 'packfile-uri path redacted in trace' '\n+\tP=\"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n+\trm -rf \"$P\" http_child log &&\n+\n+\tgit init \"$P\" &&\n+\tgit -C \"$P\" config \"uploadpack.allowsidebandall\" \"true\" &&\n+\n+\techo my-blob >\"$P/my-blob\" &&\n+\tgit -C \"$P\" add my-blob &&\n+\tgit -C \"$P\" commit -m x &&\n+\n+\tgit -C \"$P\" hash-object my-blob >objh &&\n+\tgit -C \"$P\" pack-objects \"$HTTPD_DOCUMENT_ROOT_PATH/mypack\" packh &&\n+\tgit -C \"$P\" config --add \\\n+\t\t\"uploadpack.blobpackfileuri\" \\\n+\t\t\"$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack\" &&\n+\n+\tGIT_TRACE=1 GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TEST_SIDEBAND_ALL=1 \\\n+\tgit -c protocol.version=2 \\\n+\t\t-c fetch.uriprotocols=http,https \\\n+\t\tclone \"$HTTPD_URL/smart/http_parent\" http_child &&\n+\n+\tgrep -F \"clone< \\\\1$(cat packh) $HTTPD_URL/\" log\n+'\n+\n+test_expect_success 'packfile-uri path not redacted in trace when GIT_TRACE_REDACT=0' '\n+\tP=\"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n+\trm -rf \"$P\" http_child log &&\n+\n+\tgit init \"$P\" &&\n+\tgit -C \"$P\" config \"uploadpack.allowsidebandall\" \"true\" &&\n+\n+\techo my-blob >\"$P/my-blob\" &&\n+\tgit -C \"$P\" add my-blob &&\n+\tgit -C \"$P\" commit -m x &&\n+\n+\tgit -C \"$P\" hash-object my-blob >objh &&\n+\tgit -C \"$P\" pack-objects \"$HTTPD_DOCUMENT_ROOT_PATH/mypack\" packh &&\n+\tgit -C \"$P\" config --add \\\n+\t\t\"uploadpack.blobpackfileuri\" \\\n+\t\t\"$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack\" &&\n+\n+\tGIT_TRACE=1 GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TEST_SIDEBAND_ALL=1 \\\n+\tGIT_TRACE_REDACT=0 \\\n+\tgit -c protocol.version=2 \\\n+\t\t-c fetch.uriprotocols=http,https \\\n+\t\tclone \"$HTTPD_URL/smart/http_parent\" http_child &&\n+\n+\tgrep -F \"clone< \\\\1$(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack\" log\n+'\n+\n test_expect_success 'http:// --negotiate-only' '\n \tSERVER=\"$HTTPD_DOCUMENT_ROOT_PATH/server\" &&\n \tURI=\"$HTTPD_URL/smart/server\" &&\n\nbase-commit: 9d530dc0024503ab4218fe6c4395b8a0aa245478\n-- \ngitgitgadget\n"},{"id":"439090","messageId":"211020.868rynoqfv.gmgdl@evledraar.gmail.com","threadId":"56667","inReplyTo":"pull.1052.v3.git.1634684260142.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] fetch-pack: redact packfile urls in traces","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-20T11:41:57Z","receivedAt":"2021-10-20T12:01:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Oct 19 2021, Ivan Frade via GitGitGadget wrote:\n\n> From: Ivan Frade \n>\n> In some setups, packfile uris act as bearer token. It is not\n> recommended to expose them plainly in logs, although in special\n> circunstances (e.g. debug) it makes sense to write them.\n>\n> Redact the packfile URL paths by default, unless the GIT_TRACE_REDACT\n> variable is set to false. This mimics the redacting of the Authorization\n> header in HTTP.\n>\n> Signed-off-by: Ivan Frade \n> ---\n> fetch-pack: redact packfile urls in traces\n> \n> Changes since v1:\n\nJust context for other reviewers:\n\ns/Changes since v1/Changes since v2/ I see, from the context of\nhttps://lore.kernel.org/git/pull.1052.v2.git.1633746024175.gitgitgadget@gmail.com/\n\n> * Redact only the path of the URL\n> * Test are now strict, validating the exact line expected in the log\n\nAnd this was changed in v2...\n\n> Changes since v1:\n> \n> * Removed non-POSIX flags in tests\n> * More accurate regex for the non-encrypted packfile line\n\n[...]\n\n> * Dropped documentation change\n> * Dropped redacting the die message in http-fetch\n\nSince both of those were done I think in response to my feedback I just\nwant to clarify (if needed):\n\n * That the documentation change is still good to have, although I had\n feedback on fixing that more generally in the protocol v2 docs. It\n would be great if you could still pursue it (and that I didn't\n discourage you from doing so).\n\n * I think having this redaction in die() could still be valuable,\n e.g. your packfile-uri's start failing, and now users are\n copy/pasting \"private\" URLs that contain their passwords or whatever\n to try to get help, that would be bad. \n\n But perhaps if you don't have private URLs redacting them\n unconditionally would slow down debugging for some, i.e. you have 10x\n pasted URLs, and all the errors are from one set of servers (although\n your current redaction includes the hostname, which I think would\n address most cases of say one CDN node failing).\n\n It was really just a comment that your v1's commit message didn't\n mention or justify it, but just having it make a mention of it would\n also be an OK solution, or fold that into another patch or\n whatever...\n\n> {\n> +\tint saved_options;\n> \tprocess_section_header(reader, \"packfile-uris\", 0);\n> +\t/*\n> +\t * In some setups, packfile-uris act as bearer tokens,\n> +\t * redact them by default.\n> +\t */\n> +\tsaved_options = reader->options;\n\nnit: no need to pre-declare \"int saved_options\" here, just move this &\nthe comment above \"process_section_header\" (in this case I'd say a\ncomment isn't even needed, obvious from context...), or it should be at\nthe definition of PACKET_READ_REDACT_URL_PATH... (more below)\n\n> +\tif (git_env_bool(\"GIT_TRACE_REDACT\", 1))\n\nIf we're going to use GIT_TRACE_REDACT for this the documentation needs updating:\n\nDocumentation/git.txt:`GIT_TRACE_REDACT`::\nDocumentation/git.txt- By default, when tracing is activated, Git redacts the values of\nDocumentation/git.txt- cookies, the \"Authorization:\" header, and the \"Proxy-Authorization:\"\nDocumentation/git.txt- header. Set this variable to `0` to prevent this redaction.\n\n> +\t\treader->options |= PACKET_READ_REDACT_URL_PATH;\n\n(continued)... but that was from a really narrow reading of the code, I\nthink this whole flip-flopping of options back and forth isn't needed at\nall, and you should just assign this flag at the top of\ndo_fetch_pack_v2(), no? The setting of it also looks like it belongs\nwith the reading of \"GIT_TEST_SIDEBAND_ALL\".\n\nI.e. nothing else uses PACKET_READ_REDACT_URL_PATH, why do we need to be\nflipping it back & forth? Keeping these flags in the \"reader\" is what\nthat member is for, isn't it? Maybe I'm missing something.\n\n> +static int find_url_path_start(const char* buffer)\n> +{\n> +\tconst char *URL_MARK = \"://\";\n> +\tchar *p = strstr(buffer, URL_MARK);\n> +\tif (!p) {\n> +\t\treturn -1;\n> +\t}\n> +\n> +\tp += strlen(URL_MARK);\n> +\twhile (*p && *p != '/')\n> +\t\tp++;\n> +\n> +\t// Position after '/'\n> +\tif (*p && *(p + 1))\n> +\t\treturn (p + 1) - buffer;\n> +\n> +\treturn -1;\n> +}\n\nI think that packfile URI only supports http:// and https://, not\nfile:// or whatever, so I wonder if either curl or we have a helper\nfunction for this that we can use....(more below)\n\n> enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n> \t\t\t\t\t\tsize_t *src_len, char *buffer,\n> \t\t\t\t\t\tunsigned size, int *pktlen,\n> @@ -393,6 +412,7 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n> {\n> \tint len;\n> \tchar linelen[4];\n> +\tint url_path_start;\n> \n> \tif (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0) {\n> \t\t*pktlen = -1;\n> @@ -443,7 +463,18 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n> \t\tlen--;\n> \n> \tbuffer[len] = 0;\n> -\tpacket_trace(buffer, len, 0);\n> +\tif (options & PACKET_READ_REDACT_URL_PATH &&\n> +\t (url_path_start = find_url_path_start(buffer)) != -1) {\n> +\t\tconst char *redacted = \"\";\n> +\t\tstruct strbuf tracebuf = STRBUF_INIT;\n> +\t\tstrbuf_insert(&tracebuf, 0, buffer, len);\n> +\t\tstrbuf_splice(&tracebuf, url_path_start,\n> +\t\t\t len - url_path_start, redacted, strlen(redacted));\n> +\t\tpacket_trace(tracebuf.buf, tracebuf.len, 0);\n> +\t\tstrbuf_release(&tracebuf);\n> +\t} else {\n> +\t\tpacket_trace(buffer, len, 0);\n> +\t}\n\n...If we're redacting the URL isn't (and this might be less code with a\nhelper function) saying:\n\n failed to get 'https' url from 'somehost.example.com' (full URL redacted due to XYZ setting)\n\nFriendlier than something like (which this function sets up):\n\n failed to get 'https://somehost.example.com/' url\n\nI.e. it allows us to use a get_schema_from_url() and get_host_from_url()\nfunctions (I don't know if/where we have those, but seems likelier than\n\"find path url boundary\" (or maybe I'm wrong and we always feed those\nas-is to curl et al).\n"},{"id":"439668","messageId":"CANQMx9W_Zt+qy3sppx1qGdf6S9gMSEp_7jjV4hc_aeyR62syrQ@mail.gmail.com","threadId":"56667","inReplyTo":"xmqqczobb8jd.fsf@gitster.g","subject":"Re: [PATCH v2] fetch-pack: redact packfile urls in traces","fromName":"Ivan Frade","fromEmail":"ifrade@google.com","sentAt":"2021-10-26T19:32:08Z","receivedAt":"2021-10-26T19:32:23Z","isPatch":true,"sender":{"key":"ifrade@google.com","avatar":"https://avatars.githubusercontent.com/u/58185630?v=4"},"body":"It seems I sent my original reply only to the github PR. Sorry for the\nconfusion:\n\nOn Mon, Oct 11, 2021 at 1:39 PM Junio C Hamano wrote:\n>\n> \"Ivan Frade via GitGitGadget\" writes:\n>\n> > From: Ivan Frade \n...\n>\n> It of course is a different matter if the explained idea is\n> agreeable, though ;-). Hiding the entire packet, based on the \"it\n> might be in some setups\" seems a bit too much.\n>\n> Is it often the case that the whole URI is sensitive, or perhaps\n> leading \":///pack-.pack\" part is not sensitive at\n> all, and what follows after that \"public\" part has some \"nonce\"\n> material that makes it sensitive?\n\nIn the specific case I am working on, the path of the URL is an\nencrypted string that shouldn't be completely exposed (exposing part\nof it would be fine). In general, I think we can assume that\n:/// are always \"public\" but the path could be\nsensitive.\n\nWe could redact only the path (:///REDACTED), or even a\nfixed length of the URL? (:///pack- > Changes since v1:\n...\n> Please write such material below the three-dash line.\nDone\n\n> And there is no need to duplicate the log message here ;-)\nDone\n\n> So \"original_options\" is used to save away the reader->options so\n> that it can be restored before returning to our caller?\n>\n> OK (it may be more common in this codebase to call such a variable\n> \"saved_X\", though).\n\nIn the latest iteration, the option is enabled for all sections and\nthere is no need to set/unset the flag.\n\n> > + grep \"clone< \" log\n>\n> This checks only that \"redacted\" string appears, but what the theme\n> of the change really cares about is different, no? You want to\n> ensure that no sensitive substring of the URI appears in the log.\n>\n> Imagine somebody breaking the redact logic by making it prepend that\n> string to the payload, instead of replacing the payload with that\n> string---this test will not catch such a regression.\n\nNow the tests verify the expected packfile-uri full line is in the log.\n\nThanks,\n\nIvan\n"},{"id":"439699","messageId":"pull.1052.v4.git.1635288599.gitgitgadget@gmail.com","threadId":"56667","inReplyTo":"pull.1052.v3.git.1634684260142.gitgitgadget@gmail.com","subject":"[PATCH v4 0/2] fetch-pack: redact packfile urls in traces","fromName":"Ivan Frade via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-26T22:49:57Z","receivedAt":"2021-10-26T22:50:03Z","isPatch":true,"sender":{"key":"ifrade@google.com","avatar":"https://avatars.githubusercontent.com/u/58185630?v=4"},"body":"Changes since v3:\n\n * Enable redacting URLs for all sections\n * Redact only URL path (it was until the end of line)\n * Redact URL in die() with more friendly message\n * Update doc to mention that packfile URIs are also redacted.\n\nChanges since v2:\n\n * Redact only the path of the URL\n * Test are now strict, validating the exact line expected in the log\n\nChanges since v1:\n\n * Removed non-POSIX flags in tests\n * More accurate regex for the non-encrypted packfile line\n * Dropped documentation change\n * Dropped redacting the die message in http-fetch\n\nIvan Frade (2):\n fetch-pack: redact packfile urls in traces\n http-fetch: redact url on die() message\n\n Documentation/git.txt | 5 +++--\n fetch-pack.c | 3 +++\n http-fetch.c | 15 +++++++++++--\n pkt-line.c | 40 ++++++++++++++++++++++++++++++++-\n pkt-line.h | 1 +\n t/t5702-protocol-v2.sh | 51 ++++++++++++++++++++++++++++++++++++++++++\n 6 files changed, 110 insertions(+), 5 deletions(-)\n\n\nbase-commit: e9e5ba39a78c8f5057262d49e261b42a8660d5b9\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1052%2Fifradeo%2Fredact-packfile-uri-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1052/ifradeo/redact-packfile-uri-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/1052\n\nRange-diff vs v3:\n\n 1: 9afe0093af4 ! 1: 973a250752c fetch-pack: redact packfile urls in traces\n @@ Commit message\n \n Signed-off-by: Ivan Frade \n \n - ## fetch-pack.c ##\n -@@ fetch-pack.c: static void receive_wanted_refs(struct packet_reader *reader,\n - static void receive_packfile_uris(struct packet_reader *reader,\n - \t\t\t\t struct string_list *uris)\n - {\n -+\tint saved_options;\n - \tprocess_section_header(reader, \"packfile-uris\", 0);\n -+\t/*\n -+\t * In some setups, packfile-uris act as bearer tokens,\n -+\t * redact them by default.\n -+\t */\n -+\tsaved_options = reader->options;\n -+\tif (git_env_bool(\"GIT_TRACE_REDACT\", 1))\n -+\t\treader->options |= PACKET_READ_REDACT_URL_PATH;\n -+\n - \twhile (packet_reader_read(reader) == PACKET_READ_NORMAL) {\n - \t\tif (reader->pktlen < the_hash_algo->hexsz ||\n - \t\t reader->line[the_hash_algo->hexsz] != ' ')\n -@@ fetch-pack.c: static void receive_packfile_uris(struct packet_reader *reader,\n + ## Documentation/git.txt ##\n +@@ Documentation/git.txt: for full details.\n \n - \t\tstring_list_append(uris, reader->line);\n + `GIT_TRACE_REDACT`::\n + \tBy default, when tracing is activated, Git redacts the values of\n +-\tcookies, the \"Authorization:\" header, and the \"Proxy-Authorization:\"\n +-\theader. Set this variable to `0` to prevent this redaction.\n ++\tcookies, the \"Authorization:\" header, the \"Proxy-Authorization:\"\n ++\theader and packfile URLs. Set this variable to `0` to prevent this\n ++\tredaction.\n + \n + `GIT_LITERAL_PATHSPECS`::\n + \tSetting this variable to `1` will cause Git to treat all\n +\n + ## fetch-pack.c ##\n +@@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n + \t\treader.me = \"fetch-pack\";\n \t}\n -+\treader->options = saved_options;\n + \n ++\tif (git_env_bool(\"GIT_TRACE_REDACT\", 1))\n ++\t\treader.options |= PACKET_READ_REDACT_URL_PATH;\n +\n - \tif (reader->status != PACKET_READ_DELIM)\n - \t\tdie(\"expected DELIM\");\n - }\n + \twhile (state != FETCH_DONE) {\n + \t\tswitch (state) {\n + \t\tcase FETCH_CHECK_LOCAL:\n \n ## pkt-line.c ##\n @@ pkt-line.c: int packet_length(const char lenbuf_hex[4])\n \treturn (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);\n }\n \n -+static int find_url_path_start(const char* buffer)\n ++static char *find_url_path(const char* buffer, int *path_len)\n +{\n +\tconst char *URL_MARK = \"://\";\n -+\tchar *p = strstr(buffer, URL_MARK);\n -+\tif (!p) {\n -+\t\treturn -1;\n -+\t}\n ++\tchar *path = strstr(buffer, URL_MARK);\n ++\tif (!path)\n ++\t\treturn NULL;\n +\n -+\tp += strlen(URL_MARK);\n -+\twhile (*p && *p != '/')\n -+\t\tp++;\n ++\tpath += strlen(URL_MARK);\n ++\twhile (*path && *path != '/')\n ++\t\tpath++;\n +\n -+\t// Position after '/'\n -+\tif (*p && *(p + 1))\n -+\t\treturn (p + 1) - buffer;\n ++\tif (!*path || !*(path + 1))\n ++\t\treturn NULL;\n ++\n ++\t// position after '/'\n ++\tpath++;\n ++\n ++\tif (path_len) {\n ++\t\tchar *url_end = strchrnul(path, ' ');\n ++\t\t*path_len = url_end - path;\n ++\t}\n +\n -+\treturn -1;\n ++\treturn path;\n +}\n +\n enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n @@ pkt-line.c: enum packet_read_status packet_read_with_status(int fd, char **src_b\n {\n \tint len;\n \tchar linelen[4];\n -+\tint url_path_start;\n ++\tchar *url_path_start;\n ++\tint url_path_len;\n \n \tif (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0) {\n \t\t*pktlen = -1;\n @@ pkt-line.c: enum packet_read_status packet_read_with_status(int fd, char **src_b\n \tbuffer[len] = 0;\n -\tpacket_trace(buffer, len, 0);\n +\tif (options & PACKET_READ_REDACT_URL_PATH &&\n -+\t (url_path_start = find_url_path_start(buffer)) != -1) {\n ++\t (url_path_start = find_url_path(buffer, &url_path_len))) {\n +\t\tconst char *redacted = \"\";\n +\t\tstruct strbuf tracebuf = STRBUF_INIT;\n +\t\tstrbuf_insert(&tracebuf, 0, buffer, len);\n -+\t\tstrbuf_splice(&tracebuf, url_path_start,\n -+\t\t\t len - url_path_start, redacted, strlen(redacted));\n ++\t\tstrbuf_splice(&tracebuf, url_path_start - buffer,\n ++\t\t\t url_path_len, redacted, strlen(redacted));\n +\t\tpacket_trace(tracebuf.buf, tracebuf.len, 0);\n +\t\tstrbuf_release(&tracebuf);\n +\t} else {\n @@ pkt-line.h: void packet_fflush(FILE *f);\n #define PACKET_READ_DIE_ON_ERR_PACKET (1u<<2)\n #define PACKET_READ_GENTLE_ON_READ_ERROR (1u<<3)\n +#define PACKET_READ_REDACT_URL_PATH (1u<<4)\n - int packet_read(int fd, char **src_buffer, size_t *src_len, char\n - \t\t*buffer, unsigned size, int options);\n + int packet_read(int fd, char *buffer, unsigned size, int options);\n \n + /*\n \n ## t/t5702-protocol-v2.sh ##\n @@ t/t5702-protocol-v2.sh: test_expect_success 'packfile-uri with transfer.fsckobjects fails when .gitmodul\n -: ----------- > 2: c7f0977cabd http-fetch: redact url on die() message\n\n-- \ngitgitgadget\n"},{"id":"439700","messageId":"973a250752c39c3fe835d69f3fbe8f009fc4fa74.1635288599.git.gitgitgadget@gmail.com","threadId":"56667","inReplyTo":"pull.1052.v4.git.1635288599.gitgitgadget@gmail.com","subject":"[PATCH v4 1/2] fetch-pack: redact packfile urls in traces","fromName":"Ivan Frade via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-26T22:49:58Z","receivedAt":"2021-10-26T22:50:05Z","isPatch":true,"sender":{"key":"ifrade@google.com","avatar":"https://avatars.githubusercontent.com/u/58185630?v=4"},"body":"From: Ivan Frade \n\nIn some setups, packfile uris act as bearer token. It is not\nrecommended to expose them plainly in logs, although in special\ncircunstances (e.g. debug) it makes sense to write them.\n\nRedact the packfile URL paths by default, unless the GIT_TRACE_REDACT\nvariable is set to false. This mimics the redacting of the Authorization\nheader in HTTP.\n\nSigned-off-by: Ivan Frade \n---\n Documentation/git.txt | 5 +++--\n fetch-pack.c | 3 +++\n pkt-line.c | 40 ++++++++++++++++++++++++++++++++-\n pkt-line.h | 1 +\n t/t5702-protocol-v2.sh | 51 ++++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 97 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex d63c65e67d8..f64c8ce5183 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -832,8 +832,9 @@ for full details.\n \n `GIT_TRACE_REDACT`::\n \tBy default, when tracing is activated, Git redacts the values of\n-\tcookies, the \"Authorization:\" header, and the \"Proxy-Authorization:\"\n-\theader. Set this variable to `0` to prevent this redaction.\n+\tcookies, the \"Authorization:\" header, the \"Proxy-Authorization:\"\n+\theader and packfile URLs. Set this variable to `0` to prevent this\n+\tredaction.\n \n `GIT_LITERAL_PATHSPECS`::\n \tSetting this variable to `1` will cause Git to treat all\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex a9604f35a3e..ad8ac49ca50 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1581,6 +1581,9 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\treader.me = \"fetch-pack\";\n \t}\n \n+\tif (git_env_bool(\"GIT_TRACE_REDACT\", 1))\n+\t\treader.options |= PACKET_READ_REDACT_URL_PATH;\n+\n \twhile (state != FETCH_DONE) {\n \t\tswitch (state) {\n \t\tcase FETCH_CHECK_LOCAL:\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 2dc8ac274bd..ba0a2d65f0c 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -370,6 +370,31 @@ int packet_length(const char lenbuf_hex[4])\n \treturn (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);\n }\n \n+static char *find_url_path(const char* buffer, int *path_len)\n+{\n+\tconst char *URL_MARK = \"://\";\n+\tchar *path = strstr(buffer, URL_MARK);\n+\tif (!path)\n+\t\treturn NULL;\n+\n+\tpath += strlen(URL_MARK);\n+\twhile (*path && *path != '/')\n+\t\tpath++;\n+\n+\tif (!*path || !*(path + 1))\n+\t\treturn NULL;\n+\n+\t// position after '/'\n+\tpath++;\n+\n+\tif (path_len) {\n+\t\tchar *url_end = strchrnul(path, ' ');\n+\t\t*path_len = url_end - path;\n+\t}\n+\n+\treturn path;\n+}\n+\n enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t\t\t\t\t\tsize_t *src_len, char *buffer,\n \t\t\t\t\t\tunsigned size, int *pktlen,\n@@ -377,6 +402,8 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n {\n \tint len;\n \tchar linelen[4];\n+\tchar *url_path_start;\n+\tint url_path_len;\n \n \tif (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0) {\n \t\t*pktlen = -1;\n@@ -427,7 +454,18 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t\tlen--;\n \n \tbuffer[len] = 0;\n-\tpacket_trace(buffer, len, 0);\n+\tif (options & PACKET_READ_REDACT_URL_PATH &&\n+\t (url_path_start = find_url_path(buffer, &url_path_len))) {\n+\t\tconst char *redacted = \"\";\n+\t\tstruct strbuf tracebuf = STRBUF_INIT;\n+\t\tstrbuf_insert(&tracebuf, 0, buffer, len);\n+\t\tstrbuf_splice(&tracebuf, url_path_start - buffer,\n+\t\t\t url_path_len, redacted, strlen(redacted));\n+\t\tpacket_trace(tracebuf.buf, tracebuf.len, 0);\n+\t\tstrbuf_release(&tracebuf);\n+\t} else {\n+\t\tpacket_trace(buffer, len, 0);\n+\t}\n \n \tif ((options & PACKET_READ_DIE_ON_ERR_PACKET) &&\n \t starts_with(buffer, \"ERR \"))\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 467ae013573..a610ecb88e8 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -87,6 +87,7 @@ void packet_fflush(FILE *f);\n #define PACKET_READ_CHOMP_NEWLINE (1u<<1)\n #define PACKET_READ_DIE_ON_ERR_PACKET (1u<<2)\n #define PACKET_READ_GENTLE_ON_READ_ERROR (1u<<3)\n+#define PACKET_READ_REDACT_URL_PATH (1u<<4)\n int packet_read(int fd, char *buffer, unsigned size, int options);\n \n /*\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex d527cf6c49f..f01af2f2ed3 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -1107,6 +1107,57 @@ test_expect_success 'packfile-uri with transfer.fsckobjects fails when .gitmodul\n \ttest_i18ngrep \"disallowed submodule name\" err\n '\n \n+test_expect_success 'packfile-uri path redacted in trace' '\n+\tP=\"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n+\trm -rf \"$P\" http_child log &&\n+\n+\tgit init \"$P\" &&\n+\tgit -C \"$P\" config \"uploadpack.allowsidebandall\" \"true\" &&\n+\n+\techo my-blob >\"$P/my-blob\" &&\n+\tgit -C \"$P\" add my-blob &&\n+\tgit -C \"$P\" commit -m x &&\n+\n+\tgit -C \"$P\" hash-object my-blob >objh &&\n+\tgit -C \"$P\" pack-objects \"$HTTPD_DOCUMENT_ROOT_PATH/mypack\" packh &&\n+\tgit -C \"$P\" config --add \\\n+\t\t\"uploadpack.blobpackfileuri\" \\\n+\t\t\"$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack\" &&\n+\n+\tGIT_TRACE=1 GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TEST_SIDEBAND_ALL=1 \\\n+\tgit -c protocol.version=2 \\\n+\t\t-c fetch.uriprotocols=http,https \\\n+\t\tclone \"$HTTPD_URL/smart/http_parent\" http_child &&\n+\n+\tgrep -F \"clone< \\\\1$(cat packh) $HTTPD_URL/\" log\n+'\n+\n+test_expect_success 'packfile-uri path not redacted in trace when GIT_TRACE_REDACT=0' '\n+\tP=\"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n+\trm -rf \"$P\" http_child log &&\n+\n+\tgit init \"$P\" &&\n+\tgit -C \"$P\" config \"uploadpack.allowsidebandall\" \"true\" &&\n+\n+\techo my-blob >\"$P/my-blob\" &&\n+\tgit -C \"$P\" add my-blob &&\n+\tgit -C \"$P\" commit -m x &&\n+\n+\tgit -C \"$P\" hash-object my-blob >objh &&\n+\tgit -C \"$P\" pack-objects \"$HTTPD_DOCUMENT_ROOT_PATH/mypack\" packh &&\n+\tgit -C \"$P\" config --add \\\n+\t\t\"uploadpack.blobpackfileuri\" \\\n+\t\t\"$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack\" &&\n+\n+\tGIT_TRACE=1 GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TEST_SIDEBAND_ALL=1 \\\n+\tGIT_TRACE_REDACT=0 \\\n+\tgit -c protocol.version=2 \\\n+\t\t-c fetch.uriprotocols=http,https \\\n+\t\tclone \"$HTTPD_URL/smart/http_parent\" http_child &&\n+\n+\tgrep -F \"clone< \\\\1$(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack\" log\n+'\n+\n test_expect_success 'http:// --negotiate-only' '\n \tSERVER=\"$HTTPD_DOCUMENT_ROOT_PATH/server\" &&\n \tURI=\"$HTTPD_URL/smart/server\" &&\n-- \ngitgitgadget\n\n"},{"id":"439701","messageId":"c7f0977cabd4ba7311b8045bc57e9e30198651fd.1635288599.git.gitgitgadget@gmail.com","threadId":"56667","inReplyTo":"pull.1052.v4.git.1635288599.gitgitgadget@gmail.com","subject":"[PATCH v4 2/2] http-fetch: redact url on die() message","fromName":"Ivan Frade via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-26T22:49:59Z","receivedAt":"2021-10-26T22:50:05Z","isPatch":true,"sender":{"key":"ifrade@google.com","avatar":"https://avatars.githubusercontent.com/u/58185630?v=4"},"body":"From: Ivan Frade \n\nhttp-fetch prints the URL after failing to fetch it. This can be\nconfusing to users (they cannot really do anything with it) but even\nworse, they can share by accident a sensitive URL (e.g. with\ncredentials) while looking for help.\n\nRedact the URL unless the GIT_TRACE_REDACT variable is set to false. This\nmimics the redaction of other sensitive information in git, like the\nAuthorization header in HTTP.\n\nSigned-off-by: Ivan Frade \n---\n http-fetch.c | 15 +++++++++++++--\n 1 file changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/http-fetch.c b/http-fetch.c\nindex fa642462a9e..bbe09a6ad9f 100644\n--- a/http-fetch.c\n+++ b/http-fetch.c\n@@ -4,6 +4,7 @@\n #include \"http.h\"\n #include \"walker.h\"\n #include \"strvec.h\"\n+#include \"urlmatch.h\"\n \n static const char http_fetch_usage[] = \"git http-fetch \"\n \"[-c] [-t] [-a] [-v] [--recover] [-w ref] [--stdin | --packfile=hash | commit-id] url\";\n@@ -63,8 +64,18 @@ static void fetch_single_packfile(struct object_id *packfile_hash,\n \tif (start_active_slot(preq->slot)) {\n \t\trun_active_slot(preq->slot);\n \t\tif (results.curl_result != CURLE_OK) {\n-\t\t\tdie(\"Unable to get pack file %s\\n%s\", preq->url,\n-\t\t\t curl_errorstr);\n+\t\t\tstruct url_info url;\n+\t\t\tchar *nurl = url_normalize(preq->url, &url);\n+\t\t\tif (!git_env_bool(\"GIT_TRACE_REDACT\", 1) || !nurl) {\n+\t\t\t\tdie(\"Unable to get pack file %s\\n%s\", preq->url,\n+\t\t\t\t curl_errorstr);\n+\t\t\t} else {\n+\t\t\t\tchar *schema = xstrndup(url.url, url.scheme_len);\n+\t\t\t\tchar *host = xstrndup(&url.url[url.host_off], url.host_len);\n+\t\t\t\tdie(\"failed to get '%s' url from '%s' \"\n+\t\t\t\t \"(full URL redacted due to GIT_TRACE_REDACT setting)\\n%s\",\n+\t\t\t\t schema, host, curl_errorstr);\n+\t\t\t}\n \t\t}\n \t} else {\n \t\tdie(\"Unable to start request\");\n-- \ngitgitgadget\n"},{"id":"439853","messageId":"xmqq35omt11f.fsf@gitster.g","threadId":"56667","inReplyTo":"973a250752c39c3fe835d69f3fbe8f009fc4fa74.1635288599.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 1/2] fetch-pack: redact packfile urls in traces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-28T01:01:48Z","receivedAt":"2021-10-28T01:01:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Ivan Frade via GitGitGadget\" writes:\n\n> From: Ivan Frade \n>\n> In some setups, packfile uris act as bearer token. It is not\n> recommended to expose them plainly in logs, although in special\n> circunstances (e.g. debug) it makes sense to write them.\n>\n> Redact the packfile URL paths by default, unless the GIT_TRACE_REDACT\n> variable is set to false. This mimics the redacting of the Authorization\n> header in HTTP.\n>\n> Signed-off-by: Ivan Frade \n> ---\n> Documentation/git.txt | 5 +++--\n> fetch-pack.c | 3 +++\n> pkt-line.c | 40 ++++++++++++++++++++++++++++++++-\n> pkt-line.h | 1 +\n> t/t5702-protocol-v2.sh | 51 ++++++++++++++++++++++++++++++++++++++++++\n> 5 files changed, 97 insertions(+), 3 deletions(-)\n>\n> diff --git a/Documentation/git.txt b/Documentation/git.txt\n> index d63c65e67d8..f64c8ce5183 100644\n> --- a/Documentation/git.txt\n> +++ b/Documentation/git.txt\n> @@ -832,8 +832,9 @@ for full details.\n> \n> `GIT_TRACE_REDACT`::\n> \tBy default, when tracing is activated, Git redacts the values of\n> -\tcookies, the \"Authorization:\" header, and the \"Proxy-Authorization:\"\n> -\theader. Set this variable to `0` to prevent this redaction.\n> +\tcookies, the \"Authorization:\" header, the \"Proxy-Authorization:\"\n> +\theader and packfile URLs. Set this variable to `0` to prevent this\n> +\tredaction.\n\nJust a curiosity. Do we call these packfile URI, or packfile URL?\n\n> diff --git a/pkt-line.c b/pkt-line.c\n> index 2dc8ac274bd..ba0a2d65f0c 100644\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -370,6 +370,31 @@ int packet_length(const char lenbuf_hex[4])\n> \treturn (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);\n> }\n> \n> +static char *find_url_path(const char* buffer, int *path_len)\n> +{\n> +\tconst char *URL_MARK = \"://\";\n> +\tchar *path = strstr(buffer, URL_MARK);\n> +\tif (!path)\n> +\t\treturn NULL;\n\nHmph, the format we expect is \" \"; don't we need to\nvalidate the leading followed by SP?\n\n len = strspn(buffer, \"0123456789abcdefABCDEF\");\n if (len != 40 || len != 64 || buffer[len] != ' ')\n\treturn NULL; /* required \" SP\" not seen */\n path = strstr(buffer + len + 1, URL_MARK);\n\nor somesuch?\n\n> +\tpath += strlen(URL_MARK);\n\nOK.\n\n> +\twhile (*path && *path != '/')\n> +\t\tpath++;\n\nstrchr()?\n\n> +\tif (!*path || !*(path + 1))\n> +\t\treturn NULL;\n\nOK.\n\n> +\t// position after '/'\n\nNo // comments in our codebase, please. Unless it is a borrowed\ncode, that is.\n\n> +\tpath++;\n> +\n> +\tif (path_len) {\n> +\t\tchar *url_end = strchrnul(path, ' ');\n\nIs this because SP is not a valid character in packfile URI, or at\nthis point in the callchain it would be encoded or something? The\nformat we expect is \" \", so we shouldn't even have to\nlook for SP but just redact everything to the end, no?\n\nApparently we are assuming that there won't be more than one such\nURL-path that needs redacting in the packet, but that is perfectly\nfine, as the sole goal of this helper is to identify the packfile\nURI packet and redact it in the log.\n\n> +\t\t*path_len = url_end - path;\n> +\t}\n> +\n> +\treturn path;\n> +}\n\n> -\tpacket_trace(buffer, len, 0);\n> +\tif (options & PACKET_READ_REDACT_URL_PATH &&\n> +\t (url_path_start = find_url_path(buffer, &url_path_len))) {\n> +\t\tconst char *redacted = \"\";\n> +\t\tstruct strbuf tracebuf = STRBUF_INIT;\n> +\t\tstrbuf_insert(&tracebuf, 0, buffer, len);\n> +\t\tstrbuf_splice(&tracebuf, url_path_start - buffer,\n> +\t\t\t url_path_len, redacted, strlen(redacted));\n> +\t\tpacket_trace(tracebuf.buf, tracebuf.len, 0);\n> +\t\tstrbuf_release(&tracebuf);\n\nI briefly wondered if the repeated allocation (and more\nfundamentally, preparing the redacted copy of packet whether we are\nactually tracing the packet in the first place) is blindly wasting\nthe resources too much, but this only happens in the protocol header\npart, so it might be OK.\n\nEven if that is not the case, we should be able to update\nfetch_pack.c::do_fetch_pack_v2() so that the REDACT_URL_PATH bit is\nturned on in a much narrower region of code, right? Enable when we\nenter the GET_PACK state and drop the bit when we are done with the\npackfile URI packets, or something?\n\nThanks for working on this.\n\n\n> +\t} else {\n> +\t\tpacket_trace(buffer, len, 0);\n> +\t}\n"},{"id":"439896","messageId":"211028.86sfwlw10o.gmgdl@evledraar.gmail.com","threadId":"56667","inReplyTo":"c7f0977cabd4ba7311b8045bc57e9e30198651fd.1635288599.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 2/2] http-fetch: redact url on die() message","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-28T16:39:18Z","receivedAt":"2021-10-28T16:46:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Oct 26 2021, Ivan Frade via GitGitGadget wrote:\n\n> From: Ivan Frade \n>\n> http-fetch prints the URL after failing to fetch it. This can be\n> confusing to users (they cannot really do anything with it) but even\n> worse, they can share by accident a sensitive URL (e.g. with\n> credentials) while looking for help.\n>\n> Redact the URL unless the GIT_TRACE_REDACT variable is set to false. This\n> mimics the redaction of other sensitive information in git, like the\n> Authorization header in HTTP.\n>\n> Signed-off-by: Ivan Frade \n> ---\n> http-fetch.c | 15 +++++++++++++--\n> 1 file changed, 13 insertions(+), 2 deletions(-)\n>\n> diff --git a/http-fetch.c b/http-fetch.c\n> index fa642462a9e..bbe09a6ad9f 100644\n> --- a/http-fetch.c\n> +++ b/http-fetch.c\n> @@ -4,6 +4,7 @@\n> #include \"http.h\"\n> #include \"walker.h\"\n> #include \"strvec.h\"\n> +#include \"urlmatch.h\"\n> \n> static const char http_fetch_usage[] = \"git http-fetch \"\n> \"[-c] [-t] [-a] [-v] [--recover] [-w ref] [--stdin | --packfile=hash | commit-id] url\";\n> @@ -63,8 +64,18 @@ static void fetch_single_packfile(struct object_id *packfile_hash,\n> \tif (start_active_slot(preq->slot)) {\n> \t\trun_active_slot(preq->slot);\n> \t\tif (results.curl_result != CURLE_OK) {\n> -\t\t\tdie(\"Unable to get pack file %s\\n%s\", preq->url,\n> -\t\t\t curl_errorstr);\n> +\t\t\tstruct url_info url;\n> +\t\t\tchar *nurl = url_normalize(preq->url, &url);\n> +\t\t\tif (!git_env_bool(\"GIT_TRACE_REDACT\", 1) || !nurl) {\n> +\t\t\t\tdie(\"Unable to get pack file %s\\n%s\", preq->url,\n> +\t\t\t\t curl_errorstr);\n\nsmall nit: arrange if's from \"if (cheap || expensive)\", i.e. no need for\ngetenv() if !nurl, but maybe compilers are smart enough for that...\n\nnit: die() messages should start with lower-case (in CodingGuidelines), and I think it's better to quote both, so:\n\n die(\"unable to get pack '%s': '%s'\", ...)\n\nOr maybe without the second '%s', as in 3e8084f1884 (http: check\nCURLE_SSL_PINNEDPUBKEYNOTMATCH when emitting errors, 2021-09-24) (which\nI authored, but just copy/pasted the convention in the surrounding\ncode)>\n\n> +\t\t\t} else {\n> +\t\t\t\tchar *schema = xstrndup(url.url, url.scheme_len);\n> +\t\t\t\tchar *host = xstrndup(&url.url[url.host_off], url.host_len);\n> +\t\t\t\tdie(\"failed to get '%s' url from '%s' \"\n> +\t\t\t\t \"(full URL redacted due to GIT_TRACE_REDACT setting)\\n%s\",\n> +\t\t\t\t schema, host, curl_errorstr);\n\nHrm, I haven't tested, but aren't both of those xstrndup's redundant to\nusing %*s instead of %s for the printf format? I.e.:\n\n die(\"failed to get '%*s'[...]\", url.schema_len, url.url, )\n"},{"id":"439902","messageId":"CAPig+cTKSp28oUvESCWLB+OLBjbUSt3vhz6n3eVmkfYf9arcrg@mail.gmail.com","threadId":"56667","inReplyTo":"211028.86sfwlw10o.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v4 2/2] http-fetch: redact url on die() message","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-10-28T17:25:50Z","receivedAt":"2021-10-28T17:26:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Oct 28, 2021 at 12:46 PM Ævar Arnfjörð Bjarmason\n wrote:\n> On Tue, Oct 26 2021, Ivan Frade via GitGitGadget wrote:\n> > if (results.curl_result != CURLE_OK) {\n> > - die(\"Unable to get pack file %s\\n%s\", preq->url,\n> > - curl_errorstr);\n> > + struct url_info url;\n> > + char *nurl = url_normalize(preq->url, &url);\n> > + if (!git_env_bool(\"GIT_TRACE_REDACT\", 1) || !nurl) {\n> > + die(\"Unable to get pack file %s\\n%s\", preq->url,\n> > + curl_errorstr);\n>\n> small nit: arrange if's from \"if (cheap || expensive)\", i.e. no need for\n> getenv() if !nurl, but maybe compilers are smart enough for that...\n\nI had the same passing thought when glancing over this code (although\nthis appears to be an error patch, thus not performance critical, so\nnot terribly important).\n\n> nit: die() messages should start with lower-case (in CodingGuidelines), and I think it's better to quote both, so:\n>\n> die(\"unable to get pack '%s': '%s'\", ...)\n>\n> Or maybe without the second '%s', as in 3e8084f1884 (http: check\n> CURLE_SSL_PINNEDPUBKEYNOTMATCH when emitting errors, 2021-09-24) (which\n> I authored, but just copy/pasted the convention in the surrounding\n> code)>\n\nNote that this is not a new die() call; it just got indented as-is by\nthis patch, so the changes you suggest to the message string are\npotentially outside the scope of this patch. Possibilities: (1) make\nthe changes in this patch but mention them in the commit message; (2)\nmake the changes in a preparatory patch; (3) punt on the changes for\nnow.\n\n> > + } else {\n> > + char *schema = xstrndup(url.url, url.scheme_len);\n> > + char *host = xstrndup(&url.url[url.host_off], url.host_len);\n> > + die(\"failed to get '%s' url from '%s' \"\n> > + \"(full URL redacted due to GIT_TRACE_REDACT setting)\\n%s\",\n> > + schema, host, curl_errorstr);\n>\n> Hrm, I haven't tested, but aren't both of those xstrndup's redundant to\n> using %*s instead of %s for the printf format? I.e.:\n>\n> die(\"failed to get '%*s'[...]\", url.schema_len, url.url, )\n\nI wondered the same when reading the patch. Thanks for mentioning it.\n"},{"id":"439957","messageId":"CANQMx9WFKJAGF+7zti8+-b2je9sFuNxwOx-LCPtEoGCea54Mdw@mail.gmail.com","threadId":"56667","inReplyTo":"xmqq35omt11f.fsf@gitster.g","subject":"Re: [PATCH v4 1/2] fetch-pack: redact packfile urls in traces","fromName":"Ivan Frade","fromEmail":"ifrade@google.com","sentAt":"2021-10-28T22:15:05Z","receivedAt":"2021-10-28T22:15:19Z","isPatch":true,"sender":{"key":"ifrade@google.com","avatar":"https://avatars.githubusercontent.com/u/58185630?v=4"},"body":"On Wed, Oct 27, 2021 at 6:01 PM Junio C Hamano wrote:\n>\n> \"Ivan Frade via GitGitGadget\" writes:\n>\n> > From: Ivan Frade \n> >\n\n> Just a curiosity. Do we call these packfile URI, or packfile URL?\n\nThe feature is \"packfile URI\" (and the section is called so in the\nprotocol). I changed all \"url\" to \"uri\".\n\n> > diff --git a/pkt-line.c b/pkt-line.c\n> > index 2dc8ac274bd..ba0a2d65f0c 100644\n> > --- a/pkt-line.c\n> > +++ b/pkt-line.c\n> > @@ -370,6 +370,31 @@ int packet_length(const char lenbuf_hex[4])\n> > return (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);\n> > }\n> >\n> > +static char *find_url_path(const char* buffer, int *path_len)\n> > +{\n> > + const char *URL_MARK = \"://\";\n> > + char *path = strstr(buffer, URL_MARK);\n> > + if (!path)\n> > + return NULL;\n>\n> Hmph, the format we expect is \" \"; don't we need to\n> validate the leading followed by SP?\n\nI was trying to find a uri in a packet in general, not counting on the\npackfile-uri line format. That is probably an overgeneralization.\n\nNext patch version follows these suggestions to look for a packfile-uri line.\n\n> > + if (path_len) {\n> > + char *url_end = strchrnul(path, ' ');\n>\n> Is this because SP is not a valid character in packfile URI, or at\n> this point in the callchain it would be encoded or something? The\n> format we expect is \" \", so we shouldn't even have to\n> look for SP but just redact everything to the end, no?\n\nYes, now that we count on the packfile-uri line format, we can redact\neverything to the end and there is no need to return the length.\n\n> > - packet_trace(buffer, len, 0);\n> > + if (options & PACKET_READ_REDACT_URL_PATH &&\n> > + (url_path_start = find_url_path(buffer, &url_path_len))) {\n> > + const char *redacted = \"\";\n> > + struct strbuf tracebuf = STRBUF_INIT;\n> > + strbuf_insert(&tracebuf, 0, buffer, len);\n> > + strbuf_splice(&tracebuf, url_path_start - buffer,\n> > + url_path_len, redacted, strlen(redacted));\n> > + packet_trace(tracebuf.buf, tracebuf.len, 0);\n> > + strbuf_release(&tracebuf);\n>\n> I briefly wondered if the repeated allocation (and more\n> fundamentally, preparing the redacted copy of packet whether we are\n> actually tracing the packet in the first place) is blindly wasting\n> the resources too much, but this only happens in the protocol header\n> part, so it might be OK.\n\nWe only allocate and redact if it looks like a packfile-uri line, so\nit shouldn't happen too frequently.\n\n> Even if that is not the case, we should be able to update\n> fetch_pack.c::do_fetch_pack_v2() so that the REDACT_URL_PATH bit is\n> turned on in a much narrower region of code, right? Enable when we\n> enter the GET_PACK state and drop the bit when we are done with the\n> packfile URI packets, or something?\n\nI move the set/unset of the redacting flag to the FETCH_GET_PACK\naround the \"packfile-uris\" section.\nThere is no need to check every incoming packet for a packfile-uri\nline, we know when they should come.\n\nThanks,\n\nIvan\n"},{"id":"439962","messageId":"CANQMx9WPt93e3pNghtG01L4GqGvPgGeRNH4RCedMTV=Vcu8vWQ@mail.gmail.com","threadId":"56667","inReplyTo":"211028.86sfwlw10o.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v4 2/2] http-fetch: redact url on die() message","fromName":"Ivan Frade","fromEmail":"ifrade@google.com","sentAt":"2021-10-28T22:41:13Z","receivedAt":"2021-10-28T22:41:26Z","isPatch":true,"sender":{"key":"ifrade@google.com","avatar":"https://avatars.githubusercontent.com/u/58185630?v=4"},"body":"On Thu, Oct 28, 2021 at 9:46 AM Ævar Arnfjörð Bjarmason\n wrote:\n>\n>\n> On Tue, Oct 26 2021, Ivan Frade via GitGitGadget wrote:\n>\n> > From: Ivan Frade \n> >\n...\n> > + if (!git_env_bool(\"GIT_TRACE_REDACT\", 1) || !nurl) {\n> > + die(\"Unable to get pack file %s\\n%s\", preq->url,\n> > + curl_errorstr);\n>\n> small nit: arrange if's from \"if (cheap || expensive)\", i.e. no need for\n> getenv() if !nurl, but maybe compilers are smart enough for that...\n\nDone\n\n> nit: die() messages should start with lower-case (in CodingGuidelines), and I think it's better to quote both, so:\n>\n> die(\"unable to get pack '%s': '%s'\", ...)\n>\n> Or maybe without the second '%s', as in 3e8084f1884 (http: check\n> CURLE_SSL_PINNEDPUBKEYNOTMATCH when emitting errors, 2021-09-24) (which\n> I authored, but just copy/pasted the convention in the surrounding\n> code)>\n\nDone\n\n> > + } else {\n> > + char *schema = xstrndup(url.url, url.scheme_len);\n> > + char *host = xstrndup(&url.url[url.host_off], url.host_len);\n> > + die(\"failed to get '%s' url from '%s' \"\n> > + \"(full URL redacted due to GIT_TRACE_REDACT setting)\\n%s\",\n> > + schema, host, curl_errorstr);\n>\n> Hrm, I haven't tested, but aren't both of those xstrndup's redundant to\n> using %*s instead of %s for the printf format? I.e.:\n>\n> die(\"failed to get '%*s'[...]\", url.schema_len, url.url, )\n\nIndeed, \"%.*s\" did the trick. Thanks!\n\nIvan\n"},{"id":"439963","messageId":"CANQMx9VAgj8hLkd2+hqRHdD2+zNQ8T0jUTqpzmJE65Z2UtTnDQ@mail.gmail.com","threadId":"56667","inReplyTo":"CAPig+cTKSp28oUvESCWLB+OLBjbUSt3vhz6n3eVmkfYf9arcrg@mail.gmail.com","subject":"Re: [PATCH v4 2/2] http-fetch: redact url on die() message","fromName":"Ivan Frade","fromEmail":"ifrade@google.com","sentAt":"2021-10-28T22:44:14Z","receivedAt":"2021-10-28T22:44:29Z","isPatch":true,"sender":{"key":"ifrade@google.com","avatar":"https://avatars.githubusercontent.com/u/58185630?v=4"},"body":"On Thu, Oct 28, 2021 at 10:26 AM Eric Sunshine wrote:\n>\n> On Thu, Oct 28, 2021 at 12:46 PM Ævar Arnfjörð Bjarmason\n> wrote:\n\n>\n> > nit: die() messages should start with lower-case (in CodingGuidelines), and I think it's better to quote both, so:\n> >\n> > die(\"unable to get pack '%s': '%s'\", ...)\n> >\n> > Or maybe without the second '%s', as in 3e8084f1884 (http: check\n> > CURLE_SSL_PINNEDPUBKEYNOTMATCH when emitting errors, 2021-09-24) (which\n> > I authored, but just copy/pasted the convention in the surrounding\n> > code)>\n>\n> Note that this is not a new die() call; it just got indented as-is by\n> this patch, so the changes you suggest to the message string are\n> potentially outside the scope of this patch. Possibilities: (1) make\n> the changes in this patch but mention them in the commit message; (2)\n> make the changes in a preparatory patch; (3) punt on the changes for\n> now.\n\nI went for option (1). It is a minimal change and we are moving that\nline in this commit.\n\nIvan\n"},{"id":"439964","messageId":"xmqqa6ispy28.fsf@gitster.g","threadId":"56667","inReplyTo":"CANQMx9WFKJAGF+7zti8+-b2je9sFuNxwOx-LCPtEoGCea54Mdw@mail.gmail.com","subject":"Re: [PATCH v4 1/2] fetch-pack: redact packfile urls in traces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-28T22:46:39Z","receivedAt":"2021-10-28T22:46:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ivan Frade writes:\n\n>> Hmph, the format we expect is \" \"; don't we need to\n>> validate the leading followed by SP?\n>\n> I was trying to find a uri in a packet in general, not counting on the\n> packfile-uri line format. That is probably an overgeneralization.\n\nAh, I see. This is merely a tracing, so we might benefit from a\ngeneralized version of redactor, and from that point of view, the\nuse of strstr and stopping at the whitespace do make sort-of sense\nto me, but then we lack any attempt to redact more than one instance\nof URL in a packet, so the generalization may have quite a limited\nusefulness.\n\n> Next patch version follows these suggestions to look for a packfile-uri line.\n\nYeah, I think that is a good way to go, at least for now. When we\nwant a more general one, we can revisit it, but not now.\n\n>> > - packet_trace(buffer, len, 0);\n>> > + if (options & PACKET_READ_REDACT_URL_PATH &&\n>> > + (url_path_start = find_url_path(buffer, &url_path_len))) {\n>> > + const char *redacted = \"\";\n>> > + struct strbuf tracebuf = STRBUF_INIT;\n>> > + strbuf_insert(&tracebuf, 0, buffer, len);\n>> > + strbuf_splice(&tracebuf, url_path_start - buffer,\n>> > + url_path_len, redacted, strlen(redacted));\n>> > + packet_trace(tracebuf.buf, tracebuf.len, 0);\n>> > + strbuf_release(&tracebuf);\n>>\n>> I briefly wondered if the repeated allocation (and more\n>> fundamentally, preparing the redacted copy of packet whether we are\n>> actually tracing the packet in the first place) is blindly wasting\n>> the resources too much, but this only happens in the protocol header\n>> part, so it might be OK.\n>\n> We only allocate and redact if it looks like a packfile-uri line, so\n> it shouldn't happen too frequently.\n\nI was mostly wondering about the cost of determining \"if it looks\nlike?\". But we do this only for the protocol header part, so we\nwon't have thousands of attempts to match, I guess. Oh, or if we\nalso do this for the ref advertisement packets, then we might have\nquite a many. Hmph.\n\n> I move the set/unset of the redacting flag to the FETCH_GET_PACK\n> around the \"packfile-uris\" section.\n> There is no need to check every incoming packet for a packfile-uri\n> line, we know when they should come.\n\nYeah, that is quite a wise design decision, I would think.\n\nThanks.\n"},{"id":"439968","messageId":"pull.1052.v5.git.1635461500.gitgitgadget@gmail.com","threadId":"56667","inReplyTo":"pull.1052.v4.git.1635288599.gitgitgadget@gmail.com","subject":"[PATCH v5 0/2] fetch-pack: redact packfile urls in traces","fromName":"Ivan Frade via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-28T22:51:38Z","receivedAt":"2021-10-28T22:51:44Z","isPatch":true,"sender":{"key":"ifrade@google.com","avatar":"https://avatars.githubusercontent.com/u/58185630?v=4"},"body":"Changes since v4:\n\n * Use \"uri\" instead of \"url\"\n * Look specifically for a line with packfile-uri format (instead of for a\n URL in general)\n * Limit the redacting to the packfile-uri section in do_fetch_pack_v2\n * Use \"%.*s\" instead of duplicating parts of the string to print\n\nChanges since v3:\n\n * Enable redacting URLs for all sections\n * Redact only URL path (it was until the end of line)\n * Redact URL in die() with more friendly message\n * Update doc to mention that packfile URIs are also redacted.\n\nChanges since v2:\n\n * Redact only the path of the URL\n * Test are now strict, validating the exact line expected in the log\n\nChanges since v1:\n\n * Removed non-POSIX flags in tests\n * More accurate regex for the non-encrypted packfile line\n * Dropped documentation change\n * Dropped redacting the die message in http-fetch\n\nIvan Frade (2):\n fetch-pack: redact packfile urls in traces\n http-fetch: redact url on die() message\n\n Documentation/git.txt | 5 +++--\n fetch-pack.c | 4 ++++\n http-fetch.c | 14 ++++++++++--\n pkt-line.c | 39 +++++++++++++++++++++++++++++++-\n pkt-line.h | 1 +\n t/t5702-protocol-v2.sh | 51 ++++++++++++++++++++++++++++++++++++++++++\n 6 files changed, 109 insertions(+), 5 deletions(-)\n\n\nbase-commit: e9e5ba39a78c8f5057262d49e261b42a8660d5b9\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1052%2Fifradeo%2Fredact-packfile-uri-v5\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1052/ifradeo/redact-packfile-uri-v5\nPull-Request: https://github.com/gitgitgadget/git/pull/1052\n\nRange-diff vs v4:\n\n 1: 973a250752c ! 1: c95b3cafcd6 fetch-pack: redact packfile urls in traces\n @@ Documentation/git.txt: for full details.\n -\tcookies, the \"Authorization:\" header, and the \"Proxy-Authorization:\"\n -\theader. Set this variable to `0` to prevent this redaction.\n +\tcookies, the \"Authorization:\" header, the \"Proxy-Authorization:\"\n -+\theader and packfile URLs. Set this variable to `0` to prevent this\n ++\theader and packfile URIs. Set this variable to `0` to prevent this\n +\tredaction.\n \n `GIT_LITERAL_PATHSPECS`::\n @@ Documentation/git.txt: for full details.\n \n ## fetch-pack.c ##\n @@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n - \t\treader.me = \"fetch-pack\";\n - \t}\n + \t\t\t\treceive_wanted_refs(&reader, sought, nr_sought);\n \n -+\tif (git_env_bool(\"GIT_TRACE_REDACT\", 1))\n -+\t\treader.options |= PACKET_READ_REDACT_URL_PATH;\n -+\n - \twhile (state != FETCH_DONE) {\n - \t\tswitch (state) {\n - \t\tcase FETCH_CHECK_LOCAL:\n + \t\t\t/* get the pack(s) */\n ++\t\t\tif (git_env_bool(\"GIT_TRACE_REDACT\", 1))\n ++\t\t\t\treader.options |= PACKET_READ_REDACT_URI_PATH;\n + \t\t\tif (process_section_header(&reader, \"packfile-uris\", 1))\n + \t\t\t\treceive_packfile_uris(&reader, &packfile_uris);\n ++\t\t\treader.options &= ~PACKET_READ_REDACT_URI_PATH;\n ++\n + \t\t\tprocess_section_header(&reader, \"packfile\", 0);\n + \n + \t\t\t/*\n \n ## pkt-line.c ##\n @@ pkt-line.c: int packet_length(const char lenbuf_hex[4])\n \treturn (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);\n }\n \n -+static char *find_url_path(const char* buffer, int *path_len)\n ++static char *find_packfile_uri_path(const char *buffer)\n +{\n -+\tconst char *URL_MARK = \"://\";\n -+\tchar *path = strstr(buffer, URL_MARK);\n -+\tif (!path)\n -+\t\treturn NULL;\n ++\tconst char *URI_MARK = \"://\";\n ++\tchar *path;\n ++\tint len;\n +\n -+\tpath += strlen(URL_MARK);\n -+\twhile (*path && *path != '/')\n -+\t\tpath++;\n ++\t/* First char is sideband mark */\n ++\tbuffer += 1;\n +\n -+\tif (!*path || !*(path + 1))\n -+\t\treturn NULL;\n ++\tlen = strspn(buffer, \"0123456789abcdefABCDEF\");\n ++\tif (!(len == 40 || len == 64) || buffer[len] != ' ')\n ++\t\treturn NULL; /* required \"SP\" not seen */\n +\n -+\t// position after '/'\n -+\tpath++;\n ++\tpath = strstr(buffer + len + 1, URI_MARK);\n ++\tif (!path)\n ++\t\treturn NULL;\n +\n -+\tif (path_len) {\n -+\t\tchar *url_end = strchrnul(path, ' ');\n -+\t\t*path_len = url_end - path;\n -+\t}\n ++\tpath = strchr(path + strlen(URI_MARK), '/');\n ++\tif (!path || !*(path + 1))\n ++\t\treturn NULL;\n +\n -+\treturn path;\n ++\t/* position after '/' */\n ++\treturn ++path;\n +}\n +\n enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n @@ pkt-line.c: enum packet_read_status packet_read_with_status(int fd, char **src_b\n {\n \tint len;\n \tchar linelen[4];\n -+\tchar *url_path_start;\n -+\tint url_path_len;\n ++\tchar *uri_path_start;\n \n \tif (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0) {\n \t\t*pktlen = -1;\n @@ pkt-line.c: enum packet_read_status packet_read_with_status(int fd, char **src_b\n \n \tbuffer[len] = 0;\n -\tpacket_trace(buffer, len, 0);\n -+\tif (options & PACKET_READ_REDACT_URL_PATH &&\n -+\t (url_path_start = find_url_path(buffer, &url_path_len))) {\n ++\tif (options & PACKET_READ_REDACT_URI_PATH &&\n ++\t (uri_path_start = find_packfile_uri_path(buffer))) {\n +\t\tconst char *redacted = \"\";\n +\t\tstruct strbuf tracebuf = STRBUF_INIT;\n +\t\tstrbuf_insert(&tracebuf, 0, buffer, len);\n -+\t\tstrbuf_splice(&tracebuf, url_path_start - buffer,\n -+\t\t\t url_path_len, redacted, strlen(redacted));\n ++\t\tstrbuf_splice(&tracebuf, uri_path_start - buffer,\n ++\t\t\t strlen(uri_path_start), redacted, strlen(redacted));\n +\t\tpacket_trace(tracebuf.buf, tracebuf.len, 0);\n +\t\tstrbuf_release(&tracebuf);\n +\t} else {\n @@ pkt-line.h: void packet_fflush(FILE *f);\n #define PACKET_READ_CHOMP_NEWLINE (1u<<1)\n #define PACKET_READ_DIE_ON_ERR_PACKET (1u<<2)\n #define PACKET_READ_GENTLE_ON_READ_ERROR (1u<<3)\n -+#define PACKET_READ_REDACT_URL_PATH (1u<<4)\n ++#define PACKET_READ_REDACT_URI_PATH (1u<<4)\n int packet_read(int fd, char *buffer, unsigned size, int options);\n \n /*\n 2: c7f0977cabd ! 2: 6912a690197 http-fetch: redact url on die() message\n @@ Commit message\n http-fetch: redact url on die() message\n \n http-fetch prints the URL after failing to fetch it. This can be\n - confusing to users (they cannot really do anything with it) but even\n - worse, they can share by accident a sensitive URL (e.g. with\n - credentials) while looking for help.\n + confusing to users (they cannot really do anything with it), and they\n + can share by accident a sensitive URL (e.g. with credentials) while\n + looking for help.\n \n Redact the URL unless the GIT_TRACE_REDACT variable is set to false. This\n mimics the redaction of other sensitive information in git, like the\n Authorization header in HTTP.\n \n + Fix also capitalization of previous die() message (must start in\n + lowercase).\n +\n Signed-off-by: Ivan Frade \n \n ## http-fetch.c ##\n @@ http-fetch.c: static void fetch_single_packfile(struct object_id *packfile_hash,\n -\t\t\t curl_errorstr);\n +\t\t\tstruct url_info url;\n +\t\t\tchar *nurl = url_normalize(preq->url, &url);\n -+\t\t\tif (!git_env_bool(\"GIT_TRACE_REDACT\", 1) || !nurl) {\n -+\t\t\t\tdie(\"Unable to get pack file %s\\n%s\", preq->url,\n ++\t\t\tif (!nurl || !git_env_bool(\"GIT_TRACE_REDACT\", 1)) {\n ++\t\t\t\tdie(\"unable to get pack file '%s'\\n%s\", preq->url,\n +\t\t\t\t curl_errorstr);\n +\t\t\t} else {\n -+\t\t\t\tchar *schema = xstrndup(url.url, url.scheme_len);\n -+\t\t\t\tchar *host = xstrndup(&url.url[url.host_off], url.host_len);\n -+\t\t\t\tdie(\"failed to get '%s' url from '%s' \"\n ++\t\t\t\tdie(\"failed to get '%.*s' url from '%.*s' \"\n +\t\t\t\t \"(full URL redacted due to GIT_TRACE_REDACT setting)\\n%s\",\n -+\t\t\t\t schema, host, curl_errorstr);\n ++\t\t\t\t (int)url.scheme_len, url.url,\n ++\t\t\t\t (int)url.host_len, &url.url[url.host_off], curl_errorstr);\n +\t\t\t}\n \t\t}\n \t} else {\n\n-- \ngitgitgadget\n"},{"id":"439966","messageId":"c95b3cafcd66ce64a140b767664a8fc98eb535bf.1635461500.git.gitgitgadget@gmail.com","threadId":"56667","inReplyTo":"pull.1052.v5.git.1635461500.gitgitgadget@gmail.com","subject":"[PATCH v5 1/2] fetch-pack: redact packfile urls in traces","fromName":"Ivan Frade via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-28T22:51:39Z","receivedAt":"2021-10-28T22:51:45Z","isPatch":true,"sender":{"key":"ifrade@google.com","avatar":"https://avatars.githubusercontent.com/u/58185630?v=4"},"body":"From: Ivan Frade \n\nIn some setups, packfile uris act as bearer token. It is not\nrecommended to expose them plainly in logs, although in special\ncircunstances (e.g. debug) it makes sense to write them.\n\nRedact the packfile URL paths by default, unless the GIT_TRACE_REDACT\nvariable is set to false. This mimics the redacting of the Authorization\nheader in HTTP.\n\nSigned-off-by: Ivan Frade \n---\n Documentation/git.txt | 5 +++--\n fetch-pack.c | 4 ++++\n pkt-line.c | 39 +++++++++++++++++++++++++++++++-\n pkt-line.h | 1 +\n t/t5702-protocol-v2.sh | 51 ++++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 97 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex d63c65e67d8..c91aa2737f0 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -832,8 +832,9 @@ for full details.\n \n `GIT_TRACE_REDACT`::\n \tBy default, when tracing is activated, Git redacts the values of\n-\tcookies, the \"Authorization:\" header, and the \"Proxy-Authorization:\"\n-\theader. Set this variable to `0` to prevent this redaction.\n+\tcookies, the \"Authorization:\" header, the \"Proxy-Authorization:\"\n+\theader and packfile URIs. Set this variable to `0` to prevent this\n+\tredaction.\n \n `GIT_LITERAL_PATHSPECS`::\n \tSetting this variable to `1` will cause Git to treat all\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex a9604f35a3e..62ea90541c5 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1653,8 +1653,12 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t\treceive_wanted_refs(&reader, sought, nr_sought);\n \n \t\t\t/* get the pack(s) */\n+\t\t\tif (git_env_bool(\"GIT_TRACE_REDACT\", 1))\n+\t\t\t\treader.options |= PACKET_READ_REDACT_URI_PATH;\n \t\t\tif (process_section_header(&reader, \"packfile-uris\", 1))\n \t\t\t\treceive_packfile_uris(&reader, &packfile_uris);\n+\t\t\treader.options &= ~PACKET_READ_REDACT_URI_PATH;\n+\n \t\t\tprocess_section_header(&reader, \"packfile\", 0);\n \n \t\t\t/*\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 2dc8ac274bd..06013d2a54a 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -370,6 +370,31 @@ int packet_length(const char lenbuf_hex[4])\n \treturn (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);\n }\n \n+static char *find_packfile_uri_path(const char *buffer)\n+{\n+\tconst char *URI_MARK = \"://\";\n+\tchar *path;\n+\tint len;\n+\n+\t/* First char is sideband mark */\n+\tbuffer += 1;\n+\n+\tlen = strspn(buffer, \"0123456789abcdefABCDEF\");\n+\tif (!(len == 40 || len == 64) || buffer[len] != ' ')\n+\t\treturn NULL; /* required \"SP\" not seen */\n+\n+\tpath = strstr(buffer + len + 1, URI_MARK);\n+\tif (!path)\n+\t\treturn NULL;\n+\n+\tpath = strchr(path + strlen(URI_MARK), '/');\n+\tif (!path || !*(path + 1))\n+\t\treturn NULL;\n+\n+\t/* position after '/' */\n+\treturn ++path;\n+}\n+\n enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t\t\t\t\t\tsize_t *src_len, char *buffer,\n \t\t\t\t\t\tunsigned size, int *pktlen,\n@@ -377,6 +402,7 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n {\n \tint len;\n \tchar linelen[4];\n+\tchar *uri_path_start;\n \n \tif (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0) {\n \t\t*pktlen = -1;\n@@ -427,7 +453,18 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t\tlen--;\n \n \tbuffer[len] = 0;\n-\tpacket_trace(buffer, len, 0);\n+\tif (options & PACKET_READ_REDACT_URI_PATH &&\n+\t (uri_path_start = find_packfile_uri_path(buffer))) {\n+\t\tconst char *redacted = \"\";\n+\t\tstruct strbuf tracebuf = STRBUF_INIT;\n+\t\tstrbuf_insert(&tracebuf, 0, buffer, len);\n+\t\tstrbuf_splice(&tracebuf, uri_path_start - buffer,\n+\t\t\t strlen(uri_path_start), redacted, strlen(redacted));\n+\t\tpacket_trace(tracebuf.buf, tracebuf.len, 0);\n+\t\tstrbuf_release(&tracebuf);\n+\t} else {\n+\t\tpacket_trace(buffer, len, 0);\n+\t}\n \n \tif ((options & PACKET_READ_DIE_ON_ERR_PACKET) &&\n \t starts_with(buffer, \"ERR \"))\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 467ae013573..6d2a63db238 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -87,6 +87,7 @@ void packet_fflush(FILE *f);\n #define PACKET_READ_CHOMP_NEWLINE (1u<<1)\n #define PACKET_READ_DIE_ON_ERR_PACKET (1u<<2)\n #define PACKET_READ_GENTLE_ON_READ_ERROR (1u<<3)\n+#define PACKET_READ_REDACT_URI_PATH (1u<<4)\n int packet_read(int fd, char *buffer, unsigned size, int options);\n \n /*\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex d527cf6c49f..f01af2f2ed3 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -1107,6 +1107,57 @@ test_expect_success 'packfile-uri with transfer.fsckobjects fails when .gitmodul\n \ttest_i18ngrep \"disallowed submodule name\" err\n '\n \n+test_expect_success 'packfile-uri path redacted in trace' '\n+\tP=\"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n+\trm -rf \"$P\" http_child log &&\n+\n+\tgit init \"$P\" &&\n+\tgit -C \"$P\" config \"uploadpack.allowsidebandall\" \"true\" &&\n+\n+\techo my-blob >\"$P/my-blob\" &&\n+\tgit -C \"$P\" add my-blob &&\n+\tgit -C \"$P\" commit -m x &&\n+\n+\tgit -C \"$P\" hash-object my-blob >objh &&\n+\tgit -C \"$P\" pack-objects \"$HTTPD_DOCUMENT_ROOT_PATH/mypack\" packh &&\n+\tgit -C \"$P\" config --add \\\n+\t\t\"uploadpack.blobpackfileuri\" \\\n+\t\t\"$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack\" &&\n+\n+\tGIT_TRACE=1 GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TEST_SIDEBAND_ALL=1 \\\n+\tgit -c protocol.version=2 \\\n+\t\t-c fetch.uriprotocols=http,https \\\n+\t\tclone \"$HTTPD_URL/smart/http_parent\" http_child &&\n+\n+\tgrep -F \"clone< \\\\1$(cat packh) $HTTPD_URL/\" log\n+'\n+\n+test_expect_success 'packfile-uri path not redacted in trace when GIT_TRACE_REDACT=0' '\n+\tP=\"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n+\trm -rf \"$P\" http_child log &&\n+\n+\tgit init \"$P\" &&\n+\tgit -C \"$P\" config \"uploadpack.allowsidebandall\" \"true\" &&\n+\n+\techo my-blob >\"$P/my-blob\" &&\n+\tgit -C \"$P\" add my-blob &&\n+\tgit -C \"$P\" commit -m x &&\n+\n+\tgit -C \"$P\" hash-object my-blob >objh &&\n+\tgit -C \"$P\" pack-objects \"$HTTPD_DOCUMENT_ROOT_PATH/mypack\" packh &&\n+\tgit -C \"$P\" config --add \\\n+\t\t\"uploadpack.blobpackfileuri\" \\\n+\t\t\"$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack\" &&\n+\n+\tGIT_TRACE=1 GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TEST_SIDEBAND_ALL=1 \\\n+\tGIT_TRACE_REDACT=0 \\\n+\tgit -c protocol.version=2 \\\n+\t\t-c fetch.uriprotocols=http,https \\\n+\t\tclone \"$HTTPD_URL/smart/http_parent\" http_child &&\n+\n+\tgrep -F \"clone< \\\\1$(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack\" log\n+'\n+\n test_expect_success 'http:// --negotiate-only' '\n \tSERVER=\"$HTTPD_DOCUMENT_ROOT_PATH/server\" &&\n \tURI=\"$HTTPD_URL/smart/server\" &&\n-- \ngitgitgadget\n\n"},{"id":"439967","messageId":"6912a690197ee292c7f11fa199ed52e7a9760145.1635461500.git.gitgitgadget@gmail.com","threadId":"56667","inReplyTo":"pull.1052.v5.git.1635461500.gitgitgadget@gmail.com","subject":"[PATCH v5 2/2] http-fetch: redact url on die() message","fromName":"Ivan Frade via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-28T22:51:40Z","receivedAt":"2021-10-28T22:51:46Z","isPatch":true,"sender":{"key":"ifrade@google.com","avatar":"https://avatars.githubusercontent.com/u/58185630?v=4"},"body":"From: Ivan Frade \n\nhttp-fetch prints the URL after failing to fetch it. This can be\nconfusing to users (they cannot really do anything with it), and they\ncan share by accident a sensitive URL (e.g. with credentials) while\nlooking for help.\n\nRedact the URL unless the GIT_TRACE_REDACT variable is set to false. This\nmimics the redaction of other sensitive information in git, like the\nAuthorization header in HTTP.\n\nFix also capitalization of previous die() message (must start in\nlowercase).\n\nSigned-off-by: Ivan Frade \n---\n http-fetch.c | 14 ++++++++++++--\n 1 file changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/http-fetch.c b/http-fetch.c\nindex fa642462a9e..c7c7d391ac5 100644\n--- a/http-fetch.c\n+++ b/http-fetch.c\n@@ -4,6 +4,7 @@\n #include \"http.h\"\n #include \"walker.h\"\n #include \"strvec.h\"\n+#include \"urlmatch.h\"\n \n static const char http_fetch_usage[] = \"git http-fetch \"\n \"[-c] [-t] [-a] [-v] [--recover] [-w ref] [--stdin | --packfile=hash | commit-id] url\";\n@@ -63,8 +64,17 @@ static void fetch_single_packfile(struct object_id *packfile_hash,\n \tif (start_active_slot(preq->slot)) {\n \t\trun_active_slot(preq->slot);\n \t\tif (results.curl_result != CURLE_OK) {\n-\t\t\tdie(\"Unable to get pack file %s\\n%s\", preq->url,\n-\t\t\t curl_errorstr);\n+\t\t\tstruct url_info url;\n+\t\t\tchar *nurl = url_normalize(preq->url, &url);\n+\t\t\tif (!nurl || !git_env_bool(\"GIT_TRACE_REDACT\", 1)) {\n+\t\t\t\tdie(\"unable to get pack file '%s'\\n%s\", preq->url,\n+\t\t\t\t curl_errorstr);\n+\t\t\t} else {\n+\t\t\t\tdie(\"failed to get '%.*s' url from '%.*s' \"\n+\t\t\t\t \"(full URL redacted due to GIT_TRACE_REDACT setting)\\n%s\",\n+\t\t\t\t (int)url.scheme_len, url.url,\n+\t\t\t\t (int)url.host_len, &url.url[url.host_off], curl_errorstr);\n+\t\t\t}\n \t\t}\n \t} else {\n \t\tdie(\"Unable to start request\");\n-- \ngitgitgadget\n"},{"id":"439973","messageId":"xmqqa6isohve.fsf@gitster.g","threadId":"56667","inReplyTo":"c95b3cafcd66ce64a140b767664a8fc98eb535bf.1635461500.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v5 1/2] fetch-pack: redact packfile urls in traces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-28T23:21:41Z","receivedAt":"2021-10-28T23:25:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Ivan Frade via GitGitGadget\" writes:\n\n> diff --git a/pkt-line.c b/pkt-line.c\n> index 2dc8ac274bd..06013d2a54a 100644\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -370,6 +370,31 @@ int packet_length(const char lenbuf_hex[4])\n> \treturn (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);\n> }\n> \n> +static char *find_packfile_uri_path(const char *buffer)\n> +{\n> +\tconst char *URI_MARK = \"://\";\n> +\tchar *path;\n> +\tint len;\n> +\n> +\t/* First char is sideband mark */\n> +\tbuffer += 1;\n> +\n> +\tlen = strspn(buffer, \"0123456789abcdefABCDEF\");\n> +\tif (!(len == 40 || len == 64) || buffer[len] != ' ')\n> +\t\treturn NULL; /* required \"SP\" not seen */\n\nPeople may have comments on hardcoded 40/64 here and offer a better\nway to write it ;-)\n\n> +\tpath = strstr(buffer + len + 1, URI_MARK);\n> +\tif (!path)\n> +\t\treturn NULL;\n> +\n> +\tpath = strchr(path + strlen(URI_MARK), '/');\n> +\tif (!path || !*(path + 1))\n> +\t\treturn NULL;\n> +\n> +\t/* position after '/' */\n> +\treturn ++path;\n> +}\n\nOther than that, the patch this round looks quite clean.\n\nNicely done.\n\nThanks, will queue.\n"},{"id":"440038","messageId":"CANQMx9VXRnLjMgvYM63tq8aecvWNd-0cxi+XMSkkwm-iUeX+1g@mail.gmail.com","threadId":"56667","inReplyTo":"xmqqa6isohve.fsf@gitster.g","subject":"Re: [PATCH v5 1/2] fetch-pack: redact packfile urls in traces","fromName":"Ivan Frade","fromEmail":"ifrade@google.com","sentAt":"2021-10-29T18:42:36Z","receivedAt":"2021-10-29T18:42:50Z","isPatch":true,"sender":{"key":"ifrade@google.com","avatar":"https://avatars.githubusercontent.com/u/58185630?v=4"},"body":"On Thu, Oct 28, 2021 at 4:21 PM Junio C Hamano wrote:\n>\n> \"Ivan Frade via GitGitGadget\" writes:\n>\n\n> > + len = strspn(buffer, \"0123456789abcdefABCDEF\");\n> > + if (!(len == 40 || len == 64) || buffer[len] != ' ')\n> > + return NULL; /* required \"SP\" not seen */\n>\n> People may have comments on hardcoded 40/64 here and offer a better\n> way to write it ;-)\n\nLatest version uses the_hash_algo->hexsz:\n\n+ if (len != (int)the_hash_algo->hexsz || buffer[len] != ' ')\n+ return NULL; /* required \"SP\" not seen */\n\nThanks!\n"},{"id":"440039","messageId":"pull.1052.v6.git.1635532975.gitgitgadget@gmail.com","threadId":"56667","inReplyTo":"pull.1052.v5.git.1635461500.gitgitgadget@gmail.com","subject":"[PATCH v6 0/2] fetch-pack: redact packfile urls in traces","fromName":"Ivan Frade via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-29T18:42:53Z","receivedAt":"2021-10-29T18:42:59Z","isPatch":true,"sender":{"key":"ifrade@google.com","avatar":"https://avatars.githubusercontent.com/u/58185630?v=4"},"body":"Changes since v5:\n\n * Use hexsz instead of hardcoded hash sizes\n\nChanges since v4:\n\n * Use \"uri\" instead of \"url\"\n * Look specifically for a line with packfile-uri format (instead of for a\n URL in general)\n * Limit the redacting to the packfile-uri section in do_fetch_pack_v2\n * Use \"%.*s\" instead of duplicating parts of the string to print\n\nChanges since v3:\n\n * Enable redacting URLs for all sections\n * Redact only URL path (it was until the end of line)\n * Redact URL in die() with more friendly message\n * Update doc to mention that packfile URIs are also redacted.\n\nChanges since v2:\n\n * Redact only the path of the URL\n * Test are now strict, validating the exact line expected in the log\n\nChanges since v1:\n\n * Removed non-POSIX flags in tests\n * More accurate regex for the non-encrypted packfile line\n * Dropped documentation change\n * Dropped redacting the die message in http-fetch\n\nIvan Frade (2):\n fetch-pack: redact packfile urls in traces\n http-fetch: redact url on die() message\n\n Documentation/git.txt | 5 +++--\n fetch-pack.c | 4 ++++\n http-fetch.c | 14 ++++++++++--\n pkt-line.c | 39 +++++++++++++++++++++++++++++++-\n pkt-line.h | 1 +\n t/t5702-protocol-v2.sh | 51 ++++++++++++++++++++++++++++++++++++++++++\n 6 files changed, 109 insertions(+), 5 deletions(-)\n\n\nbase-commit: e9e5ba39a78c8f5057262d49e261b42a8660d5b9\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1052%2Fifradeo%2Fredact-packfile-uri-v6\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1052/ifradeo/redact-packfile-uri-v6\nPull-Request: https://github.com/gitgitgadget/git/pull/1052\n\nRange-diff vs v5:\n\n 1: c95b3cafcd6 ! 1: a6098f98946 fetch-pack: redact packfile urls in traces\n @@ pkt-line.c: int packet_length(const char lenbuf_hex[4])\n +\tbuffer += 1;\n +\n +\tlen = strspn(buffer, \"0123456789abcdefABCDEF\");\n -+\tif (!(len == 40 || len == 64) || buffer[len] != ' ')\n ++\tif (len != (int)the_hash_algo->hexsz || buffer[len] != ' ')\n +\t\treturn NULL; /* required \"SP\" not seen */\n +\n +\tpath = strstr(buffer + len + 1, URI_MARK);\n 2: 6912a690197 = 2: 38859ae7b7d http-fetch: redact url on die() message\n\n-- \ngitgitgadget\n"},{"id":"440040","messageId":"a6098f98946bd9cc1186ab9c83d917566c78b805.1635532975.git.gitgitgadget@gmail.com","threadId":"56667","inReplyTo":"pull.1052.v6.git.1635532975.gitgitgadget@gmail.com","subject":"[PATCH v6 1/2] fetch-pack: redact packfile urls in traces","fromName":"Ivan Frade via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-29T18:42:54Z","receivedAt":"2021-10-29T18:43:00Z","isPatch":true,"sender":{"key":"ifrade@google.com","avatar":"https://avatars.githubusercontent.com/u/58185630?v=4"},"body":"From: Ivan Frade \n\nIn some setups, packfile uris act as bearer token. It is not\nrecommended to expose them plainly in logs, although in special\ncircunstances (e.g. debug) it makes sense to write them.\n\nRedact the packfile URL paths by default, unless the GIT_TRACE_REDACT\nvariable is set to false. This mimics the redacting of the Authorization\nheader in HTTP.\n\nSigned-off-by: Ivan Frade \n---\n Documentation/git.txt | 5 +++--\n fetch-pack.c | 4 ++++\n pkt-line.c | 39 +++++++++++++++++++++++++++++++-\n pkt-line.h | 1 +\n t/t5702-protocol-v2.sh | 51 ++++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 97 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex d63c65e67d8..c91aa2737f0 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -832,8 +832,9 @@ for full details.\n \n `GIT_TRACE_REDACT`::\n \tBy default, when tracing is activated, Git redacts the values of\n-\tcookies, the \"Authorization:\" header, and the \"Proxy-Authorization:\"\n-\theader. Set this variable to `0` to prevent this redaction.\n+\tcookies, the \"Authorization:\" header, the \"Proxy-Authorization:\"\n+\theader and packfile URIs. Set this variable to `0` to prevent this\n+\tredaction.\n \n `GIT_LITERAL_PATHSPECS`::\n \tSetting this variable to `1` will cause Git to treat all\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex a9604f35a3e..62ea90541c5 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1653,8 +1653,12 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t\treceive_wanted_refs(&reader, sought, nr_sought);\n \n \t\t\t/* get the pack(s) */\n+\t\t\tif (git_env_bool(\"GIT_TRACE_REDACT\", 1))\n+\t\t\t\treader.options |= PACKET_READ_REDACT_URI_PATH;\n \t\t\tif (process_section_header(&reader, \"packfile-uris\", 1))\n \t\t\t\treceive_packfile_uris(&reader, &packfile_uris);\n+\t\t\treader.options &= ~PACKET_READ_REDACT_URI_PATH;\n+\n \t\t\tprocess_section_header(&reader, \"packfile\", 0);\n \n \t\t\t/*\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 2dc8ac274bd..5a69ddc2e77 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -370,6 +370,31 @@ int packet_length(const char lenbuf_hex[4])\n \treturn (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);\n }\n \n+static char *find_packfile_uri_path(const char *buffer)\n+{\n+\tconst char *URI_MARK = \"://\";\n+\tchar *path;\n+\tint len;\n+\n+\t/* First char is sideband mark */\n+\tbuffer += 1;\n+\n+\tlen = strspn(buffer, \"0123456789abcdefABCDEF\");\n+\tif (len != (int)the_hash_algo->hexsz || buffer[len] != ' ')\n+\t\treturn NULL; /* required \"SP\" not seen */\n+\n+\tpath = strstr(buffer + len + 1, URI_MARK);\n+\tif (!path)\n+\t\treturn NULL;\n+\n+\tpath = strchr(path + strlen(URI_MARK), '/');\n+\tif (!path || !*(path + 1))\n+\t\treturn NULL;\n+\n+\t/* position after '/' */\n+\treturn ++path;\n+}\n+\n enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t\t\t\t\t\tsize_t *src_len, char *buffer,\n \t\t\t\t\t\tunsigned size, int *pktlen,\n@@ -377,6 +402,7 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n {\n \tint len;\n \tchar linelen[4];\n+\tchar *uri_path_start;\n \n \tif (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0) {\n \t\t*pktlen = -1;\n@@ -427,7 +453,18 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t\tlen--;\n \n \tbuffer[len] = 0;\n-\tpacket_trace(buffer, len, 0);\n+\tif (options & PACKET_READ_REDACT_URI_PATH &&\n+\t (uri_path_start = find_packfile_uri_path(buffer))) {\n+\t\tconst char *redacted = \"\";\n+\t\tstruct strbuf tracebuf = STRBUF_INIT;\n+\t\tstrbuf_insert(&tracebuf, 0, buffer, len);\n+\t\tstrbuf_splice(&tracebuf, uri_path_start - buffer,\n+\t\t\t strlen(uri_path_start), redacted, strlen(redacted));\n+\t\tpacket_trace(tracebuf.buf, tracebuf.len, 0);\n+\t\tstrbuf_release(&tracebuf);\n+\t} else {\n+\t\tpacket_trace(buffer, len, 0);\n+\t}\n \n \tif ((options & PACKET_READ_DIE_ON_ERR_PACKET) &&\n \t starts_with(buffer, \"ERR \"))\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 467ae013573..6d2a63db238 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -87,6 +87,7 @@ void packet_fflush(FILE *f);\n #define PACKET_READ_CHOMP_NEWLINE (1u<<1)\n #define PACKET_READ_DIE_ON_ERR_PACKET (1u<<2)\n #define PACKET_READ_GENTLE_ON_READ_ERROR (1u<<3)\n+#define PACKET_READ_REDACT_URI_PATH (1u<<4)\n int packet_read(int fd, char *buffer, unsigned size, int options);\n \n /*\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex d527cf6c49f..f01af2f2ed3 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -1107,6 +1107,57 @@ test_expect_success 'packfile-uri with transfer.fsckobjects fails when .gitmodul\n \ttest_i18ngrep \"disallowed submodule name\" err\n '\n \n+test_expect_success 'packfile-uri path redacted in trace' '\n+\tP=\"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n+\trm -rf \"$P\" http_child log &&\n+\n+\tgit init \"$P\" &&\n+\tgit -C \"$P\" config \"uploadpack.allowsidebandall\" \"true\" &&\n+\n+\techo my-blob >\"$P/my-blob\" &&\n+\tgit -C \"$P\" add my-blob &&\n+\tgit -C \"$P\" commit -m x &&\n+\n+\tgit -C \"$P\" hash-object my-blob >objh &&\n+\tgit -C \"$P\" pack-objects \"$HTTPD_DOCUMENT_ROOT_PATH/mypack\" packh &&\n+\tgit -C \"$P\" config --add \\\n+\t\t\"uploadpack.blobpackfileuri\" \\\n+\t\t\"$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack\" &&\n+\n+\tGIT_TRACE=1 GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TEST_SIDEBAND_ALL=1 \\\n+\tgit -c protocol.version=2 \\\n+\t\t-c fetch.uriprotocols=http,https \\\n+\t\tclone \"$HTTPD_URL/smart/http_parent\" http_child &&\n+\n+\tgrep -F \"clone< \\\\1$(cat packh) $HTTPD_URL/\" log\n+'\n+\n+test_expect_success 'packfile-uri path not redacted in trace when GIT_TRACE_REDACT=0' '\n+\tP=\"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n+\trm -rf \"$P\" http_child log &&\n+\n+\tgit init \"$P\" &&\n+\tgit -C \"$P\" config \"uploadpack.allowsidebandall\" \"true\" &&\n+\n+\techo my-blob >\"$P/my-blob\" &&\n+\tgit -C \"$P\" add my-blob &&\n+\tgit -C \"$P\" commit -m x &&\n+\n+\tgit -C \"$P\" hash-object my-blob >objh &&\n+\tgit -C \"$P\" pack-objects \"$HTTPD_DOCUMENT_ROOT_PATH/mypack\" packh &&\n+\tgit -C \"$P\" config --add \\\n+\t\t\"uploadpack.blobpackfileuri\" \\\n+\t\t\"$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack\" &&\n+\n+\tGIT_TRACE=1 GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TEST_SIDEBAND_ALL=1 \\\n+\tGIT_TRACE_REDACT=0 \\\n+\tgit -c protocol.version=2 \\\n+\t\t-c fetch.uriprotocols=http,https \\\n+\t\tclone \"$HTTPD_URL/smart/http_parent\" http_child &&\n+\n+\tgrep -F \"clone< \\\\1$(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack\" log\n+'\n+\n test_expect_success 'http:// --negotiate-only' '\n \tSERVER=\"$HTTPD_DOCUMENT_ROOT_PATH/server\" &&\n \tURI=\"$HTTPD_URL/smart/server\" &&\n-- \ngitgitgadget\n\n"},{"id":"440041","messageId":"38859ae7b7de0f6406180a0427b9ce07fe3b9aa3.1635532975.git.gitgitgadget@gmail.com","threadId":"56667","inReplyTo":"pull.1052.v6.git.1635532975.gitgitgadget@gmail.com","subject":"[PATCH v6 2/2] http-fetch: redact url on die() message","fromName":"Ivan Frade via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-29T18:42:55Z","receivedAt":"2021-10-29T18:43:04Z","isPatch":true,"sender":{"key":"ifrade@google.com","avatar":"https://avatars.githubusercontent.com/u/58185630?v=4"},"body":"From: Ivan Frade \n\nhttp-fetch prints the URL after failing to fetch it. This can be\nconfusing to users (they cannot really do anything with it), and they\ncan share by accident a sensitive URL (e.g. with credentials) while\nlooking for help.\n\nRedact the URL unless the GIT_TRACE_REDACT variable is set to false. This\nmimics the redaction of other sensitive information in git, like the\nAuthorization header in HTTP.\n\nFix also capitalization of previous die() message (must start in\nlowercase).\n\nSigned-off-by: Ivan Frade \n---\n http-fetch.c | 14 ++++++++++++--\n 1 file changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/http-fetch.c b/http-fetch.c\nindex fa642462a9e..c7c7d391ac5 100644\n--- a/http-fetch.c\n+++ b/http-fetch.c\n@@ -4,6 +4,7 @@\n #include \"http.h\"\n #include \"walker.h\"\n #include \"strvec.h\"\n+#include \"urlmatch.h\"\n \n static const char http_fetch_usage[] = \"git http-fetch \"\n \"[-c] [-t] [-a] [-v] [--recover] [-w ref] [--stdin | --packfile=hash | commit-id] url\";\n@@ -63,8 +64,17 @@ static void fetch_single_packfile(struct object_id *packfile_hash,\n \tif (start_active_slot(preq->slot)) {\n \t\trun_active_slot(preq->slot);\n \t\tif (results.curl_result != CURLE_OK) {\n-\t\t\tdie(\"Unable to get pack file %s\\n%s\", preq->url,\n-\t\t\t curl_errorstr);\n+\t\t\tstruct url_info url;\n+\t\t\tchar *nurl = url_normalize(preq->url, &url);\n+\t\t\tif (!nurl || !git_env_bool(\"GIT_TRACE_REDACT\", 1)) {\n+\t\t\t\tdie(\"unable to get pack file '%s'\\n%s\", preq->url,\n+\t\t\t\t curl_errorstr);\n+\t\t\t} else {\n+\t\t\t\tdie(\"failed to get '%.*s' url from '%.*s' \"\n+\t\t\t\t \"(full URL redacted due to GIT_TRACE_REDACT setting)\\n%s\",\n+\t\t\t\t (int)url.scheme_len, url.url,\n+\t\t\t\t (int)url.host_len, &url.url[url.host_off], curl_errorstr);\n+\t\t\t}\n \t\t}\n \t} else {\n \t\tdie(\"Unable to start request\");\n-- \ngitgitgadget\n"},{"id":"440054","messageId":"xmqqbl37li0o.fsf@gitster.g","threadId":"56667","inReplyTo":"CANQMx9VXRnLjMgvYM63tq8aecvWNd-0cxi+XMSkkwm-iUeX+1g@mail.gmail.com","subject":"Re: [PATCH v5 1/2] fetch-pack: redact packfile urls in traces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-29T19:59:03Z","receivedAt":"2021-10-29T19:59:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ivan Frade writes:\n\n> On Thu, Oct 28, 2021 at 4:21 PM Junio C Hamano wrote:\n>>\n>> \"Ivan Frade via GitGitGadget\" writes:\n>>\n>\n>> > + len = strspn(buffer, \"0123456789abcdefABCDEF\");\n>> > + if (!(len == 40 || len == 64) || buffer[len] != ' ')\n>> > + return NULL; /* required \"SP\" not seen */\n>>\n>> People may have comments on hardcoded 40/64 here and offer a better\n>> way to write it ;-)\n>\n> Latest version uses the_hash_algo->hexsz:\n>\n> + if (len != (int)the_hash_algo->hexsz || buffer[len] != ' ')\n> + return NULL; /* required \"SP\" not seen */\n>\n> Thanks!\n\nOK. If the is given by this side (as opposed to \"you started\nto talk to a remote, and it turns out that you are still talking\nSHA-1 but the other side talks SHA-256 and their size that is\n64 does not match your 40\" case), then checking against\nthe_hash_algo->hexsz should be sufficient. The original suggestion\nwas tried both because I didn't know where originates, and we\nwould want to redact even in such a hash type mismatch case.\n\nThanks. Will take a look at the updated one.\n"},{"id":"440110","messageId":"xmqqpmrnfmiv.fsf@gitster.g","threadId":"56667","inReplyTo":"211028.86sfwlw10o.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v4 2/2] http-fetch: redact url on die() message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-29T23:18:16Z","receivedAt":"2021-10-29T23:18:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>> +\t\t\tif (!git_env_bool(\"GIT_TRACE_REDACT\", 1) || !nurl) {\n>> +\t\t\t\tdie(\"Unable to get pack file %s\\n%s\", preq->url,\n>> +\t\t\t\t curl_errorstr);\n>\n> small nit: arrange if's from \"if (cheap || expensive)\", i.e. no need for\n> getenv() if !nurl, but maybe compilers are smart enough for that...\n\nThey typically do not see what happens inside git_env_bool() while\ncompling this compilation unit, and cannot tell if the programmer\nwanted to call it first for its side effects, hence they cannot\nswap them safely.\n\n"},{"id":"440695","messageId":"20211108224335.569596-1-jonathantanmy@google.com","threadId":"56667","inReplyTo":"xmqqbl37li0o.fsf@gitster.g","subject":"Re: [PATCH v5 1/2] fetch-pack: redact packfile urls in traces","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-11-08T22:43:35Z","receivedAt":"2021-11-08T22:43:41Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> OK. If the is given by this side (as opposed to \"you started\n> to talk to a remote, and it turns out that you are still talking\n> SHA-1 but the other side talks SHA-256 and their size that is\n> 64 does not match your 40\" case), then checking against\n> the_hash_algo->hexsz should be sufficient. The original suggestion\n> was tried both because I didn't know where originates, and we\n> would want to redact even in such a hash type mismatch case.\n> \n> Thanks. Will take a look at the updated one.\n\nThis is when reading from the remote, so comes from the other\nside. I don't think that the remote sending the wrong hash size (and\nthen needing to redact) is a big concern, but there is definitely no\nharm in checking for both (and commenting that these are the SHA-1 and\nSHA-256 hash sizes).\n"},{"id":"440697","messageId":"20211108230111.1101434-1-jonathantanmy@google.com","threadId":"56667","inReplyTo":"a6098f98946bd9cc1186ab9c83d917566c78b805.1635532975.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v6 1/2] fetch-pack: redact packfile urls in traces","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-11-08T23:01:11Z","receivedAt":"2021-11-08T23:01:16Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> diff --git a/fetch-pack.c b/fetch-pack.c\n> index a9604f35a3e..62ea90541c5 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -1653,8 +1653,12 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n> \t\t\t\treceive_wanted_refs(&reader, sought, nr_sought);\n> \n> \t\t\t/* get the pack(s) */\n> +\t\t\tif (git_env_bool(\"GIT_TRACE_REDACT\", 1))\n> +\t\t\t\treader.options |= PACKET_READ_REDACT_URI_PATH;\n> \t\t\tif (process_section_header(&reader, \"packfile-uris\", 1))\n> \t\t\t\treceive_packfile_uris(&reader, &packfile_uris);\n> +\t\t\treader.options &= ~PACKET_READ_REDACT_URI_PATH;\n\nProbably worth commenting why you're resetting the flag (avoid the\nrelatively expensive URI check when we don't need it).\n\n> diff --git a/pkt-line.c b/pkt-line.c\n> index 2dc8ac274bd..5a69ddc2e77 100644\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -370,6 +370,31 @@ int packet_length(const char lenbuf_hex[4])\n> \treturn (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);\n> }\n> \n> +static char *find_packfile_uri_path(const char *buffer)\n> +{\n> +\tconst char *URI_MARK = \"://\";\n> +\tchar *path;\n> +\tint len;\n> +\n> +\t/* First char is sideband mark */\n> +\tbuffer += 1;\n> +\n> +\tlen = strspn(buffer, \"0123456789abcdefABCDEF\");\n> +\tif (len != (int)the_hash_algo->hexsz || buffer[len] != ' ')\n> +\t\treturn NULL; /* required \"SP\" not seen */\n\nOptional: As I said in my reply (just sent out), checking for both SHA-1\nand SHA-256 lengths is reasonable too.\n\n[1] https://lore.kernel.org/git/20211108224335.569596-1-jonathantanmy@google.com/\n\n> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\n> index d527cf6c49f..f01af2f2ed3 100755\n> --- a/t/t5702-protocol-v2.sh\n> +++ b/t/t5702-protocol-v2.sh\n> @@ -1107,6 +1107,57 @@ test_expect_success 'packfile-uri with transfer.fsckobjects fails when .gitmodul\n> \ttest_i18ngrep \"disallowed submodule name\" err\n> '\n> \n> +test_expect_success 'packfile-uri path redacted in trace' '\n> +\tP=\"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n> +\trm -rf \"$P\" http_child log &&\n> +\n> +\tgit init \"$P\" &&\n> +\tgit -C \"$P\" config \"uploadpack.allowsidebandall\" \"true\" &&\n> +\n> +\techo my-blob >\"$P/my-blob\" &&\n> +\tgit -C \"$P\" add my-blob &&\n> +\tgit -C \"$P\" commit -m x &&\n> +\n> +\tgit -C \"$P\" hash-object my-blob >objh &&\n> +\tgit -C \"$P\" pack-objects \"$HTTPD_DOCUMENT_ROOT_PATH/mypack\" packh &&\n> +\tgit -C \"$P\" config --add \\\n> +\t\t\"uploadpack.blobpackfileuri\" \\\n> +\t\t\"$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack\" &&\n> +\n> +\tGIT_TRACE=1 GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TEST_SIDEBAND_ALL=1 \\\n\nNo need for GIT_TRACE=1 since you're not checking stdout. Also I don't\nthink GIT_TEST_SIDEBAND_ALL=1 is needed - we should check that it works\neven without a test variable (and I've checked and it seems to work).\n\n[snip]\n\n> +test_expect_success 'packfile-uri path not redacted in trace when GIT_TRACE_REDACT=0' '\n> +\tP=\"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n> +\trm -rf \"$P\" http_child log &&\n> +\n> +\tgit init \"$P\" &&\n> +\tgit -C \"$P\" config \"uploadpack.allowsidebandall\" \"true\" &&\n> +\n> +\techo my-blob >\"$P/my-blob\" &&\n> +\tgit -C \"$P\" add my-blob &&\n> +\tgit -C \"$P\" commit -m x &&\n> +\n> +\tgit -C \"$P\" hash-object my-blob >objh &&\n> +\tgit -C \"$P\" pack-objects \"$HTTPD_DOCUMENT_ROOT_PATH/mypack\" packh &&\n> +\tgit -C \"$P\" config --add \\\n> +\t\t\"uploadpack.blobpackfileuri\" \\\n> +\t\t\"$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack\" &&\n> +\n> +\tGIT_TRACE=1 GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TEST_SIDEBAND_ALL=1 \\\n\nSame comment here.\n"},{"id":"440699","messageId":"20211108230612.1102476-1-jonathantanmy@google.com","threadId":"56667","inReplyTo":"38859ae7b7de0f6406180a0427b9ce07fe3b9aa3.1635532975.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v6 2/2] http-fetch: redact url on die() message","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-11-08T23:06:12Z","receivedAt":"2021-11-08T23:06:17Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> @@ -63,8 +64,17 @@ static void fetch_single_packfile(struct object_id *packfile_hash,\n> \tif (start_active_slot(preq->slot)) {\n> \t\trun_active_slot(preq->slot);\n> \t\tif (results.curl_result != CURLE_OK) {\n> -\t\t\tdie(\"Unable to get pack file %s\\n%s\", preq->url,\n> -\t\t\t curl_errorstr);\n> +\t\t\tstruct url_info url;\n> +\t\t\tchar *nurl = url_normalize(preq->url, &url);\n> +\t\t\tif (!nurl || !git_env_bool(\"GIT_TRACE_REDACT\", 1)) {\n> +\t\t\t\tdie(\"unable to get pack file '%s'\\n%s\", preq->url,\n> +\t\t\t\t curl_errorstr);\n> +\t\t\t} else {\n> +\t\t\t\tdie(\"failed to get '%.*s' url from '%.*s' \"\n> +\t\t\t\t \"(full URL redacted due to GIT_TRACE_REDACT setting)\\n%s\",\n> +\t\t\t\t (int)url.scheme_len, url.url,\n> +\t\t\t\t (int)url.host_len, &url.url[url.host_off], curl_errorstr);\n> +\t\t\t}\n\nI was confused why nurl was set but never used in \"else\", but I see that\nit's because url_normalize() also sets that value in the urlinfo struct.\nThis patch looks good (and patch 1 too, with my suggested changes).\n"},{"id":"440716","messageId":"211109.86mtmedrhr.gmgdl@evledraar.gmail.com","threadId":"56667","inReplyTo":"20211108230111.1101434-1-jonathantanmy@google.com","subject":"Re: [PATCH v6 1/2] fetch-pack: redact packfile urls in traces","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-09T01:36:58Z","receivedAt":"2021-11-09T01:53:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 08 2021, Jonathan Tan wrote:\n\n>> diff --git a/fetch-pack.c b/fetch-pack.c\n>> index a9604f35a3e..62ea90541c5 100644\n>> --- a/fetch-pack.c\n>> +++ b/fetch-pack.c\n>> @@ -1653,8 +1653,12 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n>> \t\t\t\treceive_wanted_refs(&reader, sought, nr_sought);\n>> \n>> \t\t\t/* get the pack(s) */\n>> +\t\t\tif (git_env_bool(\"GIT_TRACE_REDACT\", 1))\n>> +\t\t\t\treader.options |= PACKET_READ_REDACT_URI_PATH;\n>> \t\t\tif (process_section_header(&reader, \"packfile-uris\", 1))\n>> \t\t\t\treceive_packfile_uris(&reader, &packfile_uris);\n>> +\t\t\treader.options &= ~PACKET_READ_REDACT_URI_PATH;\n>\n> Probably worth commenting why you're resetting the flag (avoid the\n> relatively expensive URI check when we don't need it).\n\n...yeah...\n\n>> diff --git a/pkt-line.c b/pkt-line.c\n>> index 2dc8ac274bd..5a69ddc2e77 100644\n>> --- a/pkt-line.c\n>> +++ b/pkt-line.c\n>> @@ -370,6 +370,31 @@ int packet_length(const char lenbuf_hex[4])\n>> \treturn (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);\n>> }\n>> \n>> +static char *find_packfile_uri_path(const char *buffer)\n>> +{\n>> +\tconst char *URI_MARK = \"://\";\n>> +\tchar *path;\n>> +\tint len;\n>> +\n>> +\t/* First char is sideband mark */\n>> +\tbuffer += 1;\n>> +\n>> +\tlen = strspn(buffer, \"0123456789abcdefABCDEF\");\n>> +\tif (len != (int)the_hash_algo->hexsz || buffer[len] != ' ')\n>> +\t\treturn NULL; /* required \"SP\" not seen */\n>\n> Optional: As I said in my reply (just sent out), checking for both SHA-1\n> and SHA-256 lengths is reasonable too.\n>\n> [1] https://lore.kernel.org/git/20211108224335.569596-1-jonathantanmy@google.com/\n\nCorrect me if I'm wrong, but I find it really strange that we're trying\nto parse things in pkt-line.c like this.\n\nWe'll only get these from a client in code that's invoked in\nfetch-pack.c, specifically the process_section_header() quoted above,\nno?\n\nFrom there we'll call packet_reader_read(), which will call\npacket_read_with_status(), and from there we'll call packet_trace().\n\nThen right after all this happens we've got a loop that parses out these\npackfile URIs, including this being-done-first-here parsing of the hex\nvalue just for logging, except in pkt-line.c we've lost the information\nabout what hash algorithm length we should be using, which fetch-pack.c\nof course needs to know.\n\nWhy can't that process_section_header() in fetch-pack.c just be made to\ncall some pkt-line.c API saying \"don't log yet\", i.e. something like\nthis pseudocode:\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex a9604f35a3e..31f5ee7fc6b 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1518,14 +1518,18 @@ static void receive_wanted_refs(struct packet_reader *reader,\n static void receive_packfile_uris(struct packet_reader *reader,\n struct string_list *uris)\n {\n+ struct string_list log = STRING_LIST_INIT_DUP;\n+\n process_section_header(reader, \"packfile-uris\", 0);\n- while (packet_reader_read(reader) == PACKET_READ_NORMAL) {\n+ while (packet_reader_read_log_to(reader, &log) == PACKET_READ_NORMAL) {\n if (reader->pktlen < the_hash_algo->hexsz ||\n reader->line[the_hash_algo->hexsz] != ' ')\n die(\"expected ' ', got: %s\\n\", reader->line);\n \n+ /* move the parsing of the URLs here */\n string_list_append(uris, reader->line);\n }\n+ log_stuff(&log);\n if (reader->status != PACKET_READ_DELIM)\n die(\"expected DELIM\");\n }\n\nI.e. we'll eventually call trace_strbuf() in pkt-line.c, and we know\nthat we're doing these packfile-uris, and we know that we're just about\nto parse them. Let's just:\n\n 1. Start reading the section\n 2. Turn off tracing\n 3. Parse the URIs as we go\n 3. When done (or on the fly), scrub URIs, log any backlog suppressed trace, and turn on tracing again\n\nInstead of:\n\n 1. Set a flag to scrub stuff\n 2. Because of the disconnect between fetch-pack.c and pkt-line.c,\n effectively implement a new parser for data we're already going to be\n parsing some microseconds later during the course of the request.\n\nThat \"turn off the trace\" could be passing down a string_list/strbuf, or\neven doing the same via a nev member in \"struct packet_reader\", both\nwould be simpler than needing to re-do the parse. Probably simplest is just:\n\n struct string_list log = STRING_LIST_INIT_DUP;\n\n reader.deferred_trace = &log;\n /* packet_reader_read() etc. code, unchanged from now */\n /* parse URIs (just move the existing code around a bit) */\n packet_reader.deferred_trace = NULL;\n for_each...(item, &log)\n trace_strbuf(...);\n"},{"id":"440717","messageId":"211109.86ilx2dr5n.gmgdl@evledraar.gmail.com","threadId":"56667","inReplyTo":"xmqqpmrnfmiv.fsf@gitster.g","subject":"Re: [PATCH v4 2/2] http-fetch: redact url on die() message","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-09T01:54:42Z","receivedAt":"2021-11-09T02:00:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Oct 29 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>>> +\t\t\tif (!git_env_bool(\"GIT_TRACE_REDACT\", 1) || !nurl) {\n>>> +\t\t\t\tdie(\"Unable to get pack file %s\\n%s\", preq->url,\n>>> +\t\t\t\t curl_errorstr);\n>>\n>> small nit: arrange if's from \"if (cheap || expensive)\", i.e. no need for\n>> getenv() if !nurl, but maybe compilers are smart enough for that...\n>\n> They typically do not see what happens inside git_env_bool() while\n> compling this compilation unit, and cannot tell if the programmer\n> wanted to call it first for its side effects, hence they cannot\n> swap them safely.\n\n*nod*, but since that function is just:\n \n int git_env_bool(const char *k, int def)\n {\n const char *v = getenv(k);\n return v ? git_config_bool(k, v) : def;\n }\n\nI was hedging and pondering if some compilers were smart enough these\ndays to optimize things like that.\n\nI.e. in this case getenv() is a simple C library function, the env\nvariable is constant, and we do a boolean test of it before calling\ngit_config_bool().\n\nSo a sufficiently smart compiler could turn that into:\n\n /* global, probably something iterated over env already */\n static int __have_seen_GIT_TRACE_REDACT = 0;\n ...\n\n if ((!__have_seen_GIT_TRACE_REDACT || !nurl) ||\n (__have_seen_GIT_TRACE_REDACT && git_env_bool_without_v_bool_check(...)))\n\nBut probably not, since it wolud need quite a bit of C library\ncooperation/hooks...\n"},{"id":"440875","messageId":"CANQMx9VFeLAJQn1+AyF-rtXikpgr_LotnudPqOP=k0qwWgZdDA@mail.gmail.com","threadId":"56667","inReplyTo":"20211108230111.1101434-1-jonathantanmy@google.com","subject":"Re: [PATCH v6 1/2] fetch-pack: redact packfile urls in traces","fromName":"Ivan Frade","fromEmail":"ifrade@google.com","sentAt":"2021-11-10T21:18:35Z","receivedAt":"2021-11-10T21:18:50Z","isPatch":true,"sender":{"key":"ifrade@google.com","avatar":"https://avatars.githubusercontent.com/u/58185630?v=4"},"body":"On Mon, Nov 8, 2021 at 3:01 PM Jonathan Tan wrote:\n>\n> > + reader.options &= ~PACKET_READ_REDACT_URI_PATH;\n>\n> Probably worth commenting why you're resetting the flag (avoid the\n> relatively expensive URI check when we don't need it).\n\nDone\n\n>\n> > diff --git a/pkt-line.c b/pkt-line.c\n...\n> > + len = strspn(buffer, \"0123456789abcdefABCDEF\");\n> > + if (len != (int)the_hash_algo->hexsz || buffer[len] != ' ')\n> > + return NULL; /* required \"SP\" not seen */\n>\n> Optional: As I said in my reply (just sent out), checking for both SHA-1\n> and SHA-256 lengths is reasonable too.\n\nDone (with a comment indicating they are the hash sizes of SHA1 and SHA256)\n\n> > + GIT_TRACE=1 GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TEST_SIDEBAND_ALL=1 \\\n>\n> No need for GIT_TRACE=1 since you're not checking stdout. Also I don't\n> think GIT_TEST_SIDEBAND_ALL=1 is needed - we should check that it works\n> even without a test variable (and I've checked and it seems to work).\n\nDone in both tests.\n\nThanks,\n"},{"id":"440884","messageId":"CANQMx9U2sRB9Qm3zxvpOwn8cqRYyA0S0jJ2=JsspJ5hcRd_XOA@mail.gmail.com","threadId":"56667","inReplyTo":"211109.86mtmedrhr.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v6 1/2] fetch-pack: redact packfile urls in traces","fromName":"Ivan Frade","fromEmail":"ifrade@google.com","sentAt":"2021-11-10T23:44:14Z","receivedAt":"2021-11-10T23:44:28Z","isPatch":true,"sender":{"key":"ifrade@google.com","avatar":"https://avatars.githubusercontent.com/u/58185630?v=4"},"body":"On Mon, Nov 8, 2021 at 5:53 PM Ævar Arnfjörð Bjarmason wrote:\n>\n...\n>... Let's just:\n>\n> 1. Start reading the section\n> 2. Turn off tracing\n> 3. Parse the URIs as we go\n> 3. When done (or on the fly), scrub URIs, log any backlog suppressed trace, and turn on tracing again\n\nThis is a more generic redacting mechanism, but I understood that\nthere is no need for it. Previous comments went in the direction of\nremoving generality (e.g. not looking for a URI anywhere in the\npacket, but specifically for the packfile line format) and now this\npatch is very specific to redact packfile-uri lines in the protocol.\n\n> Instead of:\n>\n> 1. Set a flag to scrub stuff\n> 2. Because of the disconnect between fetch-pack.c and pkt-line.c,\n> effectively implement a new parser for data we're already going to be\n> parsing some microseconds later during the course of the request.\n\npkt-line is only looking for the \"SP\" shape. True that it\nencodes some protocol knowledge, but it is hardly a new parser.\n\n> That \"turn off the trace\" could be passing down a string_list/strbuf, or\n> even doing the same via a nev member in \"struct packet_reader\", both\n> would be simpler than needing to re-do the parse.\n\nSaving the lines and delaying the tracing could also produce weird\noutputs, no? e.g. 3 lines received, the second doesn't validate, the\nprogram aborts and the trace doesn't show any of the lines that caused\nthe problem. Or we would need to iterate in parallel through lines and\nsaved-log-lines assuming they match 1:1. Nothing unsolvable, but I am\nnot sure it is worthy the effort now.\n\nThanks,\n"},{"id":"440886","messageId":"pull.1052.v7.git.1636588289.gitgitgadget@gmail.com","threadId":"56667","inReplyTo":"pull.1052.v6.git.1635532975.gitgitgadget@gmail.com","subject":"[PATCH v7 0/2] fetch-pack: redact packfile urls in traces","fromName":"Ivan Frade via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-10T23:51:27Z","receivedAt":"2021-11-10T23:51:34Z","isPatch":true,"sender":{"key":"ifrade@google.com","avatar":"https://avatars.githubusercontent.com/u/58185630?v=4"},"body":"Changes since v6:\n\n * Use specific hash sizes instead of hexsz\n * Remove unnecessary env vars in tests\n * Added comment on bit toggle\n\nChanges since v5:\n\n * Use hexsz instead of hardcoded hash sizes\n\nChanges since v4:\n\n * Use \"uri\" instead of \"url\"\n * Look specifically for a line with packfile-uri format (instead of for a\n URL in general)\n * Limit the redacting to the packfile-uri section in do_fetch_pack_v2\n * Use \"%.*s\" instead of duplicating parts of the string to print\n\nChanges since v3:\n\n * Enable redacting URLs for all sections\n * Redact only URL path (it was until the end of line)\n * Redact URL in die() with more friendly message\n * Update doc to mention that packfile URIs are also redacted.\n\nChanges since v2:\n\n * Redact only the path of the URL\n * Test are now strict, validating the exact line expected in the log\n\nChanges since v1:\n\n * Removed non-POSIX flags in tests\n * More accurate regex for the non-encrypted packfile line\n * Dropped documentation change\n * Dropped redacting the die message in http-fetch\n\nIvan Frade (2):\n fetch-pack: redact packfile urls in traces\n http-fetch: redact url on die() message\n\n Documentation/git.txt | 5 +++--\n fetch-pack.c | 5 +++++\n http-fetch.c | 14 ++++++++++--\n pkt-line.c | 40 ++++++++++++++++++++++++++++++++-\n pkt-line.h | 1 +\n t/t5702-protocol-v2.sh | 51 ++++++++++++++++++++++++++++++++++++++++++\n 6 files changed, 111 insertions(+), 5 deletions(-)\n\n\nbase-commit: 88d915a634b449147855041d44875322de2b286d\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1052%2Fifradeo%2Fredact-packfile-uri-v7\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1052/ifradeo/redact-packfile-uri-v7\nPull-Request: https://github.com/gitgitgadget/git/pull/1052\n\nRange-diff vs v6:\n\n 1: a6098f98946 ! 1: bbfdc346ede fetch-pack: redact packfile urls in traces\n @@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n +\t\t\t\treader.options |= PACKET_READ_REDACT_URI_PATH;\n \t\t\tif (process_section_header(&reader, \"packfile-uris\", 1))\n \t\t\t\treceive_packfile_uris(&reader, &packfile_uris);\n ++\t\t\t/* We don't expect more URIs. Reset to avoid expensive URI check. */\n +\t\t\treader.options &= ~PACKET_READ_REDACT_URI_PATH;\n +\n \t\t\tprocess_section_header(&reader, \"packfile\", 0);\n @@ pkt-line.c: int packet_length(const char lenbuf_hex[4])\n +\tbuffer += 1;\n +\n +\tlen = strspn(buffer, \"0123456789abcdefABCDEF\");\n -+\tif (len != (int)the_hash_algo->hexsz || buffer[len] != ' ')\n ++\t/* size of SHA1 and SHA256 hash */\n ++\tif (!(len == 40 || len == 64) || buffer[len] != ' ')\n +\t\treturn NULL; /* required \"SP\" not seen */\n +\n +\tpath = strstr(buffer + len + 1, URI_MARK);\n @@ t/t5702-protocol-v2.sh: test_expect_success 'packfile-uri with transfer.fsckobje\n +\t\t\"uploadpack.blobpackfileuri\" \\\n +\t\t\"$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack\" &&\n +\n -+\tGIT_TRACE=1 GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TEST_SIDEBAND_ALL=1 \\\n ++\tGIT_TRACE_PACKET=\"$(pwd)/log\" \\\n +\tgit -c protocol.version=2 \\\n +\t\t-c fetch.uriprotocols=http,https \\\n +\t\tclone \"$HTTPD_URL/smart/http_parent\" http_child &&\n @@ t/t5702-protocol-v2.sh: test_expect_success 'packfile-uri with transfer.fsckobje\n +\t\t\"uploadpack.blobpackfileuri\" \\\n +\t\t\"$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack\" &&\n +\n -+\tGIT_TRACE=1 GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TEST_SIDEBAND_ALL=1 \\\n ++\tGIT_TRACE_PACKET=\"$(pwd)/log\" \\\n +\tGIT_TRACE_REDACT=0 \\\n +\tgit -c protocol.version=2 \\\n +\t\t-c fetch.uriprotocols=http,https \\\n 2: 38859ae7b7d = 2: 3b210735bc8 http-fetch: redact url on die() message\n\n-- \ngitgitgadget\n"},{"id":"440887","messageId":"bbfdc346ededf1479b5622739be658ee0be2f506.1636588289.git.gitgitgadget@gmail.com","threadId":"56667","inReplyTo":"pull.1052.v7.git.1636588289.gitgitgadget@gmail.com","subject":"[PATCH v7 1/2] fetch-pack: redact packfile urls in traces","fromName":"Ivan Frade via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-10T23:51:28Z","receivedAt":"2021-11-10T23:51:36Z","isPatch":true,"sender":{"key":"ifrade@google.com","avatar":"https://avatars.githubusercontent.com/u/58185630?v=4"},"body":"From: Ivan Frade \n\nIn some setups, packfile uris act as bearer token. It is not\nrecommended to expose them plainly in logs, although in special\ncircunstances (e.g. debug) it makes sense to write them.\n\nRedact the packfile URL paths by default, unless the GIT_TRACE_REDACT\nvariable is set to false. This mimics the redacting of the Authorization\nheader in HTTP.\n\nSigned-off-by: Ivan Frade \n---\n Documentation/git.txt | 5 +++--\n fetch-pack.c | 5 +++++\n pkt-line.c | 40 ++++++++++++++++++++++++++++++++-\n pkt-line.h | 1 +\n t/t5702-protocol-v2.sh | 51 ++++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 99 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex 281c5f8caef..13f83a2a3a1 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -832,8 +832,9 @@ for full details.\n \n `GIT_TRACE_REDACT`::\n \tBy default, when tracing is activated, Git redacts the values of\n-\tcookies, the \"Authorization:\" header, and the \"Proxy-Authorization:\"\n-\theader. Set this variable to `0` to prevent this redaction.\n+\tcookies, the \"Authorization:\" header, the \"Proxy-Authorization:\"\n+\theader and packfile URIs. Set this variable to `0` to prevent this\n+\tredaction.\n \n `GIT_LITERAL_PATHSPECS`::\n \tSetting this variable to `1` will cause Git to treat all\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex a9604f35a3e..8b8c75f33aa 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1653,8 +1653,13 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t\treceive_wanted_refs(&reader, sought, nr_sought);\n \n \t\t\t/* get the pack(s) */\n+\t\t\tif (git_env_bool(\"GIT_TRACE_REDACT\", 1))\n+\t\t\t\treader.options |= PACKET_READ_REDACT_URI_PATH;\n \t\t\tif (process_section_header(&reader, \"packfile-uris\", 1))\n \t\t\t\treceive_packfile_uris(&reader, &packfile_uris);\n+\t\t\t/* We don't expect more URIs. Reset to avoid expensive URI check. */\n+\t\t\treader.options &= ~PACKET_READ_REDACT_URI_PATH;\n+\n \t\t\tprocess_section_header(&reader, \"packfile\", 0);\n \n \t\t\t/*\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 2dc8ac274bd..8e43c2def4c 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -370,6 +370,32 @@ int packet_length(const char lenbuf_hex[4])\n \treturn (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);\n }\n \n+static char *find_packfile_uri_path(const char *buffer)\n+{\n+\tconst char *URI_MARK = \"://\";\n+\tchar *path;\n+\tint len;\n+\n+\t/* First char is sideband mark */\n+\tbuffer += 1;\n+\n+\tlen = strspn(buffer, \"0123456789abcdefABCDEF\");\n+\t/* size of SHA1 and SHA256 hash */\n+\tif (!(len == 40 || len == 64) || buffer[len] != ' ')\n+\t\treturn NULL; /* required \"SP\" not seen */\n+\n+\tpath = strstr(buffer + len + 1, URI_MARK);\n+\tif (!path)\n+\t\treturn NULL;\n+\n+\tpath = strchr(path + strlen(URI_MARK), '/');\n+\tif (!path || !*(path + 1))\n+\t\treturn NULL;\n+\n+\t/* position after '/' */\n+\treturn ++path;\n+}\n+\n enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t\t\t\t\t\tsize_t *src_len, char *buffer,\n \t\t\t\t\t\tunsigned size, int *pktlen,\n@@ -377,6 +403,7 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n {\n \tint len;\n \tchar linelen[4];\n+\tchar *uri_path_start;\n \n \tif (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0) {\n \t\t*pktlen = -1;\n@@ -427,7 +454,18 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t\tlen--;\n \n \tbuffer[len] = 0;\n-\tpacket_trace(buffer, len, 0);\n+\tif (options & PACKET_READ_REDACT_URI_PATH &&\n+\t (uri_path_start = find_packfile_uri_path(buffer))) {\n+\t\tconst char *redacted = \"\";\n+\t\tstruct strbuf tracebuf = STRBUF_INIT;\n+\t\tstrbuf_insert(&tracebuf, 0, buffer, len);\n+\t\tstrbuf_splice(&tracebuf, uri_path_start - buffer,\n+\t\t\t strlen(uri_path_start), redacted, strlen(redacted));\n+\t\tpacket_trace(tracebuf.buf, tracebuf.len, 0);\n+\t\tstrbuf_release(&tracebuf);\n+\t} else {\n+\t\tpacket_trace(buffer, len, 0);\n+\t}\n \n \tif ((options & PACKET_READ_DIE_ON_ERR_PACKET) &&\n \t starts_with(buffer, \"ERR \"))\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 467ae013573..6d2a63db238 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -87,6 +87,7 @@ void packet_fflush(FILE *f);\n #define PACKET_READ_CHOMP_NEWLINE (1u<<1)\n #define PACKET_READ_DIE_ON_ERR_PACKET (1u<<2)\n #define PACKET_READ_GENTLE_ON_READ_ERROR (1u<<3)\n+#define PACKET_READ_REDACT_URI_PATH (1u<<4)\n int packet_read(int fd, char *buffer, unsigned size, int options);\n \n /*\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex d527cf6c49f..78f85b0714a 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -1107,6 +1107,57 @@ test_expect_success 'packfile-uri with transfer.fsckobjects fails when .gitmodul\n \ttest_i18ngrep \"disallowed submodule name\" err\n '\n \n+test_expect_success 'packfile-uri path redacted in trace' '\n+\tP=\"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n+\trm -rf \"$P\" http_child log &&\n+\n+\tgit init \"$P\" &&\n+\tgit -C \"$P\" config \"uploadpack.allowsidebandall\" \"true\" &&\n+\n+\techo my-blob >\"$P/my-blob\" &&\n+\tgit -C \"$P\" add my-blob &&\n+\tgit -C \"$P\" commit -m x &&\n+\n+\tgit -C \"$P\" hash-object my-blob >objh &&\n+\tgit -C \"$P\" pack-objects \"$HTTPD_DOCUMENT_ROOT_PATH/mypack\" packh &&\n+\tgit -C \"$P\" config --add \\\n+\t\t\"uploadpack.blobpackfileuri\" \\\n+\t\t\"$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack\" &&\n+\n+\tGIT_TRACE_PACKET=\"$(pwd)/log\" \\\n+\tgit -c protocol.version=2 \\\n+\t\t-c fetch.uriprotocols=http,https \\\n+\t\tclone \"$HTTPD_URL/smart/http_parent\" http_child &&\n+\n+\tgrep -F \"clone< \\\\1$(cat packh) $HTTPD_URL/\" log\n+'\n+\n+test_expect_success 'packfile-uri path not redacted in trace when GIT_TRACE_REDACT=0' '\n+\tP=\"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n+\trm -rf \"$P\" http_child log &&\n+\n+\tgit init \"$P\" &&\n+\tgit -C \"$P\" config \"uploadpack.allowsidebandall\" \"true\" &&\n+\n+\techo my-blob >\"$P/my-blob\" &&\n+\tgit -C \"$P\" add my-blob &&\n+\tgit -C \"$P\" commit -m x &&\n+\n+\tgit -C \"$P\" hash-object my-blob >objh &&\n+\tgit -C \"$P\" pack-objects \"$HTTPD_DOCUMENT_ROOT_PATH/mypack\" packh &&\n+\tgit -C \"$P\" config --add \\\n+\t\t\"uploadpack.blobpackfileuri\" \\\n+\t\t\"$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack\" &&\n+\n+\tGIT_TRACE_PACKET=\"$(pwd)/log\" \\\n+\tGIT_TRACE_REDACT=0 \\\n+\tgit -c protocol.version=2 \\\n+\t\t-c fetch.uriprotocols=http,https \\\n+\t\tclone \"$HTTPD_URL/smart/http_parent\" http_child &&\n+\n+\tgrep -F \"clone< \\\\1$(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack\" log\n+'\n+\n test_expect_success 'http:// --negotiate-only' '\n \tSERVER=\"$HTTPD_DOCUMENT_ROOT_PATH/server\" &&\n \tURI=\"$HTTPD_URL/smart/server\" &&\n-- \ngitgitgadget\n\n"},{"id":"440888","messageId":"3b210735bc86810c203bd6f16a503662a6239920.1636588289.git.gitgitgadget@gmail.com","threadId":"56667","inReplyTo":"pull.1052.v7.git.1636588289.gitgitgadget@gmail.com","subject":"[PATCH v7 2/2] http-fetch: redact url on die() message","fromName":"Ivan Frade via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-10T23:51:29Z","receivedAt":"2021-11-10T23:51:37Z","isPatch":true,"sender":{"key":"ifrade@google.com","avatar":"https://avatars.githubusercontent.com/u/58185630?v=4"},"body":"From: Ivan Frade \n\nhttp-fetch prints the URL after failing to fetch it. This can be\nconfusing to users (they cannot really do anything with it), and they\ncan share by accident a sensitive URL (e.g. with credentials) while\nlooking for help.\n\nRedact the URL unless the GIT_TRACE_REDACT variable is set to false. This\nmimics the redaction of other sensitive information in git, like the\nAuthorization header in HTTP.\n\nFix also capitalization of previous die() message (must start in\nlowercase).\n\nSigned-off-by: Ivan Frade \n---\n http-fetch.c | 14 ++++++++++++--\n 1 file changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/http-fetch.c b/http-fetch.c\nindex fa642462a9e..c7c7d391ac5 100644\n--- a/http-fetch.c\n+++ b/http-fetch.c\n@@ -4,6 +4,7 @@\n #include \"http.h\"\n #include \"walker.h\"\n #include \"strvec.h\"\n+#include \"urlmatch.h\"\n \n static const char http_fetch_usage[] = \"git http-fetch \"\n \"[-c] [-t] [-a] [-v] [--recover] [-w ref] [--stdin | --packfile=hash | commit-id] url\";\n@@ -63,8 +64,17 @@ static void fetch_single_packfile(struct object_id *packfile_hash,\n \tif (start_active_slot(preq->slot)) {\n \t\trun_active_slot(preq->slot);\n \t\tif (results.curl_result != CURLE_OK) {\n-\t\t\tdie(\"Unable to get pack file %s\\n%s\", preq->url,\n-\t\t\t curl_errorstr);\n+\t\t\tstruct url_info url;\n+\t\t\tchar *nurl = url_normalize(preq->url, &url);\n+\t\t\tif (!nurl || !git_env_bool(\"GIT_TRACE_REDACT\", 1)) {\n+\t\t\t\tdie(\"unable to get pack file '%s'\\n%s\", preq->url,\n+\t\t\t\t curl_errorstr);\n+\t\t\t} else {\n+\t\t\t\tdie(\"failed to get '%.*s' url from '%.*s' \"\n+\t\t\t\t \"(full URL redacted due to GIT_TRACE_REDACT setting)\\n%s\",\n+\t\t\t\t (int)url.scheme_len, url.url,\n+\t\t\t\t (int)url.host_len, &url.url[url.host_off], curl_errorstr);\n+\t\t\t}\n \t\t}\n \t} else {\n \t\tdie(\"Unable to start request\");\n-- \ngitgitgadget\n"},{"id":"440891","messageId":"211111.86tugjpn1x.gmgdl@evledraar.gmail.com","threadId":"56667","inReplyTo":"CANQMx9U2sRB9Qm3zxvpOwn8cqRYyA0S0jJ2=JsspJ5hcRd_XOA@mail.gmail.com","subject":"Re: [PATCH v6 1/2] fetch-pack: redact packfile urls in traces","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-11T00:01:32Z","receivedAt":"2021-11-11T00:13:02Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Nov 10 2021, Ivan Frade wrote:\n\n> On Mon, Nov 8, 2021 at 5:53 PM Ævar Arnfjörð Bjarmason wrote:\n>>\n> ...\n>>... Let's just:\n>>\n>> 1. Start reading the section\n>> 2. Turn off tracing\n>> 3. Parse the URIs as we go\n>> 3. When done (or on the fly), scrub URIs, log any backlog suppressed trace, and turn on tracing again\n>\n> This is a more generic redacting mechanism, but I understood that\n> there is no need for it. Previous comments went in the direction of\n> removing generality (e.g. not looking for a URI anywhere in the\n> packet, but specifically for the packfile line format) and now this\n> patch is very specific to redact packfile-uri lines in the protocol.\n\nIt's less generic, because it would live in the loop that consumes the\nlines. \n\n>> Instead of:\n>>\n>> 1. Set a flag to scrub stuff\n>> 2. Because of the disconnect between fetch-pack.c and pkt-line.c,\n>> effectively implement a new parser for data we're already going to be\n>> parsing some microseconds later during the course of the request.\n>\n> pkt-line is only looking for the \"SP\" shape. True that it\n> encodes some protocol knowledge, but it is hardly a new parser.\n\nYeah, but why have find_packfile_uri_path() at all instead of just\nmoving the parsing code around?\n\nWe've already got the code that parses these lines, it's just a few\nlines removed from the code you're adding...\n\n>> That \"turn off the trace\" could be passing down a string_list/strbuf, or\n>> even doing the same via a nev member in \"struct packet_reader\", both\n>> would be simpler than needing to re-do the parse.\n>\n> Saving the lines and delaying the tracing could also produce weird\n> outputs, no? e.g. 3 lines received, the second doesn't validate, the\n> program aborts and the trace doesn't show any of the lines that caused\n> the problem. Or we would need to iterate in parallel through lines and\n> saved-log-lines assuming they match 1:1. Nothing unsolvable, but I am\n> not sure it is worthy the effort now.\n\nIt would only be weird if you do :\n\n download_later =\n while (consume lines)\n download_later += buffer_lines;\n log lines;\n\nI'm suggesting:\n\n download_later =\n while (consume lines)\n raw, to_log = parse line\n log line(to_log)\n download_later += raw\n\nSure, you'll need to do something in the case where the line doesn't\nvalidate, should you redact it still, or log it as is? Anyway, that's\nalso a caveat you've got now.\n\nThat's not iterating in parallel, having one for-loop instead of two.\n\nI see now that that approach would also solve at least one\nbug/misfeature in the packfile-uri handling, i.e.:\n\n for (i = 0; i < packfile_uris.nr; i++) {\n [...]\n start_command(...) [... to download the URI ...]\n [...]\n die(\"fetch-pack: pack downloaded from %s does not match expected hash %.*s\",\n }\n\nI.e. we've already received all the URIs, but then do validation on them\none at a time, so we might only notice that the server has sent us bad\ndata for the Nth URI after first downloading the first N-1 URIs.\n"},{"id":"440956","messageId":"xmqq35o2j854.fsf@gitster.g","threadId":"56667","inReplyTo":"pull.1052.v7.git.1636588289.gitgitgadget@gmail.com","subject":"Re: [PATCH v7 0/2] fetch-pack: redact packfile urls in traces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-12T04:43:51Z","receivedAt":"2021-11-12T04:43:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Ivan Frade via GitGitGadget\" writes:\n\n> Changes since v6:\n>\n> * Use specific hash sizes instead of hexsz\n> * Remove unnecessary env vars in tests\n> * Added comment on bit toggle\n> Ivan Frade (2):\n> fetch-pack: redact packfile urls in traces\n> http-fetch: redact url on die() message\n\nThanks, will queue.\n"}]}