On 5/23/2022 3:06 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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
