{"thread":{"id":"60548","subject":"[PATCH 1/4] trace2: fix signature of trace2_def_param() macro","startedAt":"2023-11-22T19:18:40Z","lastAt":"2023-11-27T21:50:53Z","messageCount":9,"participants":["Jeff Hostetler via GitGitGadget","Johannes Schindelin via GitGitGadget","Junio C Hamano","Elijah Newren","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"485052","messageId":"pull.1616.git.1700680717.gitgitgadget@gmail.com","threadId":"60548","inReplyTo":null,"subject":"[PATCH 0/4] Redact unsafe URLs in the Trace2 output","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-11-22T19:18:33Z","receivedAt":"2023-11-22T19:18:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The Trace2 output can contain secrets when a user issues a Git command with\nsensitive information in the command-line. A typical (if highly discouraged)\nexample is: git clone https://user:password@host.com/.\n\nWith this PR, the Trace2 output redacts passwords in such URLs by default.\n\nThis series also includes a commit to temporarily disable leak checking on\nt0210,t0211 because the tests uncover other unrelated bugs in Git.\n\nThese patches were integrated into Microsoft's fork of Git, as\nhttps://github.com/microsoft/git/pull/616, and have been cooking there ever\nsince.\n\nJeff Hostetler (3):\n trace2: fix signature of trace2_def_param() macro\n t0211: test URL redacting in PERF format\n t0212: test URL redacting in EVENT format\n\nJohannes Schindelin (1):\n trace2: redact passwords from https:// URLs by default\n\n t/helper/test-trace2.c | 55 ++++++++++++++++++\n t/t0210-trace2-normal.sh | 20 ++++++-\n t/t0211-trace2-perf.sh | 21 ++++++-\n t/t0212-trace2-event.sh | 40 +++++++++++++\n trace2.c | 120 ++++++++++++++++++++++++++++++++++++++-\n trace2.h | 4 +-\n 6 files changed, 253 insertions(+), 7 deletions(-)\n\n\nbase-commit: 564d0252ca632e0264ed670534a51d18a689ef5d\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1616%2Fdscho%2Ftrace2-redact-credentials-in-https-urls-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1616/dscho/trace2-redact-credentials-in-https-urls-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1616\n-- \ngitgitgadget\n"},{"id":"485051","messageId":"97d17c22ff310c26c3ec391c7bf870e7e5bab4f8.1700680717.git.gitgitgadget@gmail.com","threadId":"60548","inReplyTo":"pull.1616.git.1700680717.gitgitgadget@gmail.com","subject":"[PATCH 1/4] trace2: fix signature of trace2_def_param() macro","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-11-22T19:18:34Z","receivedAt":"2023-11-22T19:18:41Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nAdd `struct key_value_info` argument to `trace2_def_param()`.\n\nIn dc90208497 (trace2: plumb config kvi, 2023-06-28) a `kvi`\nargument was added to `trace2_def_param_fl()` but the macro\nwas not up updated. Let's fix that.\n\nSigned-off-by: Jeff Hostetler \n---\n trace2.h | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/trace2.h b/trace2.h\nindex 40d8c2e02a5..1f0669bbd2d 100644\n--- a/trace2.h\n+++ b/trace2.h\n@@ -337,8 +337,8 @@ struct key_value_info;\n void trace2_def_param_fl(const char *file, int line, const char *param,\n \t\t\t const char *value, const struct key_value_info *kvi);\n \n-#define trace2_def_param(param, value) \\\n-\ttrace2_def_param_fl(__FILE__, __LINE__, (param), (value))\n+#define trace2_def_param(param, value, kvi) \\\n+\ttrace2_def_param_fl(__FILE__, __LINE__, (param), (value), (kvi))\n \n /*\n * Tell trace2 about a newly instantiated repo object and assign\n-- \ngitgitgadget\n\n"},{"id":"485053","messageId":"a1686ab52f1bec4bddeaab973c9b77e55e8b539b.1700680717.git.gitgitgadget@gmail.com","threadId":"60548","inReplyTo":"pull.1616.git.1700680717.gitgitgadget@gmail.com","subject":"[PATCH 2/4] trace2: redact passwords from https:// URLs by default","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-11-22T19:18:35Z","receivedAt":"2023-11-22T19:18:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIt is an unsafe practice to call something like\n\n\tgit clone https://user:password@example.com/\n\nThis not only risks leaking the password \"over the shoulder\" or into the\nreadline history of the current Unix shell, it also gets logged via\nTrace2 if enabled.\n\nLet's at least avoid logging such secrets via Trace2, much like we avoid\nlogging secrets in `http.c`. Much like the code in `http.c` is guarded\nvia `GIT_TRACE_REDACT` (defaulting to `true`), we guard the new code via\n`GIT_TRACE2_REDACT` (also defaulting to `true`).\n\nThe new tests added in this commit uncover leaks in `builtin/clone.c`\nand `remote.c`. Therefore we need to turn off\n`TEST_PASSES_SANITIZE_LEAK`. The reasons:\n\n- We observed that `the_repository->remote_status` is not released\n properly.\n\n- We are using `url...insteadOf` and that runs into a code path where an\n allocated URL is replaced with another URL, and the original URL is\n never released.\n\n- `remote_states` contains plenty of `struct remote`s whose refspecs\n seem to be usually allocated by never released.\n\nMore investigation is needed here to identify the exact cause and\nproper fixes for these leaks/bugs.\n\nCo-authored-by: Jeff Hostetler \nSigned-off-by: Jeff Hostetler \nSigned-off-by: Johannes Schindelin \n---\n t/t0210-trace2-normal.sh | 20 ++++++-\n trace2.c | 120 ++++++++++++++++++++++++++++++++++++++-\n 2 files changed, 136 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t0210-trace2-normal.sh b/t/t0210-trace2-normal.sh\nindex 80e76a4695e..c312657a12c 100755\n--- a/t/t0210-trace2-normal.sh\n+++ b/t/t0210-trace2-normal.sh\n@@ -2,7 +2,7 @@\n \n test_description='test trace2 facility (normal target)'\n \n-TEST_PASSES_SANITIZE_LEAK=true\n+TEST_PASSES_SANITIZE_LEAK=false\n . ./test-lib.sh\n \n # Turn off any inherited trace2 settings for this test.\n@@ -283,4 +283,22 @@ test_expect_success 'using global config with include' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'unsafe URLs are redacted by default' '\n+\ttest_when_finished \\\n+\t\t\"rm -r trace.normal unredacted.normal clone clone2\" &&\n+\n+\ttest_config_global \\\n+\t\t\"url.$(pwd).insteadOf\" https://user:pwd@example.com/ &&\n+\ttest_config_global trace2.configParams \"core.*,remote.*.url\" &&\n+\n+\tGIT_TRACE2=\"$(pwd)/trace.normal\" \\\n+\t\tgit clone https://user:pwd@example.com/ clone &&\n+\t! grep user:pwd trace.normal &&\n+\n+\tGIT_TRACE2_REDACT=0 GIT_TRACE2=\"$(pwd)/unredacted.normal\" \\\n+\t\tgit clone https://user:pwd@example.com/ clone2 &&\n+\tgrep \"start .* clone https://user:pwd@example.com\" unredacted.normal &&\n+\tgrep \"remote.origin.url=https://user:pwd@example.com\" unredacted.normal\n+'\n+\n test_done\ndiff --git a/trace2.c b/trace2.c\nindex 6dc74dff4c7..87d9a3a0361 100644\n--- a/trace2.c\n+++ b/trace2.c\n@@ -20,6 +20,7 @@\n #include \"trace2/tr2_tmr.h\"\n \n static int trace2_enabled;\n+static int trace2_redact = 1;\n \n static int tr2_next_child_id; /* modify under lock */\n static int tr2_next_exec_id; /* modify under lock */\n@@ -227,6 +228,8 @@ void trace2_initialize_fl(const char *file, int line)\n \tif (!tr2_tgt_want_builtins())\n \t\treturn;\n \ttrace2_enabled = 1;\n+\tif (!git_env_bool(\"GIT_TRACE2_REDACT\", 1))\n+\t\ttrace2_redact = 0;\n \n \ttr2_sid_get();\n \n@@ -247,12 +250,93 @@ int trace2_is_enabled(void)\n \treturn trace2_enabled;\n }\n \n+/*\n+ * Redacts an argument, i.e. ensures that no password in\n+ * https://user:password@host/-style URLs is logged.\n+ *\n+ * Returns the original if nothing needed to be redacted.\n+ * Returns a pointer that needs to be `free()`d otherwise.\n+ */\n+static const char *redact_arg(const char *arg)\n+{\n+\tconst char *p, *colon;\n+\tsize_t at;\n+\n+\tif (!trace2_redact ||\n+\t (!skip_prefix(arg, \"https://\", &p) &&\n+\t !skip_prefix(arg, \"http://\", &p)))\n+\t\treturn arg;\n+\n+\tat = strcspn(p, \"@/\");\n+\tif (p[at] != '@')\n+\t\treturn arg;\n+\n+\tcolon = memchr(p, ':', at);\n+\tif (!colon)\n+\t\treturn arg;\n+\n+\treturn xstrfmt(\"%.*s:%s\", (int)(colon - arg), arg, p + at);\n+}\n+\n+/*\n+ * Redacts arguments in an argument list.\n+ *\n+ * Returns the original if nothing needed to be redacted.\n+ * Otherwise, returns a new array that needs to be released\n+ * via `free_redacted_argv()`.\n+ */\n+static const char **redact_argv(const char **argv)\n+{\n+\tint i, j;\n+\tconst char *redacted = NULL;\n+\tconst char **ret;\n+\n+\tif (!trace2_redact)\n+\t\treturn argv;\n+\n+\tfor (i = 0; argv[i]; i++)\n+\t\tif ((redacted = redact_arg(argv[i])) != argv[i])\n+\t\t\tbreak;\n+\n+\tif (!argv[i])\n+\t\treturn argv;\n+\n+\tfor (j = 0; argv[j]; j++)\n+\t\t; /* keep counting */\n+\n+\tALLOC_ARRAY(ret, j + 1);\n+\tret[j] = NULL;\n+\n+\tfor (j = 0; j < i; j++)\n+\t\tret[j] = argv[j];\n+\tret[i] = redacted;\n+\tfor (++i; argv[i]; i++) {\n+\t\tredacted = redact_arg(argv[i]);\n+\t\tret[i] = redacted ? redacted : argv[i];\n+\t}\n+\n+\treturn ret;\n+}\n+\n+static void free_redacted_argv(const char **redacted, const char **argv)\n+{\n+\tint i;\n+\n+\tif (redacted != argv) {\n+\t\tfor (i = 0; argv[i]; i++)\n+\t\t\tif (redacted[i] != argv[i])\n+\t\t\t\tfree((void *)redacted[i]);\n+\t\tfree((void *)redacted);\n+\t}\n+}\n+\n void trace2_cmd_start_fl(const char *file, int line, const char **argv)\n {\n \tstruct tr2_tgt *tgt_j;\n \tint j;\n \tuint64_t us_now;\n \tuint64_t us_elapsed_absolute;\n+\tconst char **redacted;\n \n \tif (!trace2_enabled)\n \t\treturn;\n@@ -260,10 +344,14 @@ void trace2_cmd_start_fl(const char *file, int line, const char **argv)\n \tus_now = getnanotime() / 1000;\n \tus_elapsed_absolute = tr2tls_absolute_elapsed(us_now);\n \n+\tredacted = redact_argv(argv);\n+\n \tfor_each_wanted_builtin (j, tgt_j)\n \t\tif (tgt_j->pfn_start_fl)\n \t\t\ttgt_j->pfn_start_fl(file, line, us_elapsed_absolute,\n-\t\t\t\t\t argv);\n+\t\t\t\t\t redacted);\n+\n+\tfree_redacted_argv(redacted, argv);\n }\n \n void trace2_cmd_exit_fl(const char *file, int line, int code)\n@@ -409,6 +497,7 @@ void trace2_child_start_fl(const char *file, int line,\n \tint j;\n \tuint64_t us_now;\n \tuint64_t us_elapsed_absolute;\n+\tconst char **orig_argv = cmd->args.v;\n \n \tif (!trace2_enabled)\n \t\treturn;\n@@ -419,10 +508,24 @@ void trace2_child_start_fl(const char *file, int line,\n \tcmd->trace2_child_id = tr2tls_locked_increment(&tr2_next_child_id);\n \tcmd->trace2_child_us_start = us_now;\n \n+\t/*\n+\t * The `pfn_child_start_fl` API takes a `struct child_process`\n+\t * rather than a simple `argv` for the child because some\n+\t * targets make use of the additional context bits/values. So\n+\t * temporarily replace the original argv (inside the `strvec`)\n+\t * with a possibly redacted version.\n+\t */\n+\tcmd->args.v = redact_argv(orig_argv);\n+\n \tfor_each_wanted_builtin (j, tgt_j)\n \t\tif (tgt_j->pfn_child_start_fl)\n \t\t\ttgt_j->pfn_child_start_fl(file, line,\n \t\t\t\t\t\t us_elapsed_absolute, cmd);\n+\n+\tif (cmd->args.v != orig_argv) {\n+\t\tfree_redacted_argv(cmd->args.v, orig_argv);\n+\t\tcmd->args.v = orig_argv;\n+\t}\n }\n \n void trace2_child_exit_fl(const char *file, int line, struct child_process *cmd,\n@@ -493,6 +596,7 @@ int trace2_exec_fl(const char *file, int line, const char *exe,\n \tint exec_id;\n \tuint64_t us_now;\n \tuint64_t us_elapsed_absolute;\n+\tconst char **redacted;\n \n \tif (!trace2_enabled)\n \t\treturn -1;\n@@ -502,10 +606,14 @@ int trace2_exec_fl(const char *file, int line, const char *exe,\n \n \texec_id = tr2tls_locked_increment(&tr2_next_exec_id);\n \n+\tredacted = redact_argv(argv);\n+\n \tfor_each_wanted_builtin (j, tgt_j)\n \t\tif (tgt_j->pfn_exec_fl)\n \t\t\ttgt_j->pfn_exec_fl(file, line, us_elapsed_absolute,\n-\t\t\t\t\t exec_id, exe, argv);\n+\t\t\t\t\t exec_id, exe, redacted);\n+\n+\tfree_redacted_argv(redacted, argv);\n \n \treturn exec_id;\n }\n@@ -637,13 +745,19 @@ void trace2_def_param_fl(const char *file, int line, const char *param,\n {\n \tstruct tr2_tgt *tgt_j;\n \tint j;\n+\tconst char *redacted;\n \n \tif (!trace2_enabled)\n \t\treturn;\n \n+\tredacted = redact_arg(value);\n+\n \tfor_each_wanted_builtin (j, tgt_j)\n \t\tif (tgt_j->pfn_param_fl)\n-\t\t\ttgt_j->pfn_param_fl(file, line, param, value, kvi);\n+\t\t\ttgt_j->pfn_param_fl(file, line, param, redacted, kvi);\n+\n+\tif (redacted != value)\n+\t\tfree((void *)redacted);\n }\n \n void trace2_def_repo_fl(const char *file, int line, struct repository *repo)\n-- \ngitgitgadget\n\n"},{"id":"485054","messageId":"e50160fedc0ed6c07f705532878191b4c53df4f8.1700680717.git.gitgitgadget@gmail.com","threadId":"60548","inReplyTo":"pull.1616.git.1700680717.gitgitgadget@gmail.com","subject":"[PATCH 3/4] t0211: test URL redacting in PERF format","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-11-22T19:18:36Z","receivedAt":"2023-11-22T19:18:43Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nThis transmogrifies the test case that was just added to t0210, to also\ncover the `GIT_TRACE2_PERF` backend.\n\nJust like t0211, we now have to toggle the `TEST_PASSES_SANITIZE_LEAK`\nannotation.\n\nSigned-off-by: Jeff Hostetler \n---\n t/t0211-trace2-perf.sh | 21 ++++++++++++++++++++-\n 1 file changed, 20 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t0211-trace2-perf.sh b/t/t0211-trace2-perf.sh\nindex cfba6861322..290b6eaaab1 100755\n--- a/t/t0211-trace2-perf.sh\n+++ b/t/t0211-trace2-perf.sh\n@@ -2,7 +2,7 @@\n \n test_description='test trace2 facility (perf target)'\n \n-TEST_PASSES_SANITIZE_LEAK=true\n+TEST_PASSES_SANITIZE_LEAK=false\n . ./test-lib.sh\n \n # Turn off any inherited trace2 settings for this test.\n@@ -268,4 +268,23 @@ test_expect_success PTHREADS 'global counter test/test2' '\n \thave_counter_event \"main\" \"counter\" \"test\" \"test2\" 60 actual\n '\n \n+test_expect_success 'unsafe URLs are redacted by default' '\n+\ttest_when_finished \\\n+\t\t\"rm -r actual trace.perf unredacted.perf clone clone2\" &&\n+\n+\ttest_config_global \\\n+\t\t\"url.$(pwd).insteadOf\" https://user:pwd@example.com/ &&\n+\ttest_config_global trace2.configParams \"core.*,remote.*.url\" &&\n+\n+\tGIT_TRACE2_PERF=\"$(pwd)/trace.perf\" \\\n+\t\tgit clone https://user:pwd@example.com/ clone &&\n+\t! grep user:pwd trace.perf &&\n+\n+\tGIT_TRACE2_REDACT=0 GIT_TRACE2_PERF=\"$(pwd)/unredacted.perf\" \\\n+\t\tgit clone https://user:pwd@example.com/ clone2 &&\n+\tperl \"$TEST_DIRECTORY/t0211/scrub_perf.perl\" actual &&\n+\tgrep \"d0|main|start|.* clone https://user:pwd@example.com\" actual &&\n+\tgrep \"d0|main|def_param|.*|remote.origin.url:https://user:pwd@example.com\" actual\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"485055","messageId":"9bfd00c5ecb26a25589c59a0282fce26f948b6a9.1700680717.git.gitgitgadget@gmail.com","threadId":"60548","inReplyTo":"pull.1616.git.1700680717.gitgitgadget@gmail.com","subject":"[PATCH 4/4] t0212: test URL redacting in EVENT format","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-11-22T19:18:37Z","receivedAt":"2023-11-22T19:18:43Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nIn the added tests cases, skip testing the `GIT_TRACE2_REDACT=0` case\nbecause we would need to exactly model the full JSON event stream like\nwe did in the preceding basic tests and I do not think it is worth it.\n\nFurthermore, the Trace2 routines print the same content in normal, perf,\nor event format, and in t0210 and t0211 we already tested the basic\nfunctionality, so no need to repeat it here.\n\nIn this test, we use the test-helper to unit test each of the event\nmessages where URLs can appear and confirm that they are redacted in\neach event.\n\nSigned-off-by: Jeff Hostetler \n---\n t/helper/test-trace2.c | 55 +++++++++++++++++++++++++++++++++++++++++\n t/t0212-trace2-event.sh | 40 ++++++++++++++++++++++++++++++\n 2 files changed, 95 insertions(+)\n\ndiff --git a/t/helper/test-trace2.c b/t/helper/test-trace2.c\nindex d5ca0046c89..1adac29a575 100644\n--- a/t/helper/test-trace2.c\n+++ b/t/helper/test-trace2.c\n@@ -412,6 +412,56 @@ static int ut_201counter(int argc, const char **argv)\n \treturn 0;\n }\n \n+static int ut_300redact_start(int argc, const char **argv)\n+{\n+\tif (!argc)\n+\t\tdie(\"expect \");\n+\n+\ttrace2_cmd_start(argv);\n+\n+\treturn 0;\n+}\n+\n+static int ut_301redact_child_start(int argc, const char **argv)\n+{\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\tint k;\n+\n+\tif (!argc)\n+\t\tdie(\"expect \");\n+\n+\tfor (k = 0; argv[k]; k++)\n+\t\tstrvec_push(&cmd.args, argv[k]);\n+\n+\ttrace2_child_start(&cmd);\n+\n+\tstrvec_clear(&cmd.args);\n+\n+\treturn 0;\n+}\n+\n+static int ut_302redact_exec(int argc, const char **argv)\n+{\n+\tif (!argc)\n+\t\tdie(\"expect \");\n+\n+\ttrace2_exec(argv[0], &argv[1]);\n+\n+\treturn 0;\n+}\n+\n+static int ut_303redact_def_param(int argc, const char **argv)\n+{\n+\tstruct key_value_info kvi = KVI_INIT;\n+\n+\tif (argc < 2)\n+\t\tdie(\"expect \");\n+\n+\ttrace2_def_param(argv[0], argv[1], &kvi);\n+\n+\treturn 0;\n+}\n+\n /*\n * Usage:\n * test-tool trace2 \n@@ -438,6 +488,11 @@ static struct unit_test ut_table[] = {\n \n \t{ ut_200counter, \"200counter\", \" [ [ [...]]]\" },\n \t{ ut_201counter, \"201counter\", \" \" },\n+\n+\t{ ut_300redact_start, \"300redact_start\", \"\" },\n+\t{ ut_301redact_child_start, \"301redact_child_start\", \"\" },\n+\t{ ut_302redact_exec, \"302redact_exec\", \" \" },\n+\t{ ut_303redact_def_param, \"303redact_def_param\", \" \" },\n };\n /* clang-format on */\n \ndiff --git a/t/t0212-trace2-event.sh b/t/t0212-trace2-event.sh\nindex 6d3374ff773..147643d5826 100755\n--- a/t/t0212-trace2-event.sh\n+++ b/t/t0212-trace2-event.sh\n@@ -323,4 +323,44 @@ test_expect_success 'discard traces when there are too many files' '\n \thead -n2 trace_target_dir/git-trace2-discard | tail -n1 | grep \\\"event\\\":\\\"too_many_files\\\"\n '\n \n+# In the following \"...redact...\" tests, skip testing the GIT_TRACE2_REDACT=0\n+# case because we would need to exactly model the full JSON event stream like\n+# we did in the basic tests above and I do not think it is worth it.\n+\n+test_expect_success 'unsafe URLs are redacted by default in cmd_start events' '\n+\ttest_when_finished \\\n+\t\t\"rm -r trace.event\" &&\n+\n+\tGIT_TRACE2_EVENT=\"$(pwd)/trace.event\" \\\n+\t\ttest-tool trace2 300redact_start git clone https://user:pwd@example.com/ clone2 &&\n+\t! grep user:pwd trace.event\n+'\n+\n+test_expect_success 'unsafe URLs are redacted by default in child_start events' '\n+\ttest_when_finished \\\n+\t\t\"rm -r trace.event\" &&\n+\n+\tGIT_TRACE2_EVENT=\"$(pwd)/trace.event\" \\\n+\t\ttest-tool trace2 301redact_child_start git clone https://user:pwd@example.com/ clone2 &&\n+\t! grep user:pwd trace.event\n+'\n+\n+test_expect_success 'unsafe URLs are redacted by default in exec events' '\n+\ttest_when_finished \\\n+\t\t\"rm -r trace.event\" &&\n+\n+\tGIT_TRACE2_EVENT=\"$(pwd)/trace.event\" \\\n+\t\ttest-tool trace2 302redact_exec git clone https://user:pwd@example.com/ clone2 &&\n+\t! grep user:pwd trace.event\n+'\n+\n+test_expect_success 'unsafe URLs are redacted by default in def_param events' '\n+\ttest_when_finished \\\n+\t\t\"rm -r trace.event\" &&\n+\n+\tGIT_TRACE2_EVENT=\"$(pwd)/trace.event\" \\\n+\t\ttest-tool trace2 303redact_def_param url https://user:pwd@example.com/ &&\n+\t! grep user:pwd trace.event\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"485061","messageId":"xmqqil5tatl3.fsf@gitster.g","threadId":"60548","inReplyTo":"97d17c22ff310c26c3ec391c7bf870e7e5bab4f8.1700680717.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/4] trace2: fix signature of trace2_def_param() macro","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-23T06:10:00Z","receivedAt":"2023-11-23T06:10:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Jeff Hostetler via GitGitGadget\" writes:\n\n> From: Jeff Hostetler \n>\n> Add `struct key_value_info` argument to `trace2_def_param()`.\n>\n> In dc90208497 (trace2: plumb config kvi, 2023-06-28) a `kvi`\n> argument was added to `trace2_def_param_fl()` but the macro\n> was not up updated. Let's fix that.\n>\n> Signed-off-by: Jeff Hostetler \n> ---\n> trace2.h | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/trace2.h b/trace2.h\n> index 40d8c2e02a5..1f0669bbd2d 100644\n> --- a/trace2.h\n> +++ b/trace2.h\n> @@ -337,8 +337,8 @@ struct key_value_info;\n> void trace2_def_param_fl(const char *file, int line, const char *param,\n> \t\t\t const char *value, const struct key_value_info *kvi);\n> \n> -#define trace2_def_param(param, value) \\\n> -\ttrace2_def_param_fl(__FILE__, __LINE__, (param), (value))\n> +#define trace2_def_param(param, value, kvi) \\\n> +\ttrace2_def_param_fl(__FILE__, __LINE__, (param), (value), (kvi))\n\nIOW, this macro was not used back when it was updated, and nobody\nused it since then? \n\nI briefly wondered if we are better off removing it but that does\nnot make sense because you are adding a new (and only) user to it.\n\nWill queue. Thanks.\n\n> \n> /*\n> * Tell trace2 about a newly instantiated repo object and assign\n"},{"id":"485074","messageId":"CABPp-BELjVqVEB3oVx3fMzmvNfE1f7muLR_2k912_C+SaQtZtg@mail.gmail.com","threadId":"60548","inReplyTo":"a1686ab52f1bec4bddeaab973c9b77e55e8b539b.1700680717.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/4] trace2: redact passwords from https:// URLs by default","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-11-23T18:59:20Z","receivedAt":"2023-11-23T18:59:35Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Nov 22, 2023 at 11:19 AM Johannes Schindelin via GitGitGadget\n wrote:\n>\n> From: Johannes Schindelin \n>\n> It is an unsafe practice to call something like\n>\n> git clone https://user:password@example.com/\n>\n> This not only risks leaking the password \"over the shoulder\" or into the\n> readline history of the current Unix shell, it also gets logged via\n> Trace2 if enabled.\n\nIndeed. Clone urls _also_ seem to be slurped up by other tools, such\nas IDEs, and possibly sent off to various third-party cloud services\nwhen users have various AI-assist plugins installed in their IDEs,\nresulting in some infosec incidents and fire drills. (Not a\ntheoretical scenario, and not fun.)\n\n> Let's at least avoid logging such secrets via Trace2, much like we avoid\n> logging secrets in `http.c`. Much like the code in `http.c` is guarded\n> via `GIT_TRACE_REDACT` (defaulting to `true`), we guard the new code via\n> `GIT_TRACE2_REDACT` (also defaulting to `true`).\n\nTraining users is hard. I appreciate the changes here to make trace2\nnot be a leak vector, but is it time to perhaps consider bigger safety\nmeasures: At the clone/fetch level, automatically warn loudly whenever\nsuch a URL is provided, accompanied with a note that in the future it\nwill be turned into a hard error?\n\nEither way, I agree with your \"at least\" comment here and the changes\nyou are making.\n\n> The new tests added in this commit uncover leaks in `builtin/clone.c`\n> and `remote.c`. Therefore we need to turn off\n> `TEST_PASSES_SANITIZE_LEAK`. The reasons:\n>\n> - We observed that `the_repository->remote_status` is not released\n> properly.\n>\n> - We are using `url...insteadOf` and that runs into a code path where an\n> allocated URL is replaced with another URL, and the original URL is\n> never released.\n>\n> - `remote_states` contains plenty of `struct remote`s whose refspecs\n> seem to be usually allocated by never released.\n>\n> More investigation is needed here to identify the exact cause and\n> proper fixes for these leaks/bugs.\n\nThanks for carefully documenting and explaining.\n\n> Co-authored-by: Jeff Hostetler \n> Signed-off-by: Jeff Hostetler \n> Signed-off-by: Johannes Schindelin \n> ---\n> t/t0210-trace2-normal.sh | 20 ++++++-\n> trace2.c | 120 ++++++++++++++++++++++++++++++++++++++-\n> 2 files changed, 136 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/t0210-trace2-normal.sh b/t/t0210-trace2-normal.sh\n> index 80e76a4695e..c312657a12c 100755\n> --- a/t/t0210-trace2-normal.sh\n> +++ b/t/t0210-trace2-normal.sh\n> @@ -2,7 +2,7 @@\n>\n> test_description='test trace2 facility (normal target)'\n>\n> -TEST_PASSES_SANITIZE_LEAK=true\n> +TEST_PASSES_SANITIZE_LEAK=false\n> . ./test-lib.sh\n>\n> # Turn off any inherited trace2 settings for this test.\n> @@ -283,4 +283,22 @@ test_expect_success 'using global config with include' '\n> test_cmp expect actual\n> '\n>\n> +test_expect_success 'unsafe URLs are redacted by default' '\n> + test_when_finished \\\n> + \"rm -r trace.normal unredacted.normal clone clone2\" &&\n> +\n> + test_config_global \\\n> + \"url.$(pwd).insteadOf\" https://user:pwd@example.com/ &&\n> + test_config_global trace2.configParams \"core.*,remote.*.url\" &&\n> +\n> + GIT_TRACE2=\"$(pwd)/trace.normal\" \\\n> + git clone https://user:pwd@example.com/ clone &&\n> + ! grep user:pwd trace.normal &&\n> +\n> + GIT_TRACE2_REDACT=0 GIT_TRACE2=\"$(pwd)/unredacted.normal\" \\\n> + git clone https://user:pwd@example.com/ clone2 &&\n> + grep \"start .* clone https://user:pwd@example.com\" unredacted.normal &&\n> + grep \"remote.origin.url=https://user:pwd@example.com\" unredacted.normal\n> +'\n> +\n> test_done\n> diff --git a/trace2.c b/trace2.c\n> index 6dc74dff4c7..87d9a3a0361 100644\n> --- a/trace2.c\n> +++ b/trace2.c\n> @@ -20,6 +20,7 @@\n> #include \"trace2/tr2_tmr.h\"\n>\n> static int trace2_enabled;\n> +static int trace2_redact = 1;\n>\n> static int tr2_next_child_id; /* modify under lock */\n> static int tr2_next_exec_id; /* modify under lock */\n> @@ -227,6 +228,8 @@ void trace2_initialize_fl(const char *file, int line)\n> if (!tr2_tgt_want_builtins())\n> return;\n> trace2_enabled = 1;\n> + if (!git_env_bool(\"GIT_TRACE2_REDACT\", 1))\n> + trace2_redact = 0;\n>\n> tr2_sid_get();\n>\n> @@ -247,12 +250,93 @@ int trace2_is_enabled(void)\n> return trace2_enabled;\n> }\n>\n> +/*\n> + * Redacts an argument, i.e. ensures that no password in\n> + * https://user:password@host/-style URLs is logged.\n> + *\n> + * Returns the original if nothing needed to be redacted.\n> + * Returns a pointer that needs to be `free()`d otherwise.\n> + */\n> +static const char *redact_arg(const char *arg)\n> +{\n> + const char *p, *colon;\n> + size_t at;\n> +\n> + if (!trace2_redact ||\n> + (!skip_prefix(arg, \"https://\", &p) &&\n> + !skip_prefix(arg, \"http://\", &p)))\n> + return arg;\n> +\n> + at = strcspn(p, \"@/\");\n> + if (p[at] != '@')\n> + return arg;\n> +\n> + colon = memchr(p, ':', at);\n> + if (!colon)\n> + return arg;\n> +\n> + return xstrfmt(\"%.*s:%s\", (int)(colon - arg), arg, p + at);\n> +}\n> +\n> +/*\n> + * Redacts arguments in an argument list.\n> + *\n> + * Returns the original if nothing needed to be redacted.\n> + * Otherwise, returns a new array that needs to be released\n> + * via `free_redacted_argv()`.\n> + */\n> +static const char **redact_argv(const char **argv)\n> +{\n> + int i, j;\n> + const char *redacted = NULL;\n> + const char **ret;\n> +\n> + if (!trace2_redact)\n> + return argv;\n> +\n> + for (i = 0; argv[i]; i++)\n> + if ((redacted = redact_arg(argv[i])) != argv[i])\n> + break;\n> +\n> + if (!argv[i])\n> + return argv;\n> +\n> + for (j = 0; argv[j]; j++)\n> + ; /* keep counting */\n> +\n> + ALLOC_ARRAY(ret, j + 1);\n> + ret[j] = NULL;\n> +\n> + for (j = 0; j < i; j++)\n> + ret[j] = argv[j];\n> + ret[i] = redacted;\n> + for (++i; argv[i]; i++) {\n> + redacted = redact_arg(argv[i]);\n> + ret[i] = redacted ? redacted : argv[i];\n> + }\n> +\n> + return ret;\n> +}\n> +\n> +static void free_redacted_argv(const char **redacted, const char **argv)\n> +{\n> + int i;\n> +\n> + if (redacted != argv) {\n> + for (i = 0; argv[i]; i++)\n> + if (redacted[i] != argv[i])\n> + free((void *)redacted[i]);\n> + free((void *)redacted);\n> + }\n> +}\n> +\n> void trace2_cmd_start_fl(const char *file, int line, const char **argv)\n> {\n> struct tr2_tgt *tgt_j;\n> int j;\n> uint64_t us_now;\n> uint64_t us_elapsed_absolute;\n> + const char **redacted;\n>\n> if (!trace2_enabled)\n> return;\n> @@ -260,10 +344,14 @@ void trace2_cmd_start_fl(const char *file, int line, const char **argv)\n> us_now = getnanotime() / 1000;\n> us_elapsed_absolute = tr2tls_absolute_elapsed(us_now);\n>\n> + redacted = redact_argv(argv);\n> +\n> for_each_wanted_builtin (j, tgt_j)\n> if (tgt_j->pfn_start_fl)\n> tgt_j->pfn_start_fl(file, line, us_elapsed_absolute,\n> - argv);\n> + redacted);\n> +\n> + free_redacted_argv(redacted, argv);\n> }\n>\n> void trace2_cmd_exit_fl(const char *file, int line, int code)\n> @@ -409,6 +497,7 @@ void trace2_child_start_fl(const char *file, int line,\n> int j;\n> uint64_t us_now;\n> uint64_t us_elapsed_absolute;\n> + const char **orig_argv = cmd->args.v;\n>\n> if (!trace2_enabled)\n> return;\n> @@ -419,10 +508,24 @@ void trace2_child_start_fl(const char *file, int line,\n> cmd->trace2_child_id = tr2tls_locked_increment(&tr2_next_child_id);\n> cmd->trace2_child_us_start = us_now;\n>\n> + /*\n> + * The `pfn_child_start_fl` API takes a `struct child_process`\n> + * rather than a simple `argv` for the child because some\n> + * targets make use of the additional context bits/values. So\n> + * temporarily replace the original argv (inside the `strvec`)\n> + * with a possibly redacted version.\n> + */\n> + cmd->args.v = redact_argv(orig_argv);\n> +\n> for_each_wanted_builtin (j, tgt_j)\n> if (tgt_j->pfn_child_start_fl)\n> tgt_j->pfn_child_start_fl(file, line,\n> us_elapsed_absolute, cmd);\n> +\n> + if (cmd->args.v != orig_argv) {\n> + free_redacted_argv(cmd->args.v, orig_argv);\n> + cmd->args.v = orig_argv;\n> + }\n> }\n>\n> void trace2_child_exit_fl(const char *file, int line, struct child_process *cmd,\n> @@ -493,6 +596,7 @@ int trace2_exec_fl(const char *file, int line, const char *exe,\n> int exec_id;\n> uint64_t us_now;\n> uint64_t us_elapsed_absolute;\n> + const char **redacted;\n>\n> if (!trace2_enabled)\n> return -1;\n> @@ -502,10 +606,14 @@ int trace2_exec_fl(const char *file, int line, const char *exe,\n>\n> exec_id = tr2tls_locked_increment(&tr2_next_exec_id);\n>\n> + redacted = redact_argv(argv);\n> +\n> for_each_wanted_builtin (j, tgt_j)\n> if (tgt_j->pfn_exec_fl)\n> tgt_j->pfn_exec_fl(file, line, us_elapsed_absolute,\n> - exec_id, exe, argv);\n> + exec_id, exe, redacted);\n> +\n> + free_redacted_argv(redacted, argv);\n>\n> return exec_id;\n> }\n> @@ -637,13 +745,19 @@ void trace2_def_param_fl(const char *file, int line, const char *param,\n> {\n> struct tr2_tgt *tgt_j;\n> int j;\n> + const char *redacted;\n>\n> if (!trace2_enabled)\n> return;\n>\n> + redacted = redact_arg(value);\n> +\n> for_each_wanted_builtin (j, tgt_j)\n> if (tgt_j->pfn_param_fl)\n> - tgt_j->pfn_param_fl(file, line, param, value, kvi);\n> + tgt_j->pfn_param_fl(file, line, param, redacted, kvi);\n> +\n> + if (redacted != value)\n> + free((void *)redacted);\n> }\n>\n> void trace2_def_repo_fl(const char *file, int line, struct repository *repo)\n> --\n> gitgitgadget\n"},{"id":"485075","messageId":"CABPp-BHsWRPPdHzvZO+Wp01fB6yqo2fnqYsrSvk6_m932YkoeA@mail.gmail.com","threadId":"60548","inReplyTo":"pull.1616.git.1700680717.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/4] Redact unsafe URLs in the Trace2 output","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-11-23T19:08:10Z","receivedAt":"2023-11-23T19:08:24Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Nov 22, 2023 at 11:18 AM Johannes Schindelin via GitGitGadget\n wrote:\n>\n> The Trace2 output can contain secrets when a user issues a Git command with\n> sensitive information in the command-line. A typical (if highly discouraged)\n> example is: git clone https://user:password@host.com/.\n>\n> With this PR, the Trace2 output redacts passwords in such URLs by default.\n>\n> This series also includes a commit to temporarily disable leak checking on\n> t0210,t0211 because the tests uncover other unrelated bugs in Git.\n>\n> These patches were integrated into Microsoft's fork of Git, as\n> https://github.com/microsoft/git/pull/616, and have been cooking there ever\n> since.\n\nThanks for making these changes. Makes me wonder, back when we were\nlogging trace2 data, if we had some of these leaks. Eek.\n\nAs I commented in patch 2, I think this is a good start, but I'm\ncurious if others would be willing to turn clone/fetch of such bad\nURLs into warnings for now and errors later. The prevalence of\nAI-assist add-ons for various IDEs and the number of developers opting\nto use those IDEs and add-ons, and the fact that these tools sometimes\ninclude repository URLs in what they send off to third parties, makes\nme wonder if our recent infosec fire drill is soon going to be a\nwidely shared experience by lots of other companies and individuals.\nTraining users to not do bad things is hard, and it might be worth\nsaving them from themselves. Thoughts?\n"},{"id":"485182","messageId":"20231127215052.GD87495@coredump.intra.peff.net","threadId":"60548","inReplyTo":"CABPp-BELjVqVEB3oVx3fMzmvNfE1f7muLR_2k912_C+SaQtZtg@mail.gmail.com","subject":"Re: [PATCH 2/4] trace2: redact passwords from https:// URLs by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-27T21:50:52Z","receivedAt":"2023-11-27T21:50:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 23, 2023 at 10:59:20AM -0800, Elijah Newren wrote:\n\n> > Let's at least avoid logging such secrets via Trace2, much like we avoid\n> > logging secrets in `http.c`. Much like the code in `http.c` is guarded\n> > via `GIT_TRACE_REDACT` (defaulting to `true`), we guard the new code via\n> > `GIT_TRACE2_REDACT` (also defaulting to `true`).\n> \n> Training users is hard. I appreciate the changes here to make trace2\n> not be a leak vector, but is it time to perhaps consider bigger safety\n> measures: At the clone/fetch level, automatically warn loudly whenever\n> such a URL is provided, accompanied with a note that in the future it\n> will be turned into a hard error?\n\nYes, the password in such a case ends up in the plaintext .git/config\nfile, which is not great.\n\nThere's some discussion and patches here:\n\n https://lore.kernel.org/git/nycvar.QRO.7.76.6.1905172121130.46@tvgsbejvaqbjf.bet/\n\nI meant to follow up on them, but never did.\n\n-Peff\n"}]}