urlmatch: create fetch.credentialsInUrl config

From: Derrick Stolee <[email protected]>

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.

---
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/[email protected]
Reject passwords in URLs (April 2021).

[2]
https://lore.kernel.org/git/[email protected]/
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(+)

index cd65d236b43..6aa2a0bec19 100644
@@ -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.

index 4a61f2c901e..34be520b783 100755
@@ -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 &&

base-commit: f9b95943b68b6b8ca5a6072f50a08411c6449b55

gitgitgadget

The issue sounds vaguely familiar---I must have heard something similar on this list not in too distant past.

It sounds more like "pass" than "ignore", the latter of which can be read as "strip" instead of "pass it as-is".

The name "warn", and its stronger form "die", both sound right.

We are not testing the form we are not encouraging, in other words ;-).

I just peeked [2] and I am not sure why we didn't X-<. The solution there covers "git clone" that records the origin URL but this one would cover URL regardless of where the URL came from---as long as an insecure URL is used, we warn or die, and it is even against the URL that came from the command line.

In a sense, I think these are more or less orthogonal. [2]'s "clone can strip the user:pass from the URL it writes to the config, while passing user:pass to the credential API", especially if it is extended to "git remote add", would stop two common avenues that such an insecure URL can go to the configuration file. The approach taken by this patch would complement it to a degree, as long as the user cares.

I am not sure if there is a legitimate case where the user does not care, though. For a script, it may be handy if a URL can contain an ever-changing user:pass pair, where the pass is generated by something like s/key, for example, and for such a command line that knowingly have user:pass pair, having to set the configuration to "ignore" may be cumbersome.

Thanks.

It certainly felt like not too distant, but [1] was over a year ago!

Perhaps "allow" would be more clear than all of these options?

Right, but in addition we are hopefully testing most of the forms we _do_ encourage, and without triggering these warnings.

The only reason I can guess is that credential helpers were not as commonplace then. Perhaps now is the right time to revisit it with the knowledge that more users have credential helpers for HTTPS URLs (or use SSH with registered public keys).

I agree that these are mostly orthogonal. I think that the parsing logic in urlmatch.c would be involved in the

Would it make sense to check isatty(2) if we make "warn" the default? We could avoid breaking scripts and third-party tools that way.

Even with something like a global in environment.c, what would initialize it? Would we make it be part of the default Git config so it is initialized at the start of every builtin? Or, would we initialize the config the first time we parse a URL.

With that in mind, it might be good to have a static enum that stores the parsed config value and uses that immediately instead of calling git_config_get_string() repeatedly. Are there places where we might inspect a huge number of URLs?

This computation of at_ptr matches the one in url_normalize_1(), so it at least agrees about where the "username[:password]" section could be. That does mean that the password cannot contain an "@" symbol (unless it is special-cased somehow?).

Thanks,
-Stolee

Or "allow" (which I prefer over "pass").

OK.

I wasn't worried about what is valid but more about what attackers can fool users to throw at "git clone" and make our code misbehave (which can include garbage that would not parse correctly).

I think the while() loop will just become a no-op, anonymized buffer is left empty and colon_ptr does not get updated at all. Then strbuf_addstr() after the loop will put everything from '@' to the strbuf to be showed, and none of these should lead to any overrun or exploit.

Thanks.

Hi Stolee,

Hi Stolee,

Careful here. https://[email protected]:9999/ _is_ a valid URL, too.

The password cannot contain a literal @, and neither can the user name. They have to be URL-encoded, via %40.

Ciao,
Dscho

Hi Junio,

To provide one data point: a few of Git for Windows' automated builds use the https://user@pass:host/ form to clone and push, using a Personal Access Token as password (that is of course marked as a secret, read: it will get redacted out of the logs).

So yes, there are scripts that rely on Git's current behavior to work.

If Git changes behavior, I will have to adjust those build definitions.

In this instance, I believe that the benefit of safeguarding Git's users outweighs the burden of having to adjust such scripts/definitions.

Ciao,
Dscho

This documentation is not the proper place to tell the user "do this and you can trust your plaintext creds in the filesystem" because that is asking for problems. I'd rather leave a vague warning and let users go against the recommended behavior only after they have done sufficient work to be confident in taking on that risk.

Sure.

Did you intend to say more here?

I will not change this name to be less descriptive.

We do not necessarily initialize it here. The compiler doesn't notice it and the free(value) below segfaults.

I'm happy to replace the asterisks with <redacted>. Otherwise, I don't see anything we can do to share across these methods. Specifically, the test in that commit seems to indicate that the redacted portion is only the packfile name (the $HTTPD_URL is not filtered).

Since we can't rely on it, I'll leave that to another (you, perhaps?) to do that ifdef work. I don't think it's worth it right now.

OK. And that also avoids the need for initialization you mentioned.

Thanks,
-Stolee

Thanks for checking. The method should not be called unless the password region was already detected. I'll add a BUG() statement and a comment to prevent future callers from providing incorrect URLs.

I can also add a test to show that this warning is not output when the only colon is for the port number.

Thanks!
-Stolee

I'll update the method comment to make this clear.

Yes. The method is about what to do once we've detected a URL with a "username:password@" combination after the scheme. I mentioned in a different reply, but I'll make this clear with a comment.

Thanks,
-Stolee

I don't mean that we need to cover the full divergent views on different approaches to local password management, but not leave the user hanging with the rather scary "exposes the password in multiple ways".

I.e. if I read that for any software whose implementation I wasn't very familiar with I'd be very afraid, and in git's case for no reason.

Does in mean that git has some scary git-specific feature that would expose it. perhaps there's a local log that's unsecured where attempted URLs are logged, or perhaps we send the raw requested URL to the server so it can suggest alternatives for us. We do neither, but even a generally knowledgeable user won't know that about git in particular.

Whereas what I think you actually mean and are targeting here is better explained by:

Git is careful to avoid exposing passwords in URLs on its own, e.g. they won't be logged in trace2 logs. This setting is intended for those who'd like to discourage (warn) or enforce (die) the use of the password helper infrastructure over hardcoded passwords.

All of which I *think* is correct, but maybe I've missed something you know about, as that "in multiple ways" is doing a lot of work.

I also wonder if this wouldn't be even more useful if we took some lessons from ssh's book. I.e. per "git config -l --show-origin" we know the original of all config. We could be even more useful (and more aggressive about warning about) cases where we have passwords in config files that we detect don't have restrictive permissions, as OpenSSH does with your private key.

Ditto perhaps when the origin is "command line", as we do nothing to hide that from the process list on shared systems (and that would be racy whatever we did).

Probably, but if I did I forgot about it, by now. Sorry.

Sure, just a suggestion. The other way is to just re-wrap that one line... :)

In the end I don't care, "just a nit", but just as one datapoint from reading this code: I find this varibale name in particular to be the polar opposite of descriptive, we're explicitly not anonymizing the URL in this function, since we're not stripping the username part.

Wouldn't descriptive be something more like uri_redacted_password or uri_no_password in this case?

Yes, sorry I meant in combination with the *_tmp() variant below...

Yes, I just meant adding a "<redacted>". I briefly looked at whether it made sense to share the implementation, but I think probably not.

I didn't think of this at the time but your implementation also leaks the length of the password, which <redacted> will solve in any case.

Just for the implementation: It's slightly more wasteful, but in this case we don't care about performance, so perhaps a strbuf_splice() variant is easier here? I.e. add the full URL, find : and @, then strbuf_splice() it. It gets rid of much of the pointer juggling here & adding things incrementally.

By HTTPD_URL it means "the path", i.e. it's the equivalent of stripping CURLUPART_{PATH,QUERY,FRAGMENT}.

So a hypothetical shared implementation would just be a matter of searching for the '/' once we're past the (optional) '@', but better to leave it for now.

Yeah, probably not.

*nod*

I tried to be careful about how "it" (being "Using such URLs") can expose the password includes things that are not under Git's responsibility (such as command-line histories and other system-level logs) but I can add a bit about how Git stores the plaintext password in the repository's config.

How about "redacted"?

TIL. strbuf_splice() will work perfectly. Thanks.

-Stolee

From: Derrick Stolee <[email protected]>

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 "allow" 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.sh and t5541-http-push-smart.sh. This means that all other tested URLs did not trigger this logic.

---
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/[email protected]
Reject passwords in URLs (April 2021).

[2]
https://lore.kernel.org/git/[email protected]/
Re: Git ransom campaign incident report - May 2019

Updates in v2
=============

* Documentation is slightly expanded to include the fact that Git stores the given URL as plaintext in its config.
* The new method has a new documentation comment that details the necessary preconditions.
* "ignore" is now "allow"
* Additional checks on colon_ptr are added.
* Use strbuf_splice() instead of custom string-walking logic.
* Use "" instead of asterisks.
* Config value checks are no longer case sensitive.

Thanks, -Stolee

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1237%2Fderrickstolee%2Fcreds-in-url-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1237/derrickstolee/creds-in-url-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1237

Range-diff vs v1:

1: cc2befb3803 ! 1: 364f5c37c70 urlmatch: create fetch.credentialsInUrl config

     @@ Commit message
          anonymizes the sensitive information of the URL to be clear about the
          issue.
      
     -    This change currently defaults the behavior to "ignore" which does
     +    This change currently defaults the behavior to "allow" 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
     @@ Commit message
          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
     +    explicit username:password URLs used in t5550-http-fetch-dumb.sh and
          t5541-http-push-smart.sh. This means that all other tested URLs did not
          trigger this logic.
      
     @@ Documentation/config/fetch.txt: fetch.writeCommitGraph::
      +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
     ++	recommended as it exposes the password in multiple ways, including
     ++	Git storing the URL as plaintext in the repository config. 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.
     ++* `allow` (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
     @@ t/t5601-clone.sh: 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 &&
     ++	grep "warning: URL '\''https://username:<redacted>@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
     ++	grep "fatal: URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" err
     ++'
     ++
     ++test_expect_success 'clone does not detect username:password when it is https://username@domain:port/' '
     ++	test_must_fail git -c fetch.credentialsInUrl=warn clone https://username@localhost:8080 attempt3 2>err &&
     ++	! grep "uses plaintext credentials" err
      +'
      +
       test_expect_success 'clone from hooks' '
     @@ urlmatch.c: static int match_host(const struct url_info *url_info,
       	return (!url_len && !pat_len);
       }
       
     ++/*
     ++ * Call this method when we have detected credentials within the 'url' in
     ++ * the form
     ++ *
     ++ *     scheme://username:password@domain[:port][/path]
     ++ *
     ++ * The 'scheme_len' value should be equal to the string length of the
     ++ * "scheme://" portion of the URL.
     ++ *
     ++ * The fetch.credentialsInUrl config indicates what to do on such a URL,
     ++ * either ignoring, warning, or die()ing. The latter two modes write a
     ++ * redacted URL to stderr.
     ++ */
      +static void detected_credentials_in_url(const char *url, size_t scheme_len)
      +{
     -+	char *value = NULL;
     ++	const char *value;
      +	const char *at_ptr;
      +	const char *colon_ptr;
     -+	struct strbuf anonymized = STRBUF_INIT;
     ++	struct strbuf redacted = STRBUF_INIT;
      +
     -+	/* "ignore" is the default behavior. */
     -+	if (git_config_get_string("fetch.credentialsinurl", &value) ||
     -+	    !strcasecmp("ignore", value))
     -+		goto cleanup;
     ++	/* "allow" is the default behavior. */
     ++	if (git_config_get_string_tmp("fetch.credentialsinurl", &value) ||
     ++	    !strcmp("allow", value))
     ++		return;
      +
      +	at_ptr = strchr(url, '@');
      +	colon_ptr = strchr(url + scheme_len + 3, ':');
      +
     ++	/*
     ++	 * Let's do some defensive programming to ensure the given
     ++	 * URL is of the proper format.
     ++	 */
      +	if (!colon_ptr)
      +		BUG("failed to find colon in url '%s' with scheme_len %"PRIuMAX,
      +		    url, (uintmax_t) scheme_len);
     ++	if (colon_ptr > at_ptr)
     ++		BUG("input url '%s' does not include credentials",
     ++		    url);
      +
     -+	/* Include everything including the colon. */
     ++	/* Include the colon when creating the redacted URL. */
      +	colon_ptr++;
     -+	strbuf_add(&anonymized, url, colon_ptr - url);
     -+
     -+	while (colon_ptr < at_ptr) {
     -+		strbuf_addch(&anonymized, '*');
     -+		colon_ptr++;
     -+	}
     -+
     -+	strbuf_addstr(&anonymized, at_ptr);
     ++	strbuf_addstr(&redacted, url);
     ++	strbuf_splice(&redacted, colon_ptr - url, at_ptr - colon_ptr,
     ++		      "<redacted>", 10);
      +
     -+	if (!strcasecmp("warn", value))
     -+		warning(_("URL '%s' uses plaintext credentials"), anonymized.buf);
     -+	if (!strcasecmp("die", value))
     -+		die(_("URL '%s' uses plaintext credentials"), anonymized.buf);
     ++	if (!strcmp("warn", value))
     ++		warning(_("URL '%s' uses plaintext credentials"), redacted.buf);
     ++	if (!strcmp("die", value))
     ++		die(_("URL '%s' uses plaintext credentials"), redacted.buf);
      +
     -+cleanup:
     -+	free(value);
     -+	strbuf_release(&anonymized);
     ++	strbuf_release(&redacted);
      +}
      +
       static char *url_normalize_1(const char *url, struct url_info *out_info, char allow_globs)

 Documentation/config/fetch.txt | 14 +++++++++
 t/t5601-clone.sh               | 12 ++++++++
 urlmatch.c                     | 56 ++++++++++++++++++++++++++++++++++
 3 files changed, 82 insertions(+)

index cd65d236b43..7fd3ea89f5d 100644
@@ -96,3 +96,17 @@ 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, including
+	Git storing the URL as plaintext in the repository config. The
+	`fetch.credentialsInUrl` option provides instruction for how Git
+	should react to seeing such a URL, with these values:
++
+* `allow` (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.

index 4a61f2c901e..387da74d175 100755
@@ -71,6 +71,18 @@ 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:<redacted>@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:<redacted>@localhost/'\'' uses plaintext credentials" err
+'
+
+test_expect_success 'clone does not detect username:password when it is https://username@domain:port/' '
+	test_must_fail git -c fetch.credentialsInUrl=warn clone https://username@localhost:8080 attempt3 2>err &&
+	! grep "uses plaintext credentials" err
+'
+
 test_expect_success 'clone from hooks' '
 
 	test_create_repo r0 &&

base-commit: f9b95943b68b6b8ca5a6072f50a08411c6449b55

gitgitgadget

Just real quick, I hadn't taken notice of this before (the rest looks good at a glance):

	struct strbuf sb = STRBUF_INIT;
	if (!strbuf_splice_to(url, &redacted, /* same as strbuf_splice(...) */))
		warn("oh noes a password in %s", sb.buf);
	else
		warn("have no password in %s, no replacement done", url->buf);

The name means "Do what needs to be done when creds are detected in a URL".

If we decide to change the response to creds in a URL, then we would change the implementation without changing the name.

Unfortunately, at this point 'url' is not equal to 'orig_url' and 'colon_ptr' isn't even within the 'orig_url' string. It's been copied and mutated. It was my first instinct to share as much as possible, which is why 'schema' is reused.

Thanks,
-Stolee

The new function does not even have to take user_off and user_len, and work only with the while string with <ofs,len> pair for the password, I think, as that is the only thing it redacts in the output. Sorry about the noise.

Can we make this die in a bit more controlled way?

e.g. https://github.com/git/git/runs/6646450422 seems to show that depending on the timing, the call to die() on the "git clone" side may cause us stop reading early enough to kill the other side with SIGPIPE. The nicely prepared warning message seems to be lost.

Thanks for pointing this out. It took a while for me to reproduce this with --stress, but I can get it to happen on my machine.

Investigating now.

Thanks,
-Stolee

Users can specify credentials in their URL using the username:password@domain format. This is potentially hazardous since the URL is stored in plaintext and can also appear in trace2 logs and other places. Add a new config option that allows warnings or failures when discovering URLs with this format. The default behavior does not change in this series, although we may want to move to the warn state by default in the future.

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/[email protected]
Reject passwords in URLs (April 2021).

[2]
https://lore.kernel.org/git/[email protected]/
Re: Git ransom campaign incident report - May 2019

Updates in v3
=============

* Because of some flaky behavior around SIGPIPE, the URL check needed to move to be earlier in the command.
* For this reason, I moved the cred check into remote.c's valid_remote() check. This changed the previous BUG() statements into early returns.
* I repeated the check with the test suite to see if this parsing fails on any existing cases, but it is worth double-checking the parsing rules.
* Documentation is more consistent about using placeholders.
* A test for the "allow" case is now included.
* A new patch is added that creates the warn_once() helper. This reduces multiple advisory warnings with the same text from being written by the same process.

Updates in v2
=============

* Documentation is slightly expanded to include the fact that Git stores the given URL as plaintext in its config.
* The new method has a new documentation comment that details the necessary preconditions.
* "ignore" is now "allow"
* Additional checks on colon_ptr are added.
* Use strbuf_splice() instead of custom string-walking logic.
* Use "" instead of asterisks.
* Config value checks are no longer case sensitive.

Thanks, -Stolee

Derrick Stolee (2):
remote: create fetch.credentialsInUrl config
usage: add warn_once() helper for repeated warnings

 Documentation/config/fetch.txt | 14 ++++++++++
 git-compat-util.h              |  1 +
 remote.c                       | 48 ++++++++++++++++++++++++++++++++++
 t/t5601-clone.sh               | 17 ++++++++++++
 usage.c                        | 22 ++++++++++++++++
 5 files changed, 102 insertions(+)

base-commit: f9b95943b68b6b8ca5a6072f50a08411c6449b55
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1237%2Fderrickstolee%2Fcreds-in-url-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1237/derrickstolee/creds-in-url-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1237

Range-diff vs v2:

1: 364f5c37c70 ! 1: 083a918e9b1 urlmatch: create fetch.credentialsInUrl config
@@ Metadata
Author: Derrick Stolee <[email protected]>

## Commit message ##
- urlmatch: create fetch.credentialsInUrl config
+ remote: create fetch.credentialsInUrl config

Users sometimes provide a "username:password" combination in their plaintext URLs. Since Git stores these URLs in plaintext in the
@@ Commit message
advice about setting fetch.credentialsInUrl=ignore for users who still want to follow this pattern (and not receive the warning).

+ An earlier version of this change injected the logic into
+ url_normalize() in urlmatch.c. While most code paths that parse URLs
+ eventually normalize the URL, that normalization does not happen early
+ enough in the stack to avoid attempting connections to the URL first. By
+ inserting a check into the remote validation, we identify the issue
+ before making a connection. In the old code path, this was revealed by
+ testing the new t5601-clone.sh test under --stress, resulting in an
+ instance where the return code was 13 (SIGPIPE) instead of 128 from the
+ die().
+
+ Since we are not deep within url_normalize(), we need to do our own
+ parsing to detect if there is a "username:password@domain" section. We
+ begin by detecting the first '@' symbol in the URL. We then detect if
+ there is a scheme such as "https://" by finding the first slash. If that
+ slash does not exist or is after the first '@' symbol, then we consider
+ the scheme to be complete before the URL. Finally, we look for a colon
+ between the scheme and the '@' symbol, indicating a "username:password"
+ string. Replace the password with "<redacted>" when writing the error
+ message.
+
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
@@ Documentation/config/fetch.txt: fetch.writeCommitGraph::
+
+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, including
-+ Git storing the URL as plaintext in the repository config. The
-+ fetch.credentialsInUrl option provides instruction for how Git
++ <protocol>://<user>:<password>@<domain>/<path>. Using such URLs
++ is not recommended as it exposes the password in multiple ways,
++ including Git storing the URL as plaintext in the repository config.
++ The fetch.credentialsInUrl option provides instruction for how Git
+ should react to seeing such a URL, with these values:
++
+* allow (default): Git will proceed with its activity without warning.
@@ Documentation/config/fetch.txt: fetch.writeCommitGraph::
+* die: Git will write a failure message to stderr when parsing a URL
+ with a plaintext credential.

- ## t/t5601-clone.sh ##
-@@ t/t5601-clone.sh: 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:<redacted>@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:<redacted>@localhost/'\'' uses plaintext credentials" err
-+'
-+
-+test_expect_success 'clone does not detect username:password when it is https://username@domain:port/' '
-+ test_must_fail git -c fetch.credentialsInUrl=warn clone https://username@localhost:8080 attempt3 2>err &&
-+ ! grep "uses plaintext credentials" err
-+'
-+
- test_expect_success 'clone from hooks' '
-
- test_create_repo r0 &&
-
- ## urlmatch.c ##
-@@
- #include "cache.h"
- #include "urlmatch.h"
-+#include "config.h"
-
- #define URL_ALPHA "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
- #define URL_DIGIT "0123456789"
-@@ urlmatch.c: static int match_host(const struct url_info *url_info,
- return (!url_len && !pat_len);
- }
+ ## remote.c ##
+@@ remote.c: struct counted_string {
+ const char *s;
+ };

+/*
-+ * Call this method when we have detected credentials within the 'url' in
-+ * the form
++ * Check if the given URL is of the following form:
+ *
+ * scheme://username:password@domain[:port][/path]
+ *
-+ * The 'scheme_len' value should be equal to the string length of the
-+ * "scheme://" portion of the URL.
++ * Specifically, see if the ":password@" section of the URL appears.
+ *
+ * The fetch.credentialsInUrl config indicates what to do on such a URL,
-+ * either ignoring, warning, or die()ing. The latter two modes write a
++ * either ignoring, warning, or erroring. The latter two modes write a
+ * redacted URL to stderr.
+ */
-+static void detected_credentials_in_url(const char *url, size_t scheme_len) ++static void check_if_creds_in_url(const char *url)
+{
-+ const char *value;
-+ const char *at_ptr;
-+ const char *colon_ptr;
++ const char *value, *scheme_ptr, *colon_ptr, *at_ptr;
+ struct strbuf redacted = STRBUF_INIT;
+
+ /* "allow" is the default behavior. */
@@ urlmatch.c: static int match_host(const struct url_info *url_info,
+ !strcmp("allow", value))
+ return;
+
-+ at_ptr = strchr(url, '@');
-+ colon_ptr = strchr(url + scheme_len + 3, ':');
++ if (!(at_ptr = strchr(url, '@')))
++ return;
+
-+ /*
-+ * Let's do some defensive programming to ensure the given
-+ * URL is of the proper format.
-+ */
-+ if (!colon_ptr)
-+ BUG("failed to find colon in url '%s' with scheme_len %"PRIuMAX,
-+ url, (uintmax_t) scheme_len);
-+ if (colon_ptr > at_ptr)
-+ BUG("input url '%s' does not include credentials",
-+ url);
++ if (!(scheme_ptr = strchr(url, '/')) ||
++ scheme_ptr > at_ptr)
++ scheme_ptr = url;
++
++ if (!(colon_ptr = strchr(scheme_ptr, ':')))
++ return;
+
+ /* Include the colon when creating the redacted URL. */
+ colon_ptr++;
@@ urlmatch.c: static int match_host(const struct url_info *url_info,
+ strbuf_release(&redacted);
+}
+
- static char *url_normalize_1(const char *url, struct url_info *out_info, char allow_globs)
+ static int valid_remote(const struct remote *remote)
{
- /*
-@@ urlmatch.c: static char *url_normalize_1(const char *url, struct url_info *out_info, char al
- */
++ for (int i = 0; i < remote->url_nr; i++)
++ check_if_creds_in_url(remote->url[i]);
++
+ return (!!remote->url) || (!!remote->foreign_vcs);
+ }
+
+
+ ## t/t5601-clone.sh ##
+@@ t/t5601-clone.sh: test_expect_success 'clone respects GIT_WORK_TREE' '
+
+ '

- 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;
-@@ urlmatch.c: 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);
++test_expect_success 'clone warns or fails when using username:password' '
++ test_must_fail git -c fetch.credentialsInUrl=allow clone https://username:password@localhost attempt1 2>err &&
++ ! grep "URL '\''https://username:<redacted>@localhost'\'' uses plaintext credentials" err &&
++ test_must_fail git -c fetch.credentialsInUrl=warn clone https://username:password@localhost attempt1 2>err &&
++ grep "warning: URL '\''https://username:<redacted>@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:<redacted>@localhost'\'' uses plaintext credentials" err
++'
++
++test_expect_success 'clone does not detect username:password when it is https://username@domain:port/' '
++ test_must_fail git -c fetch.credentialsInUrl=warn clone https://username@localhost:8080 attempt3 2>err &&
++ ! grep "uses plaintext credentials" err
++'
++
+ test_expect_success 'clone from hooks' '
+
+ test_create_repo r0 &&
-: ----------- > 2: 8e29ac807c6 usage: add warn_once() helper for repeated warnings

gitgitgadget

From: Derrick Stolee <[email protected]>

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 "allow" 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).

An earlier version of this change injected the logic into url_normalize() in urlmatch.c. While most code paths that parse URLs eventually normalize the URL, that normalization does not happen early enough in the stack to avoid attempting connections to the URL first. By inserting a check into the remote validation, we identify the issue before making a connection. In the old code path, this was revealed by testing the new t5601-clone.sh test under --stress, resulting in an instance where the return code was 13 (SIGPIPE) instead of 128 from the die().

Since we are not deep within url_normalize(), we need to do our own parsing to detect if there is a "username:password@domain" section. We begin by detecting the first '@' symbol in the URL. We then detect if there is a scheme such as "https://" by finding the first slash. If that slash does not exist or is after the first '@' symbol, then we consider the scheme to be complete before the URL. Finally, we look for a colon between the scheme and the '@' symbol, indicating a "username:password" string. Replace the password with "<redacted>" when writing the error message.

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.sh and t5541-http-push-smart.sh. This means that all other tested URLs did not trigger this logic.

---
 Documentation/config/fetch.txt | 14 ++++++++++
 remote.c                       | 48 ++++++++++++++++++++++++++++++++++
 t/t5601-clone.sh               | 14 ++++++++++
 3 files changed, 76 insertions(+)

index cd65d236b43..0db7fe85bb8 100644
@@ -96,3 +96,17 @@ 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,
+	including Git storing the URL as plaintext in the repository config.
+	The `fetch.credentialsInUrl` option provides instruction for how Git
+	should react to seeing such a URL, with these values:
++
+* `allow` (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.

index 4a61f2c901e..cba3553b7c4 100755
@@ -71,6 +71,20 @@ 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=allow clone https://username:password@localhost attempt1 2>err &&
+	! grep "URL '\''https://username:<redacted>@localhost'\'' uses plaintext credentials" err &&
+	test_must_fail git -c fetch.credentialsInUrl=warn clone https://username:password@localhost attempt1 2>err &&
+	grep "warning: URL '\''https://username:<redacted>@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:<redacted>@localhost'\'' uses plaintext credentials" err
+'
+
+test_expect_success 'clone does not detect username:password when it is https://username@domain:port/' '
+	test_must_fail git -c fetch.credentialsInUrl=warn clone https://username@localhost:8080 attempt3 2>err &&
+	! grep "uses plaintext credentials" err
+'
+
 test_expect_success 'clone from hooks' '
 
 	test_create_repo r0 &&

gitgitgadget

From: Derrick Stolee <[email protected]>

The previous change added a warning when valid_remote() detects credentials in the URL. Since remotes are validated multiple times per process, this causes multiple warnings to print.

To avoid these kinds of repeated, advisory warnings, create a new warn_once() helper that behaves the same as warning(), but only after formatting the output string and adding it to a strset. If that addition signals that the string already exists in the strset, then do not print the warning.

In the case of the credentials in a URL, the existing test demonstrates this per-process limitation: 'git clone' runs 'git-remote-curl' as a child process, giving two messages. This is an improvement over the previous six messages.

---
 git-compat-util.h |  1 +
 remote.c          |  2 +-
 t/t5601-clone.sh  |  5 ++++-
 usage.c           | 22 ++++++++++++++++++++++
 4 files changed, 28 insertions(+), 2 deletions(-)

index 58fd813bd01..776a5f660aa 100644
@@ -526,6 +526,7 @@ int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
 int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
 void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
 void warning_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
+void warn_once(const char *warn, ...) __attribute__((format (printf, 1, 2)));
 
 #ifndef NO_OPENSSL
 #ifdef APPLE_COMMON_CRYPTO

index accf08bf51f..72fffd0d968 100644
@@ -60,7 +60,7 @@ static void check_if_creds_in_url(const char *url)
 		      "<redacted>", 10);
 
 	if (!strcmp("warn", value))
-		warning(_("URL '%s' uses plaintext credentials"), redacted.buf);
+		warn_once(_("URL '%s' uses plaintext credentials"), redacted.buf);
 	if (!strcmp("die", value))
 		die(_("URL '%s' uses plaintext credentials"), redacted.buf);
 

index cba3553b7c4..6ae3eec9eb6 100755
@@ -75,7 +75,10 @@ test_expect_success 'clone warns or fails when using username:password' '
 	test_must_fail git -c fetch.credentialsInUrl=allow clone https://username:password@localhost attempt1 2>err &&
 	! grep "URL '\''https://username:<redacted>@localhost'\'' uses plaintext credentials" err &&
 	test_must_fail git -c fetch.credentialsInUrl=warn clone https://username:password@localhost attempt1 2>err &&
-	grep "warning: URL '\''https://username:<redacted>@localhost'\'' uses plaintext credentials" err &&
+	grep "warning: URL '\''https://username:<redacted>@localhost'\'' uses plaintext credentials" err >warnings &&
+	# The warning is printed twice, for the two processes:
+	# "git clone" and "git-remote-curl".
+	test_line_count = 2 warnings &&
 	test_must_fail git -c fetch.credentialsInUrl=die clone https://username:password@localhost attempt2 2>err &&
 	grep "fatal: URL '\''https://username:<redacted>@localhost'\'' uses plaintext credentials" err
 '

index b738dd178b3..242633c5f8d 100644
@@ -5,6 +5,7 @@
  */
 #include "git-compat-util.h"
 #include "cache.h"
+#include "strmap.h"
 
 static void vreportf(const char *prefix, const char *err, va_list params)
 {

@@ -287,6 +288,27 @@ void warning(const char *warn, ...)
 	va_end(params);
 }
 
+static struct strset prev_warnings = STRSET_INIT;
+
+void warn_once(const char *warn, ...)
+{
+	char buf[1024];
+	va_list params;
+	va_start(params, warn);
+
+	if (vsnprintf(buf, sizeof(buf), warn, params) >= 0) {
+		if (!strset_add(&prev_warnings, buf)) {
+			va_end(params);
+			return;
+		}
+	}
+	va_end(params);
+
+	va_start(params, warn);
+	warn_routine(warn, params);
+	va_end(params);
+}
+
 /* Only set this, ever, from t/helper/, when verifying that bugs are caught. */
 int BUG_exit_code;
 

gitgitgadget

Why are we validating remotes multiple times per process? Can't we just do it once?

Is this a case of two callers going through the whole machinery and not being aware of one another?

Perhaps it's a pain to deal with that in this case, but it would be better to note why here than taking it as a given.

This feels quite iffy given the low-level API & the rest of it aiming to be thread-safe, see 2d3c02f5db6 (die(): stop hiding errors due to overzealous recursion guard, 2017-06-21) for such a case.

I.e. there *is* racy code there already that (ab)uses a no-lock pattern to optimistically abort early when we'd emit N of the same recursion message.

But are we as confident that concurrent strset callers in multiple threads will behave themselves? I'd think we should be adding this to the caller that knows it's not threaded, e.g. whatever calls check_if_creds_in_url().

If we know about this limitation and we're going to be checking the same URLs why not do slightly better and pass down a "don't warn please" to the sub-process?

Or do we think it might warn about *different* URLs, aren't these always the same (i.e. we read the full config)?

If you used advice instead of a warning you'd get a config key to pass for free to disable it, and that might be a better thing to do in any case (i.e. a better fit for this case). But we could also just check getenv("GIT_URL_REDACTED_WARNING") or whatever, which seems easy enough...

	warning="<your warning msg>" &&
	cat >expect <<-EOF &&
	$warning
        $warning
	EOF

	*p = '\0'; /* vsnprintf() failed, clip at prefix */

On 6/1/2022 8:29 AM, Ævar Arnfjörð Bjarmason wrote:>

We could certainly investigate this more, but it seems like a more problematic approach than the one taken here. We could add a "is_valid" bit to struct remote, but then could some code path modify that struct after it was validated?

It makes sense to keep the low-level library as thread-safe as possible.

Would it be enough to document this particular caller as not thread-safe?

While this example of 'git clone' to 'git-remote-curl' will not use a different URL, I can imagine other child process connections that could expose a different URL. Having a blanket statement "don't emit these warnings" seems unlikely to be helpful for those cases.

To make the environment variable work properly, we would need to pass a list of URLs in the variable that could seed the strset, to avoid dropping warnings for new cases as mentioned earlier.

There are other warnings coming over stderr, including the "Cloning into..." message and the resulting "fatal:" message because that URL doesn't actually exist. I chose to decouple this test to the exact phrasing of those messages.

I wouldn't call this a "oops, bug fix" but instead "let's improve what was working, but noisy." This also gives us the opportunity to see the code in usage.c get tested immediately as it is introduced, which is an important property to keep in mind when organizing a patch series.

Sure. The phrase "warn once" is a valid sentence, while "warning once" is not, but I can see some benefit to having consistent naming here.

If the "warning()" method was not already present so much throughout the codebase, then I would advocate that "warn()" is a better verb form. This would be similar to "die()" and even "error()" could be interpreted as a verb. However, that would be too large of a change to be worthwhile.

Noted.

Except that we do something after this block in both cases, so the condition needs to be swapped or else duplicate code.

No, because the params are passed to warn_routine() in the section of code you cropped out:

+ va_start(params, warn);
+ warn_routine(warn, params);
+ va_end(params);

That warn_routine does the "clip at prefix" fix. if we have a failure in vsnprintf() here, then we are skipping the "only once" check and going straight to that logic.

You are advocating that since there is one example of something that doesn't fit this exact scenario, then we shouldn't move forward on something that _does_ have immediate use (and potential use in other places). This seem unnecessarily pedantic.

This specific method is a good foundation for possibly extending it if there is value in doing so.

Well, outside of the fact that this function is tested and it works, the documentation says this about the 'params' argument (named 'ap' in the function prototype):

If ap has already been passed as first argument to a previous call to va_start or va_copy, it shall be passed to va_end before calling this function.

So, va_end(params) makes it safe to call va_start(params, warn) again.

Thanks,
-Stolee

I tested this a bit and I think this alternate approach is simpler, and it passes your tests:

	diff --git a/remote.c b/remote.c
	index faa2f834635..56ce2c83022 100644
	--- a/remote.c
	+++ b/remote.c
	@@ -60,7 +60,7 @@ static void check_if_creds_in_url(const char *url)
	 		      "<redacted>", 10);
	 
	 	if (!strcmp("warn", value))
	-		warn_once(_("URL '%s' uses plaintext credentials"), redacted.buf);
	+		warning(_("URL '%s' uses plaintext credentials"), redacted.buf);
	 	if (!strcmp("die", value))
	 		die(_("URL '%s' uses plaintext credentials"), redacted.buf);
	 
	@@ -69,12 +69,17 @@ static void check_if_creds_in_url(const char *url)
	 
	 static int valid_remote(const struct remote *remote)
	 {
	-	for (int i = 0; i < remote->url_nr; i++)
	-		check_if_creds_in_url(remote->url[i]);
	-
	 	return (!!remote->url) || (!!remote->foreign_vcs);
	 }
	 
	+static void valid_remote_at_end(const struct remote *remote)
	+{
	+	int i;
	+
	+	for (i = 0; i < remote->url_nr; i++)
	+		check_if_creds_in_url(remote->url[i]);
	+}
	+
	 static const char *alias_url(const char *url, struct rewrites *r)
	 {
	 	int i, j;
	@@ -687,6 +692,7 @@ remotes_remote_get_1(struct remote_state *remote_state, const char *name,
	 		add_url_alias(remote_state, ret, name);
	 	if (!valid_remote(ret))
	 		return NULL;
	+	valid_remote_at_end(ret);
	 	return ret;
	 }

I.e. we already have one spot where we get the full remote config, you were just hooking into valid_remote() which we call N times while doing that, let's just call it at the end.

Aside from general de-duplication APIs that to me seems like a more sensible approach here, i.e. surely we should be getting the remote config once, let's stick such a validation loop in that place.

And it seems we do, the stack is e.g.:

	(gdb) bt
	#0  warn_once (warn=0x79f1f3 "URL '%s' uses plaintext credentials") at usage.c:297
	#1  0x0000000000688a98 in check_if_creds_in_url (url=0x8677a0 "https://username:password@localhost") at remote.c:63
	#2  0x0000000000688858 in valid_remote_at_end (remote=0x872a40) at remote.c:80
	#3  0x0000000000688397 in remotes_remote_get_1 (remote_state=0x85ca00, name=0x861f10 "origin", get_default=0x681d00 <remotes_remote_for_branch>) at remote.c:695
	#4  0x0000000000681fd7 in remotes_remote_get (remote_state=0x85ca00, name=0x861f10 "origin") at remote.c:702
	#5  0x0000000000682071 in remote_get (name=0x861f10 "origin") at remote.c:709
	#6  0x0000000000433b05 in cmd_clone (argc=2, argv=0x7fffffffd028, prefix=0x0) at builtin/clone.c:1146
	#7  0x0000000000407d6b in run_builtin (p=0x8259f8 <commands+504>, argc=3, argv=0x7fffffffd028) at git.c:466
	#8  0x0000000000406802 in handle_builtin (argc=3, argv=0x7fffffffd028) at git.c:720
	#9  0x0000000000407746 in run_argv (argcp=0x7fffffffcecc, argv=0x7fffffffcec0) at git.c:787
	#10 0x00000000004065bb in cmd_main (argc=3, argv=0x7fffffffd028) at git.c:920
	#11 0x000000000051279a in main (argc=7, argv=0x7fffffffd008) at common-main.c:56

You put the validation in a function we called N times in remotes_remote_get_1(), but if we just put it once in remotes_remote_get_1()....

See above, but I'd think we really should consider alternatives before leaving such landmines in place.

I.e. this is called by remote.c, which is common library code itself, someone implementing threads somewhere isn't going to be thinking about or reading caveats about libraries remote.c itself uses unless they're quite lucky...

Yes, I can imagine we might want to do that, but until we find such an API user it seems much simpler to warn in the "primary", i.e. clone/fetch & not the helper process.

Unless there's cases I'm missing where the "primary" is aware of the URL and the others aren't (and even then we could just do this the other way around, no?)

Also isn't such a warning behavior unnecessarily verbose, i.e. does this (I haven't checked in detail) warn about any such bad URL in config anywhere, not just the one you're actually using at the moment, or at least the one for your current remote?

In that case (if we end up having trouble with this) perhaps just warning about the URL we use would be Good Enough and simpler.

I think a much simpler thing to do is to just set a variable in clone.c and fetch.c and have those emit this, and otherwise stay quiet.

The reason you have duplication now is because clone is calling a remote helper.

Speaking of which, we really should test this "we expect N warning(s)" for both fetch & clone, shouldn't we? Ditto push.

FWIW:

	grep ^warning: >warnings &&
	test_cmp ...

Would take care of that.

*nod*, just a thought.

Yeah I'd also prefer warn() myself, but since we have warning() and warning_errno() consistency is better...

*nod*, just a thought while skimming it.

Ah, makes sense. I missed that.

I didn't mean to be pedantic, sorry.

My thinking was more along the lines that I've seen roughly this pattern in a few other places, and a proposed new library utility for generic use is both more convincing, and tends to have bugs shook out of it if we go the extra mile to convert existing API users.

Anyway, per the above I think this one is more easily handled by its caller by just moving the check up one function call...

Yes, thanks. This was just from digging around in now obviously-incorrect wetware :)

Two separate parser parsing the same string to produce (supposedly) equivalent parse results is a bit disturbing, and I am not sure if "is_valid" bit helps that.

Adding "user" and "password" members to the struct, and retire the existing "parser" (instead it would just use the pre-parsed components stored in the struct) would. It would be a much more involved change, and it is something more than we would want to do in a regression fix patch.

But this series is a new feature development, so...

Do we call the valid_remote() helper on the same remote many number of times? Unless the duplicates appear by such a "stupid" reason, I am not sure if this is a good idea to do this deduplication.

Especially if we know we cannot do the deduplication across processes.

Ah, thanks. I remembered that canary going out a long time ago, but forgot to check if we had committed to it.

Thanks,
-Stolee

...

Thanks for finding a place where the current behavior only runs once.

I agree that this is a much simpler approach and reduces this series back down to one.

Thanks,
-Stolee

Yes, you're right. I should use the output 'struct url_inf' from url_normalize() to construct the redacted URL. It has the downside that the output URL can be slightly different from the input URL, but a user should still be able to diagnose how to resolve the situation.

Thanks,
-Stolee

From: Derrick Stolee <[email protected]>

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 "allow" 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).

An earlier version of this change injected the logic into url_normalize() in urlmatch.c. While most code paths that parse URLs eventually normalize the URL, that normalization does not happen early enough in the stack to avoid attempting connections to the URL first. By inserting a check into the remote validation, we identify the issue before making a connection. In the old code path, this was revealed by testing the new t5601-clone.sh test under --stress, resulting in an instance where the return code was 13 (SIGPIPE) instead of 128 from the die().

However, we can reuse the parsing information from url_normalize() in order to benefit from its well-worn parsing logic. We can use the struct url_info that is created in that method to replace the password with "<redacted>" in our error messages. This comes with a slight downside that the normalized URL might look slightly different from the input URL (for instance, the normalized version adds a closing slash). This should not hinder users figuring out what the problem is and being able to fix the issue.

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.sh and t5541-http-push-smart.sh. This means that all other tested URLs did not trigger this logic.

The tests show that the proper error messages appear (or do not appear), but also count the number of error messages. When only warning, each process validates the remote URL and outputs a warning. This happens twice for clone, three times for fetch, and once for push.

---
remote: create fetch.credentialsInUrl config

Users can specify credentials in their URL using the username:password@domain format. This is potentially hazardous since the URL is stored in plaintext and can also appear in trace2 logs and other places. Add a new config option that allows warnings or failures when discovering URLs with this format. The default behavior does not change in this series, although we may want to move to the warn state by default in the future.

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/[email protected]
Reject passwords in URLs (April 2021).

[2]
https://lore.kernel.org/git/[email protected]/
Re: Git ransom campaign incident report - May 2019

Updates in v4
=============

* The warn_once() patch is dropped in favor of using a different location for the check (remotes_remote_get_1() instead of valid_remote()).
* The parsing logic is removed in favor of using the output url_info from url_normalize().
* Tests for 'fetch' and 'push' are added.
* This requires updating the topic to be on a more-recent version of 'master' because some change since the previous base caused 'git push' to output the warning only once, not twice.

Updates in v3
=============

* Because of some flaky behavior around SIGPIPE, the URL check needed to move to be earlier in the command.
* For this reason, I moved the cred check into remote.c's valid_remote() check. This changed the previous BUG() statements into early returns.
* I repeated the check with the test suite to see if this parsing fails on any existing cases, but it is worth double-checking the parsing rules.
* Documentation is more consistent about using placeholders.
* A test for the "allow" case is now included.
* A new patch is added that creates the warn_once() helper. This reduces multiple advisory warnings with the same text from being written by the same process.

Updates in v2
=============

* Documentation is slightly expanded to include the fact that Git stores the given URL as plaintext in its config.
* The new method has a new documentation comment that details the necessary preconditions.
* "ignore" is now "allow"
* Additional checks on colon_ptr are added.
* Use strbuf_splice() instead of custom string-walking logic.
* Use "" instead of asterisks.
* Config value checks are no longer case sensitive.

Thanks, -Stolee

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1237%2Fderrickstolee%2Fcreds-in-url-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1237/derrickstolee/creds-in-url-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1237

Range-diff vs v3:

1: 083a918e9b1 ! 1: 5fb038402d6 remote: create fetch.credentialsInUrl config

     @@ Commit message
          instance where the return code was 13 (SIGPIPE) instead of 128 from the
          die().
      
     -    Since we are not deep within url_normalize(), we need to do our own
     -    parsing to detect if there is a "username:password@domain" section. We
     -    begin by detecting the first '@' symbol in the URL. We then detect if
     -    there is a scheme such as "https://" by finding the first slash. If that
     -    slash does not exist or is after the first '@' symbol, then we consider
     -    the scheme to be complete before the URL. Finally, we look for a colon
     -    between the scheme and the '@' symbol, indicating a "username:password"
     -    string. Replace the password with "<redacted>" when writing the error
     -    message.
     +    However, we can reuse the parsing information from url_normalize() in
     +    order to benefit from its well-worn parsing logic. We can use the struct
     +    url_info that is created in that method to replace the password with
     +    "<redacted>" in our error messages. This comes with a slight downside
     +    that the normalized URL might look slightly different from the input URL
     +    (for instance, the normalized version adds a closing slash). This should
     +    not hinder users figuring out what the problem is and being able to fix
     +    the issue.
      
          As an attempt to ensure the parsing logic did not catch any
          unintentional cases, I modified this change locally to to use the "die"
     @@ Commit message
          t5541-http-push-smart.sh. This means that all other tested URLs did not
          trigger this logic.
      
     +    The tests show that the proper error messages appear (or do not
     +    appear), but also count the number of error messages. When only warning,
     +    each process validates the remote URL and outputs a warning. This
     +    happens twice for clone, three times for fetch, and once for push.
     +
          Signed-off-by: Derrick Stolee <[email protected]>
      
       ## Documentation/config/fetch.txt ##
     @@ Documentation/config/fetch.txt: fetch.writeCommitGraph::
      +  with a plaintext credential.
      
       ## remote.c ##
     -@@ remote.c: struct counted_string {
     - 	const char *s;
     - };
     +@@
     + #include "cache.h"
     + #include "config.h"
     + #include "remote.h"
     ++#include "urlmatch.h"
     + #include "refs.h"
     + #include "refspec.h"
     + #include "object-store.h"
     +@@ remote.c: const char *remote_ref_for_branch(struct branch *branch, int for_push)
     + 	return NULL;
     + }
       
     -+/*
     -+ * Check if the given URL is of the following form:
     -+ *
     -+ *     scheme://username:password@domain[:port][/path]
     -+ *
     -+ * Specifically, see if the ":password@" section of the URL appears.
     -+ *
     -+ * The fetch.credentialsInUrl config indicates what to do on such a URL,
     -+ * either ignoring, warning, or erroring. The latter two modes write a
     -+ * redacted URL to stderr.
     -+ */
     -+static void check_if_creds_in_url(const char *url)
     ++static void validate_remote_url(struct remote *remote)
      +{
     -+	const char *value, *scheme_ptr, *colon_ptr, *at_ptr;
     ++	int i;
     ++	const char *value;
      +	struct strbuf redacted = STRBUF_INIT;
      +
     -+	/* "allow" is the default behavior. */
      +	if (git_config_get_string_tmp("fetch.credentialsinurl", &value) ||
      +	    !strcmp("allow", value))
      +		return;
      +
     -+	if (!(at_ptr = strchr(url, '@')))
     -+		return;
     ++	for (i = 0; i < remote->url_nr; i++) {
     ++		struct url_info url_info = { NULL };
     ++		url_normalize(remote->url[i], &url_info);
      +
     -+	if (!(scheme_ptr = strchr(url, '/')) ||
     -+	    scheme_ptr > at_ptr)
     -+		scheme_ptr = url;
     ++		if (!url_info.passwd_len)
     ++			goto loop_cleanup;
      +
     -+	if (!(colon_ptr = strchr(scheme_ptr, ':')))
     -+		return;
     ++		strbuf_add(&redacted, url_info.url, url_info.passwd_off);
     ++		strbuf_addstr(&redacted, "<redacted>");
     ++		strbuf_addstr(&redacted, url_info.url + url_info.passwd_off + url_info.passwd_len);
      +
     -+	/* Include the colon when creating the redacted URL. */
     -+	colon_ptr++;
     -+	strbuf_addstr(&redacted, url);
     -+	strbuf_splice(&redacted, colon_ptr - url, at_ptr - colon_ptr,
     -+		      "<redacted>", 10);
     ++		if (!strcmp("warn", value))
     ++			warning(_("URL '%s' uses plaintext credentials"), redacted.buf);
     ++		if (!strcmp("die", value))
     ++			die(_("URL '%s' uses plaintext credentials"), redacted.buf);
      +
     -+	if (!strcmp("warn", value))
     -+		warning(_("URL '%s' uses plaintext credentials"), redacted.buf);
     -+	if (!strcmp("die", value))
     -+		die(_("URL '%s' uses plaintext credentials"), redacted.buf);
     ++loop_cleanup:
     ++		free(url_info.url);
     ++	}
      +
      +	strbuf_release(&redacted);
      +}
      +
     - static int valid_remote(const struct remote *remote)
     - {
     -+	for (int i = 0; i < remote->url_nr; i++)
     -+		check_if_creds_in_url(remote->url[i]);
     + static struct remote *
     + remotes_remote_get_1(struct remote_state *remote_state, const char *name,
     + 		     const char *(*get_default)(struct remote_state *,
     +@@ remote.c: remotes_remote_get_1(struct remote_state *remote_state, const char *name,
     + 		add_url_alias(remote_state, ret, name);
     + 	if (!valid_remote(ret))
     + 		return NULL;
     ++
     ++	validate_remote_url(ret);
      +
     - 	return (!!remote->url) || (!!remote->foreign_vcs);
     + 	return ret;
       }
       
      
     + ## t/t5516-fetch-push.sh ##
     +@@ t/t5516-fetch-push.sh: This test checks the following functionality:
     + * --porcelain output format
     + * hiderefs
     + * reflogs
     ++* URL validation
     + '
     + 
     + GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
     +@@ t/t5516-fetch-push.sh: test_expect_success 'refuse to push a hidden ref, and make sure do not pollute t
     + 	test_dir_is_empty testrepo/.git/objects/pack
     + '
     + 
     ++test_expect_success 'fetch warns or fails when using username:password' '
     ++	message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" &&
     ++	test_must_fail git -c fetch.credentialsInUrl=allow fetch https://username:password@localhost 2>err &&
     ++	! grep "$message" err &&
     ++
     ++	test_must_fail git -c fetch.credentialsInUrl=warn fetch https://username:password@localhost 2>err &&
     ++	grep "warning: $message" err >warnings &&
     ++	test_line_count = 3 warnings &&
     ++
     ++	test_must_fail git -c fetch.credentialsInUrl=die fetch https://username:password@localhost 2>err &&
     ++	grep "fatal: $message" err >warnings &&
     ++	test_line_count = 1 warnings
     ++'
     ++
     ++
     ++test_expect_success 'push warns or fails when using username:password' '
     ++	message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" &&
     ++	test_must_fail git -c fetch.credentialsInUrl=allow push https://username:password@localhost 2>err &&
     ++	! grep "$message" err &&
     ++
     ++	test_must_fail git -c fetch.credentialsInUrl=warn push https://username:password@localhost 2>err &&
     ++	grep "warning: $message" err >warnings &&
     ++	test_must_fail git -c fetch.credentialsInUrl=die push https://username:password@localhost 2>err &&
     ++	grep "fatal: $message" err >warnings &&
     ++	test_line_count = 1 warnings
     ++'
     ++
     + test_done
     +
       ## t/t5601-clone.sh ##
      @@ t/t5601-clone.sh: test_expect_success 'clone respects GIT_WORK_TREE' '
       
       '
       
      +test_expect_success 'clone warns or fails when using username:password' '
     ++	message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" &&
      +	test_must_fail git -c fetch.credentialsInUrl=allow clone https://username:password@localhost attempt1 2>err &&
     -+	! grep "URL '\''https://username:<redacted>@localhost'\'' uses plaintext credentials" err &&
     -+	test_must_fail git -c fetch.credentialsInUrl=warn clone https://username:password@localhost attempt1 2>err &&
     -+	grep "warning: URL '\''https://username:<redacted>@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:<redacted>@localhost'\'' uses plaintext credentials" err
     ++	! grep "$message" err &&
     ++
     ++	test_must_fail git -c fetch.credentialsInUrl=warn clone https://username:password@localhost attempt2 2>err &&
     ++	grep "warning: $message" err >warnings &&
     ++	test_line_count = 2 warnings &&
     ++
     ++	test_must_fail git -c fetch.credentialsInUrl=die clone https://username:password@localhost attempt3 2>err &&
     ++	grep "fatal: $message" err >warnings &&
     ++	test_line_count = 1 warnings
      +'
      +
      +test_expect_success 'clone does not detect username:password when it is https://username@domain:port/' '

2: 8e29ac807c6 < -: ----------- usage: add warn_once() helper for repeated warnings

 Documentation/config/fetch.txt | 14 +++++++++++++
 remote.c                       | 37 ++++++++++++++++++++++++++++++++++
 t/t5516-fetch-push.sh          | 28 +++++++++++++++++++++++++
 t/t5601-clone.sh               | 19 +++++++++++++++++
 4 files changed, 98 insertions(+)

index cd65d236b43..0db7fe85bb8 100644
@@ -96,3 +96,17 @@ 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,
+	including Git storing the URL as plaintext in the repository config.
+	The `fetch.credentialsInUrl` option provides instruction for how Git
+	should react to seeing such a URL, with these values:
++
+* `allow` (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.

index 930fdc9c2f6..1f4a95e9840 100644
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "config.h"
 #include "remote.h"
+#include "urlmatch.h"
 #include "refs.h"
 #include "refspec.h"
 #include "object-store.h"

@@ -614,6 +615,39 @@ const char *remote_ref_for_branch(struct branch *branch, int for_push)
 	return NULL;
 }
 
+static void validate_remote_url(struct remote *remote)
+{
+	int i;
+	const char *value;
+	struct strbuf redacted = STRBUF_INIT;
+
+	if (git_config_get_string_tmp("fetch.credentialsinurl", &value) ||
+	    !strcmp("allow", value))
+		return;
+
+	for (i = 0; i < remote->url_nr; i++) {
+		struct url_info url_info = { NULL };
+		url_normalize(remote->url[i], &url_info);
+
+		if (!url_info.passwd_len)
+			goto loop_cleanup;
+
+		strbuf_add(&redacted, url_info.url, url_info.passwd_off);
+		strbuf_addstr(&redacted, "<redacted>");
+		strbuf_addstr(&redacted, url_info.url + url_info.passwd_off + url_info.passwd_len);
+
+		if (!strcmp("warn", value))
+			warning(_("URL '%s' uses plaintext credentials"), redacted.buf);
+		if (!strcmp("die", value))
+			die(_("URL '%s' uses plaintext credentials"), redacted.buf);
+
+loop_cleanup:
+		free(url_info.url);
+	}
+
+	strbuf_release(&redacted);
+}
+
 static struct remote *
 remotes_remote_get_1(struct remote_state *remote_state, const char *name,
 		     const char *(*get_default)(struct remote_state *,

@@ -639,6 +673,9 @@ remotes_remote_get_1(struct remote_state *remote_state, const char *name,
 		add_url_alias(remote_state, ret, name);
 	if (!valid_remote(ret))
 		return NULL;
+
+	validate_remote_url(ret);
+
 	return ret;
 }
 

index 4dfb080433e..4497617db7f 100755
@@ -12,6 +12,7 @@ This test checks the following functionality:
 * --porcelain output format
 * hiderefs
 * reflogs
+* URL validation
 '
 
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main

@@ -1813,4 +1814,31 @@ test_expect_success 'refuse to push a hidden ref, and make sure do not pollute t
 	test_dir_is_empty testrepo/.git/objects/pack
 '
 
+test_expect_success 'fetch warns or fails when using username:password' '
+	message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" &&
+	test_must_fail git -c fetch.credentialsInUrl=allow fetch https://username:password@localhost 2>err &&
+	! grep "$message" err &&
+
+	test_must_fail git -c fetch.credentialsInUrl=warn fetch https://username:password@localhost 2>err &&
+	grep "warning: $message" err >warnings &&
+	test_line_count = 3 warnings &&
+
+	test_must_fail git -c fetch.credentialsInUrl=die fetch https://username:password@localhost 2>err &&
+	grep "fatal: $message" err >warnings &&
+	test_line_count = 1 warnings
+'
+
+
+test_expect_success 'push warns or fails when using username:password' '
+	message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" &&
+	test_must_fail git -c fetch.credentialsInUrl=allow push https://username:password@localhost 2>err &&
+	! grep "$message" err &&
+
+	test_must_fail git -c fetch.credentialsInUrl=warn push https://username:password@localhost 2>err &&
+	grep "warning: $message" err >warnings &&
+	test_must_fail git -c fetch.credentialsInUrl=die push https://username:password@localhost 2>err &&
+	grep "fatal: $message" err >warnings &&
+	test_line_count = 1 warnings
+'
+
 test_done

index 4a61f2c901e..d03c19f5e07 100755
@@ -71,6 +71,25 @@ test_expect_success 'clone respects GIT_WORK_TREE' '
 
 '
 
+test_expect_success 'clone warns or fails when using username:password' '
+	message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" &&
+	test_must_fail git -c fetch.credentialsInUrl=allow clone https://username:password@localhost attempt1 2>err &&
+	! grep "$message" err &&
+
+	test_must_fail git -c fetch.credentialsInUrl=warn clone https://username:password@localhost attempt2 2>err &&
+	grep "warning: $message" err >warnings &&
+	test_line_count = 2 warnings &&
+
+	test_must_fail git -c fetch.credentialsInUrl=die clone https://username:password@localhost attempt3 2>err &&
+	grep "fatal: $message" err >warnings &&
+	test_line_count = 1 warnings
+'
+
+test_expect_success 'clone does not detect username:password when it is https://username@domain:port/' '
+	test_must_fail git -c fetch.credentialsInUrl=warn clone https://username@localhost:8080 attempt3 2>err &&
+	! grep "uses plaintext credentials" err
+'
+
 test_expect_success 'clone from hooks' '
 
 	test_create_repo r0 &&

base-commit: 2668e3608e47494f2f10ef2b6e69f08a84816bcb

gitgitgadget

Ah, the error message is based on pre-normalization which may very well be different, as that is the whole point of normalization ;-)

We'd redact password part in the error message anyway, so it may not be a huge deal.

On 6/2/2022 5:20 PM, Junio C Hamano wrote:
...

I cut all of your commentary because it was universally good and the fixup you provided does a great job of solving those issues.

Please give yourself co-authorship on the squashed commit.

Thanks,
-Stolee

From: Derrick Stolee <[email protected]>

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 "allow" 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).

An earlier version of this change injected the logic into url_normalize() in urlmatch.c. While most code paths that parse URLs eventually normalize the URL, that normalization does not happen early enough in the stack to avoid attempting connections to the URL first. By inserting a check into the remote validation, we identify the issue before making a connection. In the old code path, this was revealed by testing the new t5601-clone.sh test under --stress, resulting in an instance where the return code was 13 (SIGPIPE) instead of 128 from the die().

However, we can reuse the parsing information from url_normalize() in order to benefit from its well-worn parsing logic. We can use the struct url_info that is created in that method to replace the password with "<redacted>" in our error messages. This comes with a slight downside that the normalized URL might look slightly different from the input URL (for instance, the normalized version adds a closing slash). This should not hinder users figuring out what the problem is and being able to fix the issue.

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.sh and t5541-http-push-smart.sh. This means that all other tested URLs did not trigger this logic.

The tests show that the proper error messages appear (or do not appear), but also count the number of error messages. When only warning, each process validates the remote URL and outputs a warning. This happens twice for clone, three times for fetch, and once for push.

---
remote: create fetch.credentialsInUrl config

Users can specify credentials in their URL using the username:password@domain format. This is potentially hazardous since the URL is stored in plaintext and can also appear in trace2 logs and other places. Add a new config option that allows warnings or failures when discovering URLs with this format. The default behavior does not change in this series, although we may want to move to the warn state by default in the future.

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/[email protected]
Reject passwords in URLs (April 2021).

[2]
https://lore.kernel.org/git/[email protected]/
Re: Git ransom campaign incident report - May 2019

Updates in v5
=============

* Squashed in Junio's recommended changes.
* Noticed that the redacted strbuf wasn't reset between loop iterations, which only matters if there are multiple URLs for the remote.

Updates in v4
=============

* The warn_once() patch is dropped in favor of using a different location for the check (remotes_remote_get_1() instead of valid_remote()).
* The parsing logic is removed in favor of using the output url_info from url_normalize().
* Tests for 'fetch' and 'push' are added.
* This requires updating the topic to be on a more-recent version of 'master' because some change since the previous base caused 'git push' to output the warning only once, not twice.

Updates in v3
=============

* Because of some flaky behavior around SIGPIPE, the URL check needed to move to be earlier in the command.
* For this reason, I moved the cred check into remote.c's valid_remote() check. This changed the previous BUG() statements into early returns.
* I repeated the check with the test suite to see if this parsing fails on any existing cases, but it is worth double-checking the parsing rules.
* Documentation is more consistent about using placeholders.
* A test for the "allow" case is now included.
* A new patch is added that creates the warn_once() helper. This reduces multiple advisory warnings with the same text from being written by the same process.

Updates in v2
=============

* Documentation is slightly expanded to include the fact that Git stores the given URL as plaintext in its config.
* The new method has a new documentation comment that details the necessary preconditions.
* "ignore" is now "allow"
* Additional checks on colon_ptr are added.
* Use strbuf_splice() instead of custom string-walking logic.
* Use "" instead of asterisks.
* Config value checks are no longer case sensitive.

Thanks, -Stolee

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1237%2Fderrickstolee%2Fcreds-in-url-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1237/derrickstolee/creds-in-url-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/1237

Range-diff vs v4:

1: 5fb038402d6 ! 1: b744a9887e5 remote: create fetch.credentialsInUrl config

     @@ Commit message
          each process validates the remote URL and outputs a warning. This
          happens twice for clone, three times for fetch, and once for push.
      
     +    Co-authored-by: Junio C Hamano <[email protected]>
     +    Signed-off-by: Junio C Hamano <[email protected]>
          Signed-off-by: Derrick Stolee <[email protected]>
      
       ## Documentation/config/fetch.txt ##
     @@ remote.c: const char *remote_ref_for_branch(struct branch *branch, int for_push)
      +	int i;
      +	const char *value;
      +	struct strbuf redacted = STRBUF_INIT;
     ++	int warn_not_die;
      +
     -+	if (git_config_get_string_tmp("fetch.credentialsinurl", &value) ||
     -+	    !strcmp("allow", value))
     ++	if (git_config_get_string_tmp("fetch.credentialsinurl", &value))
      +		return;
      +
     ++	if (!strcmp("warn", value))
     ++		warn_not_die = 1;
     ++	else if (!strcmp("die", value))
     ++		warn_not_die = 0;
     ++	else if (!strcmp("allow", value))
     ++		return;
     ++	else
     ++		die(_("unrecognized value fetch.credentialsInURL: '%s'"), value);
     ++
      +	for (i = 0; i < remote->url_nr; i++) {
     -+		struct url_info url_info = { NULL };
     -+		url_normalize(remote->url[i], &url_info);
     ++		struct url_info url_info = { 0 };
      +
     -+		if (!url_info.passwd_len)
     ++		if (!url_normalize(remote->url[i], &url_info) ||
     ++		    !url_info.passwd_off)
      +			goto loop_cleanup;
      +
     ++		strbuf_reset(&redacted);
      +		strbuf_add(&redacted, url_info.url, url_info.passwd_off);
      +		strbuf_addstr(&redacted, "<redacted>");
     -+		strbuf_addstr(&redacted, url_info.url + url_info.passwd_off + url_info.passwd_len);
     ++		strbuf_addstr(&redacted,
     ++			      url_info.url + url_info.passwd_off + url_info.passwd_len);
      +
     -+		if (!strcmp("warn", value))
     ++		if (warn_not_die)
      +			warning(_("URL '%s' uses plaintext credentials"), redacted.buf);
     -+		if (!strcmp("die", value))
     ++		else
      +			die(_("URL '%s' uses plaintext credentials"), redacted.buf);
      +
      +loop_cleanup:
     @@ t/t5516-fetch-push.sh: test_expect_success 'refuse to push a hidden ref, and mak
      +
      +	test_must_fail git -c fetch.credentialsInUrl=die fetch https://username:password@localhost 2>err &&
      +	grep "fatal: $message" err >warnings &&
     ++	test_line_count = 1 warnings &&
     ++
     ++	test_must_fail git -c fetch.credentialsInUrl=die fetch https://username:@localhost 2>err &&
     ++	grep "fatal: $message" err >warnings &&
      +	test_line_count = 1 warnings
      +'
      +
     @@ t/t5601-clone.sh: test_expect_success 'clone respects GIT_WORK_TREE' '
      +
      +	test_must_fail git -c fetch.credentialsInUrl=die clone https://username:password@localhost attempt3 2>err &&
      +	grep "fatal: $message" err >warnings &&
     ++	test_line_count = 1 warnings &&
     ++
     ++	test_must_fail git -c fetch.credentialsInUrl=die clone https://username:@localhost attempt3 2>err &&
     ++	grep "fatal: $message" err >warnings &&
      +	test_line_count = 1 warnings
      +'
      +

 Documentation/config/fetch.txt | 14 ++++++++++
 remote.c                       | 48 ++++++++++++++++++++++++++++++++++
 t/t5516-fetch-push.sh          | 32 +++++++++++++++++++++++
 t/t5601-clone.sh               | 23 ++++++++++++++++
 4 files changed, 117 insertions(+)

index cd65d236b43..0db7fe85bb8 100644
@@ -96,3 +96,17 @@ 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,
+	including Git storing the URL as plaintext in the repository config.
+	The `fetch.credentialsInUrl` option provides instruction for how Git
+	should react to seeing such a URL, with these values:
++
+* `allow` (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.

index 4dfb080433e..dc6cb3cbf5d 100755
@@ -12,6 +12,7 @@ This test checks the following functionality:
 * --porcelain output format
 * hiderefs
 * reflogs
+* URL validation
 '
 
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main

@@ -1813,4 +1814,35 @@ test_expect_success 'refuse to push a hidden ref, and make sure do not pollute t
 	test_dir_is_empty testrepo/.git/objects/pack
 '
 
+test_expect_success 'fetch warns or fails when using username:password' '
+	message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" &&
+	test_must_fail git -c fetch.credentialsInUrl=allow fetch https://username:password@localhost 2>err &&
+	! grep "$message" err &&
+
+	test_must_fail git -c fetch.credentialsInUrl=warn fetch https://username:password@localhost 2>err &&
+	grep "warning: $message" err >warnings &&
+	test_line_count = 3 warnings &&
+
+	test_must_fail git -c fetch.credentialsInUrl=die fetch https://username:password@localhost 2>err &&
+	grep "fatal: $message" err >warnings &&
+	test_line_count = 1 warnings &&
+
+	test_must_fail git -c fetch.credentialsInUrl=die fetch https://username:@localhost 2>err &&
+	grep "fatal: $message" err >warnings &&
+	test_line_count = 1 warnings
+'
+
+
+test_expect_success 'push warns or fails when using username:password' '
+	message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" &&
+	test_must_fail git -c fetch.credentialsInUrl=allow push https://username:password@localhost 2>err &&
+	! grep "$message" err &&
+
+	test_must_fail git -c fetch.credentialsInUrl=warn push https://username:password@localhost 2>err &&
+	grep "warning: $message" err >warnings &&
+	test_must_fail git -c fetch.credentialsInUrl=die push https://username:password@localhost 2>err &&
+	grep "fatal: $message" err >warnings &&
+	test_line_count = 1 warnings
+'
+
 test_done

index 4a61f2c901e..d2f046b4b92 100755
@@ -71,6 +71,29 @@ test_expect_success 'clone respects GIT_WORK_TREE' '
 
 '
 
+test_expect_success 'clone warns or fails when using username:password' '
+	message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" &&
+	test_must_fail git -c fetch.credentialsInUrl=allow clone https://username:password@localhost attempt1 2>err &&
+	! grep "$message" err &&
+
+	test_must_fail git -c fetch.credentialsInUrl=warn clone https://username:password@localhost attempt2 2>err &&
+	grep "warning: $message" err >warnings &&
+	test_line_count = 2 warnings &&
+
+	test_must_fail git -c fetch.credentialsInUrl=die clone https://username:password@localhost attempt3 2>err &&
+	grep "fatal: $message" err >warnings &&
+	test_line_count = 1 warnings &&
+
+	test_must_fail git -c fetch.credentialsInUrl=die clone https://username:@localhost attempt3 2>err &&
+	grep "fatal: $message" err >warnings &&
+	test_line_count = 1 warnings
+'
+
+test_expect_success 'clone does not detect username:password when it is https://username@domain:port/' '
+	test_must_fail git -c fetch.credentialsInUrl=warn clone https://username@localhost:8080 attempt3 2>err &&
+	! grep "uses plaintext credentials" err
+'
+
 test_expect_success 'clone from hooks' '
 
 	test_create_repo r0 &&

base-commit: 2668e3608e47494f2f10ef2b6e69f08a84816bcb

gitgitgadget

Heh, that's at most helped-by; a small bugfix in the logic (zero-length) and all others are minor clean-ups.

Thanks.

Thanks, I missed that.

Will queue.