{"thread":{"id":"55600","subject":"[PATCH] urlmatch: do not allow passwords in URLs by default","startedAt":"2021-04-30T18:37:30Z","lastAt":"2021-05-03T14:53:08Z","messageCount":10,"participants":["Derrick Stolee via GitGitGadget","Jeff King","brian m. carlson","Christian Couder","Ævar Arnfjörð Bjarmason","Junio C Hamano","Robert Coup","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"423365","messageId":"pull.945.git.1619807844627.gitgitgadget@gmail.com","threadId":"55600","inReplyTo":null,"subject":"[PATCH] urlmatch: do not allow passwords in URLs by default","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-30T18:37:24Z","receivedAt":"2021-04-30T18:37:30Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nGit allows URLs of the following pattern:\n\n  https://username:password@domain/route\n\nThese URLs are then parsed to pull out the username and password for use\nwhen authenticating with the URL. Git is careful to anonymize the URL in\nstatus messages with transport_anonymize_url(), but it stores the URL as\nplaintext in the .git/config file. The password may leak in other ways.\n\nThis is not a recommended way to store credentials, especially\nauthentication tokens that do more than simply allow access to a\nrepository.\n\nUsers should be aware that when they provide a URL in this form that\nthey are not being incredibly secure. It is much better to use a\ncredential manager to securely store passwords. Even better, some\ncredential managers use more sophisticated authentication strategies\nincluding multi-factor authentication. This does not stop users from\ncontinuing to do this.\n\nSome Git hosting providers are working to completely drop\nusername/password credential strategies, which will make URLs of this\nform stop working. However, that requires certain changes to credential\nmanagers that need to be released and sufficiently adopted before making\nsuch a server-side change.\n\nIn the meantime, it might be helpful to alert users that they are doing\nsomething insecure with these URLs.\n\nCreate a new config option, core.allowUsernamePasswordUrls, which is\ndisabled by default. If Git attempts to parse a password from a URL in\nthis form, it will die() if this config is not enabled. This affects a\nfew test scripts, but enabling the config in those places is relatively\nsimple.\n\nThis will cause a significant change in behavior for users who rely upon\nthis username:password pattern. The error message describes the config\nthat they must enable to continue working with these URLs. This has a\nsignificant chance of breaking some automated workflows that use URLs in\nthis fashion, but even those workflows would be better off using a\ndifferent mechanism for credentials.\n\nI cannot understate the care in which we should consider this change.\nThe impact of this change in a Git release could be significant. We\nshould advertise this very clearly in the release notes.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n    Reject passwords in URLs\n    \n    I received multiple messages \"alerting\" me to the issue of users\n    supplying server-side tokens into the username:password field of a URL.\n    This is not a secure way to handle these tokens.\n    \n    On the one hand, this is user error: Users should not supply a token to\n    a location where they do not know what will happen to it. In Git's\n    defense, its behavior is completely open about storing the URL in the\n    .git/config file as a plain-text string and users should know that when\n    using this feature.\n    \n    However, users just. keep. doing it.\n    \n    There is some expectation that since this portion of the URL is a\n    password, then Git is responsible for tracking that password securely.\n    I'm not sure we should venture down that road, since we already have a\n    pretty good solution by using the credential helper interface.\n    \n    Here is my best effort to find a compromise here: start failing when\n    parsing a password from a URL like this, with a config option to\n    re-enable the existing behavior.\n    \n    I completely understand if this is too much of a breaking change. I\n    wonder if there is anything we can do to assist users into being more\n    careful with their secrets.\n    \n    Thanks, -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-945%2Fderrickstolee%2Freject-passwords-in-urls-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-945/derrickstolee/reject-passwords-in-urls-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/945\n\n Documentation/config/core.txt     |  7 +++++++\n t/t0110-urlmatch-normalization.sh |  4 ++++\n t/t5541-http-push-smart.sh        |  9 +++++++--\n t/t5550-http-fetch-dumb.sh        |  3 ++-\n t/t5601-clone.sh                  |  5 +++++\n urlmatch.c                        | 14 ++++++++++++++\n 6 files changed, 39 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config/core.txt b/Documentation/config/core.txt\nindex c04f62a54a15..807dc30e7321 100644\n--- a/Documentation/config/core.txt\n+++ b/Documentation/config/core.txt\n@@ -628,3 +628,10 @@ core.abbrev::\n \tIf set to \"no\", no abbreviation is made and the object names\n \tare shown in their full length.\n \tThe minimum length is 4.\n+\n+core.allowUsernamePasswordUrls::\n+\tIf enabled, allow parsing URLs that contain plain-text usernames\n+\tand passwords using `username:password@<url>` text. Defaults to\n+\t`false`, and will cause Git to fail when parsing such a URL.\n+\t*WARNING:* Storing passwords and tokens in plaintext is insecure\n+\tand should be avoided if at all possible.\ndiff --git a/t/t0110-urlmatch-normalization.sh b/t/t0110-urlmatch-normalization.sh\nindex f99529d83853..66352775497a 100755\n--- a/t/t0110-urlmatch-normalization.sh\n+++ b/t/t0110-urlmatch-normalization.sh\n@@ -6,6 +6,10 @@ test_description='urlmatch URL normalization'\n # The base name of the test url files\n tu=\"$TEST_DIRECTORY/t0110/url\"\n \n+test_expect_success 'enable username:password urls' '\n+\tgit config --global core.allowUsernamePasswordUrls true\n+'\n+\n # Note that only file: URLs should be allowed without a host\n \n test_expect_success 'url scheme' '\ndiff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\nindex c024fa281831..3ffc367bae43 100755\n--- a/t/t5541-http-push-smart.sh\n+++ b/t/t5541-http-push-smart.sh\n@@ -460,6 +460,7 @@ test_expect_success GPG 'push with post-receive to inspect certificate' '\n \n test_expect_success 'push status output scrubs password' '\n \tcd \"$ROOT_PATH/test_repo_clone\" &&\n+\tgit config core.allowUsernamePasswordUrls true &&\n \tgit push --porcelain \\\n \t\t\"$HTTPD_URL_USER_PASS/smart/test_repo.git\" \\\n \t\t+HEAD:scrub >status &&\n@@ -469,9 +470,11 @@ test_expect_success 'push status output scrubs password' '\n \n test_expect_success 'clone/fetch scrubs password from reflogs' '\n \tcd \"$ROOT_PATH\" &&\n-\tgit clone \"$HTTPD_URL_USER_PASS/smart/test_repo.git\" \\\n+\tgit -c core.allowUsernamePasswordUrls=true clone \\\n+\t\t\"$HTTPD_URL_USER_PASS/smart/test_repo.git\" \\\n \t\treflog-test &&\n \tcd reflog-test &&\n+\tgit config core.allowUsernamePasswordUrls true &&\n \ttest_commit prepare-for-force-fetch &&\n \tgit switch -c away &&\n \tgit fetch \"$HTTPD_URL_USER_PASS/smart/test_repo.git\" \\\n@@ -484,8 +487,10 @@ test_expect_success 'clone/fetch scrubs password from reflogs' '\n \n test_expect_success 'Non-ASCII branch name can be used with --force-with-lease' '\n \tcd \"$ROOT_PATH\" &&\n-\tgit clone \"$HTTPD_URL_USER_PASS/smart/test_repo.git\" non-ascii &&\n+\tgit -c core.allowUsernamePasswordUrls=true clone \\\n+\t\t\"$HTTPD_URL_USER_PASS/smart/test_repo.git\" non-ascii &&\n \tcd non-ascii &&\n+\tgit config core.allowUsernamePasswordUrls true &&\n \tgit checkout -b rama-de-árbol &&\n \ttest_commit F &&\n \tgit push --force-with-lease origin rama-de-árbol &&\ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex 6d9142afc3b2..342538e85e60 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -81,7 +81,8 @@ test_expect_success 'cloning password-protected repository can fail' '\n \n test_expect_success 'http auth can use user/pass in URL' '\n \tset_askpass wrong &&\n-\tgit clone \"$HTTPD_URL_USER_PASS/auth/dumb/repo.git\" clone-auth-none &&\n+\tgit -c core.allowUsernamePasswordUrls=true clone \\\n+\t\t\"$HTTPD_URL_USER_PASS/auth/dumb/repo.git\" clone-auth-none &&\n \texpect_askpass none\n '\n \ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 329ae599fd3c..fd7cabbafa53 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -71,6 +71,11 @@ test_expect_success 'clone respects GIT_WORK_TREE' '\n \n '\n \n+test_expect_success 'clone fails when using username:password' '\n+\ttest_must_fail git clone https://username:password@bogus.url 2>err &&\n+\ttest_i18ngrep \"attempted to parse a URL with a plain-text username and password\" 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 33a2ccd306b6..e81ec9e1fc0b 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,18 @@ static int match_host(const struct url_info *url_info,\n \treturn (!url_len && !pat_len);\n }\n \n+static void die_if_username_password_not_allowed(void)\n+{\n+\tint opt_in = 0;\n+\tif (!git_config_get_bool(\"core.allowusernamepasswordurls\", &opt_in) &&\n+\t    opt_in)\n+\t\treturn;\n+\n+\tdie(_(\"attempted to parse a URL with a plain-text username and password! \"\n+\t      \"This is insecure! \"\n+\t      \"Enable core.allowUsernamePasswordUrls to avoid this error\"));\n+}\n+\n static char *url_normalize_1(const char *url, struct url_info *out_info, char allow_globs)\n {\n \t/*\n@@ -191,6 +204,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\tdie_if_username_password_not_allowed();\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: 7e391989789db82983665667013a46eabc6fc570\n-- \ngitgitgadget\n"},{"id":"423366","messageId":"YIxRbOh4j9eFxBF3@coredump.intra.peff.net","threadId":"55600","inReplyTo":"pull.945.git.1619807844627.gitgitgadget@gmail.com","subject":"Re: [PATCH] urlmatch: do not allow passwords in URLs by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-30T18:50:20Z","receivedAt":"2021-04-30T18:50:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 30, 2021 at 06:37:24PM +0000, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee <dstolee@microsoft.com>\n> \n> Git allows URLs of the following pattern:\n> \n>   https://username:password@domain/route\n> \n> These URLs are then parsed to pull out the username and password for use\n> when authenticating with the URL. Git is careful to anonymize the URL in\n> status messages with transport_anonymize_url(), but it stores the URL as\n> plaintext in the .git/config file. The password may leak in other ways.\n\nI'm not really opposed to disallowing this entirely (with an escape\nhatch, as you have here), because it really is an awful practice for a\nlot of reasons. But another option we discussed previously was to allow\nthe initial clone, but not store the password, which would result in the\nuser being prompted for subsequent fetches:\n\n  https://lore.kernel.org/git/20190519050724.GA26179@sigill.intra.peff.net/\n\nI think that third patch there is just too gross. But with the first\ntwo, if you do have a credential helper configured, then:\n\n  git clone https://user:pass@example.com/repo.git\n\nwould do what you want: clone with that user/pass, and then store the\nresult in the credential helper.\n\n> @@ -191,6 +204,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\tdie_if_username_password_not_allowed();\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\nIt's probably a bit nicer to just ignore the password, which will prompt\nthe user. But then, it is nicer still to use it just the one time but\nnot store it in the .git/config file. :)\n\n-Peff\n"},{"id":"423394","messageId":"YIy2UZWeNiWfa1jp@camp.crustytoothpaste.net","threadId":"55600","inReplyTo":"pull.945.git.1619807844627.gitgitgadget@gmail.com","subject":"Re: [PATCH] urlmatch: do not allow passwords in URLs by default","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2021-05-01T02:00:49Z","receivedAt":"2021-05-01T02:01:28Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2021-04-30 at 18:37:24, Derrick Stolee via GitGitGadget wrote:\n> Create a new config option, core.allowUsernamePasswordUrls, which is\n> disabled by default. If Git attempts to parse a password from a URL in\n> this form, it will die() if this config is not enabled. This affects a\n> few test scripts, but enabling the config in those places is relatively\n> simple.\n\nLet's call this http.allowUsernamePasswordURLs (or\nhttp.allowCredentialsInURL) because this is ultimately about HTTP and\nHTTPS (and maybe FTP if we still support that, which I certainly hope we\ndo not).  SSH doesn't have URLs and won't read a password from either\nthe URL or a credential helper, since OpenSSH won't read a password from\nanything but a terminal (which is secure, but occasionally irritating).\n\n> This will cause a significant change in behavior for users who rely upon\n> this username:password pattern. The error message describes the config\n> that they must enable to continue working with these URLs. This has a\n> significant chance of breaking some automated workflows that use URLs in\n> this fashion, but even those workflows would be better off using a\n> different mechanism for credentials.\n\nI will admit to using this pattern in a test I was writing just this\nweek.  I ultimately switched to an environment-based credential helper\n(à la FAQ) in my test, but I think automated tests and other situations\nwhere the credentials don't matter are really the only cases where this\nis okay.  I do think we will break some systems as a result, especially\nin situations where users cannot otherwise specify credentials (e.g., a\nSaaS offering which clones your repository to provide some\nfunctionality).\n\nSo I am a bit torn about this.  On one hand, we should really encourage\nmuch more secure options whenever possible and I'm glad this does this,\nbut on the other hand, there are some useful cases where this is\nunobjectionable or at least the least terrible option and the config\noption may be a problem.  Don't let my doubts hold up this series if\neveryone else is for it, though.  It's definitely an improvement in\nsecurity.\n\nIt is my intention (in my copious free time) to adjust credential\nhelpers to support arbitrary auth schemes (e.g., Bearer).  At that\npoint, I plan to deprecate support for Authorization extra headers.  In\nthat case, because the user is guaranteed to have the opportunity to\nedit the config, they are guaranteed to have credential helper support\nand therefore there are no use cases we're excluding.\n-- \nbrian m. carlson (he/him or they/them)\nHouston, Texas, US\n"},{"id":"423395","messageId":"CAP8UFD1Wm2e7Q3XY346-fFWMhdGHV_1Kp=wo8cqsx71j7Sg-dQ@mail.gmail.com","threadId":"55600","inReplyTo":"pull.945.git.1619807844627.gitgitgadget@gmail.com","subject":"Re: [PATCH] urlmatch: do not allow passwords in URLs by default","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-05-01T06:39:53Z","receivedAt":"2021-05-01T06:40:11Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Apr 30, 2021 at 8:37 PM Derrick Stolee via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: Derrick Stolee <dstolee@microsoft.com>\n>\n> Git allows URLs of the following pattern:\n>\n>   https://username:password@domain/route\n\n[...]\n\n> Some Git hosting providers are working to completely drop\n> username/password credential strategies, which will make URLs of this\n> form stop working. However, that requires certain changes to credential\n> managers that need to be released and sufficiently adopted before making\n> such a server-side change.\n>\n> In the meantime, it might be helpful to alert users that they are doing\n> something insecure with these URLs.\n\nAnother helpful thing to do might be to add --user and maybe\n--password options to some commands like 'clone', 'fetch', 'remote\nadd', etc.\n\nI think historically we considered that authentication wasn't Git's\nresponsibility. If we now think it should be concerned about this,\nthen --user and --password options might be a good way to start taking\nresponsibility.\n\nFor example `git clone --user XXX --password YYY\nhttps://git.example.com/git/git.git` could use an HTTP header to send\nthe credentials, and then after the clone maybe (if a terminal is\nused) ask if the user would like to save the credentials using a\ncredential manager.\n\nI think this could be both as easy, or even easier, to use than an URL\nwith credentials and more secure. We could also make things more\nsecure over time by suggesting better credential managers as they\nimprove.\n\nAlso I wonder if on Linux a credential manager could encrypt HTTP\ncredentials and store them locally using the user's private ssh key if\nthere is one.\n\nThanks,\nChristian.\n"},{"id":"423396","messageId":"87fsz6ygyc.fsf@evledraar.gmail.com","threadId":"55600","inReplyTo":"pull.945.git.1619807844627.gitgitgadget@gmail.com","subject":"Re: [PATCH] urlmatch: do not allow passwords in URLs by default","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-01T08:44:12Z","receivedAt":"2021-05-01T08:52:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Apr 30 2021, Derrick Stolee via GitGitGadget wrote:\n\nJust nits on the patch, will reply to the idea in another message:\n\n> [...]\n> +test_expect_success 'enable username:password urls' '\n> +\tgit config --global core.allowUsernamePasswordUrls true\n> +'\n\nHrm, --global? In any case isn't it also better here to tweak this for\nspecific tests?\n\n>  test_expect_success 'push status output scrubs password' '\n>  \tcd \"$ROOT_PATH/test_repo_clone\" &&\n> +\tgit config core.allowUsernamePasswordUrls true &&\n>  \tgit push --porcelain \\\n>  \t\t\"$HTTPD_URL_USER_PASS/smart/test_repo.git\" \\\n>  \t\t+HEAD:scrub >status &&\n> @@ -469,9 +470,11 @@ test_expect_success 'push status output scrubs password' '\n\nUse test_config instead, unless this is really \"setup for the rest of\nthe tests\" in disguise, but IMO even more of a reason to use test_config\nfor each one.\n\n>  test_expect_success 'clone/fetch scrubs password from reflogs' '\n>  \tcd \"$ROOT_PATH\" &&\n> -\tgit clone \"$HTTPD_URL_USER_PASS/smart/test_repo.git\" \\\n> +\tgit -c core.allowUsernamePasswordUrls=true clone \\\n> +\t\t\"$HTTPD_URL_USER_PASS/smart/test_repo.git\" \\\n>  \t\treflog-test &&\n>  \tcd reflog-test &&\n> +\tgit config core.allowUsernamePasswordUrls true &&\n\nDitto. Although redundant in your patch, no, since we've set it to true\nabove?\n\n> +test_expect_success 'clone fails when using username:password' '\n> +\ttest_must_fail git clone https://username:password@bogus.url 2>err &&\n> +\ttest_i18ngrep \"attempted to parse a URL with a plain-text username and password\" err\n> +'\n> +\n\nJust use grep, not test_i18ngrep. GETTEXT_POISON is gone.\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 33a2ccd306b6..e81ec9e1fc0b 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,18 @@ static int match_host(const struct url_info *url_info,\n>  \treturn (!url_len && !pat_len);\n>  }\n>  \n> +static void die_if_username_password_not_allowed(void)\n> +{\n> +\tint opt_in = 0;\n> +\tif (!git_config_get_bool(\"core.allowusernamepasswordurls\", &opt_in) &&\n> +\t    opt_in)\n> +\t\treturn;\n\nAPI use nit: You need to either initialize \"opt_in = 0\" or check the\ngit_config_get_bool() return value. Doing both isn't strictly\nneeded. I.e. either of these would work:\n\n    int opt_in = 0;\n    git_config_get_bool(..., &opt_in);\n    if (opt_in) ...;\n\nOr:\n\n    int opt_in;\n    if (!git_config_get_bool(..., &opt_in) && opt_in)\n\nNo?\n"},{"id":"423397","messageId":"87czuayfy9.fsf@evledraar.gmail.com","threadId":"55600","inReplyTo":"pull.945.git.1619807844627.gitgitgadget@gmail.com","subject":"Re: [PATCH] urlmatch: do not allow passwords in URLs by default","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-01T08:52:09Z","receivedAt":"2021-05-01T09:13:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Apr 30 2021, Derrick Stolee via GitGitGadget wrote:\n\nNow for a comment on the direction...:\n\n> From: Derrick Stolee <dstolee@microsoft.com>\n>\n> Git allows URLs of the following pattern:\n>\n>   https://username:password@domain/route\n>\n> These URLs are then parsed to pull out the username and password for use\n> when authenticating with the URL. Git is careful to anonymize the URL in\n> status messages with transport_anonymize_url(), but it stores the URL as\n> plaintext in the .git/config file. The password may leak in other ways.\n>\n> This is not a recommended way to store credentials, especially\n> authentication tokens that do more than simply allow access to a\n> repository.\n>\n> Users should be aware that when they provide a URL in this form that\n> they are not being incredibly secure. It is much better to use a\n\n\"Incredibly secure\"? Security so good you wouldn't believe it if you saw\nit ? :)\n\n> credential manager to securely store passwords. Even better, some\n> credential managers use more sophisticated authentication strategies\n> including multi-factor authentication. This does not stop users from\n> continuing to do this.\n>\n> Some Git hosting providers are working to completely drop\n> username/password credential strategies, which will make URLs of this\n> form stop working. However, that requires certain changes to credential\n> managers that need to be released and sufficiently adopted before making\n> such a server-side change.\n>\n> In the meantime, it might be helpful to alert users that they are doing\n> something insecure with these URLs.\n>\n> Create a new config option, core.allowUsernamePasswordUrls, which is\n> disabled by default. If Git attempts to parse a password from a URL in\n> this form, it will die() if this config is not enabled. This affects a\n> few test scripts, but enabling the config in those places is relatively\n> simple.\n>\n> This will cause a significant change in behavior for users who rely upon\n> this username:password pattern. The error message describes the config\n> that they must enable to continue working with these URLs. This has a\n> significant chance of breaking some automated workflows that use URLs in\n> this fashion, but even those workflows would be better off using a\n> different mechanism for credentials.\n\n...\n\n> I cannot understate the care in which we should consider this change.\n> The impact of this change in a Git release could be significant. We\n> should advertise this very clearly in the release notes.\n\nSeems like something more for below-the-\"---\" than a commit\nmessage. If/when it lands the \"consider this change\" has already\nhappened.\n\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n>     Reject passwords in URLs\n>     \n>     I received multiple messages \"alerting\" me to the issue of users\n>     supplying server-side tokens into the username:password field of a URL.\n>     This is not a secure way to handle these tokens.\n>     \n>     On the one hand, this is user error: Users should not supply a token to\n>     a location where they do not know what will happen to it. In Git's\n>     defense, its behavior is completely open about storing the URL in the\n>     .git/config file as a plain-text string and users should know that when\n>     using this feature.\n>     \n>     However, users just. keep. doing it.\n>     \n>     There is some expectation that since this portion of the URL is a\n>     password, then Git is responsible for tracking that password securely.\n>     I'm not sure we should venture down that road, since we already have a\n>     pretty good solution by using the credential helper interface.\n>     \n>     Here is my best effort to find a compromise here: start failing when\n>     parsing a password from a URL like this, with a config option to\n>     re-enable the existing behavior.\n>     \n>     I completely understand if this is too much of a breaking change. I\n>     wonder if there is anything we can do to assist users into being more\n>     careful with their secrets.\n\nI think a good staring compromise would not be to make long-standing\nsupported behavior an error, but to use the advise() system to emit some\nnotice about this.\n\nWhile I get what problem you're trying to solve in practice, I disagree\nwith the strong assertion that a user is \"doing something insecure\" by\ndefinition by using such URLs.\n\nIf you trust your FS permissions, and as a local user, ultimately if you\ndidn't a credential manager wouldn't be much use anyway, and/or\nuse/don't care (e.g. closed network) about transport security then\nhaving a password in your config is just fine, and doesn't otherwise\ncompromise security.\n\nUnlike SSH this is a thing that the HTTP protocol supports, so I don't\nthink it's the place of git to go out of its way by default to break\nworking URL schemas by making hard breaking assumptions about the user's\nworkflow.\n\nI think a much better way to address this issue, if it's an issue that\nneeds to be addressed, is on the server-side. The service operator is in\na much better position to know if URL passwords are a bad idea in their\ncase (e.g. is the software frequently used in certain ways, is there no\ntransport security, are we using debug URL tracing etc).\n\nYou allude to some of that in your commit message. \"Some Git hosting\nproviders\". Some? GitHub? In any case, to me that's further reason we\nshould hold off on such a change in git.git. Let the big hosting\nproviders experiment with this, and if e.g. they all decide to stop\nsupporting this and it therefore becomes less common practice we'll be\nbetter informed about if/what to change in git.git.\n"},{"id":"423458","messageId":"xmqqk0oga3ma.fsf@gitster.g","threadId":"55600","inReplyTo":"CAP8UFD1Wm2e7Q3XY346-fFWMhdGHV_1Kp=wo8cqsx71j7Sg-dQ@mail.gmail.com","subject":"Re: [PATCH] urlmatch: do not allow passwords in URLs by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-03T03:38:21Z","receivedAt":"2021-05-03T03:38:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> Another helpful thing to do might be to add --user and maybe\n> --password options to some commands like 'clone', 'fetch', 'remote\n> add', etc.\n\nWhy?\n\nWe cannot get rid of <scheme>://<user>:<pass>@<host>/<path> right\naway, but I'd imagine that we'd prefer to see fewer places on the\ncommand line for users to leave the password that would end up in\ntheir .bashrc and other places.\n\nAnd I like the idea raised elsewhere in the thread to forward the\n<pass> to credential helper and leave \":<pass>\" part out of the\nstored URL.\n\nThanks.\n"},{"id":"423471","messageId":"CAFLLRp+LA8WNsmOYPEBbNuSR4o8TBOqZpX-_P8fh6h46tNWmCw@mail.gmail.com","threadId":"55600","inReplyTo":"87czuayfy9.fsf@evledraar.gmail.com","subject":"Re: [PATCH] urlmatch: do not allow passwords in URLs by default","fromName":"Robert Coup","fromEmail":"robert.coup@koordinates.com","sentAt":"2021-05-03T08:40:01Z","receivedAt":"2021-05-03T08:40:22Z","isPatch":true,"sender":{"key":"robert.coup@koordinates.com","avatar":"https://gravatar.com/avatar/d1a87d63ffb562b791992d8a119ebbdd742e703109d23333ca3fca51306ee95c?d=mp&s=160"},"body":"On Sat, 1 May 2021 at 10:13, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>\n> Unlike SSH this is a thing that the HTTP protocol supports, so I don't\n> think it's the place of git to go out of its way by default to break\n> working URL schemas by making hard breaking assumptions about the user's\n> workflow.\n>\n> I think a much better way to address this issue, if it's an issue that\n> needs to be addressed, is on the server-side. The service operator is in\n> a much better position to know if URL passwords are a bad idea in their\n> case (e.g. is the software frequently used in certain ways, is there no\n> transport security, are we using debug URL tracing etc).\n\nA URL scheme of the form https://user:password@example.com/my.git gets\ntranslated by the http *client* into a request of the form:\n\n  <Connect via TLS to example.com:443>\n  GET /my.git HTTP/1.1\n  Host: example.com\n  Authorization: Basic dXNlcjpwYXNzd29yZAo=\n  ... some more headers ...\n\nSo the server can't determine whether either/both the username or\npassword in the Authorization header were retrieved from a credential\nhelper, were parsed from a URL string, or came via a terminal prompt.\nConceivably If the server only uses a non-default Authorization method\n(\"Token\", \"Bearer\", \"Key\", etc) or uses a separate header (eg:\n\"MyGitHosting-Auth\") then it could conceivably block Basic\nAuthorization, and afaik Git only supports that approach via\nhttp.extraHeader - which doesn't go via any credential helpers, and is\nessentially obscuring authentication as something else (and is pretty\nuser-hostile).\n\nRob :)\n"},{"id":"423480","messageId":"237482e4-8e21-5cd0-010e-09fb4ba8d27e@gmail.com","threadId":"55600","inReplyTo":"YIxRbOh4j9eFxBF3@coredump.intra.peff.net","subject":"Re: [PATCH] urlmatch: do not allow passwords in URLs by default","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-05-03T11:54:50Z","receivedAt":"2021-05-03T11:54:57Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"(Forgive my top-reply, but this part of the message is intended to\nsummarize all replies in the thread.)\n\nThanks, everyone for the thoughtful comments. I appreciate the multiple\ndirections recommended in the thread, including making this config be a\ntri-state with these states:\n\n1. Keep existing behavior.\n2. Warn if Git sees credentials in a URL.\n3. Die if Git sees credentials in a URL.\n\nThis approach provides a good mechanism for transitioning from the\ncurrent state (1) to the die state (3): we can set (2) as default for\na while.\n\nI believe something like this will be necessary to alert users who have\nalready created repositories with credentials in their .git/config files.\n\nBut, there is something better we can do that will be more helpful for\nusers still using this at \"git clone\" time, without causing serious\ndamage to automated scenarios:\n\nOn 4/30/2021 2:50 PM, Jeff King wrote:\n> On Fri, Apr 30, 2021 at 06:37:24PM +0000, Derrick Stolee via GitGitGadget wrote:\n> \n>> From: Derrick Stolee <dstolee@microsoft.com>\n>>\n>> Git allows URLs of the following pattern:\n>>\n>>   https://username:password@domain/route\n>>\n>> These URLs are then parsed to pull out the username and password for use\n>> when authenticating with the URL. Git is careful to anonymize the URL in\n>> status messages with transport_anonymize_url(), but it stores the URL as\n>> plaintext in the .git/config file. The password may leak in other ways.\n> \n> I'm not really opposed to disallowing this entirely (with an escape\n> hatch, as you have here), because it really is an awful practice for a\n> lot of reasons. But another option we discussed previously was to allow\n> the initial clone, but not store the password, which would result in the\n> user being prompted for subsequent fetches:\n> \n>   https://lore.kernel.org/git/20190519050724.GA26179@sigill.intra.peff.net/\n> \n> I think that third patch there is just too gross. But with the first\n> two, if you do have a credential helper configured, then:\n> \n>   git clone https://user:pass@example.com/repo.git\n> \n> would do what you want: clone with that user/pass, and then store the\n> result in the credential helper.\n\nThis seems like the best approach, as it presents the highest likelihood\nof working as expected in the automated scenarios. I will take a look to\nsee how I could adapt those patches and maybe make the third one better.\n\nI think a combined approach would be good. We should still warn that this\nusage pattern is unsafe, because users might use it in an environment\nwhere their commands are being logged and stored another way.\n\nIs it possible that some Git installations have no credential helper? We\ncan keep the \"git clone\" working in that scenario by storing the password\nin memory until the process completes, but later \"git fetch\" commands\nwill fail.\n\nThanks,\n-Stolee\n"},{"id":"423493","messageId":"YJAOUZv0gTWrWd9L@coredump.intra.peff.net","threadId":"55600","inReplyTo":"237482e4-8e21-5cd0-010e-09fb4ba8d27e@gmail.com","subject":"Re: [PATCH] urlmatch: do not allow passwords in URLs by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-03T14:53:05Z","receivedAt":"2021-05-03T14:53:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 03, 2021 at 07:54:50AM -0400, Derrick Stolee wrote:\n\n> > I'm not really opposed to disallowing this entirely (with an escape\n> > hatch, as you have here), because it really is an awful practice for a\n> > lot of reasons. But another option we discussed previously was to allow\n> > the initial clone, but not store the password, which would result in the\n> > user being prompted for subsequent fetches:\n> > \n> >   https://lore.kernel.org/git/20190519050724.GA26179@sigill.intra.peff.net/\n> > \n> > I think that third patch there is just too gross. But with the first\n> > two, if you do have a credential helper configured, then:\n> > \n> >   git clone https://user:pass@example.com/repo.git\n> > \n> > would do what you want: clone with that user/pass, and then store the\n> > result in the credential helper.\n> \n> This seems like the best approach, as it presents the highest likelihood\n> of working as expected in the automated scenarios. I will take a look to\n> see how I could adapt those patches and maybe make the third one better.\n\nIIRC, there was nothing too wrong with the patches. Reviewers had a few\nsmall comments/fixups, but mostly I was on the fence on whether it was a\ngood idea at all, since it was not really my itch, and it was all\nmotivated by third-hand complaints about the behavior. Since nobody\nbrought it up more since then, I hadn't come back to it.\n\nSo I think it probably just needs a bit of polish, and to decide on\npatch 3. If it helps, I've been rebasing it forward as:\n\n  https://github.com/peff/git jk/clone-url-password-wip\n\nbut it's not part of my daily build, so caveat structor.\n\nI don't think the third patch is wrong in _how_ it works. It's mostly\nwhether it's a good idea at all: we are not storing the file in\n.git/config, but we are still storing it in ~/.git-credentials. That's\nmoderately better, but still not very secure.\n\nIf you do have a credential helper defined, then with just patch 2\neverything would Just Work as you'd hope (and even with patch 3, we'll\nskip using credential-store if you have something better defined).\n\n> Is it possible that some Git installations have no credential helper? We\n> can keep the \"git clone\" working in that scenario by storing the password\n> in memory until the process completes, but later \"git fetch\" commands\n> will fail.\n\nI expect lots of installations have no helper configured. I don't think\nany Linux distro packages ship with one configured. I think Apple Git\nships with osxkeychain configured, but I don't know whether the homebrew\nrecipe does, too. And of course anybody building from source is on their\nown.\n\nHopefully some of those people install and configure a credential helper\non their own, but I expect it is wishful thinking to imagine that it's a\nmajority. :)\n\nEven with just the first two patches, I believe the \"in memory\" thing\nyou suggest would already work. We feed the full URL to the transport\ncode, which can make us of it for the duration of the clone process.\nIt's only what we write into .git/config that is changed (so yes, future\nfetches would fail).\n\n-Peff\n"}]}