Re: [PATCH 14/16] promisor-remote: trust known remotes matching acceptFromServerUrl

[Junio C Hamano <gitster@pobox.com>]

Christian Couder <christian.couder@gmail.com> writes:

> diff --git a/Documentation/config/promisor.adoc b/Documentation/config/promisor.adoc
> index b0fa43b839..6f5442cd65 100644
> --- a/Documentation/config/promisor.adoc
> +++ b/Documentation/config/promisor.adoc
> @@ -51,6 +51,52 @@ promisor.acceptFromServer::
>  	to "fetch" and "clone" requests from the client. Name and URL
>  	comparisons are case sensitive. See linkgit:gitprotocol-v2[5].
>  
> +promisor.acceptFromServerUrl::
> +	A glob pattern to specify which URLs advertised by a server
> +	are considered trusted by the client. This option acts as an
> +	additive security whitelist that works in conjunction with
> +	`promisor.acceptFromServer`.

Between the first sentence and the second one, I think there needs
to be an explanation on what "trusted" means in this context.  Is it
trusted so that the URL can feed random configuration variable=value
pairs for the client to blindly apply?  Or is it trusted to do very
limited things that other remotes can do, and if so what are these
limited things?  Without knowing that, the end-users cannot assess
the security implications of setting this option.

I am guessing that the client would behave as if the existing
promisor.acceptFromServer configuration variable were set to "all"
when talking with a remote whose URL matches one of the patterns
listed?

By the way, some people may suggest "white" -> "allow".


> +1. Start with a secure protocol scheme, like `https://` or `ssh://`.

Is there a practical reason why people would want to use schemes
other than the above two?  This sounds like something a small amount
of code can easily enforce.

> +2. Only allow domain names or paths where you control and trust _ALL_
> +   the content.

Obviously.

> +3. Don't use globs (`*`) in the domain name. For example
> +   `https://cdn.example.com/*` is much safer than
> +   `https://*.example.com/*`, because the latter matches
> +   `https://evil-hacker.net/fake.example.com/repo`.

Is there a practical use case where allowing '*' to match anything
that contains a slash '/' is useful?

> +4. Make sure to have a `/` at the end of the domain name (or the end
> +   of specific directories). For example `https://cdn.example.com/*`
> +   is much safer than `https://cdn.example.com*`, because the latter
> +   matches `https://cdn.example.com.hacker.net/repo`.

Ditto.  The above two points sound like excuses to keep sloppy
asterisk matching logic.  Yes, retroactively tightening rules always
have risk to break existing deployments, but if existing code paths
of urlmatch do not have any good reason to allow '*' to match a
string that contains a slash '/', perhaps there is no fallout.