remote: create fetch.credentialsInUrl config #1237
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
derrickstolee
force-pushed
the
creds-in-url
branch
2 times, most recently
from
254c11d
to
5145dc2
Compare
derrickstolee
force-pushed
the
creds-in-url
branch
from
5145dc2
to
cc2befb
Compare
|
/submit |
|
Submitted as pull.1237.git.1653329044940.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 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.
The issue sounds vaguely familiar---I must have heard something
similar on this list not in too distant past.
> 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).
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.
> ... 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.
We are not testing the form we are not encouraging, in other words ;-).
> 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.
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.
> +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.
Sounds sensible (modulo I would suggest "ignore" -> "pass").
> + grep "warning: URL '\''https://username:\*\*\*\*\*\*\*\*@localhost/'\'' uses plaintext credentials" err &&
Makes sure that the password part is redacted, which is good.
> + 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
Ditto.
> diff --git a/urlmatch.c b/urlmatch.c
> index b615adc923a..6b91fb648a7 100644
> --- a/urlmatch.c
> +++ b/urlmatch.c
> @@ -1,5 +1,6 @@
> #include "cache.h"
> #include "urlmatch.h"
> +#include "config.h"
Yuck. Having to do config lookups at this deep a level in the
callchain does not look too attractive to me.
I am wondering if we can make it the responsibility of the callers
to figure out and pass down the settings of the new configuration
variable.
Offhand I do not think of an easy and clean way to do so (well,
"easy" is easy---add one to the list of globals in environment.c;
"clean" is the harder part).
> #define URL_ALPHA "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
> #define URL_DIGIT "0123456789"
> @@ -106,6 +107,46 @@ static int match_host(const struct url_info *url_info,
> return (!url_len && !pat_len);
> }
>
> +static void detected_credentials_in_url(const char *url, size_t scheme_len)
> +{
> + char *value = NULL;
> + const char *at_ptr;
> + const char *colon_ptr;
> + struct strbuf anonymized = STRBUF_INIT;
> +
> + /* "ignore" is the default behavior. */
> + if (git_config_get_string("fetch.credentialsinurl", &value) ||
> + !strcasecmp("ignore", value))
> + goto cleanup;
> +
> + at_ptr = strchr(url, '@');
> + colon_ptr = strchr(url + scheme_len + 3, ':');
We expect that at_ptr would come after colon_ptr (i.e. in
"scheme://<u>:<p>@host", no @ exists in <u> or <p> part) and the
while() loop below assumes that for redacting. Are we better off if
we assert it here, or has the calling parser already rejected such
cases?
> + if (!colon_ptr)
> + BUG("failed to find colon in url '%s' with scheme_len %"PRIuMAX,
> + url, (uintmax_t) scheme_len);
> +
> + /* Include everything including the colon. */
> + colon_ptr++;
> + strbuf_add(&anonymized, url, colon_ptr - url);
> +
> + while (colon_ptr < at_ptr) {
> + strbuf_addch(&anonymized, '*');
> + colon_ptr++;
> + }
> +
> + strbuf_addstr(&anonymized, at_ptr);
> +
> + if (!strcasecmp("warn", value))
> + warning(_("URL '%s' uses plaintext credentials"), anonymized.buf);
> + if (!strcasecmp("die", value))
> + die(_("URL '%s' uses plaintext credentials"), anonymized.buf);
> +
> +cleanup:
> + free(value);
> + strbuf_release(&anonymized);
> +}
> +
So far, looking good.
> @@ -144,6 +185,7 @@ static char *url_normalize_1(const char *url, struct url_info *out_info, char al
> */
>
> size_t url_len = strlen(url);
> + const char *orig_url = url;
> struct strbuf norm;
> size_t spanned;
> size_t scheme_len, user_off=0, user_len=0, passwd_off=0, passwd_len=0;
> @@ -191,6 +233,7 @@ static char *url_normalize_1(const char *url, struct url_info *out_info, char al
> }
> colon_ptr = strchr(norm.buf + scheme_len + 3, ':');
> if (colon_ptr) {
> + detected_credentials_in_url(orig_url, scheme_len);
> passwd_off = (colon_ptr + 1) - norm.buf;
> passwd_len = norm.len - passwd_off;
> user_len = (passwd_off - 1) - (scheme_len + 3);
>
> base-commit: f9b95943b68b6b8ca5a6072f50a08411c6449b55
Thanks. |
|
On the Git mailing list, Derrick Stolee wrote (reply to this): On 5/23/2022 3:06 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> 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.
>
> The issue sounds vaguely familiar---I must have heard something
> similar on this list not in too distant past.
It certainly felt like not too distant, but [1] was over a year ago!
>> 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).
>
> It sounds more like "pass" than "ignore", the latter of which can be
> read as "strip" instead of "pass it as-is".
Perhaps "allow" would be more clear than all of these options?
> The name "warn", and its stronger form "die", both sound right.
>
>> ... 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.
>
> We are not testing the form we are not encouraging, in other words ;-).
Right, but in addition we are hopefully testing most of the forms we
_do_ encourage, and without triggering these warnings.
>> 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.
>
> 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.
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).
> 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 agree that these are mostly orthogonal. I think that the parsing
logic in urlmatch.c would be involved in the
> 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.
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.
>> diff --git a/urlmatch.c b/urlmatch.c
>> index b615adc923a..6b91fb648a7 100644
>> --- a/urlmatch.c
>> +++ b/urlmatch.c
>> @@ -1,5 +1,6 @@
>> #include "cache.h"
>> #include "urlmatch.h"
>> +#include "config.h"
>
> Yuck. Having to do config lookups at this deep a level in the
> callchain does not look too attractive to me.
>
> I am wondering if we can make it the responsibility of the callers
> to figure out and pass down the settings of the new configuration
> variable.
>
> Offhand I do not think of an easy and clean way to do so (well,
> "easy" is easy---add one to the list of globals in environment.c;
> "clean" is the harder part).
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?
>> #define URL_ALPHA "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
>> #define URL_DIGIT "0123456789"
>> @@ -106,6 +107,46 @@ static int match_host(const struct url_info *url_info,
>> return (!url_len && !pat_len);
>> }
>>
>> +static void detected_credentials_in_url(const char *url, size_t scheme_len)
>> +{
>> + char *value = NULL;
>> + const char *at_ptr;
>> + const char *colon_ptr;
>> + struct strbuf anonymized = STRBUF_INIT;
>> +
>> + /* "ignore" is the default behavior. */
>> + if (git_config_get_string("fetch.credentialsinurl", &value) ||
>> + !strcasecmp("ignore", value))
>> + goto cleanup;
>> +
>> + at_ptr = strchr(url, '@');
>> + colon_ptr = strchr(url + scheme_len + 3, ':');
>
> We expect that at_ptr would come after colon_ptr (i.e. in
> "scheme://<u>:<p>@host", no @ exists in <u> or <p> part) and the
> while() loop below assumes that for redacting. Are we better off if
> we assert it here, or has the calling parser already rejected such
> cases?
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 |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Junio C Hamano <gitster@pobox.com> writes:
> It sounds more like "pass" than "ignore", the latter of which can be
> read as "strip" instead of "pass it as-is".
Or "allow" (which I prefer over "pass").
> The name "warn", and its stronger form "die", both sound right. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Derrick Stolee <derrickstolee@github.com> writes:
> 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.
OK.
> That does mean that the password cannot contain an "@"
> symbol (unless it is special-cased somehow?).
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. |
|
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this): On Mon, May 23 2022, Derrick Stolee via GitGitGadget wrote:
> +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:
Re the previous discussion about this (in the v1 patch /
https://lore.kernel.org/git/pull.945.git.1619807844627.gitgitgadget@gmail.com/):
In what ways?
That's rhetorical, the point being: Let's adjust this documentation to
discuss exactly why this is thought to be bad, what we're mitigating for
the user etc., are there situations where running git like this is
perfectly fine & not thought to be an issue? E.g. no password manager
and you trust your FS permission? Let's cover those cases too.
> ++
> +* `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.
You're implementing this with strcasecmp, so we also support DIE, DiE
etc. Let's not do that and use strcmp() instead.
> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index 4a61f2c901e..34be520b783 100755
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh
> @@ -71,6 +71,13 @@ test_expect_success 'clone respects GIT_WORK_TREE' '
>
> '
>
> +test_expect_success 'clone warns or fails when using username:password' '
> + test_must_fail git -c fetch.credentialsInUrl=warn clone https://username:password@localhost attempt1 2>err &&
> + grep "warning: URL '\''https://username:\*\*\*\*\*\*\*\*@localhost/'\'' uses plaintext credentials" err &&
> + test_must_fail git -c fetch.credentialsInUrl=die clone https://username:password@localhost attempt2 2>err &&
> + grep "fatal: URL '\''https://username:\*\*\*\*\*\*\*\*@localhost/'\'' uses plaintext credentials" err
> +'
> +
> test_expect_success 'clone from hooks' '
>
> test_create_repo r0 &&
> diff --git a/urlmatch.c b/urlmatch.c
> index b615adc923a..6b91fb648a7 100644
> --- a/urlmatch.c
> +++ b/urlmatch.c
> @@ -1,5 +1,6 @@
> #include "cache.h"
> #include "urlmatch.h"
> +#include "config.h"
>
> #define URL_ALPHA "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
> #define URL_DIGIT "0123456789"
> @@ -106,6 +107,46 @@ static int match_host(const struct url_info *url_info,
> return (!url_len && !pat_len);
> }
>
> +static void detected_credentials_in_url(const char *url, size_t scheme_len)
> +{
Just generally: This is only
> + char *value = NULL;
This init to NULL should be removedd, as we....
> + const char *at_ptr;
> + const char *colon_ptr;
> + struct strbuf anonymized = STRBUF_INIT;
nit: Just call this "sb"? The's at least one line below over 79
characters that's within the bounds with a shorter variable name, and in
this case it's obvious what we're doing here...
> +
> + /* "ignore" is the default behavior. */
> + if (git_config_get_string("fetch.credentialsinurl", &value) ||
...initialize it here, and if we didn't the compiler would have a chance
to spot that if we were getting it wrong.
> + !strcasecmp("ignore", value))
> + goto cleanup;
> +
> + at_ptr = strchr(url, '@');
> + colon_ptr = strchr(url + scheme_len + 3, ':');
> +
> + if (!colon_ptr)
> + BUG("failed to find colon in url '%s' with scheme_len %"PRIuMAX,
> + url, (uintmax_t) scheme_len);
> +
> + /* Include everything including the colon. */
> + colon_ptr++;
> + strbuf_add(&anonymized, url, colon_ptr - url);
> +
> + while (colon_ptr < at_ptr) {
> + strbuf_addch(&anonymized, '*');
> + colon_ptr++;
> + }
Could we share code with 88e9b1e3fcb (fetch-pack: redact packfile urls
in traces, 2021-11-10), or for consistency note this as <redacted>
instead of stripping it out, as we do for that related URL-part
redaction?
> + strbuf_addstr(&anonymized, at_ptr);
Maybe not worth it, but I wondered if we couldn't just use curl for
this, turns out it has an API for it:
https://curl.se/libcurl/c/libcurl-url.html
But it's too new for us to rely on unconditionally, but we could add
that to git-curl-compat.h and ifdef it, then we'll eventually drop this
custom code for ryling on the well-tested library.
I think doing that would be worth it, to show future authors that curl
can do this, so maybe we can start relying on that eventually...
> + if (!strcasecmp("warn", value))
> + warning(_("URL '%s' uses plaintext credentials"), anonymized.buf);
> + if (!strcasecmp("die", value))
> + die(_("URL '%s' uses plaintext credentials"), anonymized.buf);
> +
> +cleanup:
> + free(value);
I think you can also just use git_config_get_string_tmp() here and avoid
the alloc/free. That's safe as long as you're not calling other config
API in-between, which you're not. |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): Hi Stolee,
On Mon, 23 May 2022, Derrick Stolee via GitGitGadget wrote:
> diff --git a/urlmatch.c b/urlmatch.c
> index b615adc923a..6b91fb648a7 100644
> --- a/urlmatch.c
> +++ b/urlmatch.c
> +static void detected_credentials_in_url(const char *url, size_t scheme_len)
> +{
> + char *value = NULL;
> + const char *at_ptr;
> + const char *colon_ptr;
> + struct strbuf anonymized = STRBUF_INIT;
> +
> + /* "ignore" is the default behavior. */
> + if (git_config_get_string("fetch.credentialsinurl", &value) ||
> + !strcasecmp("ignore", value))
> + goto cleanup;
> +
> + at_ptr = strchr(url, '@');
> + colon_ptr = strchr(url + scheme_len + 3, ':');
How certain are we that `url + scheme_len + 3` is still inside the
NUL-separated `url`?
> +
> + if (!colon_ptr)
> + BUG("failed to find colon in url '%s' with scheme_len %"PRIuMAX,
> + url, (uintmax_t) scheme_len);
Wouldn't this mean that `https://github.com/git/git` with a `scheme_len`
of 5 would hit that `BUG()` code path?
Thanks,
Dscho
> +
> + /* Include everything including the colon. */
> + colon_ptr++;
> + strbuf_add(&anonymized, url, colon_ptr - url);
> +
> + while (colon_ptr < at_ptr) {
> + strbuf_addch(&anonymized, '*');
> + colon_ptr++;
> + }
> +
> + strbuf_addstr(&anonymized, at_ptr);
> +
> + if (!strcasecmp("warn", value))
> + warning(_("URL '%s' uses plaintext credentials"), anonymized.buf);
> + if (!strcasecmp("die", value))
> + die(_("URL '%s' uses plaintext credentials"), anonymized.buf);
> +
> +cleanup:
> + free(value);
> + strbuf_release(&anonymized);
> +}
> +
> static char *url_normalize_1(const char *url, struct url_info *out_info, char allow_globs)
> {
> /* |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): Hi Stolee,
On Mon, 23 May 2022, Derrick Stolee wrote:
> On 5/23/2022 3:06 PM, Junio C Hamano wrote:
> > "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >
> >> +static void detected_credentials_in_url(const char *url, size_t scheme_len)
> >> +{
> >> + char *value = NULL;
> >> + const char *at_ptr;
> >> + const char *colon_ptr;
> >> + struct strbuf anonymized = STRBUF_INIT;
> >> +
> >> + /* "ignore" is the default behavior. */
> >> + if (git_config_get_string("fetch.credentialsinurl", &value) ||
> >> + !strcasecmp("ignore", value))
> >> + goto cleanup;
> >> +
> >> + at_ptr = strchr(url, '@');
> >> + colon_ptr = strchr(url + scheme_len + 3, ':');
> >
> > We expect that at_ptr would come after colon_ptr (i.e. in
> > "scheme://<u>:<p>@host", no @ exists in <u> or <p> part) and the
> > while() loop below assumes that for redacting.
Careful here. https://me@there.com:9999/ _is_ a valid URL, too.
> > Are we better off if we assert it here, or has the calling parser
> > already rejected such cases?
>
> 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?).
The password cannot contain a literal `@`, and neither can the user name.
They have to be URL-encoded, via `%40`.
Ciao,
Dscho |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): Hi Junio,
On Mon, 23 May 2022, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > 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.
>
> 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.
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 |
|
On the Git mailing list, Derrick Stolee wrote (reply to this): On 5/24/2022 4:18 AM, Ævar Arnfjörð Bjarmason wrote:
>
> On Mon, May 23 2022, Derrick Stolee via GitGitGadget wrote:
>
>> +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:
>
> Re the previous discussion about this (in the v1 patch /
> https://lore.kernel.org/git/pull.945.git.1619807844627.gitgitgadget@gmail.com/):
> In what ways?
>
> That's rhetorical, the point being: Let's adjust this documentation to
> discuss exactly why this is thought to be bad, what we're mitigating for
> the user etc., are there situations where running git like this is
> perfectly fine & not thought to be an issue? E.g. no password manager
> and you trust your FS permission? Let's cover those cases too.
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.
>> ++
>> +* `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.
>
> You're implementing this with strcasecmp, so we also support DIE, DiE
> etc. Let's not do that and use strcmp() instead.
Sure.
>> +static void detected_credentials_in_url(const char *url, size_t scheme_len)
>> +{
>
> Just generally: This is only
Did you intend to say more here?
>> + char *value = NULL;
>
> This init to NULL should be removedd, as we....
>
>> + const char *at_ptr;
>> + const char *colon_ptr;
>> + struct strbuf anonymized = STRBUF_INIT;
>
> nit: Just call this "sb"? The's at least one line below over 79
> characters that's within the bounds with a shorter variable name, and in
> this case it's obvious what we're doing here...
I will not change this name to be less descriptive.
>> +
>> + /* "ignore" is the default behavior. */
>> + if (git_config_get_string("fetch.credentialsinurl", &value) ||
>
> ...initialize it here, and if we didn't the compiler would have a chance
> to spot that if we were getting it wrong.
We do not necessarily initialize it here. The compiler doesn't notice
it and the free(value) below segfaults.
>> + !strcasecmp("ignore", value))
>> + goto cleanup;
>> +
>> + at_ptr = strchr(url, '@');
>> + colon_ptr = strchr(url + scheme_len + 3, ':');
>> +
>> + if (!colon_ptr)
>> + BUG("failed to find colon in url '%s' with scheme_len %"PRIuMAX,
>> + url, (uintmax_t) scheme_len);
>> +
>> + /* Include everything including the colon. */
>> + colon_ptr++;
>> + strbuf_add(&anonymized, url, colon_ptr - url);
>> +
>> + while (colon_ptr < at_ptr) {
>> + strbuf_addch(&anonymized, '*');
>> + colon_ptr++;
>> + }
>
> Could we share code with 88e9b1e3fcb (fetch-pack: redact packfile urls
> in traces, 2021-11-10), or for consistency note this as <redacted>
> instead of stripping it out, as we do for that related URL-part
> redaction?
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).
>> + strbuf_addstr(&anonymized, at_ptr);
>
> Maybe not worth it, but I wondered if we couldn't just use curl for
> this, turns out it has an API for it:
> https://curl.se/libcurl/c/libcurl-url.html
>
> But it's too new for us to rely on unconditionally, but we could add
> that to git-curl-compat.h and ifdef it, then we'll eventually drop this
> custom code for ryling on the well-tested library.
>
> I think doing that would be worth it, to show future authors that curl
> can do this, so maybe we can start relying on that eventually...
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.
>> + if (!strcasecmp("warn", value))
>> + warning(_("URL '%s' uses plaintext credentials"), anonymized.buf);
>> + if (!strcasecmp("die", value))
>> + die(_("URL '%s' uses plaintext credentials"), anonymized.buf);
>> +
>> +cleanup:
>> + free(value);
>
> I think you can also just use git_config_get_string_tmp() here and avoid
> the alloc/free. That's safe as long as you're not calling other config
> API in-between, which you're not.
OK. And that also avoids the need for initialization you mentioned.
Thanks,
-Stolee |
|
On the Git mailing list, Derrick Stolee wrote (reply to this): On 5/24/2022 7:46 AM, Johannes Schindelin wrote:
> Hi Stolee,
>
> On Mon, 23 May 2022, Derrick Stolee wrote:
>
>> On 5/23/2022 3:06 PM, Junio C Hamano wrote:
>>> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>>
>>>> +static void detected_credentials_in_url(const char *url, size_t scheme_len)
>>>> +{
>>>> + char *value = NULL;
>>>> + const char *at_ptr;
>>>> + const char *colon_ptr;
>>>> + struct strbuf anonymized = STRBUF_INIT;
>>>> +
>>>> + /* "ignore" is the default behavior. */
>>>> + if (git_config_get_string("fetch.credentialsinurl", &value) ||
>>>> + !strcasecmp("ignore", value))
>>>> + goto cleanup;
>>>> +
>>>> + at_ptr = strchr(url, '@');
>>>> + colon_ptr = strchr(url + scheme_len + 3, ':');
>>>
>>> We expect that at_ptr would come after colon_ptr (i.e. in
>>> "scheme://<u>:<p>@host", no @ exists in <u> or <p> part) and the
>>> while() loop below assumes that for redacting.
>
> Careful here. https://me@there.com:9999/ _is_ a valid URL, too.
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.
>>> Are we better off if we assert it here, or has the calling parser
>>> already rejected such cases?
>>
>> 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?).
>
> The password cannot contain a literal `@`, and neither can the user name.
> They have to be URL-encoded, via `%40`.
Thanks!
-Stolee |
|
On the Git mailing list, Derrick Stolee wrote (reply to this): On 5/24/2022 7:42 AM, Johannes Schindelin wrote:
> Hi Stolee,
>
> On Mon, 23 May 2022, Derrick Stolee via GitGitGadget wrote:
>> + at_ptr = strchr(url, '@');
>> + colon_ptr = strchr(url + scheme_len + 3, ':');
>
> How certain are we that `url + scheme_len + 3` is still inside the
> NUL-separated `url`?
I'll update the method comment to make this clear.
>> +
>> + if (!colon_ptr)
>> + BUG("failed to find colon in url '%s' with scheme_len %"PRIuMAX,
>> + url, (uintmax_t) scheme_len);
>
> Wouldn't this mean that `https://github.com/git/git` with a `scheme_len`
> of 5 would hit that `BUG()` code path?
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 |
|
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this): On Tue, May 24 2022, Derrick Stolee wrote:
> On 5/24/2022 4:18 AM, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Mon, May 23 2022, Derrick Stolee via GitGitGadget wrote:
>>
>>> +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:
>>
>> Re the previous discussion about this (in the v1 patch /
>> https://lore.kernel.org/git/pull.945.git.1619807844627.gitgitgadget@gmail.com/):
>> In what ways?
>>
>> That's rhetorical, the point being: Let's adjust this documentation to
>> discuss exactly why this is thought to be bad, what we're mitigating for
>> the user etc., are there situations where running git like this is
>> perfectly fine & not thought to be an issue? E.g. no password manager
>> and you trust your FS permission? Let's cover those cases too.
>
> 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.
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).
>>> ++
>>> +* `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.
>>
>> You're implementing this with strcasecmp, so we also support DIE, DiE
>> etc. Let's not do that and use strcmp() instead.
>
> Sure.
>
>>> +static void detected_credentials_in_url(const char *url, size_t scheme_len)
>>> +{
>>
>> Just generally: This is only
>
> Did you intend to say more here?
Probably, but if I did I forgot about it, by now. Sorry.
>>> + char *value = NULL;
>>
>> This init to NULL should be removedd, as we....
>>
>>> + const char *at_ptr;
>>> + const char *colon_ptr;
>>> + struct strbuf anonymized = STRBUF_INIT;
>>
>> nit: Just call this "sb"? The's at least one line below over 79
>> characters that's within the bounds with a shorter variable name, and in
>> this case it's obvious what we're doing here...
>
> I will not change this name to be less descriptive.
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?
>>> +
>>> + /* "ignore" is the default behavior. */
>>> + if (git_config_get_string("fetch.credentialsinurl", &value) ||
>>
>> ...initialize it here, and if we didn't the compiler would have a chance
>> to spot that if we were getting it wrong.
>
> We do not necessarily initialize it here. The compiler doesn't notice
> it and the free(value) below segfaults.
Yes, sorry I meant in combination with the *_tmp() variant below...
>>> + !strcasecmp("ignore", value))
>>> + goto cleanup;
>>> +
>>> + at_ptr = strchr(url, '@');
>>> + colon_ptr = strchr(url + scheme_len + 3, ':');
>>> +
>>> + if (!colon_ptr)
>>> + BUG("failed to find colon in url '%s' with scheme_len %"PRIuMAX,
>>> + url, (uintmax_t) scheme_len);
>>> +
>>> + /* Include everything including the colon. */
>>> + colon_ptr++;
>>> + strbuf_add(&anonymized, url, colon_ptr - url);
>>> +
>>> + while (colon_ptr < at_ptr) {
>>> + strbuf_addch(&anonymized, '*');
>>> + colon_ptr++;
>>> + }
>>
>> Could we share code with 88e9b1e3fcb (fetch-pack: redact packfile urls
>> in traces, 2021-11-10), or for consistency note this as <redacted>
>> instead of stripping it out, as we do for that related URL-part
>> redaction?
>
> I'm happy to replace the asterisks with <redacted>. Otherwise, I don't
> see anything we can do to share across these methods.
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.
> Specifically,
> the test in that commit seems to indicate that the redacted portion is
> only the packfile name (the $HTTPD_URL is not filtered).
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.
>>> + strbuf_addstr(&anonymized, at_ptr);
>>
>> Maybe not worth it, but I wondered if we couldn't just use curl for
>> this, turns out it has an API for it:
>> https://curl.se/libcurl/c/libcurl-url.html
>>
>> But it's too new for us to rely on unconditionally, but we could add
>> that to git-curl-compat.h and ifdef it, then we'll eventually drop this
>> custom code for ryling on the well-tested library.
>>
>> I think doing that would be worth it, to show future authors that curl
>> can do this, so maybe we can start relying on that eventually...
>
> 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.
Yeah, probably not.
>>> + if (!strcasecmp("warn", value))
>>> + warning(_("URL '%s' uses plaintext credentials"), anonymized.buf);
>>> + if (!strcasecmp("die", value))
>>> + die(_("URL '%s' uses plaintext credentials"), anonymized.buf);
>>> +
>>> +cleanup:
>>> + free(value);
>>
>> I think you can also just use git_config_get_string_tmp() here and avoid
>> the alloc/free. That's safe as long as you're not calling other config
>> API in-between, which you're not.
>
> OK. And that also avoids the need for initialization you mentioned.
*nod* |
derrickstolee
force-pushed
the
creds-in-url
branch
from
cc2befb
to
364f5c3
Compare
|
On the Git mailing list, Derrick Stolee wrote (reply to this): On 5/24/2022 5:01 PM, Ævar Arnfjörð Bjarmason wrote:
>
> On Tue, May 24 2022, Derrick Stolee wrote:
>
>> On 5/24/2022 4:18 AM, Ævar Arnfjörð Bjarmason wrote:
>>>
>>> On Mon, May 23 2022, Derrick Stolee via GitGitGadget wrote:
>>>
>>>> +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:
>>>
>>> Re the previous discussion about this (in the v1 patch /
>>> https://lore.kernel.org/git/pull.945.git.1619807844627.gitgitgadget@gmail.com/):
>>> In what ways?
>>>
>>> That's rhetorical, the point being: Let's adjust this documentation to
>>> discuss exactly why this is thought to be bad, what we're mitigating for
>>> the user etc., are there situations where running git like this is
>>> perfectly fine & not thought to be an issue? E.g. no password manager
>>> and you trust your FS permission? Let's cover those cases too.
>>
>> 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.
>
> 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).
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.
>>>> + char *value = NULL;
>>>
>>> This init to NULL should be removedd, as we....
>>>
>>>> + const char *at_ptr;
>>>> + const char *colon_ptr;
>>>> + struct strbuf anonymized = STRBUF_INIT;
>>>
>>> nit: Just call this "sb"? The's at least one line below over 79
>>> characters that's within the bounds with a shorter variable name, and in
>>> this case it's obvious what we're doing here...
>>
>> I will not change this name to be less descriptive.
>
> 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?
How about "redacted"?
> 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.
TIL. strbuf_splice() will work perfectly. Thanks.
-Stolee |
|
/submit |
|
Submitted as pull.1237.v2.git.1653658034086.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this): On Fri, May 27 2022, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <derrickstolee@github.com>
Just real quick, I hadn't taken notice of this before (the rest looks
good at a glance):
> + /*
> + * 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);
So the function is renamed to detected_credentials_in_url(), so as a nit
I'd expect some verb like "strip", "redact" or whatever inthe name or
whatever, to make it clear what we're doing.
But since the only caller here below...
> +
> + /* 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);
> +
> + strbuf_release(&redacted);
> +}
> +
> static char *url_normalize_1(const char *url, struct url_info *out_info, char allow_globs)
> {
> /*
> @@ -144,6 +198,7 @@ static char *url_normalize_1(const char *url, struct url_info *out_info, char al
> */
>
> size_t url_len = strlen(url);
> + const char *orig_url = url;
> struct strbuf norm;
> size_t spanned;
> size_t scheme_len, user_off=0, user_len=0, passwd_off=0, passwd_len=0;
> @@ -191,6 +246,7 @@ static char *url_normalize_1(const char *url, struct url_info *out_info, char al
> }
> colon_ptr = strchr(norm.buf + scheme_len + 3, ':');
> if (colon_ptr) {
> + detected_credentials_in_url(orig_url, scheme_len);
Has already done the work of finding the colon_ptr (and at_ptr) why
re-do that paranoia since we have a static function, we could just pass
the two pointers we found already to strbuf_splice().
This also seems really close to something we could just add to strbuf.c
as e.g a strbuf_splice_to(). I.e. just:
int strbuf_splice_to(const struct strbuf *in, struct strbuf *sb,
size_t pos, size_t len,
const void *data, size_t data_len);
Which would be used as:
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);
Which re earlier talk of sharing an implementation with the other
<redacted> code looks like it could be dropped into the relevant part of
pkt-line.c.
But maybe that's all going overboard :) |
|
On the Git mailing list, Derrick Stolee wrote (reply to this): On 5/27/2022 10:22 AM, Ævar Arnfjörð Bjarmason wrote:
>
> On Fri, May 27 2022, Derrick Stolee via GitGitGadget wrote:
>
>> From: Derrick Stolee <derrickstolee@github.com>
>
> Just real quick, I hadn't taken notice of this before (the rest looks
> good at a glance):
>
>> + /*
>> + * 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);
>
> So the function is renamed to detected_credentials_in_url(), so as a nit
> I'd expect some verb like "strip", "redact" or whatever inthe name or
> whatever, to make it clear what we're doing.
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.
>> colon_ptr = strchr(norm.buf + scheme_len + 3, ':');
>> if (colon_ptr) {
>> + detected_credentials_in_url(orig_url, scheme_len);
>
> Has already done the work of finding the colon_ptr (and at_ptr) why
> re-do that paranoia since we have a static function,
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 |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt
> index cd65d236b43..7fd3ea89f5d 100644
> --- a/Documentation/config/fetch.txt
> +++ b/Documentation/config/fetch.txt
> @@ -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
In the above, 'protocol', 'domain' and 'path' are all placeholders,
just like <user> and <password> are, and it made me wonder if we
should be consistent. Enclosing all placeholders in <angle-braket>
pairs would make the resulting text too loud, so it may make sense
to drop these highlights around user and password.
That makes it consistent with what git-credential-store.txt,
git-svn.txt, gitfaq.txt and urls.txt do
git-credential-store.txt:https://user:pass@example.com
git-svn.txt: the URL, e.g. `svn+ssh://foo@svn.bar.com/project`
gitfaq.txt:$ echo url=https://author@git.example.org | git credential reject
gitfaq.txt: https://author@git.example.org/org1/project1.git and
gitfaq.txt: https://committer@git.example.org/org2/project2.git. This way, when you
gitfaq.txt: origin https://author@git.example.org/org1/project1.git` (see
> + 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.
Good.
> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index 4a61f2c901e..387da74d175 100755
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh
> @@ -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
> +'
Could we have one negative test here, too? I.e. with an explicit -c
fetch.credentialsInUrl=allow given, no credential-in-URL errors and
warnings should be issued.
> diff --git a/urlmatch.c b/urlmatch.c
> index b615adc923a..16beda37a3a 100644
> --- a/urlmatch.c
> +++ b/urlmatch.c
> @@ -1,5 +1,6 @@
> #include "cache.h"
> #include "urlmatch.h"
> +#include "config.h"
>
> #define URL_ALPHA "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
> #define URL_DIGIT "0123456789"
> @@ -106,6 +107,59 @@ 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.
"scheme" is probably more technically correct here, even though in
the documentation that faces the end-users, "protocol" would be more
widely understood, so I think this inconsistency in a single patch
across paths is a good thing ;-)
> + * 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)
> +{
> + const char *value;
> + const char *at_ptr;
> + const char *colon_ptr;
> + struct strbuf redacted = STRBUF_INIT;
> +
> + /* "allow" is the default behavior. */
> + if (git_config_get_string_tmp("fetch.credentialsinurl", &value) ||
> + !strcmp("allow", value))
> + return;
OK.
> + at_ptr = strchr(url, '@');
> + colon_ptr = strchr(url + scheme_len + 3, ':');
I am debating myself if we should explain "+ 3" that counts "://" in
a comment, but it probably is trivial to see.
> + /*
> + * 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);
OK. The caller shouldn't have called us in these two cases.
> + /* 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);
> +
> + strbuf_release(&redacted);
OK. Let's look at the caller.
> 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);
The caller apparently knows where the password and username are at
this point. It is unclear which string these offsets are pointing
into at this point, but at the end of the function, we have the
offset and length of user and passwd that point into result, all in
our local variables, even when out_info is NULL.
result = strbuf_detach(&norm, &result_len);
if (out_info) {
out_info->url = result;
out_info->err = NULL;
out_info->url_len = result_len;
out_info->scheme_len = scheme_len;
out_info->user_off = user_off;
out_info->user_len = user_len;
out_info->passwd_off = passwd_off;
out_info->passwd_len = passwd_len;
out_info->host_off = host_off;
out_info->host_len = host_len;
out_info->port_off = port_off;
out_info->port_len = port_len;
out_info->path_off = path_off;
out_info->path_len = path_len;
}
return result;
It does make me wonder if we want another parser in the new
function. Wouldn't it be easier to manage if we inserted a
call to
if (passwd_off)
apply_fetch_credentials_in_url(result,
user_off, user_len, passwd_off, passwd_len);
just after the strbuf_detach() we see near the end of the function, where
apply_fetch_credentials_in_url() only reads the configuration and
calls warning or die as appropriate? |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Junio C Hamano <gitster@pobox.com> writes:
> It does make me wonder if we want another parser in the new
> function. Wouldn't it be easier to manage if we inserted a
> call to
>
> if (passwd_off)
> apply_fetch_credentials_in_url(result,
> user_off, user_len, passwd_off, passwd_len);
>
> just after the strbuf_detach() we see near the end of the function, where
> apply_fetch_credentials_in_url() only reads the configuration and
> calls warning or die as appropriate?
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. |
|
This branch is now known as |
|
This patch series was integrated into seen via git@e0551e4. |
|
This patch series was integrated into seen via git@83cc4cd. |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -96,3 +96,17 @@ fetch.writeCommitGraph:: | |||
| merge and the write may take longer. Having an updated commit-graph | |||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Wed, Jun 01 2022, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <derrickstolee@github.com>
>
> + for (int i = 0; i < remote->url_nr; i++)
I think we're trying to avoid that bit of C99 in general use until
Junio's canary graduates. See 6563706568b (CodingGuidelines: give
deadline for "for (int i = 0; ...", 2022-03-30).There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 6/1/2022 3:19 PM, Ævar Arnfjörð Bjarmason wrote:
>
> On Wed, Jun 01 2022, Derrick Stolee via GitGitGadget wrote:
>
>> From: Derrick Stolee <derrickstolee@github.com>
>>
>> + for (int i = 0; i < remote->url_nr; i++)
>
> I think we're trying to avoid that bit of C99 in general use until
> Junio's canary graduates. See 6563706568b (CodingGuidelines: give
> deadline for "for (int i = 0; ...", 2022-03-30).
Ah, thanks. I remembered that canary going out a long time ago,
but forgot to check if we had committed to it.
Thanks,
-Stolee
git-compat-util.h
Outdated
| @@ -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))); | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 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.
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.
|
There was a status update in the "New Topics" section about the branch The "fetch.credentialsInUrl" configuration variable controls what happens when a URL with embedded login credential is used. source: <pull.1237.v3.git.1654046173.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@dc83b58. |
derrickstolee
force-pushed
the
creds-in-url
branch
2 times, most recently
from
c47ccb4
to
5fb0384
Compare
derrickstolee
changed the title
|
/submit |
|
Submitted as pull.1237.v4.git.1654190434908.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> +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 };
The initializer should be "= { 0 }" not "= { NULL }". In other
words, we shouldn't have to care if the first member in the struct
happens to be of a pointer type, and we shouldn't have to change
between 0 and NULL whenever the type of the first member changes.
Even though it frowns upon assigning 0 to a pointer variable or a
pointer member in a struct, sparse knows that such an initializer is
OK.
cf. https://lore.kernel.org/git/YVJSwuqjolz28+mG@coredump.intra.peff.net/
Please have a blank line after the variable decl.
> + url_normalize(remote->url[i], &url_info);
url_normalize() returns "char *", and you get NULL when parsing
fails; out_info->err may also help when it happens, but I think we
will call url_normalize() again later in the existing caller, and
it will give whatever error message we need to give appropriately,
so it is OK to silently jump to the loop_cleanup label from here.
> + if (!url_info.passwd_len)
> + goto loop_cleanup;
I wonder what should happen to "https://username:@localhost" (i.e.
an empty string is used as a password). I am fine if we allow it as
an obvious "this is like an anonymous ftp; anybody can connect" use
case, but I do not know how useful it would be in practice.
I am also fine if certain authentication scheme needs username and
password, the latter of which is never used because the "real thing"
like Kerberos kicks in when the real authentication happens but
becasue something needs to be there to "trigger" the authentication,
and that is why we deliberately allow an empty string case unwarned.
But if this is a deliberate thing, we'd need to caution future
developers about it.
I do not think passwd_ofs can ever be 0 if we have an embedded
password in the URL, so checking it may be a better approach, if we
care about an empty-string case. I can be persuaded either way.
> +
> + 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);
We obviously could introduce another local variable that is set
based on the "value" before we enter the loop to "optimize", but
this is an error codepath, so I do not mind repeated strcmp() on the
constant value in the loop.
I do have to wonder what we should do when value is none of the
three we know about. Right now, it makes the function an expensive
noop, so upfront at the beginning of the function, we might need
something like
int to_warn_not_die;
if (git_config_get_string_tmp(..."))
return;
if (!strcmp("warn", value))
to_warn_not_die = 1;
else if (!strcmp("die", value))
to_warn_not_die = 0;
else
return;
anyway, in which case we would also do
if (to_warn_not_die)
warning(...);
else
die(...);
in the loop, perhaps? I dunno.
I wonder if die_message() may want to report all the offending URL
for a given remote (with a concluding die() after the loop). I am
OK with dying at the first offence, though. In the worst case, the
end user experience would be:
$ git fetch there
die() about the first URL for the nickname
$ edit .git/config
$ git fetch there
die() about the second URL for the nickname
The user will learn after getting the same error message twice and
scan through the other URLs when editing .git/config for the second
time to fix the second URL. If I were writing this patch, I would
probably play lazy and die on the first offender.
> +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
Reusing warnings file for die messages is probably OK ;-)
An extra test with an empty string as a password would have caught
the differences between using passwd_off and passwd_len to detect
the presence of a password here.
Taking all together, I'll queue the following on top as a separate
fix-up patch, but I may well be giving (some) bad pieces of advice,
so I will wait for others to comment.
Thanks.
remote.c | 26 ++++++++++++++++++--------
t/t5516-fetch-push.sh | 4 ++++
t/t5601-clone.sh | 4 ++++
3 files changed, 26 insertions(+), 8 deletions(-)
diff --git c/remote.c w/remote.c
index 59b6839445..2cdc064fa8 100644
--- c/remote.c
+++ w/remote.c
@@ -619,25 +619,35 @@ static void validate_remote_url(struct remote *remote)
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_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:
diff --git c/t/t5516-fetch-push.sh w/t/t5516-fetch-push.sh
index afb9236bee..a67acc3263 100755
--- c/t/t5516-fetch-push.sh
+++ w/t/t5516-fetch-push.sh
@@ -1821,6 +1821,10 @@ test_expect_success 'fetch warns or fails when using username:password' '
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
'
diff --git c/t/t5601-clone.sh w/t/t5601-clone.sh
index ddc4cc7ec2..cf0a3ef3f4 100755
--- c/t/t5601-clone.sh
+++ w/t/t5601-clone.sh
@@ -82,6 +82,10 @@ test_expect_success 'clone warns or fails when using username:password' '
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
'
|
|
This patch series was integrated into seen via git@099b02a. |
|
This patch series was integrated into seen via git@00baa1c. |
|
On the Git mailing list, Derrick Stolee wrote (reply to this): On 6/2/2022 5:20 PM, Junio C Hamano wrote:
...
> Taking all together, I'll queue the following on top as a separate
> fix-up patch, but I may well be giving (some) bad pieces of advice,
> so I will wait for others to comment.
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 |
|
This patch series was integrated into seen via git@b9c232a. |
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. Co-authored-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
derrickstolee
force-pushed
the
creds-in-url
branch
from
5fb0384
to
b744a98
Compare
|
/submit |
|
Submitted as pull.1237.v5.git.1654526176695.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Derrick Stolee <derrickstolee@github.com> writes:
> On 6/2/2022 5:20 PM, Junio C Hamano wrote:
> ...
>> Taking all together, I'll queue the following on top as a separate
>> fix-up patch, but I may well be giving (some) bad pieces of advice,
>> so I will wait for others to comment.
>
> 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.
Heh, that's at most helped-by; a small bugfix in the logic (zero-length)
and all others are minor clean-ups.
Thanks. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> * Noticed that the redacted strbuf wasn't reset between loop
> iterations, which only matters if there are multiple URLs for the
> remote.
Thanks, I missed that.
Will queue. |
|
This patch series was integrated into seen via git@eb65220. |
|
This patch series was integrated into seen via git@2839017. |
|
This patch series was integrated into seen via git@807ee7d. |
|
There was a status update in the "Cooking" section about the branch The "fetch.credentialsInUrl" configuration variable controls what happens when a URL with embedded login credential is used. Will merge to 'next'. source: <pull.1237.v5.git.1654526176695.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@d18c764. |
|
This patch series was integrated into next via git@3db83a2. |
|
This patch series was integrated into seen via git@448d9e9. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Users can specify credentials in their URL using the
username:password@domainformat. 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 thewarnstate 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/pull.945.git.1619807844627.gitgitgadget@gmail.com
Reject passwords in URLs (April 2021).
[2] https://lore.kernel.org/git/20190519050724.GA26179@sigill.intra.peff.net/
Re: Git ransom campaign incident report - May 2019
Updates in v5
Updates in v4
Updates in v3
warn_once()helper. This reduces multiple advisory warnings with the same text from being written by the same process.Updates in v2
Thanks,
-Stolee
Cc: gitster@pobox.com
Cc: peff@peff.net
Cc: me@ttaylorr.com
Cc: avarab@gmail.com
Cc: christian.couder@gmail.com
Cc: johannes.schindelin@gmx.de
Cc: jrnieder@gmail.com
cc: "brian m. carlson" sandals@crustytoothpaste.net
cc: Robert Coup robert.coup@koordinates.com