Re: [PATCH 2/3] credential: teach `credential_from_url()` a non-strict mode

Hi Jonathan,

On Wed, 22 Apr 2020, Jonathan Nieder wrote:

I don't really want to be too verbose about this in _this_ commit message. But I do see your point. This is my current version of the commit message:

    credential: optionally allow partial URLs in credential_from_url_gently()

    Prior to the fixes for CVE-2020-11008, we were _very_ lenient in what we
    required from a URL in order to parse it into a `struct credential`.
    That led to serious vulnerabilities.

    There was one call site, though, that really needed that leniency: when
    parsing config settings a la `credential.dev.azure.com.useHTTPPath`.
    Settings like this might be desired when users want to use, say, a given
    user name on a given host, regardless of the protocol to be used.

    In preparation for fixing that bug, let's add a parameter called
    `allow_partial_url` to the `credential_from_url_gently()` function and
    convert the existing callers to set that parameter to 0, i.e. do not
    change the existing behavior, just add the option to be more lenient.

    Please note that this patch does more than just reinstating a way to
    imitate the behavior before those CVE-2020-11008 fixes: Before that, we
    would simply ignore URLs without a protocol. In other words,
    misleadingly, the following setting would be applied to _all_ URLs:

            [credential "example.com"]
                    username = that-me

    The obvious intention is to match the host name only. With this patch,
    we allow precisely that: when parsing the URL with non-zero
    `allow_partial_url`, we do not simply return success if there was no
    protocol, but we simply leave the protocol unset and continue parsing
    the URL.

As you see, I added substantially more information there.

On the contrary, I _really_ want them to be separate, so that it is easier to review. I know that _I_ had an easier time looking over the patch that introduces the non-strict mode, to make sure that it does not change a thing in strict mode (and yes, v1 made that harder than necessary by _not_ using `strict ||` in the condition that guards the `c->host` assignment).

In short: it is my intention to keep the two patches separate, with the main goal to make sure that I don't introduce any of those regressions Peff is worried about.

I resisted the temptation because I don't want this to be a big patch series, but a fast one. The reports keep coming in, and the current plan seems to be for GitHub Desktop to release another version on Monday, for which I need to release a MinGit backport probably tomorrow, otherwise it won't work timewise.

Meaning: I don't really want to do "nice to have" work like this flag.

Looks good, but there are only three callers (and I don't expect more). And the only caller interested in the lenient mode (read: the only now-complicated call) lives in the very same file as the called function. So it strikes me as quite a bit of an overkill to introduce two new functions.

Almost. My intention was to handle missing host names, too: `/repo.git` should also match `https://example.com/repo.git`.

The user name is optional already, anyway.

BTW I realized (while working on these patches) that it would probably be a good idea to warn about passwords in these `credential.<URL>.<key>` settings: the password will be ignored as far as `credential_match()` is concerned, and they should probably not live in the config.

But for aforementioned reasons, I decided against including a patch that makes that happen.

Again, the sole caller of this lenient mode is the config parser which then wants to match the (partial) URL against the actual URL, using this function:

	int credential_match(const struct credential *want,
			     const struct credential *have)
	{
	#define CHECK(x) (!want->x || (have->x && !strcmp(want->x, have->x)))
		return CHECK(protocol) &&
		       CHECK(host) &&
		       CHECK(path) &&
		       CHECK(username);
	#undef CHECK
	}

The lenient parser is supposed to populate the `want` of this function. In other words, the code _expects_ `protocol` (or for that matter, _any_ attribute of `want`) to be optional.

Oh... I thought it was obvious that a partial URL starting with a slash would refer to the path part only... Does that not make sense?

Ciao, Dscho