git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH] urlmatch: create fetch.credentialsInUrl config

From
Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Date
May 23, 2022, 18:04 UTC
Message-ID
<pull.1237.git.1653329044940.gitgitgadget@gmail.com>
From: Derrick Stolee <derrickstolee@github.com>

Users sometimes provide a "username:password" combination in their plaintext URLs. Since Git stores these URLs in plaintext in the .git/config file, this is a very insecure way of storing these credentials. Credential managers are a more secure way of storing this information.

System administrators might want to prevent this kind of use by users on their machines.

Create a new "fetch.credentialsInUrl" config option and teach Git to warn or die when seeing a URL with this kind of information. The warning anonymizes the sensitive information of the URL to be clear about the issue.

This change currently defaults the behavior to "ignore" which does nothing with these URLs. We can consider changing this behavior to "warn" by default if we wish. At that time, we may want to add some advice about setting fetch.credentialsInUrl=ignore for users who still want to follow this pattern (and not receive the warning).

As an attempt to ensure the parsing logic did not catch any unintentional cases, I modified this change locally to to use the "die" option by default. Running the test suite succeeds except for the explicit username:password URLs used in t5550-http-fetch-dumb.s and t5541-http-push-smart.sh. This means that all other tested URLs did not trigger this logic.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
    urlmatch: create fetch.credentialsInUrl config
    
    This is a modified version of the patch I submitted a while ago [1].
    
    Based on the feedback, changing the behavior to fail by default was not
    a good approach. Further, the idea to stop storing the credentials in
    config and redirect them to a credential manager was already considered
    by Peff [2] but not merged.
    
    This patch does what should be the simplest thing we can do: create a
    config option that will cause the user to get a warning or a failure,
    depending on its value. The default is to ignore the setting, identical
    to the current behavior. We can talk about changing this default to
    "warn" in the future, but it would be safest to release with ignore as
    the default until we are sure that we are not going to start warning on
    false positives.
    
    This patch would be sufficient for the interested internal parties that
    want to prevent users from storing credentials this way. System
    administrators can modify system-level Git config into "die" mode to
    prevent this behavior.
    
    [1]
    https://lore.kernel.org/git/pull.945.git.1619807844627.gitgitgadget@gmail.com
    Reject passwords in URLs (April 2021).
    
    [2]
    https://lore.kernel.org/git/20190519050724.GA26179@sigill.intra.peff.net/
    Re: Git ransom campaign incident report - May 2019
    
    Thanks, -Stolee
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1237%2Fderrickstolee%2Fcreds-in-url-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1237/derrickstolee/creds-in-url-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1237
 Documentation/config/fetch.txt | 13 ++++++++++
 t/t5601-clone.sh               |  7 ++++++
 urlmatch.c                     | 43 ++++++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+)
diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt
index cd65d236b43..6aa2a0bec19 100644
--- a/Documentation/config/fetch.txt
+++ b/Documentation/config/fetch.txt
@@ -96,3 +96,16 @@ fetch.writeCommitGraph::
 	merge and the write may take longer. Having an updated commit-graph
 	file helps performance of many Git commands, including `git merge-base`,
 	`git push -f`, and `git log --graph`. Defaults to false.
+
+fetch.credentialsInUrl::
+	A URL can contain plaintext credentials in the form
+	`protocol://<user>:<password>@domain/path`. Using such URLs is not
+	recommended as it exposes the password in multiple ways. The
+	`fetch.credentialsInUrl` option provides instruction for how Git
+	should react to seeing such a URL, with these values:
++
+* `ignore` (default): Git will proceed with its activity without warning.
+* `warn`: Git will write a warning message to `stderr` when parsing a URL
+  with a plaintext credential.
+* `die`: Git will write a failure message to `stderr` when parsing a URL
+  with a plaintext credential.
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 4a61f2c901e..34be520b783 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -71,6 +71,13 @@ test_expect_success 'clone respects GIT_WORK_TREE' '
 
 '
 
+test_expect_success 'clone warns or fails when using username:password' '
+	test_must_fail git -c fetch.credentialsInUrl=warn clone https://username:password@localhost attempt1 2>err &&
+	grep "warning: URL '\''https://username:\*\*\*\*\*\*\*\*@localhost/'\'' uses plaintext credentials" err &&
+	test_must_fail git -c fetch.credentialsInUrl=die clone https://username:password@localhost attempt2 2>err &&
+	grep "fatal: URL '\''https://username:\*\*\*\*\*\*\*\*@localhost/'\'' uses plaintext credentials" err
+'
+
 test_expect_success 'clone from hooks' '
 
 	test_create_repo r0 &&
diff --git a/urlmatch.c b/urlmatch.c
index b615adc923a..6b91fb648a7 100644
--- a/urlmatch.c
+++ b/urlmatch.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "urlmatch.h"
+#include "config.h"
 
 #define URL_ALPHA "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
 #define URL_DIGIT "0123456789"
@@ -106,6 +107,46 @@ static int match_host(const struct url_info *url_info,
 	return (!url_len && !pat_len);
 }
 
+static void detected_credentials_in_url(const char *url, size_t scheme_len)
+{
+	char *value = NULL;
+	const char *at_ptr;
+	const char *colon_ptr;
+	struct strbuf anonymized = STRBUF_INIT;
+
+	/* "ignore" is the default behavior. */
+	if (git_config_get_string("fetch.credentialsinurl", &value) ||
+	    !strcasecmp("ignore", value))
+		goto cleanup;
+
+	at_ptr = strchr(url, '@');
+	colon_ptr = strchr(url + scheme_len + 3, ':');
+
+	if (!colon_ptr)
+		BUG("failed to find colon in url '%s' with scheme_len %"PRIuMAX,
+		    url, (uintmax_t) scheme_len);
+
+	/* Include everything including the colon. */
+	colon_ptr++;
+	strbuf_add(&anonymized, url, colon_ptr - url);
+
+	while (colon_ptr < at_ptr) {
+		strbuf_addch(&anonymized, '*');
+		colon_ptr++;
+	}
+
+	strbuf_addstr(&anonymized, at_ptr);
+
+	if (!strcasecmp("warn", value))
+		warning(_("URL '%s' uses plaintext credentials"), anonymized.buf);
+	if (!strcasecmp("die", value))
+		die(_("URL '%s' uses plaintext credentials"), anonymized.buf);
+
+cleanup:
+	free(value);
+	strbuf_release(&anonymized);
+}
+
 static char *url_normalize_1(const char *url, struct url_info *out_info, char allow_globs)
 {
 	/*
@@ -144,6 +185,7 @@ static char *url_normalize_1(const char *url, struct url_info *out_info, char al
 	 */
 
 	size_t url_len = strlen(url);
+	const char *orig_url = url;
 	struct strbuf norm;
 	size_t spanned;
 	size_t scheme_len, user_off=0, user_len=0, passwd_off=0, passwd_len=0;
@@ -191,6 +233,7 @@ static char *url_normalize_1(const char *url, struct url_info *out_info, char al
 			}
 			colon_ptr = strchr(norm.buf + scheme_len + 3, ':');
 			if (colon_ptr) {
+				detected_credentials_in_url(orig_url, scheme_len);
 				passwd_off = (colon_ptr + 1) - norm.buf;
 				passwd_len = norm.len - passwd_off;
 				user_len = (passwd_off - 1) - (scheme_len + 3);

base-commit: f9b95943b68b6b8ca5a6072f50a08411c6449b55

-- 
gitgitgadget
Next: Junio C Hamano
Message 1 of 40 in “urlmatch: create fetch.credentialsInUrl config”
  1. urlmatch: create fetch.credentialsInUrl configDerrick Stolee via GitGitGadget, May 23, 2022
  2. Junio C HamanoMay 23, 2022
  3. Derrick StoleeMay 23, 2022
  4. Junio C HamanoMay 23, 2022
  5. Johannes SchindelinMay 24, 2022
  6. Derrick StoleeMay 24, 2022
  7. Junio C HamanoMay 23, 2022
  8. Johannes SchindelinMay 24, 2022
  9. Ævar Arnfjörð BjarmasonMay 24, 2022
  10. Derrick StoleeMay 24, 2022
  11. Ævar Arnfjörð BjarmasonMay 24, 2022
  12. Derrick StoleeMay 25, 2022
  13. Johannes SchindelinMay 24, 2022
  14. Derrick StoleeMay 24, 2022
  15. urlmatch: create fetch.credentialsInUrl configDerrick Stolee via GitGitGadget, May 27, 2022
  16. Ævar Arnfjörð BjarmasonMay 27, 2022
  17. Derrick StoleeMay 27, 2022
  18. Junio C HamanoMay 27, 2022
  19. Junio C HamanoMay 27, 2022
  20. Junio C HamanoMay 30, 2022
  21. Derrick StoleeMay 31, 2022
  22. 0/2 fetch: create fetch.credentialsInUrl configDerrick Stolee via GitGitGadget, Jun 1, 2022
  23. 1/2 remote: create fetch.credentialsInUrl configDerrick Stolee via GitGitGadget, Jun 1, 2022
  24. Ævar Arnfjörð BjarmasonJun 1, 2022
  25. Derrick StoleeJun 2, 2022
  26. 2/2 usage: add warn_once() helper for repeated warningsDerrick Stolee via GitGitGadget, Jun 1, 2022
  27. Ævar Arnfjörð BjarmasonJun 1, 2022
  28. Derrick StoleeJun 1, 2022
  29. Ævar Arnfjörð BjarmasonJun 1, 2022
  30. Derrick StoleeJun 2, 2022
  31. Junio C HamanoJun 1, 2022
  32. Derrick StoleeJun 2, 2022
  33. Junio C HamanoJun 2, 2022
  34. Junio C HamanoJun 1, 2022
  35. remote: create fetch.credentialsInUrl configDerrick Stolee via GitGitGadget, Jun 2, 2022
  36. Junio C HamanoJun 2, 2022
  37. Derrick StoleeJun 3, 2022
  38. Junio C HamanoJun 6, 2022
  39. remote: create fetch.credentialsInUrl configDerrick Stolee via GitGitGadget, Jun 6, 2022
  40. Junio C HamanoJun 6, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.