| Message ID | 66823c735b1d5ea2047a29656e82fa6fe895f6f1.1587588665.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | credential: handle partial URLs in config settings again | expand |
Johannes Schindelin wrote: > In the patches for CVE-2020-11008, the ability to specify credential > settings in the config for partial URLs got lost. For example, it used > to be possible to specify a credential helper for a specific protocol: > > [credential "https://"] > helper = my-https-helper > > Likewise, it used to be possible to configure settings for a specific > host, e.g.: > > [credential "dev.azure.com"] > useHTTPPath = true Ah, I see. [...] > --- a/credential.c > +++ b/credential.c > @@ -53,7 +53,12 @@ static int credential_config_callback(const char *var, const char *value, > char *url = xmemdupz(key, dot - key); > int matched; > > - credential_from_url(&want, url); > + if (credential_from_url_gently(&want, url, 0, 0) < 0) { > + warning(_("skipping credential lookup for url: %s"), url); > + credential_clear(c); Hm, the error message doesn't seem right here, since `url` is a config key instead of URL whose credential's are being looked up. Should the error message include the entirety of `var` to make debugging easier? [...] > --- a/t/t0300-credentials.sh > +++ b/t/t0300-credentials.sh > @@ -448,4 +448,17 @@ test_expect_success 'credential system refuses to work with missing protocol' ' > test_i18ncmp expect stderr > ' > > +test_expect_success 'credential config accepts partial URLs' ' > + echo url=https://example.com | > + git -c credential.example.com.username=boo \ > + credential fill >actual && Can the tests also check the behavior with bad URLs (that we are appropriately tolerating and warning about them? Stepping back: one thing I like about the code in "master" that uses urlmatch_config_entry is that it is not treating these config keys in the same way as URLs that Git would fetch from. For example, if one of the config keys contains %0a, then that's perfectly fine --- we are not going to write them to a credential helper or to libcurl. The only problem is that the pattern matching syntax doesn't match the behavior that users historically expected. (Keeping in mind that we never actually provided the behavior that users expected. `credential.example.com.helper` settings applied to all hosts.) Would it make sense for parsing these config url patterns to share *less* code with credential_from_url? Ramifications: - unless we add specific support for it, we'd lose support for patterns that are specific to a user (e.g. "credential.https://user@example.com.helper"). - likewise, we'd lose support for "credential.https://user:pass@example.com.helper". - we could control what "credential.https:///repo.git.helper" means, for example by rejecting it. When we reject it, the error message could be specific to this use case instead of shared with other URL handling. - we wouldn't suport "credential.example.com/repo.git.helper" by mistake. - to sum up, we could specifically define exactly what cases we want to support: [credential "example.com"] # settings for the host ... [credential "user@example.com"] # maybe # settings for the host/user combination ... [credential "https://"] # settings for the scheme ... [credential "https://example.com"] # settings for the host/scheme combination ... [credential "https://example.com/"] # likewise ... [credential "https://user@example.com"] # maybe # settings for the host/scheme/user combination ... [credential "https://user@example.com/"] # maybe # likewise ... [credential "https://example.com/repo.git"] # settings for the host/scheme/path combination ... [credential "https://user@example.com/repo.git"] # maybe # settings for the host/scheme/user/path combination ... without accidentally promising support for anything else What do you think? Thanks, Jonathan
Hi Jonathan, On Wed, 22 Apr 2020, Jonathan Nieder wrote: > Johannes Schindelin wrote: > > [...] > > --- a/credential.c > > +++ b/credential.c > > @@ -53,7 +53,12 @@ static int credential_config_callback(const char *var, const char *value, > > char *url = xmemdupz(key, dot - key); > > int matched; > > > > - credential_from_url(&want, url); > > + if (credential_from_url_gently(&want, url, 0, 0) < 0) { > > + warning(_("skipping credential lookup for url: %s"), url); > > + credential_clear(c); > > Hm, the error message doesn't seem right here, since `url` is a config > key instead of URL whose credential's are being looked up. > > Should the error message include the entirety of `var` to make > debugging easier? I suppose you're right. BTW I just realized a much worse issue: `credential_clear(c);`. This clears the wrong struct. It should clear `want`, not `c`. > [...] > > --- a/t/t0300-credentials.sh > > +++ b/t/t0300-credentials.sh > > @@ -448,4 +448,17 @@ test_expect_success 'credential system refuses to work with missing protocol' ' > > test_i18ncmp expect stderr > > ' > > > > +test_expect_success 'credential config accepts partial URLs' ' > > + echo url=https://example.com | > > + git -c credential.example.com.username=boo \ > > + credential fill >actual && > > Can the tests also check the behavior with bad URLs (that we are > appropriately tolerating and warning about them? Yes, my bad! The next iteration will have a test for that. > Stepping back: one thing I like about the code in "master" that uses > urlmatch_config_entry is that it is not treating these config keys > in the same way as URLs that Git would fetch from. For example, if > one of the config keys contains %0a, then that's perfectly fine --- > we are not going to write them to a credential helper or to libcurl. That is actually not what the code does, at least not in `maint-2.17`. It very much warns about `%0a` in config keys. The test I am adding to verify that the warning above is exercised correctly looks like this: git -c credential.$partial.helper=yep \ -c credential.with%0anewline.username=uh-oh \ credential fill >output 2>error && test_i18ngrep "skipping credential lookup for url" error That is literally the only way I get `credential_from_url_gently()` to return `-1` in the lenient mode. > The only problem is that the pattern matching syntax doesn't match > the behavior that users historically expected. (Keeping in mind > that we never actually provided the behavior that users expected. > `credential.example.com.helper` settings applied to all hosts.) Yes, I fix that, too. It is a bad usability bug, in my eyes, and I think it is better to fix it while I'm in the space anyway. > Would it make sense for parsing these config url patterns to share > *less* code with credential_from_url? Ramifications: > > - unless we add specific support for it, we'd lose support for > patterns that are specific to a user (e.g. > "credential.https://user@example.com.helper"). > > - likewise, we'd lose support for > "credential.https://user:pass@example.com.helper". > > - we could control what "credential.https:///repo.git.helper" > means, for example by rejecting it. When we reject it, the > error message could be specific to this use case instead of > shared with other URL handling. > > - we wouldn't suport "credential.example.com/repo.git.helper" > by mistake. I think we can have _all_ of this _without_ violating the DRY principle. Remember: my main motivation for keeping 2/3 apart from 3/3 was so that it is really easy to verify that 2/3 does not break the callers that _need_ the strict mode. And since that is the case, we can then enjoy the benefit of the shared code for the one caller that wants to match also partial URLs. > - to sum up, we could specifically define exactly what cases we want > to support: > > [credential "example.com"] > # settings for the host > ... > > [credential "user@example.com"] # maybe > # settings for the host/user combination > ... > > [credential "https://"] > # settings for the scheme > ... > > [credential "https://example.com"] > # settings for the host/scheme combination > ... > > [credential "https://example.com/"] > # likewise > ... > > [credential "https://user@example.com"] # maybe > # settings for the host/scheme/user combination > ... > > [credential "https://user@example.com/"] # maybe > # likewise > ... > > [credential "https://example.com/repo.git"] > # settings for the host/scheme/path combination > ... > > [credential "https://user@example.com/repo.git"] # maybe > # settings for the host/scheme/user/path combination > ... > > without accidentally promising support for anything else > > What do you think? I added tests for all of those. They all work as a naive user (like me) would expect them to. I threw in another test, too: [credential "/repo.git"] ... And I also threw in negative tests, to verify that non-matching protocol or host name or path mean that the config setting is ignored. Thank you for your thorough review, it really helps me, Dscho
diff --git a/credential.c b/credential.c index c73260ac40f..b9e8daa5406 100644 --- a/credential.c +++ b/credential.c @@ -53,7 +53,12 @@ static int credential_config_callback(const char *var, const char *value, char *url = xmemdupz(key, dot - key); int matched; - credential_from_url(&want, url); + if (credential_from_url_gently(&want, url, 0, 0) < 0) { + warning(_("skipping credential lookup for url: %s"), url); + credential_clear(c); + free(url); + return 0; + } matched = credential_match(&want, c); credential_clear(&want); diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh index efed3ea2955..9dcba6a7ad9 100755 --- a/t/t0300-credentials.sh +++ b/t/t0300-credentials.sh @@ -448,4 +448,17 @@ test_expect_success 'credential system refuses to work with missing protocol' ' test_i18ncmp expect stderr ' +test_expect_success 'credential config accepts partial URLs' ' + echo url=https://example.com | + git -c credential.example.com.username=boo \ + credential fill >actual && + cat >expect <<-EOF && + protocol=https + host=example.com + username=boo + password=askpass-password + EOF + test_cmp expect actual +' + test_done