{"thread":{"id":"57913","subject":"[PATCH] urlmatch: create fetch.credentialsInUrl config","startedAt":"2022-05-23T18:30:13Z","lastAt":"2022-06-06T16:34:40Z","messageCount":40,"participants":["Derrick Stolee via GitGitGadget","Junio C Hamano","Derrick Stolee","Ævar Arnfjörð Bjarmason","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"455798","messageId":"pull.1237.git.1653329044940.gitgitgadget@gmail.com","threadId":"57913","inReplyTo":null,"subject":"[PATCH] urlmatch: create fetch.credentialsInUrl config","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-05-23T18:04:04Z","receivedAt":"2022-05-23T18:30:13Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nUsers sometimes provide a \"username:password\" combination in their\nplaintext URLs. Since Git stores these URLs in plaintext in the\n.git/config file, this is a very insecure way of storing these\ncredentials. Credential managers are a more secure way of storing this\ninformation.\n\nSystem administrators might want to prevent this kind of use by users on\ntheir machines.\n\nCreate a new \"fetch.credentialsInUrl\" config option and teach Git to\nwarn or die when seeing a URL with this kind of information. The warning\nanonymizes the sensitive information of the URL to be clear about the\nissue.\n\nThis change currently defaults the behavior to \"ignore\" which does\nnothing with these URLs. We can consider changing this behavior to\n\"warn\" by default if we wish. At that time, we may want to add some\nadvice about setting fetch.credentialsInUrl=ignore for users who still\nwant to follow this pattern (and not receive the warning).\n\nAs an attempt to ensure the parsing logic did not catch any\nunintentional cases, I modified this change locally to to use the \"die\"\noption by default. Running the test suite succeeds except for the\nexplicit username:password URLs used in t5550-http-fetch-dumb.s and\nt5541-http-push-smart.sh. This means that all other tested URLs did not\ntrigger this logic.\n\nSigned-off-by: Derrick Stolee \n---\n urlmatch: create fetch.credentialsInUrl config\n \n This is a modified version of the patch I submitted a while ago [1].\n \n Based on the feedback, changing the behavior to fail by default was not\n a good approach. Further, the idea to stop storing the credentials in\n config and redirect them to a credential manager was already considered\n by Peff [2] but not merged.\n \n This patch does what should be the simplest thing we can do: create a\n config option that will cause the user to get a warning or a failure,\n depending on its value. The default is to ignore the setting, identical\n to the current behavior. We can talk about changing this default to\n \"warn\" in the future, but it would be safest to release with ignore as\n the default until we are sure that we are not going to start warning on\n false positives.\n \n This patch would be sufficient for the interested internal parties that\n want to prevent users from storing credentials this way. System\n administrators can modify system-level Git config into \"die\" mode to\n prevent this behavior.\n \n [1]\n https://lore.kernel.org/git/pull.945.git.1619807844627.gitgitgadget@gmail.com\n Reject passwords in URLs (April 2021).\n \n [2]\n https://lore.kernel.org/git/20190519050724.GA26179@sigill.intra.peff.net/\n Re: Git ransom campaign incident report - May 2019\n \n Thanks, -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1237%2Fderrickstolee%2Fcreds-in-url-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1237/derrickstolee/creds-in-url-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1237\n\n Documentation/config/fetch.txt | 13 ++++++++++\n t/t5601-clone.sh | 7 ++++++\n urlmatch.c | 43 ++++++++++++++++++++++++++++++++++\n 3 files changed, 63 insertions(+)\n\ndiff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt\nindex cd65d236b43..6aa2a0bec19 100644\n--- a/Documentation/config/fetch.txt\n+++ b/Documentation/config/fetch.txt\n@@ -96,3 +96,16 @@ fetch.writeCommitGraph::\n \tmerge and the write may take longer. Having an updated commit-graph\n \tfile helps performance of many Git commands, including `git merge-base`,\n \t`git push -f`, and `git log --graph`. Defaults to false.\n+\n+fetch.credentialsInUrl::\n+\tA URL can contain plaintext credentials in the form\n+\t`protocol://:@domain/path`. Using such URLs is not\n+\trecommended as it exposes the password in multiple ways. The\n+\t`fetch.credentialsInUrl` option provides instruction for how Git\n+\tshould react to seeing such a URL, with these values:\n++\n+* `ignore` (default): Git will proceed with its activity without warning.\n+* `warn`: Git will write a warning message to `stderr` when parsing a URL\n+ with a plaintext credential.\n+* `die`: Git will write a failure message to `stderr` when parsing a URL\n+ with a plaintext credential.\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 4a61f2c901e..34be520b783 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -71,6 +71,13 @@ test_expect_success 'clone respects GIT_WORK_TREE' '\n \n '\n \n+test_expect_success 'clone warns or fails when using username:password' '\n+\ttest_must_fail git -c fetch.credentialsInUrl=warn clone https://username:password@localhost attempt1 2>err &&\n+\tgrep \"warning: URL '\\''https://username:\\*\\*\\*\\*\\*\\*\\*\\*@localhost/'\\'' uses plaintext credentials\" err &&\n+\ttest_must_fail git -c fetch.credentialsInUrl=die clone https://username:password@localhost attempt2 2>err &&\n+\tgrep \"fatal: URL '\\''https://username:\\*\\*\\*\\*\\*\\*\\*\\*@localhost/'\\'' uses plaintext credentials\" err\n+'\n+\n test_expect_success 'clone from hooks' '\n \n \ttest_create_repo r0 &&\ndiff --git a/urlmatch.c b/urlmatch.c\nindex b615adc923a..6b91fb648a7 100644\n--- a/urlmatch.c\n+++ b/urlmatch.c\n@@ -1,5 +1,6 @@\n #include \"cache.h\"\n #include \"urlmatch.h\"\n+#include \"config.h\"\n \n #define URL_ALPHA \"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz\"\n #define URL_DIGIT \"0123456789\"\n@@ -106,6 +107,46 @@ static int match_host(const struct url_info *url_info,\n \treturn (!url_len && !pat_len);\n }\n \n+static void detected_credentials_in_url(const char *url, size_t scheme_len)\n+{\n+\tchar *value = NULL;\n+\tconst char *at_ptr;\n+\tconst char *colon_ptr;\n+\tstruct strbuf anonymized = STRBUF_INIT;\n+\n+\t/* \"ignore\" is the default behavior. */\n+\tif (git_config_get_string(\"fetch.credentialsinurl\", &value) ||\n+\t !strcasecmp(\"ignore\", value))\n+\t\tgoto cleanup;\n+\n+\tat_ptr = strchr(url, '@');\n+\tcolon_ptr = strchr(url + scheme_len + 3, ':');\n+\n+\tif (!colon_ptr)\n+\t\tBUG(\"failed to find colon in url '%s' with scheme_len %\"PRIuMAX,\n+\t\t url, (uintmax_t) scheme_len);\n+\n+\t/* Include everything including the colon. */\n+\tcolon_ptr++;\n+\tstrbuf_add(&anonymized, url, colon_ptr - url);\n+\n+\twhile (colon_ptr < at_ptr) {\n+\t\tstrbuf_addch(&anonymized, '*');\n+\t\tcolon_ptr++;\n+\t}\n+\n+\tstrbuf_addstr(&anonymized, at_ptr);\n+\n+\tif (!strcasecmp(\"warn\", value))\n+\t\twarning(_(\"URL '%s' uses plaintext credentials\"), anonymized.buf);\n+\tif (!strcasecmp(\"die\", value))\n+\t\tdie(_(\"URL '%s' uses plaintext credentials\"), anonymized.buf);\n+\n+cleanup:\n+\tfree(value);\n+\tstrbuf_release(&anonymized);\n+}\n+\n static char *url_normalize_1(const char *url, struct url_info *out_info, char allow_globs)\n {\n \t/*\n@@ -144,6 +185,7 @@ static char *url_normalize_1(const char *url, struct url_info *out_info, char al\n \t */\n \n \tsize_t url_len = strlen(url);\n+\tconst char *orig_url = url;\n \tstruct strbuf norm;\n \tsize_t spanned;\n \tsize_t scheme_len, user_off=0, user_len=0, passwd_off=0, passwd_len=0;\n@@ -191,6 +233,7 @@ static char *url_normalize_1(const char *url, struct url_info *out_info, char al\n \t\t\t}\n \t\t\tcolon_ptr = strchr(norm.buf + scheme_len + 3, ':');\n \t\t\tif (colon_ptr) {\n+\t\t\t\tdetected_credentials_in_url(orig_url, scheme_len);\n \t\t\t\tpasswd_off = (colon_ptr + 1) - norm.buf;\n \t\t\t\tpasswd_len = norm.len - passwd_off;\n \t\t\t\tuser_len = (passwd_off - 1) - (scheme_len + 3);\n\nbase-commit: f9b95943b68b6b8ca5a6072f50a08411c6449b55\n-- \ngitgitgadget\n"},{"id":"455809","messageId":"xmqqr14kqdag.fsf@gitster.g","threadId":"57913","inReplyTo":"pull.1237.git.1653329044940.gitgitgadget@gmail.com","subject":"Re: [PATCH] urlmatch: create fetch.credentialsInUrl config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-23T19:06:31Z","receivedAt":"2022-05-23T19:27:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> Create a new \"fetch.credentialsInUrl\" config option and teach Git to\n> warn or die when seeing a URL with this kind of information. The warning\n> anonymizes the sensitive information of the URL to be clear about the\n> issue.\n\nThe issue sounds vaguely familiar---I must have heard something\nsimilar on this list not in too distant past.\n\n> This change currently defaults the behavior to \"ignore\" which does\n> nothing with these URLs. We can consider changing this behavior to\n> \"warn\" by default if we wish. At that time, we may want to add some\n> advice about setting fetch.credentialsInUrl=ignore for users who still\n> want to follow this pattern (and not receive the warning).\n\nIt sounds more like \"pass\" than \"ignore\", the latter of which can be\nread as \"strip\" instead of \"pass it as-is\".\n\nThe name \"warn\", and its stronger form \"die\", both sound right.\n\n> ... Running the test suite succeeds except for the\n> explicit username:password URLs used in t5550-http-fetch-dumb.s and\n> t5541-http-push-smart.sh. This means that all other tested URLs did not\n> trigger this logic.\n\nWe are not testing the form we are not encouraging, in other words ;-).\n\n> urlmatch: create fetch.credentialsInUrl config\n> \n> This is a modified version of the patch I submitted a while ago [1].\n> \n> Based on the feedback, changing the behavior to fail by default was not\n> a good approach. Further, the idea to stop storing the credentials in\n> config and redirect them to a credential manager was already considered\n> by Peff [2] but not merged.\n\nI just peeked [2] and I am not sure why we didn't X-<. The solution\nthere covers \"git clone\" that records the origin URL but this one\nwould cover URL regardless of where the URL came from---as long as\nan insecure URL is used, we warn or die, and it is even against the\nURL that came from the command line.\n\nIn a sense, I think these are more or less orthogonal. [2]'s \"clone\ncan strip the user:pass from the URL it writes to the config, while\npassing user:pass to the credential API\", especially if it is\nextended to \"git remote add\", would stop two common avenues that\nsuch an insecure URL can go to the configuration file. The approach\ntaken by this patch would complement it to a degree, as long as the\nuser cares.\n\nI am not sure if there is a legitimate case where the user does not\ncare, though. For a script, it may be handy if a URL can contain an\never-changing user:pass pair, where the pass is generated by\nsomething like s/key, for example, and for such a command line that\nknowingly have user:pass pair, having to set the configuration to\n\"ignore\" may be cumbersome.\n\n> +fetch.credentialsInUrl::\n> +\tA URL can contain plaintext credentials in the form\n> +\t`protocol://:@domain/path`. Using such URLs is not\n> +\trecommended as it exposes the password in multiple ways. The\n> +\t`fetch.credentialsInUrl` option provides instruction for how Git\n> +\tshould react to seeing such a URL, with these values:\n> ++\n> +* `ignore` (default): Git will proceed with its activity without warning.\n> +* `warn`: Git will write a warning message to `stderr` when parsing a URL\n> + with a plaintext credential.\n> +* `die`: Git will write a failure message to `stderr` when parsing a URL\n> + with a plaintext credential.\n\nSounds sensible (modulo I would suggest \"ignore\" -> \"pass\").\n\n> +\tgrep \"warning: URL '\\''https://username:\\*\\*\\*\\*\\*\\*\\*\\*@localhost/'\\'' uses plaintext credentials\" err &&\n\nMakes sure that the password part is redacted, which is good.\n\n> +\ttest_must_fail git -c fetch.credentialsInUrl=die clone https://username:password@localhost attempt2 2>err &&\n> +\tgrep \"fatal: URL '\\''https://username:\\*\\*\\*\\*\\*\\*\\*\\*@localhost/'\\'' uses plaintext credentials\" err\n\nDitto.\n\n> diff --git a/urlmatch.c b/urlmatch.c\n> index b615adc923a..6b91fb648a7 100644\n> --- a/urlmatch.c\n> +++ b/urlmatch.c\n> @@ -1,5 +1,6 @@\n> #include \"cache.h\"\n> #include \"urlmatch.h\"\n> +#include \"config.h\"\n\nYuck. Having to do config lookups at this deep a level in the\ncallchain does not look too attractive to me.\n\nI am wondering if we can make it the responsibility of the callers\nto figure out and pass down the settings of the new configuration\nvariable.\n\nOffhand I do not think of an easy and clean way to do so (well,\n\"easy\" is easy---add one to the list of globals in environment.c;\n\"clean\" is the harder part).\n\n> #define URL_ALPHA \"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz\"\n> #define URL_DIGIT \"0123456789\"\n> @@ -106,6 +107,46 @@ static int match_host(const struct url_info *url_info,\n> \treturn (!url_len && !pat_len);\n> }\n> \n> +static void detected_credentials_in_url(const char *url, size_t scheme_len)\n> +{\n> +\tchar *value = NULL;\n> +\tconst char *at_ptr;\n> +\tconst char *colon_ptr;\n> +\tstruct strbuf anonymized = STRBUF_INIT;\n> +\n> +\t/* \"ignore\" is the default behavior. */\n> +\tif (git_config_get_string(\"fetch.credentialsinurl\", &value) ||\n> +\t !strcasecmp(\"ignore\", value))\n> +\t\tgoto cleanup;\n> +\n> +\tat_ptr = strchr(url, '@');\n> +\tcolon_ptr = strchr(url + scheme_len + 3, ':');\n\nWe expect that at_ptr would come after colon_ptr (i.e. in\n\"scheme://:

@host\", no @ exists in or

part) and the\nwhile() loop below assumes that for redacting. Are we better off if\nwe assert it here, or has the calling parser already rejected such\ncases?\n\n> +\tif (!colon_ptr)\n> +\t\tBUG(\"failed to find colon in url '%s' with scheme_len %\"PRIuMAX,\n> +\t\t url, (uintmax_t) scheme_len);\n> +\n> +\t/* Include everything including the colon. */\n> +\tcolon_ptr++;\n> +\tstrbuf_add(&anonymized, url, colon_ptr - url);\n> +\n> +\twhile (colon_ptr < at_ptr) {\n> +\t\tstrbuf_addch(&anonymized, '*');\n> +\t\tcolon_ptr++;\n> +\t}\n> +\n> +\tstrbuf_addstr(&anonymized, at_ptr);\n> +\n> +\tif (!strcasecmp(\"warn\", value))\n> +\t\twarning(_(\"URL '%s' uses plaintext credentials\"), anonymized.buf);\n> +\tif (!strcasecmp(\"die\", value))\n> +\t\tdie(_(\"URL '%s' uses plaintext credentials\"), anonymized.buf);\n> +\n> +cleanup:\n> +\tfree(value);\n> +\tstrbuf_release(&anonymized);\n> +}\n> +\n\nSo far, looking good.\n\n> @@ -144,6 +185,7 @@ static char *url_normalize_1(const char *url, struct url_info *out_info, char al\n> \t */\n> \n> \tsize_t url_len = strlen(url);\n> +\tconst char *orig_url = url;\n> \tstruct strbuf norm;\n> \tsize_t spanned;\n> \tsize_t scheme_len, user_off=0, user_len=0, passwd_off=0, passwd_len=0;\n> @@ -191,6 +233,7 @@ static char *url_normalize_1(const char *url, struct url_info *out_info, char al\n> \t\t\t}\n> \t\t\tcolon_ptr = strchr(norm.buf + scheme_len + 3, ':');\n> \t\t\tif (colon_ptr) {\n> +\t\t\t\tdetected_credentials_in_url(orig_url, scheme_len);\n> \t\t\t\tpasswd_off = (colon_ptr + 1) - norm.buf;\n> \t\t\t\tpasswd_len = norm.len - passwd_off;\n> \t\t\t\tuser_len = (passwd_off - 1) - (scheme_len + 3);\n>\n> base-commit: f9b95943b68b6b8ca5a6072f50a08411c6449b55\n\nThanks.\n"},{"id":"455848","messageId":"7495dac8-0241-ad56-59e3-100673c88c52@github.com","threadId":"57913","inReplyTo":"xmqqr14kqdag.fsf@gitster.g","subject":"Re: [PATCH] urlmatch: create fetch.credentialsInUrl config","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-05-23T20:31:42Z","receivedAt":"2022-05-23T20:31:54Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/23/2022 3:06 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n> \n>> Create a new \"fetch.credentialsInUrl\" config option and teach Git to\n>> warn or die when seeing a URL with this kind of information. The warning\n>> anonymizes the sensitive information of the URL to be clear about the\n>> issue.\n> \n> The issue sounds vaguely familiar---I must have heard something\n> similar on this list not in too distant past.\n\nIt certainly felt like not too distant, but [1] was over a year ago!\n \n>> This change currently defaults the behavior to \"ignore\" which does\n>> nothing with these URLs. We can consider changing this behavior to\n>> \"warn\" by default if we wish. At that time, we may want to add some\n>> advice about setting fetch.credentialsInUrl=ignore for users who still\n>> want to follow this pattern (and not receive the warning).\n> \n> It sounds more like \"pass\" than \"ignore\", the latter of which can be\n> read as \"strip\" instead of \"pass it as-is\".\n\nPerhaps \"allow\" would be more clear than all of these options?\n\n> The name \"warn\", and its stronger form \"die\", both sound right.\n> \n>> ... Running the test suite succeeds except for the\n>> explicit username:password URLs used in t5550-http-fetch-dumb.s and\n>> t5541-http-push-smart.sh. This means that all other tested URLs did not\n>> trigger this logic.\n> \n> We are not testing the form we are not encouraging, in other words ;-).\n\nRight, but in addition we are hopefully testing most of the forms we\n_do_ encourage, and without triggering these warnings.\n\n>> urlmatch: create fetch.credentialsInUrl config\n>> \n>> This is a modified version of the patch I submitted a while ago [1].\n>> \n>> Based on the feedback, changing the behavior to fail by default was not\n>> a good approach. Further, the idea to stop storing the credentials in\n>> config and redirect them to a credential manager was already considered\n>> by Peff [2] but not merged.\n> \n> I just peeked [2] and I am not sure why we didn't X-<. The solution\n> there covers \"git clone\" that records the origin URL but this one\n> would cover URL regardless of where the URL came from---as long as\n> an insecure URL is used, we warn or die, and it is even against the\n> URL that came from the command line.\n\nThe only reason I can guess is that credential helpers were not as\ncommonplace then. Perhaps now is the right time to revisit it with\nthe knowledge that more users have credential helpers for HTTPS URLs\n(or use SSH with registered public keys).\n\n> In a sense, I think these are more or less orthogonal. [2]'s \"clone\n> can strip the user:pass from the URL it writes to the config, while\n> passing user:pass to the credential API\", especially if it is\n> extended to \"git remote add\", would stop two common avenues that\n> such an insecure URL can go to the configuration file. The approach\n> taken by this patch would complement it to a degree, as long as the\n> user cares.\n\nI agree that these are mostly orthogonal. I think that the parsing\nlogic in urlmatch.c would be involved in the \n\n> I am not sure if there is a legitimate case where the user does not\n> care, though. For a script, it may be handy if a URL can contain an\n> ever-changing user:pass pair, where the pass is generated by\n> something like s/key, for example, and for such a command line that\n> knowingly have user:pass pair, having to set the configuration to\n> \"ignore\" may be cumbersome.\n\nWould it make sense to check isatty(2) if we make \"warn\" the default?\nWe could avoid breaking scripts and third-party tools that way.\n\n>> diff --git a/urlmatch.c b/urlmatch.c\n>> index b615adc923a..6b91fb648a7 100644\n>> --- a/urlmatch.c\n>> +++ b/urlmatch.c\n>> @@ -1,5 +1,6 @@\n>> #include \"cache.h\"\n>> #include \"urlmatch.h\"\n>> +#include \"config.h\"\n> \n> Yuck. Having to do config lookups at this deep a level in the\n> callchain does not look too attractive to me.\n> \n> I am wondering if we can make it the responsibility of the callers\n> to figure out and pass down the settings of the new configuration\n> variable.\n> \n> Offhand I do not think of an easy and clean way to do so (well,\n> \"easy\" is easy---add one to the list of globals in environment.c;\n> \"clean\" is the harder part).\n\nEven with something like a global in environment.c, what would\ninitialize it? Would we make it be part of the default Git\nconfig so it is initialized at the start of every builtin? Or,\nwould we initialize the config the first time we parse a URL.\n\nWith that in mind, it might be good to have a static enum that\nstores the parsed config value and uses that immediately instead\nof calling git_config_get_string() repeatedly. Are there places\nwhere we might inspect a huge number of URLs?\n\n>> #define URL_ALPHA \"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz\"\n>> #define URL_DIGIT \"0123456789\"\n>> @@ -106,6 +107,46 @@ static int match_host(const struct url_info *url_info,\n>> \treturn (!url_len && !pat_len);\n>> }\n>> \n>> +static void detected_credentials_in_url(const char *url, size_t scheme_len)\n>> +{\n>> +\tchar *value = NULL;\n>> +\tconst char *at_ptr;\n>> +\tconst char *colon_ptr;\n>> +\tstruct strbuf anonymized = STRBUF_INIT;\n>> +\n>> +\t/* \"ignore\" is the default behavior. */\n>> +\tif (git_config_get_string(\"fetch.credentialsinurl\", &value) ||\n>> +\t !strcasecmp(\"ignore\", value))\n>> +\t\tgoto cleanup;\n>> +\n>> +\tat_ptr = strchr(url, '@');\n>> +\tcolon_ptr = strchr(url + scheme_len + 3, ':');\n> \n> We expect that at_ptr would come after colon_ptr (i.e. in\n> \"scheme://:

@host\", no @ exists in or

part) and the\n> while() loop below assumes that for redacting. Are we better off if\n> we assert it here, or has the calling parser already rejected such\n> cases?\n\nThis computation of at_ptr matches the one in url_normalize_1(),\nso it at least agrees about where the \"username[:password]\" section\ncould be. That does mean that the password cannot contain an \"@\"\nsymbol (unless it is special-cased somehow?).\n\nThanks,\n-Stolee\n"},{"id":"455850","messageId":"xmqqwnecoui1.fsf@gitster.g","threadId":"57913","inReplyTo":"xmqqr14kqdag.fsf@gitster.g","subject":"Re: [PATCH] urlmatch: create fetch.credentialsInUrl config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-23T20:37:42Z","receivedAt":"2022-05-23T20:37:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> It sounds more like \"pass\" than \"ignore\", the latter of which can be\n> read as \"strip\" instead of \"pass it as-is\".\n\nOr \"allow\" (which I prefer over \"pass\").\n\n> The name \"warn\", and its stronger form \"die\", both sound right.\n"},{"id":"455852","messageId":"xmqqo7zoostd.fsf@gitster.g","threadId":"57913","inReplyTo":"7495dac8-0241-ad56-59e3-100673c88c52@github.com","subject":"Re: [PATCH] urlmatch: create fetch.credentialsInUrl config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-23T21:14:06Z","receivedAt":"2022-05-23T21:14:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> This computation of at_ptr matches the one in url_normalize_1(),\n> so it at least agrees about where the \"username[:password]\" section\n> could be.\n\nOK.\n\n> That does mean that the password cannot contain an \"@\"\n> symbol (unless it is special-cased somehow?).\n\nI wasn't worried about what is valid but more about what attackers\ncan fool users to throw at \"git clone\" and make our code misbehave\n(which can include garbage that would not parse correctly).\n\nI think the while() loop will just become a no-op, anonymized buffer\nis left empty and colon_ptr does not get updated at all. Then\nstrbuf_addstr() after the loop will put everything from '@' to the\nstrbuf to be showed, and none of these should lead to any overrun or\nexploit.\n\nThanks.\n"},{"id":"455878","messageId":"220524.8635gzxr9h.gmgdl@evledraar.gmail.com","threadId":"57913","inReplyTo":"pull.1237.git.1653329044940.gitgitgadget@gmail.com","subject":"Re: [PATCH] urlmatch: create fetch.credentialsInUrl config","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-24T08:18:13Z","receivedAt":"2022-05-24T08:35:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, May 23 2022, Derrick Stolee via GitGitGadget wrote:\n\n> +fetch.credentialsInUrl::\n> +\tA URL can contain plaintext credentials in the form\n> +\t`protocol://:@domain/path`. Using such URLs is not\n> +\trecommended as it exposes the password in multiple ways. The\n> +\t`fetch.credentialsInUrl` option provides instruction for how Git\n> +\tshould react to seeing such a URL, with these values:\n\nRe the previous discussion about this (in the v1 patch /\nhttps://lore.kernel.org/git/pull.945.git.1619807844627.gitgitgadget@gmail.com/):\nIn what ways?\n\nThat's rhetorical, the point being: Let's adjust this documentation to\ndiscuss exactly why this is thought to be bad, what we're mitigating for\nthe user etc., are there situations where running git like this is\nperfectly fine & not thought to be an issue? E.g. no password manager\nand you trust your FS permission? Let's cover those cases too.\n\n> ++\n> +* `ignore` (default): Git will proceed with its activity without warning.\n> +* `warn`: Git will write a warning message to `stderr` when parsing a URL\n> + with a plaintext credential.\n> +* `die`: Git will write a failure message to `stderr` when parsing a URL\n> + with a plaintext credential.\n\nYou're implementing this with strcasecmp, so we also support DIE, DiE\netc. Let's not do that and use strcmp() instead.\n\n> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\n> index 4a61f2c901e..34be520b783 100755\n> --- a/t/t5601-clone.sh\n> +++ b/t/t5601-clone.sh\n> @@ -71,6 +71,13 @@ test_expect_success 'clone respects GIT_WORK_TREE' '\n> \n> '\n> \n> +test_expect_success 'clone warns or fails when using username:password' '\n> +\ttest_must_fail git -c fetch.credentialsInUrl=warn clone https://username:password@localhost attempt1 2>err &&\n> +\tgrep \"warning: URL '\\''https://username:\\*\\*\\*\\*\\*\\*\\*\\*@localhost/'\\'' uses plaintext credentials\" err &&\n> +\ttest_must_fail git -c fetch.credentialsInUrl=die clone https://username:password@localhost attempt2 2>err &&\n> +\tgrep \"fatal: URL '\\''https://username:\\*\\*\\*\\*\\*\\*\\*\\*@localhost/'\\'' uses plaintext credentials\" err\n> +'\n> +\n> test_expect_success 'clone from hooks' '\n> \n> \ttest_create_repo r0 &&\n> diff --git a/urlmatch.c b/urlmatch.c\n> index b615adc923a..6b91fb648a7 100644\n> --- a/urlmatch.c\n> +++ b/urlmatch.c\n> @@ -1,5 +1,6 @@\n> #include \"cache.h\"\n> #include \"urlmatch.h\"\n> +#include \"config.h\"\n> \n> #define URL_ALPHA \"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz\"\n> #define URL_DIGIT \"0123456789\"\n> @@ -106,6 +107,46 @@ static int match_host(const struct url_info *url_info,\n> \treturn (!url_len && !pat_len);\n> }\n> \n> +static void detected_credentials_in_url(const char *url, size_t scheme_len)\n> +{\n\nJust generally: This is only \n\n> +\tchar *value = NULL;\n\nThis init to NULL should be removedd, as we....\n\n> +\tconst char *at_ptr;\n> +\tconst char *colon_ptr;\n> +\tstruct strbuf anonymized = STRBUF_INIT;\n\nnit: Just call this \"sb\"? The's at least one line below over 79\ncharacters that's within the bounds with a shorter variable name, and in\nthis case it's obvious what we're doing here...\n> +\n> +\t/* \"ignore\" is the default behavior. */\n> +\tif (git_config_get_string(\"fetch.credentialsinurl\", &value) ||\n\n...initialize it here, and if we didn't the compiler would have a chance\nto spot that if we were getting it wrong.\n\n> +\t !strcasecmp(\"ignore\", value))\n> +\t\tgoto cleanup;\n> +\n> +\tat_ptr = strchr(url, '@');\n> +\tcolon_ptr = strchr(url + scheme_len + 3, ':');\n> +\n> +\tif (!colon_ptr)\n> +\t\tBUG(\"failed to find colon in url '%s' with scheme_len %\"PRIuMAX,\n> +\t\t url, (uintmax_t) scheme_len);\n> +\n> +\t/* Include everything including the colon. */\n> +\tcolon_ptr++;\n> +\tstrbuf_add(&anonymized, url, colon_ptr - url);\n> +\n> +\twhile (colon_ptr < at_ptr) {\n> +\t\tstrbuf_addch(&anonymized, '*');\n> +\t\tcolon_ptr++;\n> +\t}\n\nCould we share code with 88e9b1e3fcb (fetch-pack: redact packfile urls\nin traces, 2021-11-10), or for consistency note this as \ninstead of stripping it out, as we do for that related URL-part\nredaction?\n\n> +\tstrbuf_addstr(&anonymized, at_ptr);\n\nMaybe not worth it, but I wondered if we couldn't just use curl for\nthis, turns out it has an API for it:\nhttps://curl.se/libcurl/c/libcurl-url.html\n\nBut it's too new for us to rely on unconditionally, but we could add\nthat to git-curl-compat.h and ifdef it, then we'll eventually drop this\ncustom code for ryling on the well-tested library.\n\nI think doing that would be worth it, to show future authors that curl\ncan do this, so maybe we can start relying on that eventually...\n\n> +\tif (!strcasecmp(\"warn\", value))\n> +\t\twarning(_(\"URL '%s' uses plaintext credentials\"), anonymized.buf);\n> +\tif (!strcasecmp(\"die\", value))\n> +\t\tdie(_(\"URL '%s' uses plaintext credentials\"), anonymized.buf);\n> +\n> +cleanup:\n> +\tfree(value);\n\nI think you can also just use git_config_get_string_tmp() here and avoid\nthe alloc/free. That's safe as long as you're not calling other config\nAPI in-between, which you're not.\n"},{"id":"455884","messageId":"nycvar.QRO.7.76.6.2205241339440.352@tvgsbejvaqbjf.bet","threadId":"57913","inReplyTo":"pull.1237.git.1653329044940.gitgitgadget@gmail.com","subject":"Re: [PATCH] urlmatch: create fetch.credentialsInUrl config","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-05-24T11:42:42Z","receivedAt":"2022-05-24T11:44:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stolee,\n\nOn Mon, 23 May 2022, Derrick Stolee via GitGitGadget wrote:\n\n> diff --git a/urlmatch.c b/urlmatch.c\n> index b615adc923a..6b91fb648a7 100644\n> --- a/urlmatch.c\n> +++ b/urlmatch.c\n> +static void detected_credentials_in_url(const char *url, size_t scheme_len)\n> +{\n> +\tchar *value = NULL;\n> +\tconst char *at_ptr;\n> +\tconst char *colon_ptr;\n> +\tstruct strbuf anonymized = STRBUF_INIT;\n> +\n> +\t/* \"ignore\" is the default behavior. */\n> +\tif (git_config_get_string(\"fetch.credentialsinurl\", &value) ||\n> +\t !strcasecmp(\"ignore\", value))\n> +\t\tgoto cleanup;\n> +\n> +\tat_ptr = strchr(url, '@');\n> +\tcolon_ptr = strchr(url + scheme_len + 3, ':');\n\nHow certain are we that `url + scheme_len + 3` is still inside the\nNUL-separated `url`?\n\n> +\n> +\tif (!colon_ptr)\n> +\t\tBUG(\"failed to find colon in url '%s' with scheme_len %\"PRIuMAX,\n> +\t\t url, (uintmax_t) scheme_len);\n\nWouldn't this mean that `https://github.com/git/git` with a `scheme_len`\nof 5 would hit that `BUG()` code path?\n\nThanks,\nDscho\n\n> +\n> +\t/* Include everything including the colon. */\n> +\tcolon_ptr++;\n> +\tstrbuf_add(&anonymized, url, colon_ptr - url);\n> +\n> +\twhile (colon_ptr < at_ptr) {\n> +\t\tstrbuf_addch(&anonymized, '*');\n> +\t\tcolon_ptr++;\n> +\t}\n> +\n> +\tstrbuf_addstr(&anonymized, at_ptr);\n> +\n> +\tif (!strcasecmp(\"warn\", value))\n> +\t\twarning(_(\"URL '%s' uses plaintext credentials\"), anonymized.buf);\n> +\tif (!strcasecmp(\"die\", value))\n> +\t\tdie(_(\"URL '%s' uses plaintext credentials\"), anonymized.buf);\n> +\n> +cleanup:\n> +\tfree(value);\n> +\tstrbuf_release(&anonymized);\n> +}\n> +\n> static char *url_normalize_1(const char *url, struct url_info *out_info, char allow_globs)\n> {\n> \t/*\n"},{"id":"455885","messageId":"nycvar.QRO.7.76.6.2205241343390.352@tvgsbejvaqbjf.bet","threadId":"57913","inReplyTo":"7495dac8-0241-ad56-59e3-100673c88c52@github.com","subject":"Re: [PATCH] urlmatch: create fetch.credentialsInUrl config","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-05-24T11:46:43Z","receivedAt":"2022-05-24T11:47:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stolee,\n\nOn Mon, 23 May 2022, Derrick Stolee wrote:\n\n> On 5/23/2022 3:06 PM, Junio C Hamano wrote:\n> > \"Derrick Stolee via GitGitGadget\" writes:\n> >\n> >> +static void detected_credentials_in_url(const char *url, size_t scheme_len)\n> >> +{\n> >> +\tchar *value = NULL;\n> >> +\tconst char *at_ptr;\n> >> +\tconst char *colon_ptr;\n> >> +\tstruct strbuf anonymized = STRBUF_INIT;\n> >> +\n> >> +\t/* \"ignore\" is the default behavior. */\n> >> +\tif (git_config_get_string(\"fetch.credentialsinurl\", &value) ||\n> >> +\t !strcasecmp(\"ignore\", value))\n> >> +\t\tgoto cleanup;\n> >> +\n> >> +\tat_ptr = strchr(url, '@');\n> >> +\tcolon_ptr = strchr(url + scheme_len + 3, ':');\n> >\n> > We expect that at_ptr would come after colon_ptr (i.e. in\n> > \"scheme://:

@host\", no @ exists in or

part) and the\n> > while() loop below assumes that for redacting.\n\nCareful here. https://me@there.com:9999/ _is_ a valid URL, too.\n\n> > Are we better off if we assert it here, or has the calling parser\n> > already rejected such cases?\n>\n> This computation of at_ptr matches the one in url_normalize_1(),\n> so it at least agrees about where the \"username[:password]\" section\n> could be. That does mean that the password cannot contain an \"@\"\n> symbol (unless it is special-cased somehow?).\n\nThe password cannot contain a literal `@`, and neither can the user name.\nThey have to be URL-encoded, via `%40`.\n\nCiao,\nDscho\n"},{"id":"455887","messageId":"nycvar.QRO.7.76.6.2205241346590.352@tvgsbejvaqbjf.bet","threadId":"57913","inReplyTo":"xmqqr14kqdag.fsf@gitster.g","subject":"Re: [PATCH] urlmatch: create fetch.credentialsInUrl config","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-05-24T11:51:40Z","receivedAt":"2022-05-24T11:52:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 23 May 2022, Junio C Hamano wrote:\n\n> \"Derrick Stolee via GitGitGadget\" writes:\n>\n> > urlmatch: create fetch.credentialsInUrl config\n> >\n> > This is a modified version of the patch I submitted a while ago [1].\n> >\n> > Based on the feedback, changing the behavior to fail by default was not\n> > a good approach. Further, the idea to stop storing the credentials in\n> > config and redirect them to a credential manager was already considered\n> > by Peff [2] but not merged.\n>\n> I just peeked [2] and I am not sure why we didn't X-<. The solution\n> there covers \"git clone\" that records the origin URL but this one\n> would cover URL regardless of where the URL came from---as long as\n> an insecure URL is used, we warn or die, and it is even against the\n> URL that came from the command line.\n>\n> In a sense, I think these are more or less orthogonal. [2]'s \"clone\n> can strip the user:pass from the URL it writes to the config, while\n> passing user:pass to the credential API\", especially if it is\n> extended to \"git remote add\", would stop two common avenues that\n> such an insecure URL can go to the configuration file. The approach\n> taken by this patch would complement it to a degree, as long as the\n> user cares.\n>\n> I am not sure if there is a legitimate case where the user does not\n> care, though. For a script, it may be handy if a URL can contain an\n> ever-changing user:pass pair, where the pass is generated by\n> something like s/key, for example, and for such a command line that\n> knowingly have user:pass pair, having to set the configuration to\n> \"ignore\" may be cumbersome.\n\nTo provide one data point: a few of Git for Windows' automated builds use\nthe `https://user@pass:host/` form to clone and push, using a Personal\nAccess Token as password (that is of course marked as a secret, read: it\nwill get redacted out of the logs).\n\nSo yes, there are scripts that rely on Git's current behavior to work.\n\nIf Git changes behavior, I will have to adjust those build definitions.\n\nIn this instance, I believe that the benefit of safeguarding Git's users\noutweighs the burden of having to adjust such scripts/definitions.\n\nCiao,\nDscho\n"},{"id":"455901","messageId":"0dc38d46-7c5a-5532-c843-ef3c8b6cfa28@github.com","threadId":"57913","inReplyTo":"220524.8635gzxr9h.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] urlmatch: create fetch.credentialsInUrl config","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-05-24T13:50:03Z","receivedAt":"2022-05-24T13:50:09Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/24/2022 4:18 AM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Mon, May 23 2022, Derrick Stolee via GitGitGadget wrote:\n> \n>> +fetch.credentialsInUrl::\n>> +\tA URL can contain plaintext credentials in the form\n>> +\t`protocol://:@domain/path`. Using such URLs is not\n>> +\trecommended as it exposes the password in multiple ways. The\n>> +\t`fetch.credentialsInUrl` option provides instruction for how Git\n>> +\tshould react to seeing such a URL, with these values:\n> \n> Re the previous discussion about this (in the v1 patch /\n> https://lore.kernel.org/git/pull.945.git.1619807844627.gitgitgadget@gmail.com/):\n> In what ways?\n> \n> That's rhetorical, the point being: Let's adjust this documentation to\n> discuss exactly why this is thought to be bad, what we're mitigating for\n> the user etc., are there situations where running git like this is\n> perfectly fine & not thought to be an issue? E.g. no password manager\n> and you trust your FS permission? Let's cover those cases too.\n\nThis documentation is not the proper place to tell the user \"do this\nand you can trust your plaintext creds in the filesystem\" because that\nis asking for problems. I'd rather leave a vague warning and let users\ngo against the recommended behavior only after they have done sufficient\nwork to be confident in taking on that risk.\n \n>> ++\n>> +* `ignore` (default): Git will proceed with its activity without warning.\n>> +* `warn`: Git will write a warning message to `stderr` when parsing a URL\n>> + with a plaintext credential.\n>> +* `die`: Git will write a failure message to `stderr` when parsing a URL\n>> + with a plaintext credential.\n> \n> You're implementing this with strcasecmp, so we also support DIE, DiE\n> etc. Let's not do that and use strcmp() instead.\n\nSure.\n\n>> +static void detected_credentials_in_url(const char *url, size_t scheme_len)\n>> +{\n> \n> Just generally: This is only \n\nDid you intend to say more here?\n\n>> +\tchar *value = NULL;\n> \n> This init to NULL should be removedd, as we....\n> \n>> +\tconst char *at_ptr;\n>> +\tconst char *colon_ptr;\n>> +\tstruct strbuf anonymized = STRBUF_INIT;\n> \n> nit: Just call this \"sb\"? The's at least one line below over 79\n> characters that's within the bounds with a shorter variable name, and in\n> this case it's obvious what we're doing here...\n\nI will not change this name to be less descriptive.\n\n>> +\n>> +\t/* \"ignore\" is the default behavior. */\n>> +\tif (git_config_get_string(\"fetch.credentialsinurl\", &value) ||\n> \n> ...initialize it here, and if we didn't the compiler would have a chance\n> to spot that if we were getting it wrong.\n\nWe do not necessarily initialize it here. The compiler doesn't notice\nit and the free(value) below segfaults.\n\n>> +\t !strcasecmp(\"ignore\", value))\n>> +\t\tgoto cleanup;\n>> +\n>> +\tat_ptr = strchr(url, '@');\n>> +\tcolon_ptr = strchr(url + scheme_len + 3, ':');\n>> +\n>> +\tif (!colon_ptr)\n>> +\t\tBUG(\"failed to find colon in url '%s' with scheme_len %\"PRIuMAX,\n>> +\t\t url, (uintmax_t) scheme_len);\n>> +\n>> +\t/* Include everything including the colon. */\n>> +\tcolon_ptr++;\n>> +\tstrbuf_add(&anonymized, url, colon_ptr - url);\n>> +\n>> +\twhile (colon_ptr < at_ptr) {\n>> +\t\tstrbuf_addch(&anonymized, '*');\n>> +\t\tcolon_ptr++;\n>> +\t}\n> \n> Could we share code with 88e9b1e3fcb (fetch-pack: redact packfile urls\n> in traces, 2021-11-10), or for consistency note this as \n> instead of stripping it out, as we do for that related URL-part\n> redaction?\n\nI'm happy to replace the asterisks with . Otherwise, I don't\nsee anything we can do to share across these methods. Specifically,\nthe test in that commit seems to indicate that the redacted portion is\nonly the packfile name (the $HTTPD_URL is not filtered).\n\n>> +\tstrbuf_addstr(&anonymized, at_ptr);\n> \n> Maybe not worth it, but I wondered if we couldn't just use curl for\n> this, turns out it has an API for it:\n> https://curl.se/libcurl/c/libcurl-url.html\n> \n> But it's too new for us to rely on unconditionally, but we could add\n> that to git-curl-compat.h and ifdef it, then we'll eventually drop this\n> custom code for ryling on the well-tested library.\n> \n> I think doing that would be worth it, to show future authors that curl\n> can do this, so maybe we can start relying on that eventually...\n\nSince we can't rely on it, I'll leave that to another (you, perhaps?)\nto do that ifdef work. I don't think it's worth it right now.\n\n>> +\tif (!strcasecmp(\"warn\", value))\n>> +\t\twarning(_(\"URL '%s' uses plaintext credentials\"), anonymized.buf);\n>> +\tif (!strcasecmp(\"die\", value))\n>> +\t\tdie(_(\"URL '%s' uses plaintext credentials\"), anonymized.buf);\n>> +\n>> +cleanup:\n>> +\tfree(value);\n> \n> I think you can also just use git_config_get_string_tmp() here and avoid\n> the alloc/free. That's safe as long as you're not calling other config\n> API in-between, which you're not.\n\nOK. And that also avoids the need for initialization you mentioned.\n\nThanks,\n-Stolee\n"},{"id":"455947","messageId":"9318a400-479f-4029-c10b-3bfe498fcbb7@github.com","threadId":"57913","inReplyTo":"nycvar.QRO.7.76.6.2205241343390.352@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] urlmatch: create fetch.credentialsInUrl config","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-05-24T20:14:08Z","receivedAt":"2022-05-24T20:14:18Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/24/2022 7:46 AM, Johannes Schindelin wrote:\n> Hi Stolee,\n> \n> On Mon, 23 May 2022, Derrick Stolee wrote:\n> \n>> On 5/23/2022 3:06 PM, Junio C Hamano wrote:\n>>> \"Derrick Stolee via GitGitGadget\" writes:\n>>>\n>>>> +static void detected_credentials_in_url(const char *url, size_t scheme_len)\n>>>> +{\n>>>> +\tchar *value = NULL;\n>>>> +\tconst char *at_ptr;\n>>>> +\tconst char *colon_ptr;\n>>>> +\tstruct strbuf anonymized = STRBUF_INIT;\n>>>> +\n>>>> +\t/* \"ignore\" is the default behavior. */\n>>>> +\tif (git_config_get_string(\"fetch.credentialsinurl\", &value) ||\n>>>> +\t !strcasecmp(\"ignore\", value))\n>>>> +\t\tgoto cleanup;\n>>>> +\n>>>> +\tat_ptr = strchr(url, '@');\n>>>> +\tcolon_ptr = strchr(url + scheme_len + 3, ':');\n>>>\n>>> We expect that at_ptr would come after colon_ptr (i.e. in\n>>> \"scheme://:

@host\", no @ exists in or

part) and the\n>>> while() loop below assumes that for redacting.\n> \n> Careful here. https://me@there.com:9999/ _is_ a valid URL, too.\n\nThanks for checking. The method should not be called unless the\npassword region was already detected. I'll add a BUG() statement and\na comment to prevent future callers from providing incorrect URLs.\n\nI can also add a test to show that this warning is not output when\nthe only colon is for the port number.\n \n>>> Are we better off if we assert it here, or has the calling parser\n>>> already rejected such cases?\n>>\n>> This computation of at_ptr matches the one in url_normalize_1(),\n>> so it at least agrees about where the \"username[:password]\" section\n>> could be. That does mean that the password cannot contain an \"@\"\n>> symbol (unless it is special-cased somehow?).\n> \n> The password cannot contain a literal `@`, and neither can the user name.\n> They have to be URL-encoded, via `%40`.\n\nThanks!\n-Stolee\n"},{"id":"455948","messageId":"14f71d47-e2c1-2a43-6434-af6f23d18754@github.com","threadId":"57913","inReplyTo":"nycvar.QRO.7.76.6.2205241339440.352@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] urlmatch: create fetch.credentialsInUrl config","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-05-24T20:16:08Z","receivedAt":"2022-05-24T20:16:15Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/24/2022 7:42 AM, Johannes Schindelin wrote:\n> Hi Stolee,\n> \n> On Mon, 23 May 2022, Derrick Stolee via GitGitGadget wrote:\n>> +\tat_ptr = strchr(url, '@');\n>> +\tcolon_ptr = strchr(url + scheme_len + 3, ':');\n> \n> How certain are we that `url + scheme_len + 3` is still inside the\n> NUL-separated `url`?\n\nI'll update the method comment to make this clear.\n \n>> +\n>> +\tif (!colon_ptr)\n>> +\t\tBUG(\"failed to find colon in url '%s' with scheme_len %\"PRIuMAX,\n>> +\t\t url, (uintmax_t) scheme_len);\n> \n> Wouldn't this mean that `https://github.com/git/git` with a `scheme_len`\n> of 5 would hit that `BUG()` code path?\n\nYes. The method is about what to do once we've detected a URL\nwith a \"username:password@\" combination after the scheme. I\nmentioned in a different reply, but I'll make this clear with\na comment.\n\nThanks,\n-Stolee\n"},{"id":"455962","messageId":"220524.86ilpuvcqh.gmgdl@evledraar.gmail.com","threadId":"57913","inReplyTo":"0dc38d46-7c5a-5532-c843-ef3c8b6cfa28@github.com","subject":"Re: [PATCH] urlmatch: create fetch.credentialsInUrl config","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-24T21:01:35Z","receivedAt":"2022-05-24T21:32:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, May 24 2022, Derrick Stolee wrote:\n\n> On 5/24/2022 4:18 AM, Ævar Arnfjörð Bjarmason wrote:\n>> \n>> On Mon, May 23 2022, Derrick Stolee via GitGitGadget wrote:\n>> \n>>> +fetch.credentialsInUrl::\n>>> +\tA URL can contain plaintext credentials in the form\n>>> +\t`protocol://:@domain/path`. Using such URLs is not\n>>> +\trecommended as it exposes the password in multiple ways. The\n>>> +\t`fetch.credentialsInUrl` option provides instruction for how Git\n>>> +\tshould react to seeing such a URL, with these values:\n>> \n>> Re the previous discussion about this (in the v1 patch /\n>> https://lore.kernel.org/git/pull.945.git.1619807844627.gitgitgadget@gmail.com/):\n>> In what ways?\n>> \n>> That's rhetorical, the point being: Let's adjust this documentation to\n>> discuss exactly why this is thought to be bad, what we're mitigating for\n>> the user etc., are there situations where running git like this is\n>> perfectly fine & not thought to be an issue? E.g. no password manager\n>> and you trust your FS permission? Let's cover those cases too.\n>\n> This documentation is not the proper place to tell the user \"do this\n> and you can trust your plaintext creds in the filesystem\" because that\n> is asking for problems. I'd rather leave a vague warning and let users\n> go against the recommended behavior only after they have done sufficient\n> work to be confident in taking on that risk.\n\nI don't mean that we need to cover the full divergent views on different\napproaches to local password management, but not leave the user hanging\nwith the rather scary \"exposes the password in multiple ways\".\n\nI.e. if I read that for any software whose implementation I wasn't very\nfamiliar with I'd be very afraid, and in git's case for no reason.\n\nDoes in mean that git has some scary git-specific feature that would\nexpose it. perhaps there's a local log that's unsecured where attempted\nURLs are logged, or perhaps we send the raw requested URL to the server\nso it can suggest alternatives for us. We do neither, but even a\ngenerally knowledgeable user won't know that about git in particular.\n\nWhereas what I think you actually mean and are targeting here is better\nexplained by:\n\n Git is careful to avoid exposing passwords in URLs on its own,\n e.g. they won't be logged in trace2 logs. This setting is intended\n for those who'd like to discourage (warn) or enforce (die) the use\n of the password helper infrastructure over hardcoded passwords.\n\nAll of which I *think* is correct, but maybe I've missed something you\nknow about, as that \"in multiple ways\" is doing a lot of work.\n\nI also wonder if this wouldn't be even more useful if we took some\nlessons from ssh's book. I.e. per \"git config -l --show-origin\" we know\nthe original of all config. We could be even more useful (and more\naggressive about warning about) cases where we have passwords in config\nfiles that we detect don't have restrictive permissions, as OpenSSH does\nwith your private key.\n\nDitto perhaps when the origin is \"command line\", as we do nothing to\nhide that from the process list on shared systems (and that would be\nracy whatever we did).\n\n>>> ++\n>>> +* `ignore` (default): Git will proceed with its activity without warning.\n>>> +* `warn`: Git will write a warning message to `stderr` when parsing a URL\n>>> + with a plaintext credential.\n>>> +* `die`: Git will write a failure message to `stderr` when parsing a URL\n>>> + with a plaintext credential.\n>> \n>> You're implementing this with strcasecmp, so we also support DIE, DiE\n>> etc. Let's not do that and use strcmp() instead.\n>\n> Sure.\n>\n>>> +static void detected_credentials_in_url(const char *url, size_t scheme_len)\n>>> +{\n>> \n>> Just generally: This is only \n>\n> Did you intend to say more here?\n\nProbably, but if I did I forgot about it, by now. Sorry.\n\n>>> +\tchar *value = NULL;\n>> \n>> This init to NULL should be removedd, as we....\n>> \n>>> +\tconst char *at_ptr;\n>>> +\tconst char *colon_ptr;\n>>> +\tstruct strbuf anonymized = STRBUF_INIT;\n>> \n>> nit: Just call this \"sb\"? The's at least one line below over 79\n>> characters that's within the bounds with a shorter variable name, and in\n>> this case it's obvious what we're doing here...\n>\n> I will not change this name to be less descriptive.\n\nSure, just a suggestion. The other way is to just re-wrap that one\nline... :)\n\nIn the end I don't care, \"just a nit\", but just as one datapoint from\nreading this code: I find this varibale name in particular to be the\npolar opposite of descriptive, we're explicitly not anonymizing the URL\nin this function, since we're not stripping the username part.\n\nWouldn't descriptive be something more like uri_redacted_password or\nuri_no_password in this case?\n\n>>> +\n>>> +\t/* \"ignore\" is the default behavior. */\n>>> +\tif (git_config_get_string(\"fetch.credentialsinurl\", &value) ||\n>> \n>> ...initialize it here, and if we didn't the compiler would have a chance\n>> to spot that if we were getting it wrong.\n>\n> We do not necessarily initialize it here. The compiler doesn't notice\n> it and the free(value) below segfaults.\n\nYes, sorry I meant in combination with the *_tmp() variant below...\n\n>>> +\t !strcasecmp(\"ignore\", value))\n>>> +\t\tgoto cleanup;\n>>> +\n>>> +\tat_ptr = strchr(url, '@');\n>>> +\tcolon_ptr = strchr(url + scheme_len + 3, ':');\n>>> +\n>>> +\tif (!colon_ptr)\n>>> +\t\tBUG(\"failed to find colon in url '%s' with scheme_len %\"PRIuMAX,\n>>> +\t\t url, (uintmax_t) scheme_len);\n>>> +\n>>> +\t/* Include everything including the colon. */\n>>> +\tcolon_ptr++;\n>>> +\tstrbuf_add(&anonymized, url, colon_ptr - url);\n>>> +\n>>> +\twhile (colon_ptr < at_ptr) {\n>>> +\t\tstrbuf_addch(&anonymized, '*');\n>>> +\t\tcolon_ptr++;\n>>> +\t}\n>> \n>> Could we share code with 88e9b1e3fcb (fetch-pack: redact packfile urls\n>> in traces, 2021-11-10), or for consistency note this as \n>> instead of stripping it out, as we do for that related URL-part\n>> redaction?\n>\n> I'm happy to replace the asterisks with . Otherwise, I don't\n> see anything we can do to share across these methods.\n\nYes, I just meant adding a \"\". I briefly looked at whether it\nmade sense to share the implementation, but I think probably not.\n\nI didn't think of this at the time but your implementation also leaks\nthe length of the password, which will solve in any case.\n\nJust for the implementation: It's slightly more wasteful, but in this\ncase we don't care about performance, so perhaps a strbuf_splice()\nvariant is easier here? I.e. add the full URL, find : and @, then\nstrbuf_splice() it. It gets rid of much of the pointer juggling here &\nadding things incrementally.\n\n> Specifically,\n> the test in that commit seems to indicate that the redacted portion is\n> only the packfile name (the $HTTPD_URL is not filtered).\n\nBy HTTPD_URL it means \"the path\", i.e. it's the equivalent of stripping\nCURLUPART_{PATH,QUERY,FRAGMENT}.\n\nSo a hypothetical shared implementation would just be a matter of\nsearching for the '/' once we're past the (optional) '@', but better to\nleave it for now.\n\n>>> +\tstrbuf_addstr(&anonymized, at_ptr);\n>> \n>> Maybe not worth it, but I wondered if we couldn't just use curl for\n>> this, turns out it has an API for it:\n>> https://curl.se/libcurl/c/libcurl-url.html\n>> \n>> But it's too new for us to rely on unconditionally, but we could add\n>> that to git-curl-compat.h and ifdef it, then we'll eventually drop this\n>> custom code for ryling on the well-tested library.\n>> \n>> I think doing that would be worth it, to show future authors that curl\n>> can do this, so maybe we can start relying on that eventually...\n>\n> Since we can't rely on it, I'll leave that to another (you, perhaps?)\n> to do that ifdef work. I don't think it's worth it right now.\n\nYeah, probably not.\n\n>>> +\tif (!strcasecmp(\"warn\", value))\n>>> +\t\twarning(_(\"URL '%s' uses plaintext credentials\"), anonymized.buf);\n>>> +\tif (!strcasecmp(\"die\", value))\n>>> +\t\tdie(_(\"URL '%s' uses plaintext credentials\"), anonymized.buf);\n>>> +\n>>> +cleanup:\n>>> +\tfree(value);\n>> \n>> I think you can also just use git_config_get_string_tmp() here and avoid\n>> the alloc/free. That's safe as long as you're not calling other config\n>> API in-between, which you're not.\n>\n> OK. And that also avoids the need for initialization you mentioned.\n\n*nod*\n"},{"id":"456068","messageId":"b9eff859-ee1a-6cfd-7d83-e24284c14eec@github.com","threadId":"57913","inReplyTo":"220524.86ilpuvcqh.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] urlmatch: create fetch.credentialsInUrl config","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-05-25T14:03:43Z","receivedAt":"2022-05-25T14:07:08Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/24/2022 5:01 PM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Tue, May 24 2022, Derrick Stolee wrote:\n> \n>> On 5/24/2022 4:18 AM, Ævar Arnfjörð Bjarmason wrote:\n>>>\n>>> On Mon, May 23 2022, Derrick Stolee via GitGitGadget wrote:\n>>>\n>>>> +fetch.credentialsInUrl::\n>>>> +\tA URL can contain plaintext credentials in the form\n>>>> +\t`protocol://:@domain/path`. Using such URLs is not\n>>>> +\trecommended as it exposes the password in multiple ways. The\n>>>> +\t`fetch.credentialsInUrl` option provides instruction for how Git\n>>>> +\tshould react to seeing such a URL, with these values:\n>>>\n>>> Re the previous discussion about this (in the v1 patch /\n>>> https://lore.kernel.org/git/pull.945.git.1619807844627.gitgitgadget@gmail.com/):\n>>> In what ways?\n>>>\n>>> That's rhetorical, the point being: Let's adjust this documentation to\n>>> discuss exactly why this is thought to be bad, what we're mitigating for\n>>> the user etc., are there situations where running git like this is\n>>> perfectly fine & not thought to be an issue? E.g. no password manager\n>>> and you trust your FS permission? Let's cover those cases too.\n>>\n>> This documentation is not the proper place to tell the user \"do this\n>> and you can trust your plaintext creds in the filesystem\" because that\n>> is asking for problems. I'd rather leave a vague warning and let users\n>> go against the recommended behavior only after they have done sufficient\n>> work to be confident in taking on that risk.\n> \n> I don't mean that we need to cover the full divergent views on different\n> approaches to local password management, but not leave the user hanging\n> with the rather scary \"exposes the password in multiple ways\".\n> \n> I.e. if I read that for any software whose implementation I wasn't very\n> familiar with I'd be very afraid, and in git's case for no reason.\n> \n> Does in mean that git has some scary git-specific feature that would\n> expose it. perhaps there's a local log that's unsecured where attempted\n> URLs are logged, or perhaps we send the raw requested URL to the server\n> so it can suggest alternatives for us. We do neither, but even a\n> generally knowledgeable user won't know that about git in particular.\n> \n> Whereas what I think you actually mean and are targeting here is better\n> explained by:\n> \n> Git is careful to avoid exposing passwords in URLs on its own,\n> e.g. they won't be logged in trace2 logs. This setting is intended\n> for those who'd like to discourage (warn) or enforce (die) the use\n> of the password helper infrastructure over hardcoded passwords.\n> \n> All of which I *think* is correct, but maybe I've missed something you\n> know about, as that \"in multiple ways\" is doing a lot of work.\n> \n> I also wonder if this wouldn't be even more useful if we took some\n> lessons from ssh's book. I.e. per \"git config -l --show-origin\" we know\n> the original of all config. We could be even more useful (and more\n> aggressive about warning about) cases where we have passwords in config\n> files that we detect don't have restrictive permissions, as OpenSSH does\n> with your private key.\n> \n> Ditto perhaps when the origin is \"command line\", as we do nothing to\n> hide that from the process list on shared systems (and that would be\n> racy whatever we did).\n\nI tried to be careful about how \"it\" (being \"Using such URLs\") can\nexpose the password includes things that are not under Git's\nresponsibility (such as command-line histories and other system-level\nlogs) but I can add a bit about how Git stores the plaintext password\nin the repository's config.\n\n>>>> +\tchar *value = NULL;\n>>>\n>>> This init to NULL should be removedd, as we....\n>>>\n>>>> +\tconst char *at_ptr;\n>>>> +\tconst char *colon_ptr;\n>>>> +\tstruct strbuf anonymized = STRBUF_INIT;\n>>>\n>>> nit: Just call this \"sb\"? The's at least one line below over 79\n>>> characters that's within the bounds with a shorter variable name, and in\n>>> this case it's obvious what we're doing here...\n>>\n>> I will not change this name to be less descriptive.\n> \n> Sure, just a suggestion. The other way is to just re-wrap that one\n> line... :)\n> \n> In the end I don't care, \"just a nit\", but just as one datapoint from\n> reading this code: I find this varibale name in particular to be the\n> polar opposite of descriptive, we're explicitly not anonymizing the URL\n> in this function, since we're not stripping the username part.\n> \n> Wouldn't descriptive be something more like uri_redacted_password or\n> uri_no_password in this case?\n\nHow about \"redacted\"?\n\n> Just for the implementation: It's slightly more wasteful, but in this\n> case we don't care about performance, so perhaps a strbuf_splice()\n> variant is easier here? I.e. add the full URL, find : and @, then\n> strbuf_splice() it. It gets rid of much of the pointer juggling here &\n> adding things incrementally.\n\nTIL. strbuf_splice() will work perfectly. Thanks.\n\n-Stolee\n"},{"id":"456288","messageId":"pull.1237.v2.git.1653658034086.gitgitgadget@gmail.com","threadId":"57913","inReplyTo":"pull.1237.git.1653329044940.gitgitgadget@gmail.com","subject":"[PATCH v2] urlmatch: create fetch.credentialsInUrl config","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-05-27T13:27:13Z","receivedAt":"2022-05-27T13:27:21Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nUsers sometimes provide a \"username:password\" combination in their\nplaintext URLs. Since Git stores these URLs in plaintext in the\n.git/config file, this is a very insecure way of storing these\ncredentials. Credential managers are a more secure way of storing this\ninformation.\n\nSystem administrators might want to prevent this kind of use by users on\ntheir machines.\n\nCreate a new \"fetch.credentialsInUrl\" config option and teach Git to\nwarn or die when seeing a URL with this kind of information. The warning\nanonymizes the sensitive information of the URL to be clear about the\nissue.\n\nThis change currently defaults the behavior to \"allow\" which does\nnothing with these URLs. We can consider changing this behavior to\n\"warn\" by default if we wish. At that time, we may want to add some\nadvice about setting fetch.credentialsInUrl=ignore for users who still\nwant to follow this pattern (and not receive the warning).\n\nAs an attempt to ensure the parsing logic did not catch any\nunintentional cases, I modified this change locally to to use the \"die\"\noption by default. Running the test suite succeeds except for the\nexplicit username:password URLs used in t5550-http-fetch-dumb.sh and\nt5541-http-push-smart.sh. This means that all other tested URLs did not\ntrigger this logic.\n\nSigned-off-by: Derrick Stolee \n---\n urlmatch: create fetch.credentialsInUrl config\n \n This is a modified version of the patch I submitted a while ago [1].\n \n Based on the feedback, changing the behavior to fail by default was not\n a good approach. Further, the idea to stop storing the credentials in\n config and redirect them to a credential manager was already considered\n by Peff [2] but not merged.\n \n This patch does what should be the simplest thing we can do: create a\n config option that will cause the user to get a warning or a failure,\n depending on its value. The default is to ignore the setting, identical\n to the current behavior. We can talk about changing this default to\n \"warn\" in the future, but it would be safest to release with ignore as\n the default until we are sure that we are not going to start warning on\n false positives.\n \n This patch would be sufficient for the interested internal parties that\n want to prevent users from storing credentials this way. System\n administrators can modify system-level Git config into \"die\" mode to\n prevent this behavior.\n \n [1]\n https://lore.kernel.org/git/pull.945.git.1619807844627.gitgitgadget@gmail.com\n Reject passwords in URLs (April 2021).\n \n [2]\n https://lore.kernel.org/git/20190519050724.GA26179@sigill.intra.peff.net/\n Re: Git ransom campaign incident report - May 2019\n \n \n Updates in v2\n =============\n \n * Documentation is slightly expanded to include the fact that Git\n stores the given URL as plaintext in its config.\n * The new method has a new documentation comment that details the\n necessary preconditions.\n * \"ignore\" is now \"allow\"\n * Additional checks on colon_ptr are added.\n * Use strbuf_splice() instead of custom string-walking logic.\n * Use \"\" instead of asterisks.\n * Config value checks are no longer case sensitive.\n \n Thanks, -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1237%2Fderrickstolee%2Fcreds-in-url-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1237/derrickstolee/creds-in-url-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1237\n\nRange-diff vs v1:\n\n 1: cc2befb3803 ! 1: 364f5c37c70 urlmatch: create fetch.credentialsInUrl config\n @@ Commit message\n anonymizes the sensitive information of the URL to be clear about the\n issue.\n \n - This change currently defaults the behavior to \"ignore\" which does\n + This change currently defaults the behavior to \"allow\" which does\n nothing with these URLs. We can consider changing this behavior to\n \"warn\" by default if we wish. At that time, we may want to add some\n advice about setting fetch.credentialsInUrl=ignore for users who still\n @@ Commit message\n As an attempt to ensure the parsing logic did not catch any\n unintentional cases, I modified this change locally to to use the \"die\"\n option by default. Running the test suite succeeds except for the\n - explicit username:password URLs used in t5550-http-fetch-dumb.s and\n + explicit username:password URLs used in t5550-http-fetch-dumb.sh and\n t5541-http-push-smart.sh. This means that all other tested URLs did not\n trigger this logic.\n \n @@ Documentation/config/fetch.txt: fetch.writeCommitGraph::\n +fetch.credentialsInUrl::\n +\tA URL can contain plaintext credentials in the form\n +\t`protocol://:@domain/path`. Using such URLs is not\n -+\trecommended as it exposes the password in multiple ways. The\n ++\trecommended as it exposes the password in multiple ways, including\n ++\tGit storing the URL as plaintext in the repository config. The\n +\t`fetch.credentialsInUrl` option provides instruction for how Git\n +\tshould react to seeing such a URL, with these values:\n ++\n -+* `ignore` (default): Git will proceed with its activity without warning.\n ++* `allow` (default): Git will proceed with its activity without warning.\n +* `warn`: Git will write a warning message to `stderr` when parsing a URL\n + with a plaintext credential.\n +* `die`: Git will write a failure message to `stderr` when parsing a URL\n @@ t/t5601-clone.sh: test_expect_success 'clone respects GIT_WORK_TREE' '\n \n +test_expect_success 'clone warns or fails when using username:password' '\n +\ttest_must_fail git -c fetch.credentialsInUrl=warn clone https://username:password@localhost attempt1 2>err &&\n -+\tgrep \"warning: URL '\\''https://username:\\*\\*\\*\\*\\*\\*\\*\\*@localhost/'\\'' uses plaintext credentials\" err &&\n ++\tgrep \"warning: URL '\\''https://username:@localhost/'\\'' uses plaintext credentials\" err &&\n +\ttest_must_fail git -c fetch.credentialsInUrl=die clone https://username:password@localhost attempt2 2>err &&\n -+\tgrep \"fatal: URL '\\''https://username:\\*\\*\\*\\*\\*\\*\\*\\*@localhost/'\\'' uses plaintext credentials\" err\n ++\tgrep \"fatal: URL '\\''https://username:@localhost/'\\'' uses plaintext credentials\" err\n ++'\n ++\n ++test_expect_success 'clone does not detect username:password when it is https://username@domain:port/' '\n ++\ttest_must_fail git -c fetch.credentialsInUrl=warn clone https://username@localhost:8080 attempt3 2>err &&\n ++\t! grep \"uses plaintext credentials\" err\n +'\n +\n test_expect_success 'clone from hooks' '\n @@ urlmatch.c: static int match_host(const struct url_info *url_info,\n \treturn (!url_len && !pat_len);\n }\n \n ++/*\n ++ * Call this method when we have detected credentials within the 'url' in\n ++ * the form\n ++ *\n ++ * scheme://username:password@domain[:port][/path]\n ++ *\n ++ * The 'scheme_len' value should be equal to the string length of the\n ++ * \"scheme://\" portion of the URL.\n ++ *\n ++ * The fetch.credentialsInUrl config indicates what to do on such a URL,\n ++ * either ignoring, warning, or die()ing. The latter two modes write a\n ++ * redacted URL to stderr.\n ++ */\n +static void detected_credentials_in_url(const char *url, size_t scheme_len)\n +{\n -+\tchar *value = NULL;\n ++\tconst char *value;\n +\tconst char *at_ptr;\n +\tconst char *colon_ptr;\n -+\tstruct strbuf anonymized = STRBUF_INIT;\n ++\tstruct strbuf redacted = STRBUF_INIT;\n +\n -+\t/* \"ignore\" is the default behavior. */\n -+\tif (git_config_get_string(\"fetch.credentialsinurl\", &value) ||\n -+\t !strcasecmp(\"ignore\", value))\n -+\t\tgoto cleanup;\n ++\t/* \"allow\" is the default behavior. */\n ++\tif (git_config_get_string_tmp(\"fetch.credentialsinurl\", &value) ||\n ++\t !strcmp(\"allow\", value))\n ++\t\treturn;\n +\n +\tat_ptr = strchr(url, '@');\n +\tcolon_ptr = strchr(url + scheme_len + 3, ':');\n +\n ++\t/*\n ++\t * Let's do some defensive programming to ensure the given\n ++\t * URL is of the proper format.\n ++\t */\n +\tif (!colon_ptr)\n +\t\tBUG(\"failed to find colon in url '%s' with scheme_len %\"PRIuMAX,\n +\t\t url, (uintmax_t) scheme_len);\n ++\tif (colon_ptr > at_ptr)\n ++\t\tBUG(\"input url '%s' does not include credentials\",\n ++\t\t url);\n +\n -+\t/* Include everything including the colon. */\n ++\t/* Include the colon when creating the redacted URL. */\n +\tcolon_ptr++;\n -+\tstrbuf_add(&anonymized, url, colon_ptr - url);\n -+\n -+\twhile (colon_ptr < at_ptr) {\n -+\t\tstrbuf_addch(&anonymized, '*');\n -+\t\tcolon_ptr++;\n -+\t}\n -+\n -+\tstrbuf_addstr(&anonymized, at_ptr);\n ++\tstrbuf_addstr(&redacted, url);\n ++\tstrbuf_splice(&redacted, colon_ptr - url, at_ptr - colon_ptr,\n ++\t\t \"\", 10);\n +\n -+\tif (!strcasecmp(\"warn\", value))\n -+\t\twarning(_(\"URL '%s' uses plaintext credentials\"), anonymized.buf);\n -+\tif (!strcasecmp(\"die\", value))\n -+\t\tdie(_(\"URL '%s' uses plaintext credentials\"), anonymized.buf);\n ++\tif (!strcmp(\"warn\", value))\n ++\t\twarning(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n ++\tif (!strcmp(\"die\", value))\n ++\t\tdie(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n +\n -+cleanup:\n -+\tfree(value);\n -+\tstrbuf_release(&anonymized);\n ++\tstrbuf_release(&redacted);\n +}\n +\n static char *url_normalize_1(const char *url, struct url_info *out_info, char allow_globs)\n\n\n Documentation/config/fetch.txt | 14 +++++++++\n t/t5601-clone.sh | 12 ++++++++\n urlmatch.c | 56 ++++++++++++++++++++++++++++++++++\n 3 files changed, 82 insertions(+)\n\ndiff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt\nindex cd65d236b43..7fd3ea89f5d 100644\n--- a/Documentation/config/fetch.txt\n+++ b/Documentation/config/fetch.txt\n@@ -96,3 +96,17 @@ fetch.writeCommitGraph::\n \tmerge and the write may take longer. Having an updated commit-graph\n \tfile helps performance of many Git commands, including `git merge-base`,\n \t`git push -f`, and `git log --graph`. Defaults to false.\n+\n+fetch.credentialsInUrl::\n+\tA URL can contain plaintext credentials in the form\n+\t`protocol://:@domain/path`. Using such URLs is not\n+\trecommended as it exposes the password in multiple ways, including\n+\tGit storing the URL as plaintext in the repository config. The\n+\t`fetch.credentialsInUrl` option provides instruction for how Git\n+\tshould react to seeing such a URL, with these values:\n++\n+* `allow` (default): Git will proceed with its activity without warning.\n+* `warn`: Git will write a warning message to `stderr` when parsing a URL\n+ with a plaintext credential.\n+* `die`: Git will write a failure message to `stderr` when parsing a URL\n+ with a plaintext credential.\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 4a61f2c901e..387da74d175 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -71,6 +71,18 @@ test_expect_success 'clone respects GIT_WORK_TREE' '\n \n '\n \n+test_expect_success 'clone warns or fails when using username:password' '\n+\ttest_must_fail git -c fetch.credentialsInUrl=warn clone https://username:password@localhost attempt1 2>err &&\n+\tgrep \"warning: URL '\\''https://username:@localhost/'\\'' uses plaintext credentials\" err &&\n+\ttest_must_fail git -c fetch.credentialsInUrl=die clone https://username:password@localhost attempt2 2>err &&\n+\tgrep \"fatal: URL '\\''https://username:@localhost/'\\'' uses plaintext credentials\" err\n+'\n+\n+test_expect_success 'clone does not detect username:password when it is https://username@domain:port/' '\n+\ttest_must_fail git -c fetch.credentialsInUrl=warn clone https://username@localhost:8080 attempt3 2>err &&\n+\t! grep \"uses plaintext credentials\" err\n+'\n+\n test_expect_success 'clone from hooks' '\n \n \ttest_create_repo r0 &&\ndiff --git a/urlmatch.c b/urlmatch.c\nindex b615adc923a..16beda37a3a 100644\n--- a/urlmatch.c\n+++ b/urlmatch.c\n@@ -1,5 +1,6 @@\n #include \"cache.h\"\n #include \"urlmatch.h\"\n+#include \"config.h\"\n \n #define URL_ALPHA \"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz\"\n #define URL_DIGIT \"0123456789\"\n@@ -106,6 +107,59 @@ static int match_host(const struct url_info *url_info,\n \treturn (!url_len && !pat_len);\n }\n \n+/*\n+ * Call this method when we have detected credentials within the 'url' in\n+ * the form\n+ *\n+ * scheme://username:password@domain[:port][/path]\n+ *\n+ * The 'scheme_len' value should be equal to the string length of the\n+ * \"scheme://\" portion of the URL.\n+ *\n+ * The fetch.credentialsInUrl config indicates what to do on such a URL,\n+ * either ignoring, warning, or die()ing. The latter two modes write a\n+ * redacted URL to stderr.\n+ */\n+static void detected_credentials_in_url(const char *url, size_t scheme_len)\n+{\n+\tconst char *value;\n+\tconst char *at_ptr;\n+\tconst char *colon_ptr;\n+\tstruct strbuf redacted = STRBUF_INIT;\n+\n+\t/* \"allow\" is the default behavior. */\n+\tif (git_config_get_string_tmp(\"fetch.credentialsinurl\", &value) ||\n+\t !strcmp(\"allow\", value))\n+\t\treturn;\n+\n+\tat_ptr = strchr(url, '@');\n+\tcolon_ptr = strchr(url + scheme_len + 3, ':');\n+\n+\t/*\n+\t * Let's do some defensive programming to ensure the given\n+\t * URL is of the proper format.\n+\t */\n+\tif (!colon_ptr)\n+\t\tBUG(\"failed to find colon in url '%s' with scheme_len %\"PRIuMAX,\n+\t\t url, (uintmax_t) scheme_len);\n+\tif (colon_ptr > at_ptr)\n+\t\tBUG(\"input url '%s' does not include credentials\",\n+\t\t url);\n+\n+\t/* Include the colon when creating the redacted URL. */\n+\tcolon_ptr++;\n+\tstrbuf_addstr(&redacted, url);\n+\tstrbuf_splice(&redacted, colon_ptr - url, at_ptr - colon_ptr,\n+\t\t \"\", 10);\n+\n+\tif (!strcmp(\"warn\", value))\n+\t\twarning(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n+\tif (!strcmp(\"die\", value))\n+\t\tdie(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n+\n+\tstrbuf_release(&redacted);\n+}\n+\n static char *url_normalize_1(const char *url, struct url_info *out_info, char allow_globs)\n {\n \t/*\n@@ -144,6 +198,7 @@ static char *url_normalize_1(const char *url, struct url_info *out_info, char al\n \t */\n \n \tsize_t url_len = strlen(url);\n+\tconst char *orig_url = url;\n \tstruct strbuf norm;\n \tsize_t spanned;\n \tsize_t scheme_len, user_off=0, user_len=0, passwd_off=0, passwd_len=0;\n@@ -191,6 +246,7 @@ static char *url_normalize_1(const char *url, struct url_info *out_info, char al\n \t\t\t}\n \t\t\tcolon_ptr = strchr(norm.buf + scheme_len + 3, ':');\n \t\t\tif (colon_ptr) {\n+\t\t\t\tdetected_credentials_in_url(orig_url, scheme_len);\n \t\t\t\tpasswd_off = (colon_ptr + 1) - norm.buf;\n \t\t\t\tpasswd_len = norm.len - passwd_off;\n \t\t\t\tuser_len = (passwd_off - 1) - (scheme_len + 3);\n\nbase-commit: f9b95943b68b6b8ca5a6072f50a08411c6449b55\n-- \ngitgitgadget\n"},{"id":"456289","messageId":"220527.86fskv2g50.gmgdl@evledraar.gmail.com","threadId":"57913","inReplyTo":"pull.1237.v2.git.1653658034086.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] urlmatch: create fetch.credentialsInUrl config","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-27T14:22:41Z","receivedAt":"2022-05-27T14:40:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, May 27 2022, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee \n\nJust real quick, I hadn't taken notice of this before (the rest looks\ngood at a glance):\n\n> +\t/*\n> +\t * Let's do some defensive programming to ensure the given\n> +\t * URL is of the proper format.\n> +\t */\n> +\tif (!colon_ptr)\n> +\t\tBUG(\"failed to find colon in url '%s' with scheme_len %\"PRIuMAX,\n> +\t\t url, (uintmax_t) scheme_len);\n> +\tif (colon_ptr > at_ptr)\n> +\t\tBUG(\"input url '%s' does not include credentials\",\n> +\t\t url);\n\nSo the function is renamed to detected_credentials_in_url(), so as a nit\nI'd expect some verb like \"strip\", \"redact\" or whatever inthe name or\nwhatever, to make it clear what we're doing.\n\nBut since the only caller here below...\n\n> +\n> +\t/* Include the colon when creating the redacted URL. */\n> +\tcolon_ptr++;\n> +\tstrbuf_addstr(&redacted, url);\n> +\tstrbuf_splice(&redacted, colon_ptr - url, at_ptr - colon_ptr,\n> +\t\t \"\", 10);\n> +\n> +\tif (!strcmp(\"warn\", value))\n> +\t\twarning(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n> +\tif (!strcmp(\"die\", value))\n> +\t\tdie(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n> +\n> +\tstrbuf_release(&redacted);\n> +}\n> +\n> static char *url_normalize_1(const char *url, struct url_info *out_info, char allow_globs)\n> {\n> \t/*\n> @@ -144,6 +198,7 @@ static char *url_normalize_1(const char *url, struct url_info *out_info, char al\n> \t */\n> \n> \tsize_t url_len = strlen(url);\n> +\tconst char *orig_url = url;\n> \tstruct strbuf norm;\n> \tsize_t spanned;\n> \tsize_t scheme_len, user_off=0, user_len=0, passwd_off=0, passwd_len=0;\n> @@ -191,6 +246,7 @@ static char *url_normalize_1(const char *url, struct url_info *out_info, char al\n> \t\t\t}\n> \t\t\tcolon_ptr = strchr(norm.buf + scheme_len + 3, ':');\n> \t\t\tif (colon_ptr) {\n> +\t\t\t\tdetected_credentials_in_url(orig_url, scheme_len);\n\nHas already done the work of finding the colon_ptr (and at_ptr) why\nre-do that paranoia since we have a static function, we could just pass\nthe two pointers we found already to strbuf_splice().\n\nThis also seems really close to something we could just add to strbuf.c\nas e.g a strbuf_splice_to(). I.e. just:\n\t\n\tint strbuf_splice_to(const struct strbuf *in, struct strbuf *sb,\n\t\t\t size_t pos, size_t len,\n\t\t\t const void *data, size_t data_len);\n\t\nWhich would be used as:\n\t\n\tstruct strbuf sb = STRBUF_INIT;\n\tif (!strbuf_splice_to(url, &redacted, /* same as strbuf_splice(...) */))\n\t\twarn(\"oh noes a password in %s\", sb.buf);\n\telse\n\t\twarn(\"have no password in %s, no replacement done\", url->buf);\n\nWhich re earlier talk of sharing an implementation with the other\n code looks like it could be dropped into the relevant part of\npkt-line.c.\n\nBut maybe that's all going overboard :)\n"},{"id":"456291","messageId":"0cf130d2-464b-8661-d53a-723041f32fc6@github.com","threadId":"57913","inReplyTo":"220527.86fskv2g50.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2] urlmatch: create fetch.credentialsInUrl config","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-05-27T14:43:39Z","receivedAt":"2022-05-27T14:43:45Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/27/2022 10:22 AM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Fri, May 27 2022, Derrick Stolee via GitGitGadget wrote:\n> \n>> From: Derrick Stolee \n> \n> Just real quick, I hadn't taken notice of this before (the rest looks\n> good at a glance):\n> \n>> +\t/*\n>> +\t * Let's do some defensive programming to ensure the given\n>> +\t * URL is of the proper format.\n>> +\t */\n>> +\tif (!colon_ptr)\n>> +\t\tBUG(\"failed to find colon in url '%s' with scheme_len %\"PRIuMAX,\n>> +\t\t url, (uintmax_t) scheme_len);\n>> +\tif (colon_ptr > at_ptr)\n>> +\t\tBUG(\"input url '%s' does not include credentials\",\n>> +\t\t url);\n> \n> So the function is renamed to detected_credentials_in_url(), so as a nit\n> I'd expect some verb like \"strip\", \"redact\" or whatever inthe name or\n> whatever, to make it clear what we're doing.\n\nThe name means \"Do what needs to be done when creds are detected\nin a URL\".\n\nIf we decide to change the response to creds in a URL, then we\nwould change the implementation without changing the name.\n\n>> \t\t\tcolon_ptr = strchr(norm.buf + scheme_len + 3, ':');\n>> \t\t\tif (colon_ptr) {\n>> +\t\t\t\tdetected_credentials_in_url(orig_url, scheme_len);\n> \n> Has already done the work of finding the colon_ptr (and at_ptr) why\n> re-do that paranoia since we have a static function,\n\nUnfortunately, at this point 'url' is not equal to 'orig_url' and\n'colon_ptr' isn't even within the 'orig_url' string. It's been\ncopied and mutated. It was my first instinct to share as much as\npossible, which is why 'schema' is reused.\n\nThanks,\n-Stolee\n"},{"id":"456309","messageId":"xmqqsfou3l0q.fsf@gitster.g","threadId":"57913","inReplyTo":"pull.1237.v2.git.1653658034086.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] urlmatch: create fetch.credentialsInUrl config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-27T18:09:25Z","receivedAt":"2022-05-27T18:09:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt\n> index cd65d236b43..7fd3ea89f5d 100644\n> --- a/Documentation/config/fetch.txt\n> +++ b/Documentation/config/fetch.txt\n> @@ -96,3 +96,17 @@ fetch.writeCommitGraph::\n> \tmerge and the write may take longer. Having an updated commit-graph\n> \tfile helps performance of many Git commands, including `git merge-base`,\n> \t`git push -f`, and `git log --graph`. Defaults to false.\n> +\n> +fetch.credentialsInUrl::\n> +\tA URL can contain plaintext credentials in the form\n> +\t`protocol://:@domain/path`. Using such URLs is not\n\nIn the above, 'protocol', 'domain' and 'path' are all placeholders,\njust like and are, and it made me wonder if we\nshould be consistent. Enclosing all placeholders in \npairs would make the resulting text too loud, so it may make sense\nto drop these highlights around user and password.\n\nThat makes it consistent with what git-credential-store.txt,\ngit-svn.txt, gitfaq.txt and urls.txt do\n\n git-credential-store.txt:https://user:pass@example.com\n git-svn.txt:\tthe URL, e.g. `svn+ssh://foo@svn.bar.com/project`\n gitfaq.txt:$ echo url=https://author@git.example.org | git credential reject\n gitfaq.txt:\thttps://author@git.example.org/org1/project1.git and\n gitfaq.txt:\thttps://committer@git.example.org/org2/project2.git. This way, when you\n gitfaq.txt:\torigin https://author@git.example.org/org1/project1.git` (see\n\n\n> +\trecommended as it exposes the password in multiple ways, including\n> +\tGit storing the URL as plaintext in the repository config. The\n> +\t`fetch.credentialsInUrl` option provides instruction for how Git\n> +\tshould react to seeing such a URL, with these values:\n> ++\n> +* `allow` (default): Git will proceed with its activity without warning.\n> +* `warn`: Git will write a warning message to `stderr` when parsing a URL\n> + with a plaintext credential.\n> +* `die`: Git will write a failure message to `stderr` when parsing a URL\n> + with a plaintext credential.\n\nGood.\n\n> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\n> index 4a61f2c901e..387da74d175 100755\n> --- a/t/t5601-clone.sh\n> +++ b/t/t5601-clone.sh\n> @@ -71,6 +71,18 @@ test_expect_success 'clone respects GIT_WORK_TREE' '\n> \n> '\n> \n> +test_expect_success 'clone warns or fails when using username:password' '\n> +\ttest_must_fail git -c fetch.credentialsInUrl=warn clone https://username:password@localhost attempt1 2>err &&\n> +\tgrep \"warning: URL '\\''https://username:@localhost/'\\'' uses plaintext credentials\" err &&\n> +\ttest_must_fail git -c fetch.credentialsInUrl=die clone https://username:password@localhost attempt2 2>err &&\n> +\tgrep \"fatal: URL '\\''https://username:@localhost/'\\'' uses plaintext credentials\" err\n> +'\n> +\n> +test_expect_success 'clone does not detect username:password when it is https://username@domain:port/' '\n> +\ttest_must_fail git -c fetch.credentialsInUrl=warn clone https://username@localhost:8080 attempt3 2>err &&\n> +\t! grep \"uses plaintext credentials\" err\n> +'\n\nCould we have one negative test here, too? I.e. with an explicit -c\nfetch.credentialsInUrl=allow given, no credential-in-URL errors and\nwarnings should be issued.\n\n> diff --git a/urlmatch.c b/urlmatch.c\n> index b615adc923a..16beda37a3a 100644\n> --- a/urlmatch.c\n> +++ b/urlmatch.c\n> @@ -1,5 +1,6 @@\n> #include \"cache.h\"\n> #include \"urlmatch.h\"\n> +#include \"config.h\"\n> \n> #define URL_ALPHA \"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz\"\n> #define URL_DIGIT \"0123456789\"\n> @@ -106,6 +107,59 @@ static int match_host(const struct url_info *url_info,\n> \treturn (!url_len && !pat_len);\n> }\n> \n> +/*\n> + * Call this method when we have detected credentials within the 'url' in\n> + * the form\n> + *\n> + * scheme://username:password@domain[:port][/path]\n> + *\n> + * The 'scheme_len' value should be equal to the string length of the\n> + * \"scheme://\" portion of the URL.\n\n\"scheme\" is probably more technically correct here, even though in\nthe documentation that faces the end-users, \"protocol\" would be more\nwidely understood, so I think this inconsistency in a single patch\nacross paths is a good thing ;-)\n\n> + * The fetch.credentialsInUrl config indicates what to do on such a URL,\n> + * either ignoring, warning, or die()ing. The latter two modes write a\n> + * redacted URL to stderr.\n> + */\n> +static void detected_credentials_in_url(const char *url, size_t scheme_len)\n> +{\n> +\tconst char *value;\n> +\tconst char *at_ptr;\n> +\tconst char *colon_ptr;\n> +\tstruct strbuf redacted = STRBUF_INIT;\n> +\n> +\t/* \"allow\" is the default behavior. */\n> +\tif (git_config_get_string_tmp(\"fetch.credentialsinurl\", &value) ||\n> +\t !strcmp(\"allow\", value))\n> +\t\treturn;\n\nOK.\n\n> +\tat_ptr = strchr(url, '@');\n> +\tcolon_ptr = strchr(url + scheme_len + 3, ':');\n\nI am debating myself if we should explain \"+ 3\" that counts \"://\" in\na comment, but it probably is trivial to see.\n\n> +\t/*\n> +\t * Let's do some defensive programming to ensure the given\n> +\t * URL is of the proper format.\n> +\t */\n> +\tif (!colon_ptr)\n> +\t\tBUG(\"failed to find colon in url '%s' with scheme_len %\"PRIuMAX,\n> +\t\t url, (uintmax_t) scheme_len);\n> +\tif (colon_ptr > at_ptr)\n> +\t\tBUG(\"input url '%s' does not include credentials\",\n> +\t\t url);\n\nOK. The caller shouldn't have called us in these two cases.\n\n> +\t/* Include the colon when creating the redacted URL. */\n> +\tcolon_ptr++;\n> +\tstrbuf_addstr(&redacted, url);\n> +\tstrbuf_splice(&redacted, colon_ptr - url, at_ptr - colon_ptr,\n> +\t\t \"\", 10);\n> +\n> +\tif (!strcmp(\"warn\", value))\n> +\t\twarning(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n> +\tif (!strcmp(\"die\", value))\n> +\t\tdie(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n> +\n> +\tstrbuf_release(&redacted);\n\nOK. Let's look at the caller.\n\n> \t\t\tif (colon_ptr) {\n> +\t\t\t\tdetected_credentials_in_url(orig_url, scheme_len);\n> \t\t\t\tpasswd_off = (colon_ptr + 1) - norm.buf;\n> \t\t\t\tpasswd_len = norm.len - passwd_off;\n> \t\t\t\tuser_len = (passwd_off - 1) - (scheme_len + 3);\n\nThe caller apparently knows where the password and username are at\nthis point. It is unclear which string these offsets are pointing\ninto at this point, but at the end of the function, we have the\noffset and length of user and passwd that point into result, all in\nour local variables, even when out_info is NULL.\n\n\tresult = strbuf_detach(&norm, &result_len);\n\tif (out_info) {\n\t\tout_info->url = result;\n\t\tout_info->err = NULL;\n\t\tout_info->url_len = result_len;\n\t\tout_info->scheme_len = scheme_len;\n\t\tout_info->user_off = user_off;\n\t\tout_info->user_len = user_len;\n\t\tout_info->passwd_off = passwd_off;\n\t\tout_info->passwd_len = passwd_len;\n\t\tout_info->host_off = host_off;\n\t\tout_info->host_len = host_len;\n\t\tout_info->port_off = port_off;\n\t\tout_info->port_len = port_len;\n\t\tout_info->path_off = path_off;\n\t\tout_info->path_len = path_len;\n\t}\n\treturn result;\n\nIt does make me wonder if we want another parser in the new\nfunction. Wouldn't it be easier to manage if we inserted a\ncall to \n\n\tif (passwd_off)\n\t\tapply_fetch_credentials_in_url(result,\n \t\t\tuser_off, user_len, passwd_off, passwd_len);\n\njust after the strbuf_detach() we see near the end of the function, where\napply_fetch_credentials_in_url() only reads the configuration and\ncalls warning or die as appropriate?\n"},{"id":"456310","messageId":"xmqqo7zi3jkg.fsf@gitster.g","threadId":"57913","inReplyTo":"xmqqsfou3l0q.fsf@gitster.g","subject":"Re: [PATCH v2] urlmatch: create fetch.credentialsInUrl config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-27T18:40:47Z","receivedAt":"2022-05-27T18:41:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> It does make me wonder if we want another parser in the new\n> function. Wouldn't it be easier to manage if we inserted a\n> call to \n>\n> \tif (passwd_off)\n> \t\tapply_fetch_credentials_in_url(result,\n> \t\t\tuser_off, user_len, passwd_off, passwd_len);\n>\n> just after the strbuf_detach() we see near the end of the function, where\n> apply_fetch_credentials_in_url() only reads the configuration and\n> calls warning or die as appropriate?\n\nThe new function does not even have to take user_off and user_len,\nand work only with the while string with pair for the\npassword, I think, as that is the only thing it redacts in the\noutput. Sorry about the noise.\n"},{"id":"456361","messageId":"xmqqilpnyiwe.fsf@gitster.g","threadId":"57913","inReplyTo":"pull.1237.v2.git.1653658034086.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] urlmatch: create fetch.credentialsInUrl config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-30T00:16:17Z","receivedAt":"2022-05-30T00:16:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> Create a new \"fetch.credentialsInUrl\" config option and teach Git to\n> warn or die when seeing a URL with this kind of information. The warning\n> anonymizes the sensitive information of the URL to be clear about the\n> issue.\n>\n> This change currently defaults the behavior to \"allow\" which does\n> nothing with these URLs. We can consider changing this behavior to\n> \"warn\" by default if we wish. At that time, we may want to add some\n> advice about setting fetch.credentialsInUrl=ignore for users who still\n> want to follow this pattern (and not receive the warning).\n\nCan we make this die in a bit more controlled way?\n\ne.g. https://github.com/git/git/runs/6646450422 seems to show that\ndepending on the timing, the call to die() on the \"git clone\" side\nmay cause us stop reading early enough to kill the other side with\nSIGPIPE. The nicely prepared warning message seems to be lost.\n\n\n"},{"id":"456387","messageId":"a9f1f310-ef48-b5f5-39ae-ca45ae48038d@github.com","threadId":"57913","inReplyTo":"xmqqilpnyiwe.fsf@gitster.g","subject":"Re: [PATCH v2] urlmatch: create fetch.credentialsInUrl config","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-05-31T13:32:45Z","receivedAt":"2022-05-31T13:32:51Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/29/2022 8:16 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n> \n>> Create a new \"fetch.credentialsInUrl\" config option and teach Git to\n>> warn or die when seeing a URL with this kind of information. The warning\n>> anonymizes the sensitive information of the URL to be clear about the\n>> issue.\n>>\n>> This change currently defaults the behavior to \"allow\" which does\n>> nothing with these URLs. We can consider changing this behavior to\n>> \"warn\" by default if we wish. At that time, we may want to add some\n>> advice about setting fetch.credentialsInUrl=ignore for users who still\n>> want to follow this pattern (and not receive the warning).\n> \n> Can we make this die in a bit more controlled way?\n> \n> e.g. https://github.com/git/git/runs/6646450422 seems to show that\n> depending on the timing, the call to die() on the \"git clone\" side\n> may cause us stop reading early enough to kill the other side with\n> SIGPIPE. The nicely prepared warning message seems to be lost.\n\nThanks for pointing this out. It took a while for me to reproduce\nthis with --stress, but I can get it to happen on my machine.\n\nInvestigating now.\n\nThanks,\n-Stolee\n"},{"id":"456429","messageId":"pull.1237.v3.git.1654046173.gitgitgadget@gmail.com","threadId":"57913","inReplyTo":"pull.1237.v2.git.1653658034086.gitgitgadget@gmail.com","subject":"[PATCH v3 0/2] fetch: create fetch.credentialsInUrl config","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-01T01:16:11Z","receivedAt":"2022-06-01T01:16:20Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Users can specify credentials in their URL using the\nusername:password@domain format. This is potentially hazardous since the URL\nis stored in plaintext and can also appear in trace2 logs and other places.\nAdd a new config option that allows warnings or failures when discovering\nURLs with this format. The default behavior does not change in this series,\nalthough we may want to move to the warn state by default in the future.\n\nThis is a modified version of the patch I submitted a while ago [1].\n\nBased on the feedback, changing the behavior to fail by default was not a\ngood approach. Further, the idea to stop storing the credentials in config\nand redirect them to a credential manager was already considered by Peff [2]\nbut not merged.\n\nThis patch does what should be the simplest thing we can do: create a config\noption that will cause the user to get a warning or a failure, depending on\nits value. The default is to ignore the setting, identical to the current\nbehavior. We can talk about changing this default to \"warn\" in the future,\nbut it would be safest to release with ignore as the default until we are\nsure that we are not going to start warning on false positives.\n\nThis patch would be sufficient for the interested internal parties that want\nto prevent users from storing credentials this way. System administrators\ncan modify system-level Git config into \"die\" mode to prevent this behavior.\n\n[1]\nhttps://lore.kernel.org/git/pull.945.git.1619807844627.gitgitgadget@gmail.com\nReject passwords in URLs (April 2021).\n\n[2]\nhttps://lore.kernel.org/git/20190519050724.GA26179@sigill.intra.peff.net/\nRe: Git ransom campaign incident report - May 2019\n\n\nUpdates in v3\n=============\n\n * Because of some flaky behavior around SIGPIPE, the URL check needed to\n move to be earlier in the command.\n * For this reason, I moved the cred check into remote.c's valid_remote()\n check. This changed the previous BUG() statements into early returns.\n * I repeated the check with the test suite to see if this parsing fails on\n any existing cases, but it is worth double-checking the parsing rules.\n * Documentation is more consistent about using placeholders.\n * A test for the \"allow\" case is now included.\n * A new patch is added that creates the warn_once() helper. This reduces\n multiple advisory warnings with the same text from being written by the\n same process.\n\n\nUpdates in v2\n=============\n\n * Documentation is slightly expanded to include the fact that Git stores\n the given URL as plaintext in its config.\n * The new method has a new documentation comment that details the necessary\n preconditions.\n * \"ignore\" is now \"allow\"\n * Additional checks on colon_ptr are added.\n * Use strbuf_splice() instead of custom string-walking logic.\n * Use \"\" instead of asterisks.\n * Config value checks are no longer case sensitive.\n\nThanks, -Stolee\n\nDerrick Stolee (2):\n remote: create fetch.credentialsInUrl config\n usage: add warn_once() helper for repeated warnings\n\n Documentation/config/fetch.txt | 14 ++++++++++\n git-compat-util.h | 1 +\n remote.c | 48 ++++++++++++++++++++++++++++++++++\n t/t5601-clone.sh | 17 ++++++++++++\n usage.c | 22 ++++++++++++++++\n 5 files changed, 102 insertions(+)\n\n\nbase-commit: f9b95943b68b6b8ca5a6072f50a08411c6449b55\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1237%2Fderrickstolee%2Fcreds-in-url-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1237/derrickstolee/creds-in-url-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1237\n\nRange-diff vs v2:\n\n 1: 364f5c37c70 ! 1: 083a918e9b1 urlmatch: create fetch.credentialsInUrl config\n @@ Metadata\n Author: Derrick Stolee \n \n ## Commit message ##\n - urlmatch: create fetch.credentialsInUrl config\n + remote: create fetch.credentialsInUrl config\n \n Users sometimes provide a \"username:password\" combination in their\n plaintext URLs. Since Git stores these URLs in plaintext in the\n @@ Commit message\n advice about setting fetch.credentialsInUrl=ignore for users who still\n want to follow this pattern (and not receive the warning).\n \n + An earlier version of this change injected the logic into\n + url_normalize() in urlmatch.c. While most code paths that parse URLs\n + eventually normalize the URL, that normalization does not happen early\n + enough in the stack to avoid attempting connections to the URL first. By\n + inserting a check into the remote validation, we identify the issue\n + before making a connection. In the old code path, this was revealed by\n + testing the new t5601-clone.sh test under --stress, resulting in an\n + instance where the return code was 13 (SIGPIPE) instead of 128 from the\n + die().\n +\n + Since we are not deep within url_normalize(), we need to do our own\n + parsing to detect if there is a \"username:password@domain\" section. We\n + begin by detecting the first '@' symbol in the URL. We then detect if\n + there is a scheme such as \"https://\" by finding the first slash. If that\n + slash does not exist or is after the first '@' symbol, then we consider\n + the scheme to be complete before the URL. Finally, we look for a colon\n + between the scheme and the '@' symbol, indicating a \"username:password\"\n + string. Replace the password with \"\" when writing the error\n + message.\n +\n As an attempt to ensure the parsing logic did not catch any\n unintentional cases, I modified this change locally to to use the \"die\"\n option by default. Running the test suite succeeds except for the\n @@ Documentation/config/fetch.txt: fetch.writeCommitGraph::\n +\n +fetch.credentialsInUrl::\n +\tA URL can contain plaintext credentials in the form\n -+\t`protocol://:@domain/path`. Using such URLs is not\n -+\trecommended as it exposes the password in multiple ways, including\n -+\tGit storing the URL as plaintext in the repository config. The\n -+\t`fetch.credentialsInUrl` option provides instruction for how Git\n ++\t`://:@/`. Using such URLs\n ++\tis not recommended as it exposes the password in multiple ways,\n ++\tincluding Git storing the URL as plaintext in the repository config.\n ++\tThe `fetch.credentialsInUrl` option provides instruction for how Git\n +\tshould react to seeing such a URL, with these values:\n ++\n +* `allow` (default): Git will proceed with its activity without warning.\n @@ Documentation/config/fetch.txt: fetch.writeCommitGraph::\n +* `die`: Git will write a failure message to `stderr` when parsing a URL\n + with a plaintext credential.\n \n - ## t/t5601-clone.sh ##\n -@@ t/t5601-clone.sh: test_expect_success 'clone respects GIT_WORK_TREE' '\n - \n - '\n - \n -+test_expect_success 'clone warns or fails when using username:password' '\n -+\ttest_must_fail git -c fetch.credentialsInUrl=warn clone https://username:password@localhost attempt1 2>err &&\n -+\tgrep \"warning: URL '\\''https://username:@localhost/'\\'' uses plaintext credentials\" err &&\n -+\ttest_must_fail git -c fetch.credentialsInUrl=die clone https://username:password@localhost attempt2 2>err &&\n -+\tgrep \"fatal: URL '\\''https://username:@localhost/'\\'' uses plaintext credentials\" err\n -+'\n -+\n -+test_expect_success 'clone does not detect username:password when it is https://username@domain:port/' '\n -+\ttest_must_fail git -c fetch.credentialsInUrl=warn clone https://username@localhost:8080 attempt3 2>err &&\n -+\t! grep \"uses plaintext credentials\" err\n -+'\n -+\n - test_expect_success 'clone from hooks' '\n - \n - \ttest_create_repo r0 &&\n -\n - ## urlmatch.c ##\n -@@\n - #include \"cache.h\"\n - #include \"urlmatch.h\"\n -+#include \"config.h\"\n - \n - #define URL_ALPHA \"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz\"\n - #define URL_DIGIT \"0123456789\"\n -@@ urlmatch.c: static int match_host(const struct url_info *url_info,\n - \treturn (!url_len && !pat_len);\n - }\n + ## remote.c ##\n +@@ remote.c: struct counted_string {\n + \tconst char *s;\n + };\n \n +/*\n -+ * Call this method when we have detected credentials within the 'url' in\n -+ * the form\n ++ * Check if the given URL is of the following form:\n + *\n + * scheme://username:password@domain[:port][/path]\n + *\n -+ * The 'scheme_len' value should be equal to the string length of the\n -+ * \"scheme://\" portion of the URL.\n ++ * Specifically, see if the \":password@\" section of the URL appears.\n + *\n + * The fetch.credentialsInUrl config indicates what to do on such a URL,\n -+ * either ignoring, warning, or die()ing. The latter two modes write a\n ++ * either ignoring, warning, or erroring. The latter two modes write a\n + * redacted URL to stderr.\n + */\n -+static void detected_credentials_in_url(const char *url, size_t scheme_len)\n ++static void check_if_creds_in_url(const char *url)\n +{\n -+\tconst char *value;\n -+\tconst char *at_ptr;\n -+\tconst char *colon_ptr;\n ++\tconst char *value, *scheme_ptr, *colon_ptr, *at_ptr;\n +\tstruct strbuf redacted = STRBUF_INIT;\n +\n +\t/* \"allow\" is the default behavior. */\n @@ urlmatch.c: static int match_host(const struct url_info *url_info,\n +\t !strcmp(\"allow\", value))\n +\t\treturn;\n +\n -+\tat_ptr = strchr(url, '@');\n -+\tcolon_ptr = strchr(url + scheme_len + 3, ':');\n ++\tif (!(at_ptr = strchr(url, '@')))\n ++\t\treturn;\n +\n -+\t/*\n -+\t * Let's do some defensive programming to ensure the given\n -+\t * URL is of the proper format.\n -+\t */\n -+\tif (!colon_ptr)\n -+\t\tBUG(\"failed to find colon in url '%s' with scheme_len %\"PRIuMAX,\n -+\t\t url, (uintmax_t) scheme_len);\n -+\tif (colon_ptr > at_ptr)\n -+\t\tBUG(\"input url '%s' does not include credentials\",\n -+\t\t url);\n ++\tif (!(scheme_ptr = strchr(url, '/')) ||\n ++\t scheme_ptr > at_ptr)\n ++\t\tscheme_ptr = url;\n ++\n ++\tif (!(colon_ptr = strchr(scheme_ptr, ':')))\n ++\t\treturn;\n +\n +\t/* Include the colon when creating the redacted URL. */\n +\tcolon_ptr++;\n @@ urlmatch.c: static int match_host(const struct url_info *url_info,\n +\tstrbuf_release(&redacted);\n +}\n +\n - static char *url_normalize_1(const char *url, struct url_info *out_info, char allow_globs)\n + static int valid_remote(const struct remote *remote)\n {\n - \t/*\n -@@ urlmatch.c: static char *url_normalize_1(const char *url, struct url_info *out_info, char al\n - \t */\n ++\tfor (int i = 0; i < remote->url_nr; i++)\n ++\t\tcheck_if_creds_in_url(remote->url[i]);\n ++\n + \treturn (!!remote->url) || (!!remote->foreign_vcs);\n + }\n + \n +\n + ## t/t5601-clone.sh ##\n +@@ t/t5601-clone.sh: test_expect_success 'clone respects GIT_WORK_TREE' '\n + \n + '\n \n - \tsize_t url_len = strlen(url);\n -+\tconst char *orig_url = url;\n - \tstruct strbuf norm;\n - \tsize_t spanned;\n - \tsize_t scheme_len, user_off=0, user_len=0, passwd_off=0, passwd_len=0;\n -@@ urlmatch.c: static char *url_normalize_1(const char *url, struct url_info *out_info, char al\n - \t\t\t}\n - \t\t\tcolon_ptr = strchr(norm.buf + scheme_len + 3, ':');\n - \t\t\tif (colon_ptr) {\n -+\t\t\t\tdetected_credentials_in_url(orig_url, scheme_len);\n - \t\t\t\tpasswd_off = (colon_ptr + 1) - norm.buf;\n - \t\t\t\tpasswd_len = norm.len - passwd_off;\n - \t\t\t\tuser_len = (passwd_off - 1) - (scheme_len + 3);\n ++test_expect_success 'clone warns or fails when using username:password' '\n ++\ttest_must_fail git -c fetch.credentialsInUrl=allow clone https://username:password@localhost attempt1 2>err &&\n ++\t! grep \"URL '\\''https://username:@localhost'\\'' uses plaintext credentials\" err &&\n ++\ttest_must_fail git -c fetch.credentialsInUrl=warn clone https://username:password@localhost attempt1 2>err &&\n ++\tgrep \"warning: URL '\\''https://username:@localhost'\\'' uses plaintext credentials\" err &&\n ++\ttest_must_fail git -c fetch.credentialsInUrl=die clone https://username:password@localhost attempt2 2>err &&\n ++\tgrep \"fatal: URL '\\''https://username:@localhost'\\'' uses plaintext credentials\" err\n ++'\n ++\n ++test_expect_success 'clone does not detect username:password when it is https://username@domain:port/' '\n ++\ttest_must_fail git -c fetch.credentialsInUrl=warn clone https://username@localhost:8080 attempt3 2>err &&\n ++\t! grep \"uses plaintext credentials\" err\n ++'\n ++\n + test_expect_success 'clone from hooks' '\n + \n + \ttest_create_repo r0 &&\n -: ----------- > 2: 8e29ac807c6 usage: add warn_once() helper for repeated warnings\n\n-- \ngitgitgadget\n"},{"id":"456430","messageId":"083a918e9b1474eff0d51c4502b6d54de9b63764.1654046173.git.gitgitgadget@gmail.com","threadId":"57913","inReplyTo":"pull.1237.v3.git.1654046173.gitgitgadget@gmail.com","subject":"[PATCH v3 1/2] remote: create fetch.credentialsInUrl config","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-01T01:16:12Z","receivedAt":"2022-06-01T01:16:24Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nUsers sometimes provide a \"username:password\" combination in their\nplaintext URLs. Since Git stores these URLs in plaintext in the\n.git/config file, this is a very insecure way of storing these\ncredentials. Credential managers are a more secure way of storing this\ninformation.\n\nSystem administrators might want to prevent this kind of use by users on\ntheir machines.\n\nCreate a new \"fetch.credentialsInUrl\" config option and teach Git to\nwarn or die when seeing a URL with this kind of information. The warning\nanonymizes the sensitive information of the URL to be clear about the\nissue.\n\nThis change currently defaults the behavior to \"allow\" which does\nnothing with these URLs. We can consider changing this behavior to\n\"warn\" by default if we wish. At that time, we may want to add some\nadvice about setting fetch.credentialsInUrl=ignore for users who still\nwant to follow this pattern (and not receive the warning).\n\nAn earlier version of this change injected the logic into\nurl_normalize() in urlmatch.c. While most code paths that parse URLs\neventually normalize the URL, that normalization does not happen early\nenough in the stack to avoid attempting connections to the URL first. By\ninserting a check into the remote validation, we identify the issue\nbefore making a connection. In the old code path, this was revealed by\ntesting the new t5601-clone.sh test under --stress, resulting in an\ninstance where the return code was 13 (SIGPIPE) instead of 128 from the\ndie().\n\nSince we are not deep within url_normalize(), we need to do our own\nparsing to detect if there is a \"username:password@domain\" section. We\nbegin by detecting the first '@' symbol in the URL. We then detect if\nthere is a scheme such as \"https://\" by finding the first slash. If that\nslash does not exist or is after the first '@' symbol, then we consider\nthe scheme to be complete before the URL. Finally, we look for a colon\nbetween the scheme and the '@' symbol, indicating a \"username:password\"\nstring. Replace the password with \"\" when writing the error\nmessage.\n\nAs an attempt to ensure the parsing logic did not catch any\nunintentional cases, I modified this change locally to to use the \"die\"\noption by default. Running the test suite succeeds except for the\nexplicit username:password URLs used in t5550-http-fetch-dumb.sh and\nt5541-http-push-smart.sh. This means that all other tested URLs did not\ntrigger this logic.\n\nSigned-off-by: Derrick Stolee \n---\n Documentation/config/fetch.txt | 14 ++++++++++\n remote.c | 48 ++++++++++++++++++++++++++++++++++\n t/t5601-clone.sh | 14 ++++++++++\n 3 files changed, 76 insertions(+)\n\ndiff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt\nindex cd65d236b43..0db7fe85bb8 100644\n--- a/Documentation/config/fetch.txt\n+++ b/Documentation/config/fetch.txt\n@@ -96,3 +96,17 @@ fetch.writeCommitGraph::\n \tmerge and the write may take longer. Having an updated commit-graph\n \tfile helps performance of many Git commands, including `git merge-base`,\n \t`git push -f`, and `git log --graph`. Defaults to false.\n+\n+fetch.credentialsInUrl::\n+\tA URL can contain plaintext credentials in the form\n+\t`://:@/`. Using such URLs\n+\tis not recommended as it exposes the password in multiple ways,\n+\tincluding Git storing the URL as plaintext in the repository config.\n+\tThe `fetch.credentialsInUrl` option provides instruction for how Git\n+\tshould react to seeing such a URL, with these values:\n++\n+* `allow` (default): Git will proceed with its activity without warning.\n+* `warn`: Git will write a warning message to `stderr` when parsing a URL\n+ with a plaintext credential.\n+* `die`: Git will write a failure message to `stderr` when parsing a URL\n+ with a plaintext credential.\ndiff --git a/remote.c b/remote.c\nindex 42a4e7106e1..accf08bf51f 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -22,8 +22,56 @@ struct counted_string {\n \tconst char *s;\n };\n \n+/*\n+ * Check if the given URL is of the following form:\n+ *\n+ * scheme://username:password@domain[:port][/path]\n+ *\n+ * Specifically, see if the \":password@\" section of the URL appears.\n+ *\n+ * The fetch.credentialsInUrl config indicates what to do on such a URL,\n+ * either ignoring, warning, or erroring. The latter two modes write a\n+ * redacted URL to stderr.\n+ */\n+static void check_if_creds_in_url(const char *url)\n+{\n+\tconst char *value, *scheme_ptr, *colon_ptr, *at_ptr;\n+\tstruct strbuf redacted = STRBUF_INIT;\n+\n+\t/* \"allow\" is the default behavior. */\n+\tif (git_config_get_string_tmp(\"fetch.credentialsinurl\", &value) ||\n+\t !strcmp(\"allow\", value))\n+\t\treturn;\n+\n+\tif (!(at_ptr = strchr(url, '@')))\n+\t\treturn;\n+\n+\tif (!(scheme_ptr = strchr(url, '/')) ||\n+\t scheme_ptr > at_ptr)\n+\t\tscheme_ptr = url;\n+\n+\tif (!(colon_ptr = strchr(scheme_ptr, ':')))\n+\t\treturn;\n+\n+\t/* Include the colon when creating the redacted URL. */\n+\tcolon_ptr++;\n+\tstrbuf_addstr(&redacted, url);\n+\tstrbuf_splice(&redacted, colon_ptr - url, at_ptr - colon_ptr,\n+\t\t \"\", 10);\n+\n+\tif (!strcmp(\"warn\", value))\n+\t\twarning(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n+\tif (!strcmp(\"die\", value))\n+\t\tdie(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n+\n+\tstrbuf_release(&redacted);\n+}\n+\n static int valid_remote(const struct remote *remote)\n {\n+\tfor (int i = 0; i < remote->url_nr; i++)\n+\t\tcheck_if_creds_in_url(remote->url[i]);\n+\n \treturn (!!remote->url) || (!!remote->foreign_vcs);\n }\n \ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 4a61f2c901e..cba3553b7c4 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -71,6 +71,20 @@ test_expect_success 'clone respects GIT_WORK_TREE' '\n \n '\n \n+test_expect_success 'clone warns or fails when using username:password' '\n+\ttest_must_fail git -c fetch.credentialsInUrl=allow clone https://username:password@localhost attempt1 2>err &&\n+\t! grep \"URL '\\''https://username:@localhost'\\'' uses plaintext credentials\" err &&\n+\ttest_must_fail git -c fetch.credentialsInUrl=warn clone https://username:password@localhost attempt1 2>err &&\n+\tgrep \"warning: URL '\\''https://username:@localhost'\\'' uses plaintext credentials\" err &&\n+\ttest_must_fail git -c fetch.credentialsInUrl=die clone https://username:password@localhost attempt2 2>err &&\n+\tgrep \"fatal: URL '\\''https://username:@localhost'\\'' uses plaintext credentials\" err\n+'\n+\n+test_expect_success 'clone does not detect username:password when it is https://username@domain:port/' '\n+\ttest_must_fail git -c fetch.credentialsInUrl=warn clone https://username@localhost:8080 attempt3 2>err &&\n+\t! grep \"uses plaintext credentials\" err\n+'\n+\n test_expect_success 'clone from hooks' '\n \n \ttest_create_repo r0 &&\n-- \ngitgitgadget\n\n"},{"id":"456431","messageId":"8e29ac807c6a0cf94ea3a44ee3304011c2ad159c.1654046173.git.gitgitgadget@gmail.com","threadId":"57913","inReplyTo":"pull.1237.v3.git.1654046173.gitgitgadget@gmail.com","subject":"[PATCH v3 2/2] usage: add warn_once() helper for repeated warnings","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-01T01:16:13Z","receivedAt":"2022-06-01T01:16:27Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe previous change added a warning when valid_remote() detects\ncredentials in the URL. Since remotes are validated multiple times per\nprocess, this causes multiple warnings to print.\n\nTo avoid these kinds of repeated, advisory warnings, create a new\nwarn_once() helper that behaves the same as warning(), but only after\nformatting the output string and adding it to a strset. If that addition\nsignals that the string already exists in the strset, then do not print\nthe warning.\n\nIn the case of the credentials in a URL, the existing test demonstrates\nthis per-process limitation: 'git clone' runs 'git-remote-curl' as a\nchild process, giving two messages. This is an improvement over the\nprevious six messages.\n\nSigned-off-by: Derrick Stolee \n---\n git-compat-util.h | 1 +\n remote.c | 2 +-\n t/t5601-clone.sh | 5 ++++-\n usage.c | 22 ++++++++++++++++++++++\n 4 files changed, 28 insertions(+), 2 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 58fd813bd01..776a5f660aa 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -526,6 +526,7 @@ int error(const char *err, ...) __attribute__((format (printf, 1, 2)));\n int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));\n void warning_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n+void warn_once(const char *warn, ...) __attribute__((format (printf, 1, 2)));\n \n #ifndef NO_OPENSSL\n #ifdef APPLE_COMMON_CRYPTO\ndiff --git a/remote.c b/remote.c\nindex accf08bf51f..72fffd0d968 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -60,7 +60,7 @@ static void check_if_creds_in_url(const char *url)\n \t\t \"\", 10);\n \n \tif (!strcmp(\"warn\", value))\n-\t\twarning(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n+\t\twarn_once(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n \tif (!strcmp(\"die\", value))\n \t\tdie(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n \ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex cba3553b7c4..6ae3eec9eb6 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -75,7 +75,10 @@ test_expect_success 'clone warns or fails when using username:password' '\n \ttest_must_fail git -c fetch.credentialsInUrl=allow clone https://username:password@localhost attempt1 2>err &&\n \t! grep \"URL '\\''https://username:@localhost'\\'' uses plaintext credentials\" err &&\n \ttest_must_fail git -c fetch.credentialsInUrl=warn clone https://username:password@localhost attempt1 2>err &&\n-\tgrep \"warning: URL '\\''https://username:@localhost'\\'' uses plaintext credentials\" err &&\n+\tgrep \"warning: URL '\\''https://username:@localhost'\\'' uses plaintext credentials\" err >warnings &&\n+\t# The warning is printed twice, for the two processes:\n+\t# \"git clone\" and \"git-remote-curl\".\n+\ttest_line_count = 2 warnings &&\n \ttest_must_fail git -c fetch.credentialsInUrl=die clone https://username:password@localhost attempt2 2>err &&\n \tgrep \"fatal: URL '\\''https://username:@localhost'\\'' uses plaintext credentials\" err\n '\ndiff --git a/usage.c b/usage.c\nindex b738dd178b3..242633c5f8d 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -5,6 +5,7 @@\n */\n #include \"git-compat-util.h\"\n #include \"cache.h\"\n+#include \"strmap.h\"\n \n static void vreportf(const char *prefix, const char *err, va_list params)\n {\n@@ -287,6 +288,27 @@ void warning(const char *warn, ...)\n \tva_end(params);\n }\n \n+static struct strset prev_warnings = STRSET_INIT;\n+\n+void warn_once(const char *warn, ...)\n+{\n+\tchar buf[1024];\n+\tva_list params;\n+\tva_start(params, warn);\n+\n+\tif (vsnprintf(buf, sizeof(buf), warn, params) >= 0) {\n+\t\tif (!strset_add(&prev_warnings, buf)) {\n+\t\t\tva_end(params);\n+\t\t\treturn;\n+\t\t}\n+\t}\n+\tva_end(params);\n+\n+\tva_start(params, warn);\n+\twarn_routine(warn, params);\n+\tva_end(params);\n+}\n+\n /* Only set this, ever, from t/helper/, when verifying that bugs are caught. */\n int BUG_exit_code;\n \n-- \ngitgitgadget\n"},{"id":"456443","messageId":"220601.868rqg1qo2.gmgdl@evledraar.gmail.com","threadId":"57913","inReplyTo":"8e29ac807c6a0cf94ea3a44ee3304011c2ad159c.1654046173.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 2/2] usage: add warn_once() helper for repeated warnings","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-01T12:29:38Z","receivedAt":"2022-06-01T13:04:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jun 01 2022, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee \n>\n> The previous change added a warning when valid_remote() detects\n> credentials in the URL. Since remotes are validated multiple times per\n> process, this causes multiple warnings to print.\n\nWhy are we validating remotes multiple times per process? Can't we just\ndo it once?\n\nIs this a case of two callers going through the whole machinery and not\nbeing aware of one another?\n\nPerhaps it's a pain to deal with that in this case, but it would be\nbetter to note why here than taking it as a given.\n\n> To avoid these kinds of repeated, advisory warnings, create a new\n> warn_once() helper that behaves the same as warning(), but only after\n> formatting the output string and adding it to a strset. If that addition\n> signals that the string already exists in the strset, then do not print\n> the warning.\n\nThis feels quite iffy given the low-level API & the rest of it aiming to\nbe thread-safe, see 2d3c02f5db6 (die(): stop hiding errors due to\noverzealous recursion guard, 2017-06-21) for such a case.\n\nI.e. there *is* racy code there already that (ab)uses a no-lock pattern\nto optimistically abort early when we'd emit N of the same recursion\nmessage.\n\nBut are we as confident that concurrent strset callers in multiple\nthreads will behave themselves? I'd think we should be adding this to\nthe caller that knows it's not threaded, e.g. whatever calls\ncheck_if_creds_in_url().\n\n> In the case of the credentials in a URL, the existing test demonstrates\n> this per-process limitation: 'git clone' runs 'git-remote-curl' as a\n> child process, giving two messages. This is an improvement over the\n> previous six messages.\n\nIf we know about this limitation and we're going to be checking the same\nURLs why not do slightly better and pass down a \"don't warn please\" to\nthe sub-process?\n\nOr do we think it might warn about *different* URLs, aren't these always\nthe same (i.e. we read the full config)?\n\nIf you used advice instead of a warning you'd get a config key to pass\nfor free to disable it, and that might be a better thing to do in any\ncase (i.e. a better fit for this case). But we could also just check\ngetenv(\"GIT_URL_REDACTED_WARNING\") or whatever, which seems easy\nenough...\n\n> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\n> index cba3553b7c4..6ae3eec9eb6 100755\n> --- a/t/t5601-clone.sh\n> +++ b/t/t5601-clone.sh\n> @@ -75,7 +75,10 @@ test_expect_success 'clone warns or fails when using username:password' '\n> \ttest_must_fail git -c fetch.credentialsInUrl=allow clone https://username:password@localhost attempt1 2>err &&\n> \t! grep \"URL '\\''https://username:@localhost'\\'' uses plaintext credentials\" err &&\n> \ttest_must_fail git -c fetch.credentialsInUrl=warn clone https://username:password@localhost attempt1 2>err &&\n> -\tgrep \"warning: URL '\\''https://username:@localhost'\\'' uses plaintext credentials\" err &&\n> +\tgrep \"warning: URL '\\''https://username:@localhost'\\'' uses plaintext credentials\" err >warnings &&\n> +\t# The warning is printed twice, for the two processes:\n> +\t# \"git clone\" and \"git-remote-curl\".\n> +\ttest_line_count = 2 warnings &&\n> \ttest_must_fail git -c fetch.credentialsInUrl=die clone https://username:password@localhost attempt2 2>err &&\n> \tgrep \"fatal: URL '\\''https://username:@localhost'\\'' uses plaintext credentials\" err\n> '\n\nHrm, between 1/2 and this I think this is a good example of a \"just use\ntest_cmp\" caveat.\n\nI.e. reading 1/2 it's snuck past us that there's this existing caveat in\nthe warning being added, i.e. that we should really warn 1 times, but\nare doing in N times, but as we use ad-hoc \"grep\" instead of\ntest_cmp-ing the full output that's not obvious.\n\nI think this would be much more obvious as:\n\n\twarning=\"\" &&\n\tcat >expect <<-EOF &&\n\t$warning\n $warning\n\tEOF\n\nOr whatever, followed by a:\n\n\ttest_cmp expect actual\n\nThen 1/2 could make a brief note that we're leaving this duplication\nissue for now, and 2/2 would fix it.\n\nBut even better (and per the above, I'm not convinced about the\ndirection, but leaving that aside): Here we have a choice between having\n1/2 and 2/2 in that order and having 2/2 add a new API that has its\nfirst user right away, but at the cost of fixing a bug we just\nintroduced in 1/2.\n\nI think even if we can't find another API user for this proposed usage.c\naddition, just adding it without a user & adding what's now 1/2 as the\n2nd commit would be preferrable. Then we're not doing a \"oops, bug fix\"\nwhile we're at it.\n\n> +static struct strset prev_warnings = STRSET_INIT;\n> +\n> +void warn_once(const char *warn, ...)\n> +{\n\nIf we do end up keeping this (per the above I'm thinking while it's just\nthis caller it should probably own this problem):\n\nI have a local change to clean up this warn v.s. warning inconsistency\nin usage.c, but now it's all in internal code.\n\nBut let's not add to it by adding a warn_once(), it should be called\nwarning_once().\n\nWe're also missing an \"errno\" variant, which might be fine for now,\nditto variants for the other non-fatal functions (error &\ndie_message). That might be OK for now, but probably worth noting.\n\n> +void warn_once(const char *warn, ...)\n> +{\n> +\tchar buf[1024];\n> +\tva_list params;\n> +\tva_start(params, warn);\n> +\n> +\tif (vsnprintf(buf, sizeof(buf), warn, params) >= 0) {\n\nIt doesn't matter for the end result, but let's compare with \"< 0\" like\nthe other vsnprintf() caller in the file.\n\nAnd how is this unlike that vsnprintf() in not needing the same error\nhandling vreportf() does here:\n\n\t*p = '\\0'; /* vsnprintf() failed, clip at prefix */\n\nSeems like either a bug, or that other code being something we should be\ndropping.\n\n> +\t\tif (!strset_add(&prev_warnings, buf)) {\n\nMore on the general API usability front: E.g. builtin/pack-objects.c\nseems like it could make use of thing like this in two places.\n\nBut the API you've added here would only be usable for 1/2, i.e. you\nformat and de-dup on the full warning, whereas at least 1/2 of those\ncallers wants to de-dup on the un-formatted string (grep for 'static int\nwarned').\n\nWhich (and I'm sounding like a broken record:) is another thing that me\nwonder if the general API is premature, i.e. it's a red flag that we\nhave existing code that could benefit from it, if not for an arbitrary\nlimitation, in this case fixing the limitation would mean the churn of\neither adding a new function, or a new parameter to all callers. I.e. a\n\"int dedup_on_args\" or similar.\n\n> +\t\t\tva_end(params);\n> +\t\t\treturn;\n> +\t\t}\n> +\t}\n> +\tva_end(params);\n> +\n> +\tva_start(params, warn);\n\nI'm rusty on the varargs API (and haven't tested) but didn't we need\nva_copy() if we're iterating twice, or was that just if we're passing it\nto another function and using it ourselves...\n\n"},{"id":"456459","messageId":"8f96563b-87c5-2a8c-fe35-19fd0cc3c7d2@github.com","threadId":"57913","inReplyTo":"220601.868rqg1qo2.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 2/2] usage: add warn_once() helper for repeated warnings","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-06-01T18:42:13Z","receivedAt":"2022-06-01T18:42:21Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/1/2022 8:29 AM, Ævar Arnfjörð Bjarmason wrote:> \n> On Wed, Jun 01 2022, Derrick Stolee via GitGitGadget wrote:\n> \n>> From: Derrick Stolee \n>>\n>> The previous change added a warning when valid_remote() detects\n>> credentials in the URL. Since remotes are validated multiple times per\n>> process, this causes multiple warnings to print.\n> \n> Why are we validating remotes multiple times per process? Can't we just\n> do it once?\n> \n> Is this a case of two callers going through the whole machinery and not\n> being aware of one another?\n> \n> Perhaps it's a pain to deal with that in this case, but it would be\n> better to note why here than taking it as a given.\n\nWe could certainly investigate this more, but it seems like a more\nproblematic approach than the one taken here. We could add a \"is_valid\"\nbit to struct remote, but then could some code path modify that struct\nafter it was validated?\n\n>> To avoid these kinds of repeated, advisory warnings, create a new\n>> warn_once() helper that behaves the same as warning(), but only after\n>> formatting the output string and adding it to a strset. If that addition\n>> signals that the string already exists in the strset, then do not print\n>> the warning.\n> \n> This feels quite iffy given the low-level API & the rest of it aiming to\n> be thread-safe, see 2d3c02f5db6 (die(): stop hiding errors due to\n> overzealous recursion guard, 2017-06-21) for such a case.\n\nIt makes sense to keep the low-level library as thread-safe as possible.\n\nWould it be enough to document this particular caller as not thread-safe?\n\n>> In the case of the credentials in a URL, the existing test demonstrates\n>> this per-process limitation: 'git clone' runs 'git-remote-curl' as a\n>> child process, giving two messages. This is an improvement over the\n>> previous six messages.\n> \n> If we know about this limitation and we're going to be checking the same\n> URLs why not do slightly better and pass down a \"don't warn please\" to\n> the sub-process?>> Or do we think it might warn about *different* URLs, aren't these always\n> the same (i.e. we read the full config)?\n\nWhile this example of 'git clone' to 'git-remote-curl' will not use a\ndifferent URL, I can imagine other child process connections that could\nexpose a different URL. Having a blanket statement \"don't emit these\nwarnings\" seems unlikely to be helpful for those cases.\n\n> If you used advice instead of a warning you'd get a config key to pass\n> for free to disable it, and that might be a better thing to do in any\n> case (i.e. a better fit for this case). But we could also just check\n> getenv(\"GIT_URL_REDACTED_WARNING\") or whatever, which seems easy\n> enough...\n\nTo make the environment variable work properly, we would need to pass a\nlist of URLs in the variable that could seed the strset, to avoid dropping\nwarnings for new cases as mentioned earlier.\n\n>> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\n>> index cba3553b7c4..6ae3eec9eb6 100755\n>> --- a/t/t5601-clone.sh\n>> +++ b/t/t5601-clone.sh\n>> @@ -75,7 +75,10 @@ test_expect_success 'clone warns or fails when using username:password' '\n>> \ttest_must_fail git -c fetch.credentialsInUrl=allow clone https://username:password@localhost attempt1 2>err &&\n>> \t! grep \"URL '\\''https://username:@localhost'\\'' uses plaintext credentials\" err &&\n>> \ttest_must_fail git -c fetch.credentialsInUrl=warn clone https://username:password@localhost attempt1 2>err &&\n>> -\tgrep \"warning: URL '\\''https://username:@localhost'\\'' uses plaintext credentials\" err &&\n>> +\tgrep \"warning: URL '\\''https://username:@localhost'\\'' uses plaintext credentials\" err >warnings &&\n>> +\t# The warning is printed twice, for the two processes:\n>> +\t# \"git clone\" and \"git-remote-curl\".\n>> +\ttest_line_count = 2 warnings &&\n>> \ttest_must_fail git -c fetch.credentialsInUrl=die clone https://username:password@localhost attempt2 2>err &&\n>> \tgrep \"fatal: URL '\\''https://username:@localhost'\\'' uses plaintext credentials\" err\n>> '\n> \n> Hrm, between 1/2 and this I think this is a good example of a \"just use\n> test_cmp\" caveat.\n> \n> I.e. reading 1/2 it's snuck past us that there's this existing caveat in\n> the warning being added, i.e. that we should really warn 1 times, but\n> are doing in N times, but as we use ad-hoc \"grep\" instead of\n> test_cmp-ing the full output that's not obvious.\n> \n> I think this would be much more obvious as:\n> \n> \twarning=\"\" &&\n> \tcat >expect <<-EOF &&\n> \t$warning\n> $warning\n> \tEOF\n\nThere are other warnings coming over stderr, including the \"Cloning\ninto...\" message and the resulting \"fatal:\" message because that URL\ndoesn't actually exist. I chose to decouple this test to the exact\nphrasing of those messages.\n\n> But even better (and per the above, I'm not convinced about the\n> direction, but leaving that aside): Here we have a choice between having\n> 1/2 and 2/2 in that order and having 2/2 add a new API that has its\n> first user right away, but at the cost of fixing a bug we just\n> introduced in 1/2.\n> \n> I think even if we can't find another API user for this proposed usage.c\n> addition, just adding it without a user & adding what's now 1/2 as the\n> 2nd commit would be preferrable. Then we're not doing a \"oops, bug fix\"\n> while we're at it.\n\nI wouldn't call this a \"oops, bug fix\" but instead \"let's improve what was\nworking, but noisy.\" This also gives us the opportunity to see the code in\nusage.c get tested immediately as it is introduced, which is an important\nproperty to keep in mind when organizing a patch series.\n\n>> +static struct strset prev_warnings = STRSET_INIT;\n>> +\n>> +void warn_once(const char *warn, ...)\n>> +{\n> \n> If we do end up keeping this (per the above I'm thinking while it's just\n> this caller it should probably own this problem):\n> \n> I have a local change to clean up this warn v.s. warning inconsistency\n> in usage.c, but now it's all in internal code.\n> \n> But let's not add to it by adding a warn_once(), it should be called\n> warning_once().\n\nSure. The phrase \"warn once\" is a valid sentence, while \"warning once\" is\nnot, but I can see some benefit to having consistent naming here.\n\nIf the \"warning()\" method was not already present so much throughout the\ncodebase, then I would advocate that \"warn()\" is a better verb form. This\nwould be similar to \"die()\" and even \"error()\" could be interpreted as a\nverb. However, that would be too large of a change to be worthwhile.\n\n> We're also missing an \"errno\" variant, which might be fine for now,\n> ditto variants for the other non-fatal functions (error &\n> die_message). That might be OK for now, but probably worth noting.\n\nNoted.\n\n>> +void warn_once(const char *warn, ...)\n>> +{\n>> +\tchar buf[1024];\n>> +\tva_list params;\n>> +\tva_start(params, warn);\n>> +\n>> +\tif (vsnprintf(buf, sizeof(buf), warn, params) >= 0) {\n> \n> It doesn't matter for the end result, but let's compare with \"< 0\" like\n> the other vsnprintf() caller in the file.\n\nExcept that we do something after this block in both cases, so the\ncondition needs to be swapped or else duplicate code.\n\n> And how is this unlike that vsnprintf() in not needing the same error\n> handling vreportf() does here:\n> \n> \t*p = '\\0'; /* vsnprintf() failed, clip at prefix */\n> \n> Seems like either a bug, or that other code being something we should be\n> dropping.\n\nNo, because the params are passed to warn_routine() in the section of code\nyou cropped out:\n\n+\tva_start(params, warn);\n+\twarn_routine(warn, params);\n+\tva_end(params);\n\nThat warn_routine does the \"clip at prefix\" fix. if we have a failure in\nvsnprintf() here, then we are skipping the \"only once\" check and going\nstraight to that logic.\n\n>> +\t\tif (!strset_add(&prev_warnings, buf)) {\n> \n> More on the general API usability front: E.g. builtin/pack-objects.c\n> seems like it could make use of thing like this in two places.\n> \n> But the API you've added here would only be usable for 1/2, i.e. you\n> format and de-dup on the full warning, whereas at least 1/2 of those\n> callers wants to de-dup on the un-formatted string (grep for 'static int\n> warned').\n> \n> Which (and I'm sounding like a broken record:) is another thing that me\n> wonder if the general API is premature, i.e. it's a red flag that we\n> have existing code that could benefit from it, if not for an arbitrary\n> limitation, in this case fixing the limitation would mean the churn of\n> either adding a new function, or a new parameter to all callers. I.e. a\n> \"int dedup_on_args\" or similar.\n\nYou are advocating that since there is one example of something that\ndoesn't fit this exact scenario, then we shouldn't move forward on\nsomething that _does_ have immediate use (and potential use in other\nplaces). This seem unnecessarily pedantic.\n\nThis specific method is a good foundation for possibly extending it if\nthere is value in doing so.\n\n>> +\t\t\tva_end(params);\n>> +\t\t\treturn;\n>> +\t\t}\n>> +\t}\n>> +\tva_end(params);\n>> +\n>> +\tva_start(params, warn);\n> \n> I'm rusty on the varargs API (and haven't tested) but didn't we need\n> va_copy() if we're iterating twice, or was that just if we're passing it\n> to another function and using it ourselves...\n\nWell, outside of the fact that this function is tested and it works,\nthe documentation says this about the 'params' argument (named 'ap'\nin the function prototype):\n\n If ap has already been passed as first argument to a previous call to\n va_start or va_copy, it shall be passed to va_end before calling this\n function.\n\nSo, va_end(params) makes it safe to call va_start(params, warn) again.\n\nThanks,\n-Stolee\n"},{"id":"456462","messageId":"220601.86r148yxi3.gmgdl@evledraar.gmail.com","threadId":"57913","inReplyTo":"8f96563b-87c5-2a8c-fe35-19fd0cc3c7d2@github.com","subject":"Re: [PATCH v3 2/2] usage: add warn_once() helper for repeated warnings","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-01T19:33:08Z","receivedAt":"2022-06-01T20:42:50Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jun 01 2022, Derrick Stolee wrote:\n\n> On 6/1/2022 8:29 AM, Ævar Arnfjörð Bjarmason wrote:> \n>> On Wed, Jun 01 2022, Derrick Stolee via GitGitGadget wrote:\n>> \n>>> From: Derrick Stolee \n>>>\n>>> The previous change added a warning when valid_remote() detects\n>>> credentials in the URL. Since remotes are validated multiple times per\n>>> process, this causes multiple warnings to print.\n>> \n>> Why are we validating remotes multiple times per process? Can't we just\n>> do it once?\n>> \n>> Is this a case of two callers going through the whole machinery and not\n>> being aware of one another?\n>> \n>> Perhaps it's a pain to deal with that in this case, but it would be\n>> better to note why here than taking it as a given.\n>\n> We could certainly investigate this more, but it seems like a more\n> problematic approach than the one taken here. We could add a \"is_valid\"\n> bit to struct remote, but then could some code path modify that struct\n> after it was validated?\n\nI tested this a bit and I think this alternate approach is simpler, and\nit passes your tests:\n\t\n\tdiff --git a/remote.c b/remote.c\n\tindex faa2f834635..56ce2c83022 100644\n\t--- a/remote.c\n\t+++ b/remote.c\n\t@@ -60,7 +60,7 @@ static void check_if_creds_in_url(const char *url)\n\t \t\t \"\", 10);\n\t \n\t \tif (!strcmp(\"warn\", value))\n\t-\t\twarn_once(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n\t+\t\twarning(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n\t \tif (!strcmp(\"die\", value))\n\t \t\tdie(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n\t \n\t@@ -69,12 +69,17 @@ static void check_if_creds_in_url(const char *url)\n\t \n\t static int valid_remote(const struct remote *remote)\n\t {\n\t-\tfor (int i = 0; i < remote->url_nr; i++)\n\t-\t\tcheck_if_creds_in_url(remote->url[i]);\n\t-\n\t \treturn (!!remote->url) || (!!remote->foreign_vcs);\n\t }\n\t \n\t+static void valid_remote_at_end(const struct remote *remote)\n\t+{\n\t+\tint i;\n\t+\n\t+\tfor (i = 0; i < remote->url_nr; i++)\n\t+\t\tcheck_if_creds_in_url(remote->url[i]);\n\t+}\n\t+\n\t static const char *alias_url(const char *url, struct rewrites *r)\n\t {\n\t \tint i, j;\n\t@@ -687,6 +692,7 @@ remotes_remote_get_1(struct remote_state *remote_state, const char *name,\n\t \t\tadd_url_alias(remote_state, ret, name);\n\t \tif (!valid_remote(ret))\n\t \t\treturn NULL;\n\t+\tvalid_remote_at_end(ret);\n\t \treturn ret;\n\t }\n\nI.e. we already have one spot where we get the full remote config, you\nwere just hooking into valid_remote() which we call N times while doing\nthat, let's just call it at the end.\n\nAside from general de-duplication APIs that to me seems like a more\nsensible approach here, i.e. surely we should be getting the remote\nconfig once, let's stick such a validation loop in that place.\n\nAnd it seems we do, the stack is e.g.:\n\t\n\t(gdb) bt\n\t#0 warn_once (warn=0x79f1f3 \"URL '%s' uses plaintext credentials\") at usage.c:297\n\t#1 0x0000000000688a98 in check_if_creds_in_url (url=0x8677a0 \"https://username:password@localhost\") at remote.c:63\n\t#2 0x0000000000688858 in valid_remote_at_end (remote=0x872a40) at remote.c:80\n\t#3 0x0000000000688397 in remotes_remote_get_1 (remote_state=0x85ca00, name=0x861f10 \"origin\", get_default=0x681d00 ) at remote.c:695\n\t#4 0x0000000000681fd7 in remotes_remote_get (remote_state=0x85ca00, name=0x861f10 \"origin\") at remote.c:702\n\t#5 0x0000000000682071 in remote_get (name=0x861f10 \"origin\") at remote.c:709\n\t#6 0x0000000000433b05 in cmd_clone (argc=2, argv=0x7fffffffd028, prefix=0x0) at builtin/clone.c:1146\n\t#7 0x0000000000407d6b in run_builtin (p=0x8259f8 , argc=3, argv=0x7fffffffd028) at git.c:466\n\t#8 0x0000000000406802 in handle_builtin (argc=3, argv=0x7fffffffd028) at git.c:720\n\t#9 0x0000000000407746 in run_argv (argcp=0x7fffffffcecc, argv=0x7fffffffcec0) at git.c:787\n\t#10 0x00000000004065bb in cmd_main (argc=3, argv=0x7fffffffd028) at git.c:920\n\t#11 0x000000000051279a in main (argc=7, argv=0x7fffffffd008) at common-main.c:56\n\nYou put the validation in a function we called N times in\nremotes_remote_get_1(), but if we just put it once in\nremotes_remote_get_1()....\n\n>>> To avoid these kinds of repeated, advisory warnings, create a new\n>>> warn_once() helper that behaves the same as warning(), but only after\n>>> formatting the output string and adding it to a strset. If that addition\n>>> signals that the string already exists in the strset, then do not print\n>>> the warning.\n>> \n>> This feels quite iffy given the low-level API & the rest of it aiming to\n>> be thread-safe, see 2d3c02f5db6 (die(): stop hiding errors due to\n>> overzealous recursion guard, 2017-06-21) for such a case.\n>\n> It makes sense to keep the low-level library as thread-safe as possible.\n>\n> Would it be enough to document this particular caller as not thread-safe?\n\nSee above, but I'd think we really should consider alternatives before\nleaving such landmines in place.\n\nI.e. this is called by remote.c, which is common library code itself,\nsomeone implementing threads somewhere isn't going to be thinking about\nor reading caveats about libraries remote.c itself uses unless they're\nquite lucky...\n\n>>> In the case of the credentials in a URL, the existing test demonstrates\n>>> this per-process limitation: 'git clone' runs 'git-remote-curl' as a\n>>> child process, giving two messages. This is an improvement over the\n>>> previous six messages.\n>> \n>> If we know about this limitation and we're going to be checking the same\n>> URLs why not do slightly better and pass down a \"don't warn please\" to\n>> the sub-process?>> Or do we think it might warn about *different* URLs, aren't these always\n>> the same (i.e. we read the full config)?\n>\n> While this example of 'git clone' to 'git-remote-curl' will not use a\n> different URL, I can imagine other child process connections that could\n> expose a different URL. Having a blanket statement \"don't emit these\n> warnings\" seems unlikely to be helpful for those cases.\n\nYes, I can imagine we might want to do that, but until we find such an\nAPI user it seems much simpler to warn in the \"primary\",\ni.e. clone/fetch & not the helper process.\n\nUnless there's cases I'm missing where the \"primary\" is aware of the URL\nand the others aren't (and even then we could just do this the other way\naround, no?)\n\nAlso isn't such a warning behavior unnecessarily verbose, i.e. does this\n(I haven't checked in detail) warn about any such bad URL in config\nanywhere, not just the one you're actually using at the moment, or at\nleast the one for your current remote?\n\nIn that case (if we end up having trouble with this) perhaps just\nwarning about the URL we use would be Good Enough and simpler.\n\n>> If you used advice instead of a warning you'd get a config key to pass\n>> for free to disable it, and that might be a better thing to do in any\n>> case (i.e. a better fit for this case). But we could also just check\n>> getenv(\"GIT_URL_REDACTED_WARNING\") or whatever, which seems easy\n>> enough...\n>\n> To make the environment variable work properly, we would need to pass a\n> list of URLs in the variable that could seed the strset, to avoid dropping\n> warnings for new cases as mentioned earlier.\n\nI think a much simpler thing to do is to just set a variable in clone.c\nand fetch.c and have those emit this, and otherwise stay quiet.\n\nThe reason you have duplication now is because clone is calling a remote\nhelper.\n\n>>> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\n>>> index cba3553b7c4..6ae3eec9eb6 100755\n>>> --- a/t/t5601-clone.sh\n>>> +++ b/t/t5601-clone.sh\n>>> @@ -75,7 +75,10 @@ test_expect_success 'clone warns or fails when using username:password' '\n>>> \ttest_must_fail git -c fetch.credentialsInUrl=allow clone https://username:password@localhost attempt1 2>err &&\n>>> \t! grep \"URL '\\''https://username:@localhost'\\'' uses plaintext credentials\" err &&\n>>> \ttest_must_fail git -c fetch.credentialsInUrl=warn clone https://username:password@localhost attempt1 2>err &&\n>>> -\tgrep \"warning: URL '\\''https://username:@localhost'\\'' uses plaintext credentials\" err &&\n>>> +\tgrep \"warning: URL '\\''https://username:@localhost'\\'' uses plaintext credentials\" err >warnings &&\n>>> +\t# The warning is printed twice, for the two processes:\n>>> +\t# \"git clone\" and \"git-remote-curl\".\n>>> +\ttest_line_count = 2 warnings &&\n>>> \ttest_must_fail git -c fetch.credentialsInUrl=die clone https://username:password@localhost attempt2 2>err &&\n>>> \tgrep \"fatal: URL '\\''https://username:@localhost'\\'' uses plaintext credentials\" err\n\nSpeaking of which, we really should test this \"we expect N warning(s)\"\nfor both fetch & clone, shouldn't we? Ditto push.\n\n>>> '\n>> \n>> Hrm, between 1/2 and this I think this is a good example of a \"just use\n>> test_cmp\" caveat.\n>> \n>> I.e. reading 1/2 it's snuck past us that there's this existing caveat in\n>> the warning being added, i.e. that we should really warn 1 times, but\n>> are doing in N times, but as we use ad-hoc \"grep\" instead of\n>> test_cmp-ing the full output that's not obvious.\n>> \n>> I think this would be much more obvious as:\n>> \n>> \twarning=\"\" &&\n>> \tcat >expect <<-EOF &&\n>> \t$warning\n>> $warning\n>> \tEOF\n>\n> There are other warnings coming over stderr, including the \"Cloning\n> into...\" message and the resulting \"fatal:\" message because that URL\n> doesn't actually exist. I chose to decouple this test to the exact\n> phrasing of those messages.\n\nFWIW:\n\n\tgrep ^warning: >warnings &&\n\ttest_cmp ...\n\nWould take care of that.\n\n>> But even better (and per the above, I'm not convinced about the\n>> direction, but leaving that aside): Here we have a choice between having\n>> 1/2 and 2/2 in that order and having 2/2 add a new API that has its\n>> first user right away, but at the cost of fixing a bug we just\n>> introduced in 1/2.\n>> \n>> I think even if we can't find another API user for this proposed usage.c\n>> addition, just adding it without a user & adding what's now 1/2 as the\n>> 2nd commit would be preferrable. Then we're not doing a \"oops, bug fix\"\n>> while we're at it.\n>\n> I wouldn't call this a \"oops, bug fix\" but instead \"let's improve what was\n> working, but noisy.\" This also gives us the opportunity to see the code in\n> usage.c get tested immediately as it is introduced, which is an important\n> property to keep in mind when organizing a patch series.\n\n*nod*, just a thought.\n\n>>> +static struct strset prev_warnings = STRSET_INIT;\n>>> +\n>>> +void warn_once(const char *warn, ...)\n>>> +{\n>> \n>> If we do end up keeping this (per the above I'm thinking while it's just\n>> this caller it should probably own this problem):\n>> \n>> I have a local change to clean up this warn v.s. warning inconsistency\n>> in usage.c, but now it's all in internal code.\n>> \n>> But let's not add to it by adding a warn_once(), it should be called\n>> warning_once().\n>\n> Sure. The phrase \"warn once\" is a valid sentence, while \"warning once\" is\n> not, but I can see some benefit to having consistent naming here.\n>\n> If the \"warning()\" method was not already present so much throughout the\n> codebase, then I would advocate that \"warn()\" is a better verb form. This\n> would be similar to \"die()\" and even \"error()\" could be interpreted as a\n> verb. However, that would be too large of a change to be worthwhile.\n\nYeah I'd also prefer warn() myself, but since we have warning() and\nwarning_errno() consistency is better...\n\n>> We're also missing an \"errno\" variant, which might be fine for now,\n>> ditto variants for the other non-fatal functions (error &\n>> die_message). That might be OK for now, but probably worth noting.\n>\n> Noted.\n>\n>>> +void warn_once(const char *warn, ...)\n>>> +{\n>>> +\tchar buf[1024];\n>>> +\tva_list params;\n>>> +\tva_start(params, warn);\n>>> +\n>>> +\tif (vsnprintf(buf, sizeof(buf), warn, params) >= 0) {\n>> \n>> It doesn't matter for the end result, but let's compare with \"< 0\" like\n>> the other vsnprintf() caller in the file.\n>\n> Except that we do something after this block in both cases, so the\n> condition needs to be swapped or else duplicate code.\n\n*nod*, just a thought while skimming it.\n\n>> And how is this unlike that vsnprintf() in not needing the same error\n>> handling vreportf() does here:\n>> \n>> \t*p = '\\0'; /* vsnprintf() failed, clip at prefix */\n>> \n>> Seems like either a bug, or that other code being something we should be\n>> dropping.\n>\n> No, because the params are passed to warn_routine() in the section of code\n> you cropped out:\n>\n> +\tva_start(params, warn);\n> +\twarn_routine(warn, params);\n> +\tva_end(params);\n>\n> That warn_routine does the \"clip at prefix\" fix. if we have a failure in\n> vsnprintf() here, then we are skipping the \"only once\" check and going\n> straight to that logic.\n\nAh, makes sense. I missed that.\n\n>>> +\t\tif (!strset_add(&prev_warnings, buf)) {\n>> \n>> More on the general API usability front: E.g. builtin/pack-objects.c\n>> seems like it could make use of thing like this in two places.\n>> \n>> But the API you've added here would only be usable for 1/2, i.e. you\n>> format and de-dup on the full warning, whereas at least 1/2 of those\n>> callers wants to de-dup on the un-formatted string (grep for 'static int\n>> warned').\n>> \n>> Which (and I'm sounding like a broken record:) is another thing that me\n>> wonder if the general API is premature, i.e. it's a red flag that we\n>> have existing code that could benefit from it, if not for an arbitrary\n>> limitation, in this case fixing the limitation would mean the churn of\n>> either adding a new function, or a new parameter to all callers. I.e. a\n>> \"int dedup_on_args\" or similar.\n>\n> You are advocating that since there is one example of something that\n> doesn't fit this exact scenario, then we shouldn't move forward on\n> something that _does_ have immediate use (and potential use in other\n> places). This seem unnecessarily pedantic.\n\nI didn't mean to be pedantic, sorry.\n\nMy thinking was more along the lines that I've seen roughly this pattern\nin a few other places, and a proposed new library utility for generic\nuse is both more convincing, and tends to have bugs shook out of it if\nwe go the extra mile to convert existing API users.\n\nAnyway, per the above I think this one is more easily handled by its\ncaller by just moving the check up one function call...\n\n> This specific method is a good foundation for possibly extending it if\n> there is value in doing so.\n\n>>> +\t\t\tva_end(params);\n>>> +\t\t\treturn;\n>>> +\t\t}\n>>> +\t}\n>>> +\tva_end(params);\n>>> +\n>>> +\tva_start(params, warn);\n>> \n>> I'm rusty on the varargs API (and haven't tested) but didn't we need\n>> va_copy() if we're iterating twice, or was that just if we're passing it\n>> to another function and using it ourselves...\n>\n> Well, outside of the fact that this function is tested and it works,\n> the documentation says this about the 'params' argument (named 'ap'\n> in the function prototype):\n>\n> If ap has already been passed as first argument to a previous call to\n> va_start or va_copy, it shall be passed to va_end before calling this\n> function.\n>\n> So, va_end(params) makes it safe to call va_start(params, warn) again.\n\nYes, thanks. This was just from digging around in now\nobviously-incorrect wetware :)\n"},{"id":"456463","messageId":"xmqq35gorv7c.fsf@gitster.g","threadId":"57913","inReplyTo":"8f96563b-87c5-2a8c-fe35-19fd0cc3c7d2@github.com","subject":"Re: [PATCH v3 2/2] usage: add warn_once() helper for repeated warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-01T20:21:27Z","receivedAt":"2022-06-01T20:43:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> We could certainly investigate this more, but it seems like a more\n> problematic approach than the one taken here. We could add a \"is_valid\"\n> bit to struct remote, but then could some code path modify that struct\n> after it was validated?\n\nTwo separate parser parsing the same string to produce (supposedly)\nequivalent parse results is a bit disturbing, and I am not sure if\n\"is_valid\" bit helps that.\n\nAdding \"user\" and \"password\" members to the struct, and retire the\nexisting \"parser\" (instead it would just use the pre-parsed\ncomponents stored in the struct) would. It would be a much more\ninvolved change, and it is something more than we would want to do\nin a regression fix patch.\n\nBut this series is a new feature development, so...\n\n\n\n\n"},{"id":"456464","messageId":"220601.86v8tkyyt4.gmgdl@evledraar.gmail.com","threadId":"57913","inReplyTo":"083a918e9b1474eff0d51c4502b6d54de9b63764.1654046173.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/2] remote: create fetch.credentialsInUrl config","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-01T19:19:54Z","receivedAt":"2022-06-01T20:45:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jun 01 2022, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee \n>\n> +\tfor (int i = 0; i < remote->url_nr; i++)\n\nI think we're trying to avoid that bit of C99 in general use until\nJunio's canary graduates. See 6563706568b (CodingGuidelines: give\ndeadline for \"for (int i = 0; ...\", 2022-03-30).\n"},{"id":"456466","messageId":"xmqqtu94qfrt.fsf@gitster.g","threadId":"57913","inReplyTo":"8e29ac807c6a0cf94ea3a44ee3304011c2ad159c.1654046173.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 2/2] usage: add warn_once() helper for repeated warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-01T20:40:06Z","receivedAt":"2022-06-01T20:47:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> In the case of the credentials in a URL, the existing test demonstrates\n> this per-process limitation: 'git clone' runs 'git-remote-curl' as a\n> child process, giving two messages. This is an improvement over the\n> previous six messages.\n\nDo we call the valid_remote() helper on the same remote many number\nof times? Unless the duplicates appear by such a \"stupid\" reason, I\nam not sure if this is a good idea to do this deduplication.\n\nEspecially if we know we cannot do the deduplication across\nprocesses.\n"},{"id":"456509","messageId":"73cdf9e3-7c8f-048b-0845-f87fa0861cd5@github.com","threadId":"57913","inReplyTo":"220601.86v8tkyyt4.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 1/2] remote: create fetch.credentialsInUrl config","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-06-02T13:38:09Z","receivedAt":"2022-06-02T13:38:20Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/1/2022 3:19 PM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Wed, Jun 01 2022, Derrick Stolee via GitGitGadget wrote:\n> \n>> From: Derrick Stolee \n>>\n>> +\tfor (int i = 0; i < remote->url_nr; i++)\n> \n> I think we're trying to avoid that bit of C99 in general use until\n> Junio's canary graduates. See 6563706568b (CodingGuidelines: give\n> deadline for \"for (int i = 0; ...\", 2022-03-30).\n\nAh, thanks. I remembered that canary going out a long time ago,\nbut forgot to check if we had committed to it.\n\nThanks,\n-Stolee\n"},{"id":"456510","messageId":"049aae88-f5b3-788e-3979-34e8178e6bd4@github.com","threadId":"57913","inReplyTo":"220601.86r148yxi3.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 2/2] usage: add warn_once() helper for repeated warnings","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-06-02T13:43:33Z","receivedAt":"2022-06-02T13:43:40Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/1/2022 3:33 PM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Wed, Jun 01 2022, Derrick Stolee wrote:\n> \n>> On 6/1/2022 8:29 AM, Ævar Arnfjörð Bjarmason wrote:> \n>>> On Wed, Jun 01 2022, Derrick Stolee via GitGitGadget wrote:\n>>>\n>>>> From: Derrick Stolee \n>>>>\n>>>> The previous change added a warning when valid_remote() detects\n>>>> credentials in the URL. Since remotes are validated multiple times per\n>>>> process, this causes multiple warnings to print.\n>>>\n>>> Why are we validating remotes multiple times per process? Can't we just\n>>> do it once?\n>>>\n>>> Is this a case of two callers going through the whole machinery and not\n>>> being aware of one another?\n>>>\n>>> Perhaps it's a pain to deal with that in this case, but it would be\n>>> better to note why here than taking it as a given.\n>>\n>> We could certainly investigate this more, but it seems like a more\n>> problematic approach than the one taken here. We could add a \"is_valid\"\n>> bit to struct remote, but then could some code path modify that struct\n>> after it was validated?\n> \n> I tested this a bit and I think this alternate approach is simpler, and\n> it passes your tests:\n... \n> \t+static void valid_remote_at_end(const struct remote *remote)\n> \t+{\n> \t+\tint i;\n> \t+\n> \t+\tfor (i = 0; i < remote->url_nr; i++)\n> \t+\t\tcheck_if_creds_in_url(remote->url[i]);\n> \t+}\n> \t+\n> \t static const char *alias_url(const char *url, struct rewrites *r)\n> \t {\n> \t \tint i, j;\n> \t@@ -687,6 +692,7 @@ remotes_remote_get_1(struct remote_state *remote_state, const char *name,\n> \t \t\tadd_url_alias(remote_state, ret, name);\n> \t \tif (!valid_remote(ret))\n> \t \t\treturn NULL;\n> \t+\tvalid_remote_at_end(ret);\n> \t \treturn ret;\n> \t }\n> \n> I.e. we already have one spot where we get the full remote config, you\n> were just hooking into valid_remote() which we call N times while doing\n> that, let's just call it at the end.\n\nThanks for finding a place where the current behavior only runs once.\n\nI agree that this is a much simpler approach and reduces this series\nback down to one.\n\nThanks,\n-Stolee\n"},{"id":"456520","messageId":"376e65b1-3001-9bf8-8d08-dcfd9c6a8caf@github.com","threadId":"57913","inReplyTo":"xmqq35gorv7c.fsf@gitster.g","subject":"Re: [PATCH v3 2/2] usage: add warn_once() helper for repeated warnings","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-06-02T14:24:18Z","receivedAt":"2022-06-02T14:24:24Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/1/2022 4:21 PM, Junio C Hamano wrote:\n> Derrick Stolee writes:\n> \n>> We could certainly investigate this more, but it seems like a more\n>> problematic approach than the one taken here. We could add a \"is_valid\"\n>> bit to struct remote, but then could some code path modify that struct\n>> after it was validated?\n> \n> Two separate parser parsing the same string to produce (supposedly)\n> equivalent parse results is a bit disturbing, and I am not sure if\n> \"is_valid\" bit helps that.\n> \n> Adding \"user\" and \"password\" members to the struct, and retire the\n> existing \"parser\" (instead it would just use the pre-parsed\n> components stored in the struct) would. It would be a much more\n> involved change, and it is something more than we would want to do\n> in a regression fix patch.\n> \n> But this series is a new feature development, so...\n\nYes, you're right. I should use the output 'struct url_inf' from\nurl_normalize() to construct the redacted URL. It has the downside\nthat the output URL can be slightly different from the input URL,\nbut a user should still be able to diagnose how to resolve the\nsituation.\n\nThanks,\n-Stolee\n"},{"id":"456532","messageId":"pull.1237.v4.git.1654190434908.gitgitgadget@gmail.com","threadId":"57913","inReplyTo":"pull.1237.v3.git.1654046173.gitgitgadget@gmail.com","subject":"[PATCH v4] remote: create fetch.credentialsInUrl config","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-02T17:20:34Z","receivedAt":"2022-06-02T17:21:02Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nUsers sometimes provide a \"username:password\" combination in their\nplaintext URLs. Since Git stores these URLs in plaintext in the\n.git/config file, this is a very insecure way of storing these\ncredentials. Credential managers are a more secure way of storing this\ninformation.\n\nSystem administrators might want to prevent this kind of use by users on\ntheir machines.\n\nCreate a new \"fetch.credentialsInUrl\" config option and teach Git to\nwarn or die when seeing a URL with this kind of information. The warning\nanonymizes the sensitive information of the URL to be clear about the\nissue.\n\nThis change currently defaults the behavior to \"allow\" which does\nnothing with these URLs. We can consider changing this behavior to\n\"warn\" by default if we wish. At that time, we may want to add some\nadvice about setting fetch.credentialsInUrl=ignore for users who still\nwant to follow this pattern (and not receive the warning).\n\nAn earlier version of this change injected the logic into\nurl_normalize() in urlmatch.c. While most code paths that parse URLs\neventually normalize the URL, that normalization does not happen early\nenough in the stack to avoid attempting connections to the URL first. By\ninserting a check into the remote validation, we identify the issue\nbefore making a connection. In the old code path, this was revealed by\ntesting the new t5601-clone.sh test under --stress, resulting in an\ninstance where the return code was 13 (SIGPIPE) instead of 128 from the\ndie().\n\nHowever, we can reuse the parsing information from url_normalize() in\norder to benefit from its well-worn parsing logic. We can use the struct\nurl_info that is created in that method to replace the password with\n\"\" in our error messages. This comes with a slight downside\nthat the normalized URL might look slightly different from the input URL\n(for instance, the normalized version adds a closing slash). This should\nnot hinder users figuring out what the problem is and being able to fix\nthe issue.\n\nAs an attempt to ensure the parsing logic did not catch any\nunintentional cases, I modified this change locally to to use the \"die\"\noption by default. Running the test suite succeeds except for the\nexplicit username:password URLs used in t5550-http-fetch-dumb.sh and\nt5541-http-push-smart.sh. This means that all other tested URLs did not\ntrigger this logic.\n\nThe tests show that the proper error messages appear (or do not\nappear), but also count the number of error messages. When only warning,\neach process validates the remote URL and outputs a warning. This\nhappens twice for clone, three times for fetch, and once for push.\n\nSigned-off-by: Derrick Stolee \n---\n remote: create fetch.credentialsInUrl config\n \n Users can specify credentials in their URL using the\n username:password@domain format. This is potentially hazardous since the\n URL is stored in plaintext and can also appear in trace2 logs and other\n places. Add a new config option that allows warnings or failures when\n discovering URLs with this format. The default behavior does not change\n in this series, although we may want to move to the warn state by\n default in the future.\n \n This is a modified version of the patch I submitted a while ago [1].\n \n Based on the feedback, changing the behavior to fail by default was not\n a good approach. Further, the idea to stop storing the credentials in\n config and redirect them to a credential manager was already considered\n by Peff [2] but not merged.\n \n This patch does what should be the simplest thing we can do: create a\n config option that will cause the user to get a warning or a failure,\n depending on its value. The default is to ignore the setting, identical\n to the current behavior. We can talk about changing this default to\n \"warn\" in the future, but it would be safest to release with ignore as\n the default until we are sure that we are not going to start warning on\n false positives.\n \n This patch would be sufficient for the interested internal parties that\n want to prevent users from storing credentials this way. System\n administrators can modify system-level Git config into \"die\" mode to\n prevent this behavior.\n \n [1]\n https://lore.kernel.org/git/pull.945.git.1619807844627.gitgitgadget@gmail.com\n Reject passwords in URLs (April 2021).\n \n [2]\n https://lore.kernel.org/git/20190519050724.GA26179@sigill.intra.peff.net/\n Re: Git ransom campaign incident report - May 2019\n \n \n Updates in v4\n =============\n \n * The warn_once() patch is dropped in favor of using a different\n location for the check (remotes_remote_get_1() instead of\n valid_remote()).\n * The parsing logic is removed in favor of using the output url_info\n from url_normalize().\n * Tests for 'fetch' and 'push' are added.\n * This requires updating the topic to be on a more-recent version of\n 'master' because some change since the previous base caused 'git\n push' to output the warning only once, not twice.\n \n \n Updates in v3\n =============\n \n * Because of some flaky behavior around SIGPIPE, the URL check needed\n to move to be earlier in the command.\n * For this reason, I moved the cred check into remote.c's\n valid_remote() check. This changed the previous BUG() statements into\n early returns.\n * I repeated the check with the test suite to see if this parsing fails\n on any existing cases, but it is worth double-checking the parsing\n rules.\n * Documentation is more consistent about using placeholders.\n * A test for the \"allow\" case is now included.\n * A new patch is added that creates the warn_once() helper. This\n reduces multiple advisory warnings with the same text from being\n written by the same process.\n \n \n Updates in v2\n =============\n \n * Documentation is slightly expanded to include the fact that Git\n stores the given URL as plaintext in its config.\n * The new method has a new documentation comment that details the\n necessary preconditions.\n * \"ignore\" is now \"allow\"\n * Additional checks on colon_ptr are added.\n * Use strbuf_splice() instead of custom string-walking logic.\n * Use \"\" instead of asterisks.\n * Config value checks are no longer case sensitive.\n \n Thanks, -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1237%2Fderrickstolee%2Fcreds-in-url-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1237/derrickstolee/creds-in-url-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/1237\n\nRange-diff vs v3:\n\n 1: 083a918e9b1 ! 1: 5fb038402d6 remote: create fetch.credentialsInUrl config\n @@ Commit message\n instance where the return code was 13 (SIGPIPE) instead of 128 from the\n die().\n \n - Since we are not deep within url_normalize(), we need to do our own\n - parsing to detect if there is a \"username:password@domain\" section. We\n - begin by detecting the first '@' symbol in the URL. We then detect if\n - there is a scheme such as \"https://\" by finding the first slash. If that\n - slash does not exist or is after the first '@' symbol, then we consider\n - the scheme to be complete before the URL. Finally, we look for a colon\n - between the scheme and the '@' symbol, indicating a \"username:password\"\n - string. Replace the password with \"\" when writing the error\n - message.\n + However, we can reuse the parsing information from url_normalize() in\n + order to benefit from its well-worn parsing logic. We can use the struct\n + url_info that is created in that method to replace the password with\n + \"\" in our error messages. This comes with a slight downside\n + that the normalized URL might look slightly different from the input URL\n + (for instance, the normalized version adds a closing slash). This should\n + not hinder users figuring out what the problem is and being able to fix\n + the issue.\n \n As an attempt to ensure the parsing logic did not catch any\n unintentional cases, I modified this change locally to to use the \"die\"\n @@ Commit message\n t5541-http-push-smart.sh. This means that all other tested URLs did not\n trigger this logic.\n \n + The tests show that the proper error messages appear (or do not\n + appear), but also count the number of error messages. When only warning,\n + each process validates the remote URL and outputs a warning. This\n + happens twice for clone, three times for fetch, and once for push.\n +\n Signed-off-by: Derrick Stolee \n \n ## Documentation/config/fetch.txt ##\n @@ Documentation/config/fetch.txt: fetch.writeCommitGraph::\n + with a plaintext credential.\n \n ## remote.c ##\n -@@ remote.c: struct counted_string {\n - \tconst char *s;\n - };\n +@@\n + #include \"cache.h\"\n + #include \"config.h\"\n + #include \"remote.h\"\n ++#include \"urlmatch.h\"\n + #include \"refs.h\"\n + #include \"refspec.h\"\n + #include \"object-store.h\"\n +@@ remote.c: const char *remote_ref_for_branch(struct branch *branch, int for_push)\n + \treturn NULL;\n + }\n \n -+/*\n -+ * Check if the given URL is of the following form:\n -+ *\n -+ * scheme://username:password@domain[:port][/path]\n -+ *\n -+ * Specifically, see if the \":password@\" section of the URL appears.\n -+ *\n -+ * The fetch.credentialsInUrl config indicates what to do on such a URL,\n -+ * either ignoring, warning, or erroring. The latter two modes write a\n -+ * redacted URL to stderr.\n -+ */\n -+static void check_if_creds_in_url(const char *url)\n ++static void validate_remote_url(struct remote *remote)\n +{\n -+\tconst char *value, *scheme_ptr, *colon_ptr, *at_ptr;\n ++\tint i;\n ++\tconst char *value;\n +\tstruct strbuf redacted = STRBUF_INIT;\n +\n -+\t/* \"allow\" is the default behavior. */\n +\tif (git_config_get_string_tmp(\"fetch.credentialsinurl\", &value) ||\n +\t !strcmp(\"allow\", value))\n +\t\treturn;\n +\n -+\tif (!(at_ptr = strchr(url, '@')))\n -+\t\treturn;\n ++\tfor (i = 0; i < remote->url_nr; i++) {\n ++\t\tstruct url_info url_info = { NULL };\n ++\t\turl_normalize(remote->url[i], &url_info);\n +\n -+\tif (!(scheme_ptr = strchr(url, '/')) ||\n -+\t scheme_ptr > at_ptr)\n -+\t\tscheme_ptr = url;\n ++\t\tif (!url_info.passwd_len)\n ++\t\t\tgoto loop_cleanup;\n +\n -+\tif (!(colon_ptr = strchr(scheme_ptr, ':')))\n -+\t\treturn;\n ++\t\tstrbuf_add(&redacted, url_info.url, url_info.passwd_off);\n ++\t\tstrbuf_addstr(&redacted, \"\");\n ++\t\tstrbuf_addstr(&redacted, url_info.url + url_info.passwd_off + url_info.passwd_len);\n +\n -+\t/* Include the colon when creating the redacted URL. */\n -+\tcolon_ptr++;\n -+\tstrbuf_addstr(&redacted, url);\n -+\tstrbuf_splice(&redacted, colon_ptr - url, at_ptr - colon_ptr,\n -+\t\t \"\", 10);\n ++\t\tif (!strcmp(\"warn\", value))\n ++\t\t\twarning(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n ++\t\tif (!strcmp(\"die\", value))\n ++\t\t\tdie(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n +\n -+\tif (!strcmp(\"warn\", value))\n -+\t\twarning(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n -+\tif (!strcmp(\"die\", value))\n -+\t\tdie(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n ++loop_cleanup:\n ++\t\tfree(url_info.url);\n ++\t}\n +\n +\tstrbuf_release(&redacted);\n +}\n +\n - static int valid_remote(const struct remote *remote)\n - {\n -+\tfor (int i = 0; i < remote->url_nr; i++)\n -+\t\tcheck_if_creds_in_url(remote->url[i]);\n + static struct remote *\n + remotes_remote_get_1(struct remote_state *remote_state, const char *name,\n + \t\t const char *(*get_default)(struct remote_state *,\n +@@ remote.c: remotes_remote_get_1(struct remote_state *remote_state, const char *name,\n + \t\tadd_url_alias(remote_state, ret, name);\n + \tif (!valid_remote(ret))\n + \t\treturn NULL;\n ++\n ++\tvalidate_remote_url(ret);\n +\n - \treturn (!!remote->url) || (!!remote->foreign_vcs);\n + \treturn ret;\n }\n \n \n + ## t/t5516-fetch-push.sh ##\n +@@ t/t5516-fetch-push.sh: This test checks the following functionality:\n + * --porcelain output format\n + * hiderefs\n + * reflogs\n ++* URL validation\n + '\n + \n + GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n +@@ t/t5516-fetch-push.sh: test_expect_success 'refuse to push a hidden ref, and make sure do not pollute t\n + \ttest_dir_is_empty testrepo/.git/objects/pack\n + '\n + \n ++test_expect_success 'fetch warns or fails when using username:password' '\n ++\tmessage=\"URL '\\''https://username:@localhost/'\\'' uses plaintext credentials\" &&\n ++\ttest_must_fail git -c fetch.credentialsInUrl=allow fetch https://username:password@localhost 2>err &&\n ++\t! grep \"$message\" err &&\n ++\n ++\ttest_must_fail git -c fetch.credentialsInUrl=warn fetch https://username:password@localhost 2>err &&\n ++\tgrep \"warning: $message\" err >warnings &&\n ++\ttest_line_count = 3 warnings &&\n ++\n ++\ttest_must_fail git -c fetch.credentialsInUrl=die fetch https://username:password@localhost 2>err &&\n ++\tgrep \"fatal: $message\" err >warnings &&\n ++\ttest_line_count = 1 warnings\n ++'\n ++\n ++\n ++test_expect_success 'push warns or fails when using username:password' '\n ++\tmessage=\"URL '\\''https://username:@localhost/'\\'' uses plaintext credentials\" &&\n ++\ttest_must_fail git -c fetch.credentialsInUrl=allow push https://username:password@localhost 2>err &&\n ++\t! grep \"$message\" err &&\n ++\n ++\ttest_must_fail git -c fetch.credentialsInUrl=warn push https://username:password@localhost 2>err &&\n ++\tgrep \"warning: $message\" err >warnings &&\n ++\ttest_must_fail git -c fetch.credentialsInUrl=die push https://username:password@localhost 2>err &&\n ++\tgrep \"fatal: $message\" err >warnings &&\n ++\ttest_line_count = 1 warnings\n ++'\n ++\n + test_done\n +\n ## t/t5601-clone.sh ##\n @@ t/t5601-clone.sh: test_expect_success 'clone respects GIT_WORK_TREE' '\n \n '\n \n +test_expect_success 'clone warns or fails when using username:password' '\n ++\tmessage=\"URL '\\''https://username:@localhost/'\\'' uses plaintext credentials\" &&\n +\ttest_must_fail git -c fetch.credentialsInUrl=allow clone https://username:password@localhost attempt1 2>err &&\n -+\t! grep \"URL '\\''https://username:@localhost'\\'' uses plaintext credentials\" err &&\n -+\ttest_must_fail git -c fetch.credentialsInUrl=warn clone https://username:password@localhost attempt1 2>err &&\n -+\tgrep \"warning: URL '\\''https://username:@localhost'\\'' uses plaintext credentials\" err &&\n -+\ttest_must_fail git -c fetch.credentialsInUrl=die clone https://username:password@localhost attempt2 2>err &&\n -+\tgrep \"fatal: URL '\\''https://username:@localhost'\\'' uses plaintext credentials\" err\n ++\t! grep \"$message\" err &&\n ++\n ++\ttest_must_fail git -c fetch.credentialsInUrl=warn clone https://username:password@localhost attempt2 2>err &&\n ++\tgrep \"warning: $message\" err >warnings &&\n ++\ttest_line_count = 2 warnings &&\n ++\n ++\ttest_must_fail git -c fetch.credentialsInUrl=die clone https://username:password@localhost attempt3 2>err &&\n ++\tgrep \"fatal: $message\" err >warnings &&\n ++\ttest_line_count = 1 warnings\n +'\n +\n +test_expect_success 'clone does not detect username:password when it is https://username@domain:port/' '\n 2: 8e29ac807c6 < -: ----------- usage: add warn_once() helper for repeated warnings\n\n\n Documentation/config/fetch.txt | 14 +++++++++++++\n remote.c | 37 ++++++++++++++++++++++++++++++++++\n t/t5516-fetch-push.sh | 28 +++++++++++++++++++++++++\n t/t5601-clone.sh | 19 +++++++++++++++++\n 4 files changed, 98 insertions(+)\n\ndiff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt\nindex cd65d236b43..0db7fe85bb8 100644\n--- a/Documentation/config/fetch.txt\n+++ b/Documentation/config/fetch.txt\n@@ -96,3 +96,17 @@ fetch.writeCommitGraph::\n \tmerge and the write may take longer. Having an updated commit-graph\n \tfile helps performance of many Git commands, including `git merge-base`,\n \t`git push -f`, and `git log --graph`. Defaults to false.\n+\n+fetch.credentialsInUrl::\n+\tA URL can contain plaintext credentials in the form\n+\t`://:@/`. Using such URLs\n+\tis not recommended as it exposes the password in multiple ways,\n+\tincluding Git storing the URL as plaintext in the repository config.\n+\tThe `fetch.credentialsInUrl` option provides instruction for how Git\n+\tshould react to seeing such a URL, with these values:\n++\n+* `allow` (default): Git will proceed with its activity without warning.\n+* `warn`: Git will write a warning message to `stderr` when parsing a URL\n+ with a plaintext credential.\n+* `die`: Git will write a failure message to `stderr` when parsing a URL\n+ with a plaintext credential.\ndiff --git a/remote.c b/remote.c\nindex 930fdc9c2f6..1f4a95e9840 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1,6 +1,7 @@\n #include \"cache.h\"\n #include \"config.h\"\n #include \"remote.h\"\n+#include \"urlmatch.h\"\n #include \"refs.h\"\n #include \"refspec.h\"\n #include \"object-store.h\"\n@@ -614,6 +615,39 @@ const char *remote_ref_for_branch(struct branch *branch, int for_push)\n \treturn NULL;\n }\n \n+static void validate_remote_url(struct remote *remote)\n+{\n+\tint i;\n+\tconst char *value;\n+\tstruct strbuf redacted = STRBUF_INIT;\n+\n+\tif (git_config_get_string_tmp(\"fetch.credentialsinurl\", &value) ||\n+\t !strcmp(\"allow\", value))\n+\t\treturn;\n+\n+\tfor (i = 0; i < remote->url_nr; i++) {\n+\t\tstruct url_info url_info = { NULL };\n+\t\turl_normalize(remote->url[i], &url_info);\n+\n+\t\tif (!url_info.passwd_len)\n+\t\t\tgoto loop_cleanup;\n+\n+\t\tstrbuf_add(&redacted, url_info.url, url_info.passwd_off);\n+\t\tstrbuf_addstr(&redacted, \"\");\n+\t\tstrbuf_addstr(&redacted, url_info.url + url_info.passwd_off + url_info.passwd_len);\n+\n+\t\tif (!strcmp(\"warn\", value))\n+\t\t\twarning(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n+\t\tif (!strcmp(\"die\", value))\n+\t\t\tdie(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n+\n+loop_cleanup:\n+\t\tfree(url_info.url);\n+\t}\n+\n+\tstrbuf_release(&redacted);\n+}\n+\n static struct remote *\n remotes_remote_get_1(struct remote_state *remote_state, const char *name,\n \t\t const char *(*get_default)(struct remote_state *,\n@@ -639,6 +673,9 @@ remotes_remote_get_1(struct remote_state *remote_state, const char *name,\n \t\tadd_url_alias(remote_state, ret, name);\n \tif (!valid_remote(ret))\n \t\treturn NULL;\n+\n+\tvalidate_remote_url(ret);\n+\n \treturn ret;\n }\n \ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 4dfb080433e..4497617db7f 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -12,6 +12,7 @@ This test checks the following functionality:\n * --porcelain output format\n * hiderefs\n * reflogs\n+* URL validation\n '\n \n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n@@ -1813,4 +1814,31 @@ test_expect_success 'refuse to push a hidden ref, and make sure do not pollute t\n \ttest_dir_is_empty testrepo/.git/objects/pack\n '\n \n+test_expect_success 'fetch warns or fails when using username:password' '\n+\tmessage=\"URL '\\''https://username:@localhost/'\\'' uses plaintext credentials\" &&\n+\ttest_must_fail git -c fetch.credentialsInUrl=allow fetch https://username:password@localhost 2>err &&\n+\t! grep \"$message\" err &&\n+\n+\ttest_must_fail git -c fetch.credentialsInUrl=warn fetch https://username:password@localhost 2>err &&\n+\tgrep \"warning: $message\" err >warnings &&\n+\ttest_line_count = 3 warnings &&\n+\n+\ttest_must_fail git -c fetch.credentialsInUrl=die fetch https://username:password@localhost 2>err &&\n+\tgrep \"fatal: $message\" err >warnings &&\n+\ttest_line_count = 1 warnings\n+'\n+\n+\n+test_expect_success 'push warns or fails when using username:password' '\n+\tmessage=\"URL '\\''https://username:@localhost/'\\'' uses plaintext credentials\" &&\n+\ttest_must_fail git -c fetch.credentialsInUrl=allow push https://username:password@localhost 2>err &&\n+\t! grep \"$message\" err &&\n+\n+\ttest_must_fail git -c fetch.credentialsInUrl=warn push https://username:password@localhost 2>err &&\n+\tgrep \"warning: $message\" err >warnings &&\n+\ttest_must_fail git -c fetch.credentialsInUrl=die push https://username:password@localhost 2>err &&\n+\tgrep \"fatal: $message\" err >warnings &&\n+\ttest_line_count = 1 warnings\n+'\n+\n test_done\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 4a61f2c901e..d03c19f5e07 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -71,6 +71,25 @@ test_expect_success 'clone respects GIT_WORK_TREE' '\n \n '\n \n+test_expect_success 'clone warns or fails when using username:password' '\n+\tmessage=\"URL '\\''https://username:@localhost/'\\'' uses plaintext credentials\" &&\n+\ttest_must_fail git -c fetch.credentialsInUrl=allow clone https://username:password@localhost attempt1 2>err &&\n+\t! grep \"$message\" err &&\n+\n+\ttest_must_fail git -c fetch.credentialsInUrl=warn clone https://username:password@localhost attempt2 2>err &&\n+\tgrep \"warning: $message\" err >warnings &&\n+\ttest_line_count = 2 warnings &&\n+\n+\ttest_must_fail git -c fetch.credentialsInUrl=die clone https://username:password@localhost attempt3 2>err &&\n+\tgrep \"fatal: $message\" err >warnings &&\n+\ttest_line_count = 1 warnings\n+'\n+\n+test_expect_success 'clone does not detect username:password when it is https://username@domain:port/' '\n+\ttest_must_fail git -c fetch.credentialsInUrl=warn clone https://username@localhost:8080 attempt3 2>err &&\n+\t! grep \"uses plaintext credentials\" err\n+'\n+\n test_expect_success 'clone from hooks' '\n \n \ttest_create_repo r0 &&\n\nbase-commit: 2668e3608e47494f2f10ef2b6e69f08a84816bcb\n-- \ngitgitgadget\n"},{"id":"456536","messageId":"xmqqilpjne95.fsf@gitster.g","threadId":"57913","inReplyTo":"376e65b1-3001-9bf8-8d08-dcfd9c6a8caf@github.com","subject":"Re: [PATCH v3 2/2] usage: add warn_once() helper for repeated warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-02T17:53:26Z","receivedAt":"2022-06-02T17:53:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> Yes, you're right. I should use the output 'struct url_inf' from\n> url_normalize() to construct the redacted URL. It has the downside\n> that the output URL can be slightly different from the input URL,\n> but a user should still be able to diagnose how to resolve the\n> situation.\n\nAh, the error message is based on pre-normalization which may very\nwell be different, as that is the whole point of normalization ;-)\n\nWe'd redact password part in the error message anyway, so it may not\nbe a huge deal.\n"},{"id":"456542","messageId":"xmqq35gmkbic.fsf@gitster.g","threadId":"57913","inReplyTo":"pull.1237.v4.git.1654190434908.gitgitgadget@gmail.com","subject":"Re: [PATCH v4] remote: create fetch.credentialsInUrl config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-02T21:20:59Z","receivedAt":"2022-06-02T21:21:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> +static void validate_remote_url(struct remote *remote)\n> +{\n> +\tint i;\n> +\tconst char *value;\n> +\tstruct strbuf redacted = STRBUF_INIT;\n> +\n> +\tif (git_config_get_string_tmp(\"fetch.credentialsinurl\", &value) ||\n> +\t !strcmp(\"allow\", value))\n> +\t\treturn;\n> +\n> +\tfor (i = 0; i < remote->url_nr; i++) {\n> +\t\tstruct url_info url_info = { NULL };\n\nThe initializer should be \"= { 0 }\" not \"= { NULL }\". In other\nwords, we shouldn't have to care if the first member in the struct\nhappens to be of a pointer type, and we shouldn't have to change\nbetween 0 and NULL whenever the type of the first member changes.\n\nEven though it frowns upon assigning 0 to a pointer variable or a\npointer member in a struct, sparse knows that such an initializer is\nOK.\n\ncf. https://lore.kernel.org/git/YVJSwuqjolz28+mG@coredump.intra.peff.net/\n\nPlease have a blank line after the variable decl. \n\n> +\t\turl_normalize(remote->url[i], &url_info);\n\nurl_normalize() returns \"char *\", and you get NULL when parsing\nfails; out_info->err may also help when it happens, but I think we\nwill call url_normalize() again later in the existing caller, and\nit will give whatever error message we need to give appropriately,\nso it is OK to silently jump to the loop_cleanup label from here.\n\n> +\t\tif (!url_info.passwd_len)\n> +\t\t\tgoto loop_cleanup;\n\nI wonder what should happen to \"https://username:@localhost\" (i.e.\nan empty string is used as a password). I am fine if we allow it as\nan obvious \"this is like an anonymous ftp; anybody can connect\" use\ncase, but I do not know how useful it would be in practice. \n\nI am also fine if certain authentication scheme needs username and\npassword, the latter of which is never used because the \"real thing\"\nlike Kerberos kicks in when the real authentication happens but\nbecasue something needs to be there to \"trigger\" the authentication,\nand that is why we deliberately allow an empty string case unwarned.\nBut if this is a deliberate thing, we'd need to caution future\ndevelopers about it.\n\nI do not think passwd_ofs can ever be 0 if we have an embedded\npassword in the URL, so checking it may be a better approach, if we\ncare about an empty-string case. I can be persuaded either way.\n\n> +\n> +\t\tstrbuf_add(&redacted, url_info.url, url_info.passwd_off);\n> +\t\tstrbuf_addstr(&redacted, \"\");\n> +\t\tstrbuf_addstr(&redacted, url_info.url + url_info.passwd_off + url_info.passwd_len);\n> +\n> +\t\tif (!strcmp(\"warn\", value))\n> +\t\t\twarning(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n> +\t\tif (!strcmp(\"die\", value))\n> +\t\t\tdie(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n\nWe obviously could introduce another local variable that is set\nbased on the \"value\" before we enter the loop to \"optimize\", but\nthis is an error codepath, so I do not mind repeated strcmp() on the\nconstant value in the loop.\n\nI do have to wonder what we should do when value is none of the\nthree we know about. Right now, it makes the function an expensive\nnoop, so upfront at the beginning of the function, we might need\nsomething like\n\n\tint to_warn_not_die;\n\n\tif (git_config_get_string_tmp(...\"))\n \t\treturn;\n\tif (!strcmp(\"warn\", value))\n\t\tto_warn_not_die = 1;\n\telse if (!strcmp(\"die\", value))\n\t\tto_warn_not_die = 0;\n\telse\n\t\treturn;\n\nanyway, in which case we would also do\n\n\tif (to_warn_not_die)\n\t\twarning(...);\n\telse\n\t\tdie(...);\n\nin the loop, perhaps? I dunno.\n\nI wonder if die_message() may want to report all the offending URL\nfor a given remote (with a concluding die() after the loop). I am\nOK with dying at the first offence, though. In the worst case, the\nend user experience would be:\n\n $ git fetch there\n die() about the first URL for the nickname\n $ edit .git/config\n $ git fetch there\n die() about the second URL for the nickname\n\nThe user will learn after getting the same error message twice and\nscan through the other URLs when editing .git/config for the second\ntime to fix the second URL. If I were writing this patch, I would\nprobably play lazy and die on the first offender.\n\n> +test_expect_success 'fetch warns or fails when using username:password' '\n> +\tmessage=\"URL '\\''https://username:@localhost/'\\'' uses plaintext credentials\" &&\n> +\ttest_must_fail git -c fetch.credentialsInUrl=allow fetch https://username:password@localhost 2>err &&\n> +\t! grep \"$message\" err &&\n> +\n> +\ttest_must_fail git -c fetch.credentialsInUrl=warn fetch https://username:password@localhost 2>err &&\n> +\tgrep \"warning: $message\" err >warnings &&\n> +\ttest_line_count = 3 warnings &&\n> +\n> +\ttest_must_fail git -c fetch.credentialsInUrl=die fetch https://username:password@localhost 2>err &&\n> +\tgrep \"fatal: $message\" err >warnings &&\n> +\ttest_line_count = 1 warnings\n\nReusing warnings file for die messages is probably OK ;-)\n\nAn extra test with an empty string as a password would have caught\nthe differences between using passwd_off and passwd_len to detect\nthe presence of a password here.\n\nTaking all together, I'll queue the following on top as a separate\nfix-up patch, but I may well be giving (some) bad pieces of advice,\nso I will wait for others to comment.\n\nThanks.\n\n remote.c | 26 ++++++++++++++++++--------\n t/t5516-fetch-push.sh | 4 ++++\n t/t5601-clone.sh | 4 ++++\n 3 files changed, 26 insertions(+), 8 deletions(-)\n\ndiff --git c/remote.c w/remote.c\nindex 59b6839445..2cdc064fa8 100644\n--- c/remote.c\n+++ w/remote.c\n@@ -619,25 +619,35 @@ static void validate_remote_url(struct remote *remote)\n \tint i;\n \tconst char *value;\n \tstruct strbuf redacted = STRBUF_INIT;\n+\tint warn_not_die;\n \n-\tif (git_config_get_string_tmp(\"fetch.credentialsinurl\", &value) ||\n-\t !strcmp(\"allow\", value))\n+\tif (git_config_get_string_tmp(\"fetch.credentialsinurl\", &value))\n \t\treturn;\n \n+\tif (!strcmp(\"warn\", value))\n+\t\twarn_not_die = 1;\n+\telse if (!strcmp(\"die\", value))\n+\t\twarn_not_die = 0;\n+\telse if (!strcmp(\"allow\", value))\n+\t\treturn;\n+\telse\n+\t\tdie(_(\"unrecognized value fetch.credentialsInURL: '%s'\"), value);\n+\n \tfor (i = 0; i < remote->url_nr; i++) {\n-\t\tstruct url_info url_info = { NULL };\n-\t\turl_normalize(remote->url[i], &url_info);\n+\t\tstruct url_info url_info = { 0 };\n \n-\t\tif (!url_info.passwd_len)\n+\t\tif (!url_normalize(remote->url[i], &url_info) ||\n+\t\t !url_info.passwd_off)\n \t\t\tgoto loop_cleanup;\n \n \t\tstrbuf_add(&redacted, url_info.url, url_info.passwd_off);\n \t\tstrbuf_addstr(&redacted, \"\");\n-\t\tstrbuf_addstr(&redacted, url_info.url + url_info.passwd_off + url_info.passwd_len);\n+\t\tstrbuf_addstr(&redacted,\n+\t\t\t url_info.url + url_info.passwd_off + url_info.passwd_len);\n \n-\t\tif (!strcmp(\"warn\", value))\n+\t\tif (warn_not_die)\n \t\t\twarning(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n-\t\tif (!strcmp(\"die\", value))\n+\t\telse\n \t\t\tdie(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n \n loop_cleanup:\ndiff --git c/t/t5516-fetch-push.sh w/t/t5516-fetch-push.sh\nindex afb9236bee..a67acc3263 100755\n--- c/t/t5516-fetch-push.sh\n+++ w/t/t5516-fetch-push.sh\n@@ -1821,6 +1821,10 @@ test_expect_success 'fetch warns or fails when using username:password' '\n \n \ttest_must_fail git -c fetch.credentialsInUrl=die fetch https://username:password@localhost 2>err &&\n \tgrep \"fatal: $message\" err >warnings &&\n+\ttest_line_count = 1 warnings &&\n+\n+\ttest_must_fail git -c fetch.credentialsInUrl=die fetch https://username:@localhost 2>err &&\n+\tgrep \"fatal: $message\" err >warnings &&\n \ttest_line_count = 1 warnings\n '\n \ndiff --git c/t/t5601-clone.sh w/t/t5601-clone.sh\nindex ddc4cc7ec2..cf0a3ef3f4 100755\n--- c/t/t5601-clone.sh\n+++ w/t/t5601-clone.sh\n@@ -82,6 +82,10 @@ test_expect_success 'clone warns or fails when using username:password' '\n \n \ttest_must_fail git -c fetch.credentialsInUrl=die clone https://username:password@localhost attempt3 2>err &&\n \tgrep \"fatal: $message\" err >warnings &&\n+\ttest_line_count = 1 warnings &&\n+\n+\ttest_must_fail git -c fetch.credentialsInUrl=die clone https://username:@localhost attempt3 2>err &&\n+\tgrep \"fatal: $message\" err >warnings &&\n \ttest_line_count = 1 warnings\n '\n \n"},{"id":"456572","messageId":"c414de1f-b742-0880-09e6-446f1212e3bd@github.com","threadId":"57913","inReplyTo":"xmqq35gmkbic.fsf@gitster.g","subject":"Re: [PATCH v4] remote: create fetch.credentialsInUrl config","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-06-03T12:54:47Z","receivedAt":"2022-06-03T12:55:02Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/2/2022 5:20 PM, Junio C Hamano wrote:\n...\n> Taking all together, I'll queue the following on top as a separate\n> fix-up patch, but I may well be giving (some) bad pieces of advice,\n> so I will wait for others to comment.\n\nI cut all of your commentary because it was universally good and\nthe fixup you provided does a great job of solving those issues.\n\nPlease give yourself co-authorship on the squashed commit.\n\nThanks,\n-Stolee\n"},{"id":"456713","messageId":"pull.1237.v5.git.1654526176695.gitgitgadget@gmail.com","threadId":"57913","inReplyTo":"pull.1237.v4.git.1654190434908.gitgitgadget@gmail.com","subject":"[PATCH v5] remote: create fetch.credentialsInUrl config","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-06T14:36:16Z","receivedAt":"2022-06-06T14:36:29Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nUsers sometimes provide a \"username:password\" combination in their\nplaintext URLs. Since Git stores these URLs in plaintext in the\n.git/config file, this is a very insecure way of storing these\ncredentials. Credential managers are a more secure way of storing this\ninformation.\n\nSystem administrators might want to prevent this kind of use by users on\ntheir machines.\n\nCreate a new \"fetch.credentialsInUrl\" config option and teach Git to\nwarn or die when seeing a URL with this kind of information. The warning\nanonymizes the sensitive information of the URL to be clear about the\nissue.\n\nThis change currently defaults the behavior to \"allow\" which does\nnothing with these URLs. We can consider changing this behavior to\n\"warn\" by default if we wish. At that time, we may want to add some\nadvice about setting fetch.credentialsInUrl=ignore for users who still\nwant to follow this pattern (and not receive the warning).\n\nAn earlier version of this change injected the logic into\nurl_normalize() in urlmatch.c. While most code paths that parse URLs\neventually normalize the URL, that normalization does not happen early\nenough in the stack to avoid attempting connections to the URL first. By\ninserting a check into the remote validation, we identify the issue\nbefore making a connection. In the old code path, this was revealed by\ntesting the new t5601-clone.sh test under --stress, resulting in an\ninstance where the return code was 13 (SIGPIPE) instead of 128 from the\ndie().\n\nHowever, we can reuse the parsing information from url_normalize() in\norder to benefit from its well-worn parsing logic. We can use the struct\nurl_info that is created in that method to replace the password with\n\"\" in our error messages. This comes with a slight downside\nthat the normalized URL might look slightly different from the input URL\n(for instance, the normalized version adds a closing slash). This should\nnot hinder users figuring out what the problem is and being able to fix\nthe issue.\n\nAs an attempt to ensure the parsing logic did not catch any\nunintentional cases, I modified this change locally to to use the \"die\"\noption by default. Running the test suite succeeds except for the\nexplicit username:password URLs used in t5550-http-fetch-dumb.sh and\nt5541-http-push-smart.sh. This means that all other tested URLs did not\ntrigger this logic.\n\nThe tests show that the proper error messages appear (or do not\nappear), but also count the number of error messages. When only warning,\neach process validates the remote URL and outputs a warning. This\nhappens twice for clone, three times for fetch, and once for push.\n\nCo-authored-by: Junio C Hamano \nSigned-off-by: Junio C Hamano \nSigned-off-by: Derrick Stolee \n---\n remote: create fetch.credentialsInUrl config\n \n Users can specify credentials in their URL using the\n username:password@domain format. This is potentially hazardous since the\n URL is stored in plaintext and can also appear in trace2 logs and other\n places. Add a new config option that allows warnings or failures when\n discovering URLs with this format. The default behavior does not change\n in this series, although we may want to move to the warn state by\n default in the future.\n \n This is a modified version of the patch I submitted a while ago [1].\n \n Based on the feedback, changing the behavior to fail by default was not\n a good approach. Further, the idea to stop storing the credentials in\n config and redirect them to a credential manager was already considered\n by Peff [2] but not merged.\n \n This patch does what should be the simplest thing we can do: create a\n config option that will cause the user to get a warning or a failure,\n depending on its value. The default is to ignore the setting, identical\n to the current behavior. We can talk about changing this default to\n \"warn\" in the future, but it would be safest to release with ignore as\n the default until we are sure that we are not going to start warning on\n false positives.\n \n This patch would be sufficient for the interested internal parties that\n want to prevent users from storing credentials this way. System\n administrators can modify system-level Git config into \"die\" mode to\n prevent this behavior.\n \n [1]\n https://lore.kernel.org/git/pull.945.git.1619807844627.gitgitgadget@gmail.com\n Reject passwords in URLs (April 2021).\n \n [2]\n https://lore.kernel.org/git/20190519050724.GA26179@sigill.intra.peff.net/\n Re: Git ransom campaign incident report - May 2019\n \n \n Updates in v5\n =============\n \n * Squashed in Junio's recommended changes.\n * Noticed that the redacted strbuf wasn't reset between loop\n iterations, which only matters if there are multiple URLs for the\n remote.\n \n \n Updates in v4\n =============\n \n * The warn_once() patch is dropped in favor of using a different\n location for the check (remotes_remote_get_1() instead of\n valid_remote()).\n * The parsing logic is removed in favor of using the output url_info\n from url_normalize().\n * Tests for 'fetch' and 'push' are added.\n * This requires updating the topic to be on a more-recent version of\n 'master' because some change since the previous base caused 'git\n push' to output the warning only once, not twice.\n \n \n Updates in v3\n =============\n \n * Because of some flaky behavior around SIGPIPE, the URL check needed\n to move to be earlier in the command.\n * For this reason, I moved the cred check into remote.c's\n valid_remote() check. This changed the previous BUG() statements into\n early returns.\n * I repeated the check with the test suite to see if this parsing fails\n on any existing cases, but it is worth double-checking the parsing\n rules.\n * Documentation is more consistent about using placeholders.\n * A test for the \"allow\" case is now included.\n * A new patch is added that creates the warn_once() helper. This\n reduces multiple advisory warnings with the same text from being\n written by the same process.\n \n \n Updates in v2\n =============\n \n * Documentation is slightly expanded to include the fact that Git\n stores the given URL as plaintext in its config.\n * The new method has a new documentation comment that details the\n necessary preconditions.\n * \"ignore\" is now \"allow\"\n * Additional checks on colon_ptr are added.\n * Use strbuf_splice() instead of custom string-walking logic.\n * Use \"\" instead of asterisks.\n * Config value checks are no longer case sensitive.\n \n Thanks, -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1237%2Fderrickstolee%2Fcreds-in-url-v5\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1237/derrickstolee/creds-in-url-v5\nPull-Request: https://github.com/gitgitgadget/git/pull/1237\n\nRange-diff vs v4:\n\n 1: 5fb038402d6 ! 1: b744a9887e5 remote: create fetch.credentialsInUrl config\n @@ Commit message\n each process validates the remote URL and outputs a warning. This\n happens twice for clone, three times for fetch, and once for push.\n \n + Co-authored-by: Junio C Hamano \n + Signed-off-by: Junio C Hamano \n Signed-off-by: Derrick Stolee \n \n ## Documentation/config/fetch.txt ##\n @@ remote.c: const char *remote_ref_for_branch(struct branch *branch, int for_push)\n +\tint i;\n +\tconst char *value;\n +\tstruct strbuf redacted = STRBUF_INIT;\n ++\tint warn_not_die;\n +\n -+\tif (git_config_get_string_tmp(\"fetch.credentialsinurl\", &value) ||\n -+\t !strcmp(\"allow\", value))\n ++\tif (git_config_get_string_tmp(\"fetch.credentialsinurl\", &value))\n +\t\treturn;\n +\n ++\tif (!strcmp(\"warn\", value))\n ++\t\twarn_not_die = 1;\n ++\telse if (!strcmp(\"die\", value))\n ++\t\twarn_not_die = 0;\n ++\telse if (!strcmp(\"allow\", value))\n ++\t\treturn;\n ++\telse\n ++\t\tdie(_(\"unrecognized value fetch.credentialsInURL: '%s'\"), value);\n ++\n +\tfor (i = 0; i < remote->url_nr; i++) {\n -+\t\tstruct url_info url_info = { NULL };\n -+\t\turl_normalize(remote->url[i], &url_info);\n ++\t\tstruct url_info url_info = { 0 };\n +\n -+\t\tif (!url_info.passwd_len)\n ++\t\tif (!url_normalize(remote->url[i], &url_info) ||\n ++\t\t !url_info.passwd_off)\n +\t\t\tgoto loop_cleanup;\n +\n ++\t\tstrbuf_reset(&redacted);\n +\t\tstrbuf_add(&redacted, url_info.url, url_info.passwd_off);\n +\t\tstrbuf_addstr(&redacted, \"\");\n -+\t\tstrbuf_addstr(&redacted, url_info.url + url_info.passwd_off + url_info.passwd_len);\n ++\t\tstrbuf_addstr(&redacted,\n ++\t\t\t url_info.url + url_info.passwd_off + url_info.passwd_len);\n +\n -+\t\tif (!strcmp(\"warn\", value))\n ++\t\tif (warn_not_die)\n +\t\t\twarning(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n -+\t\tif (!strcmp(\"die\", value))\n ++\t\telse\n +\t\t\tdie(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n +\n +loop_cleanup:\n @@ t/t5516-fetch-push.sh: test_expect_success 'refuse to push a hidden ref, and mak\n +\n +\ttest_must_fail git -c fetch.credentialsInUrl=die fetch https://username:password@localhost 2>err &&\n +\tgrep \"fatal: $message\" err >warnings &&\n ++\ttest_line_count = 1 warnings &&\n ++\n ++\ttest_must_fail git -c fetch.credentialsInUrl=die fetch https://username:@localhost 2>err &&\n ++\tgrep \"fatal: $message\" err >warnings &&\n +\ttest_line_count = 1 warnings\n +'\n +\n @@ t/t5601-clone.sh: test_expect_success 'clone respects GIT_WORK_TREE' '\n +\n +\ttest_must_fail git -c fetch.credentialsInUrl=die clone https://username:password@localhost attempt3 2>err &&\n +\tgrep \"fatal: $message\" err >warnings &&\n ++\ttest_line_count = 1 warnings &&\n ++\n ++\ttest_must_fail git -c fetch.credentialsInUrl=die clone https://username:@localhost attempt3 2>err &&\n ++\tgrep \"fatal: $message\" err >warnings &&\n +\ttest_line_count = 1 warnings\n +'\n +\n\n\n Documentation/config/fetch.txt | 14 ++++++++++\n remote.c | 48 ++++++++++++++++++++++++++++++++++\n t/t5516-fetch-push.sh | 32 +++++++++++++++++++++++\n t/t5601-clone.sh | 23 ++++++++++++++++\n 4 files changed, 117 insertions(+)\n\ndiff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt\nindex cd65d236b43..0db7fe85bb8 100644\n--- a/Documentation/config/fetch.txt\n+++ b/Documentation/config/fetch.txt\n@@ -96,3 +96,17 @@ fetch.writeCommitGraph::\n \tmerge and the write may take longer. Having an updated commit-graph\n \tfile helps performance of many Git commands, including `git merge-base`,\n \t`git push -f`, and `git log --graph`. Defaults to false.\n+\n+fetch.credentialsInUrl::\n+\tA URL can contain plaintext credentials in the form\n+\t`://:@/`. Using such URLs\n+\tis not recommended as it exposes the password in multiple ways,\n+\tincluding Git storing the URL as plaintext in the repository config.\n+\tThe `fetch.credentialsInUrl` option provides instruction for how Git\n+\tshould react to seeing such a URL, with these values:\n++\n+* `allow` (default): Git will proceed with its activity without warning.\n+* `warn`: Git will write a warning message to `stderr` when parsing a URL\n+ with a plaintext credential.\n+* `die`: Git will write a failure message to `stderr` when parsing a URL\n+ with a plaintext credential.\ndiff --git a/remote.c b/remote.c\nindex 930fdc9c2f6..2b6a8b3df7d 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1,6 +1,7 @@\n #include \"cache.h\"\n #include \"config.h\"\n #include \"remote.h\"\n+#include \"urlmatch.h\"\n #include \"refs.h\"\n #include \"refspec.h\"\n #include \"object-store.h\"\n@@ -614,6 +615,50 @@ const char *remote_ref_for_branch(struct branch *branch, int for_push)\n \treturn NULL;\n }\n \n+static void validate_remote_url(struct remote *remote)\n+{\n+\tint i;\n+\tconst char *value;\n+\tstruct strbuf redacted = STRBUF_INIT;\n+\tint warn_not_die;\n+\n+\tif (git_config_get_string_tmp(\"fetch.credentialsinurl\", &value))\n+\t\treturn;\n+\n+\tif (!strcmp(\"warn\", value))\n+\t\twarn_not_die = 1;\n+\telse if (!strcmp(\"die\", value))\n+\t\twarn_not_die = 0;\n+\telse if (!strcmp(\"allow\", value))\n+\t\treturn;\n+\telse\n+\t\tdie(_(\"unrecognized value fetch.credentialsInURL: '%s'\"), value);\n+\n+\tfor (i = 0; i < remote->url_nr; i++) {\n+\t\tstruct url_info url_info = { 0 };\n+\n+\t\tif (!url_normalize(remote->url[i], &url_info) ||\n+\t\t !url_info.passwd_off)\n+\t\t\tgoto loop_cleanup;\n+\n+\t\tstrbuf_reset(&redacted);\n+\t\tstrbuf_add(&redacted, url_info.url, url_info.passwd_off);\n+\t\tstrbuf_addstr(&redacted, \"\");\n+\t\tstrbuf_addstr(&redacted,\n+\t\t\t url_info.url + url_info.passwd_off + url_info.passwd_len);\n+\n+\t\tif (warn_not_die)\n+\t\t\twarning(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n+\t\telse\n+\t\t\tdie(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n+\n+loop_cleanup:\n+\t\tfree(url_info.url);\n+\t}\n+\n+\tstrbuf_release(&redacted);\n+}\n+\n static struct remote *\n remotes_remote_get_1(struct remote_state *remote_state, const char *name,\n \t\t const char *(*get_default)(struct remote_state *,\n@@ -639,6 +684,9 @@ remotes_remote_get_1(struct remote_state *remote_state, const char *name,\n \t\tadd_url_alias(remote_state, ret, name);\n \tif (!valid_remote(ret))\n \t\treturn NULL;\n+\n+\tvalidate_remote_url(ret);\n+\n \treturn ret;\n }\n \ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 4dfb080433e..dc6cb3cbf5d 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -12,6 +12,7 @@ This test checks the following functionality:\n * --porcelain output format\n * hiderefs\n * reflogs\n+* URL validation\n '\n \n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n@@ -1813,4 +1814,35 @@ test_expect_success 'refuse to push a hidden ref, and make sure do not pollute t\n \ttest_dir_is_empty testrepo/.git/objects/pack\n '\n \n+test_expect_success 'fetch warns or fails when using username:password' '\n+\tmessage=\"URL '\\''https://username:@localhost/'\\'' uses plaintext credentials\" &&\n+\ttest_must_fail git -c fetch.credentialsInUrl=allow fetch https://username:password@localhost 2>err &&\n+\t! grep \"$message\" err &&\n+\n+\ttest_must_fail git -c fetch.credentialsInUrl=warn fetch https://username:password@localhost 2>err &&\n+\tgrep \"warning: $message\" err >warnings &&\n+\ttest_line_count = 3 warnings &&\n+\n+\ttest_must_fail git -c fetch.credentialsInUrl=die fetch https://username:password@localhost 2>err &&\n+\tgrep \"fatal: $message\" err >warnings &&\n+\ttest_line_count = 1 warnings &&\n+\n+\ttest_must_fail git -c fetch.credentialsInUrl=die fetch https://username:@localhost 2>err &&\n+\tgrep \"fatal: $message\" err >warnings &&\n+\ttest_line_count = 1 warnings\n+'\n+\n+\n+test_expect_success 'push warns or fails when using username:password' '\n+\tmessage=\"URL '\\''https://username:@localhost/'\\'' uses plaintext credentials\" &&\n+\ttest_must_fail git -c fetch.credentialsInUrl=allow push https://username:password@localhost 2>err &&\n+\t! grep \"$message\" err &&\n+\n+\ttest_must_fail git -c fetch.credentialsInUrl=warn push https://username:password@localhost 2>err &&\n+\tgrep \"warning: $message\" err >warnings &&\n+\ttest_must_fail git -c fetch.credentialsInUrl=die push https://username:password@localhost 2>err &&\n+\tgrep \"fatal: $message\" err >warnings &&\n+\ttest_line_count = 1 warnings\n+'\n+\n test_done\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 4a61f2c901e..d2f046b4b92 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -71,6 +71,29 @@ test_expect_success 'clone respects GIT_WORK_TREE' '\n \n '\n \n+test_expect_success 'clone warns or fails when using username:password' '\n+\tmessage=\"URL '\\''https://username:@localhost/'\\'' uses plaintext credentials\" &&\n+\ttest_must_fail git -c fetch.credentialsInUrl=allow clone https://username:password@localhost attempt1 2>err &&\n+\t! grep \"$message\" err &&\n+\n+\ttest_must_fail git -c fetch.credentialsInUrl=warn clone https://username:password@localhost attempt2 2>err &&\n+\tgrep \"warning: $message\" err >warnings &&\n+\ttest_line_count = 2 warnings &&\n+\n+\ttest_must_fail git -c fetch.credentialsInUrl=die clone https://username:password@localhost attempt3 2>err &&\n+\tgrep \"fatal: $message\" err >warnings &&\n+\ttest_line_count = 1 warnings &&\n+\n+\ttest_must_fail git -c fetch.credentialsInUrl=die clone https://username:@localhost attempt3 2>err &&\n+\tgrep \"fatal: $message\" err >warnings &&\n+\ttest_line_count = 1 warnings\n+'\n+\n+test_expect_success 'clone does not detect username:password when it is https://username@domain:port/' '\n+\ttest_must_fail git -c fetch.credentialsInUrl=warn clone https://username@localhost:8080 attempt3 2>err &&\n+\t! grep \"uses plaintext credentials\" err\n+'\n+\n test_expect_success 'clone from hooks' '\n \n \ttest_create_repo r0 &&\n\nbase-commit: 2668e3608e47494f2f10ef2b6e69f08a84816bcb\n-- \ngitgitgadget\n"},{"id":"456717","messageId":"xmqqa6apdcrb.fsf@gitster.g","threadId":"57913","inReplyTo":"c414de1f-b742-0880-09e6-446f1212e3bd@github.com","subject":"Re: [PATCH v4] remote: create fetch.credentialsInUrl config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-06T15:37:12Z","receivedAt":"2022-06-06T15:37:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> On 6/2/2022 5:20 PM, Junio C Hamano wrote:\n> ...\n>> Taking all together, I'll queue the following on top as a separate\n>> fix-up patch, but I may well be giving (some) bad pieces of advice,\n>> so I will wait for others to comment.\n>\n> I cut all of your commentary because it was universally good and\n> the fixup you provided does a great job of solving those issues.\n>\n> Please give yourself co-authorship on the squashed commit.\n\nHeh, that's at most helped-by; a small bugfix in the logic (zero-length)\nand all others are minor clean-ups.\n\nThanks.\n"},{"id":"456722","messageId":"xmqqv8tdagyv.fsf@gitster.g","threadId":"57913","inReplyTo":"pull.1237.v5.git.1654526176695.gitgitgadget@gmail.com","subject":"Re: [PATCH v5] remote: create fetch.credentialsInUrl config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-06T16:34:32Z","receivedAt":"2022-06-06T16:34:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> * Noticed that the redacted strbuf wasn't reset between loop\n> iterations, which only matters if there are multiple URLs for the\n> remote.\n\nThanks, I missed that.\n\nWill queue.\n"}]}