Re: [PATCH 3/3] credential: handle `credential.<partial-URL>.<key>` again

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Apr 22, 2020, 23:57 UTC
Message-ID: <20200422235708.GF140314@google.com>
In-Reply-To: <66823c735b1d5ea2047a29656e82fa6fe895f6f1.1587588665.git.gitgitgadget@gmail.com>

Johannes Schindelin wrote:

Show 12 quoted lines

> 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.

[...]

Show 10 quoted lines

> --- 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?

[...]

Show 10 quoted lines

> --- 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