Re: [PATCH] docs/config: mention protocol implications of url.insteadOf

[Brandon Williams <bmwill@google.com>]

On 05/31, Jeff King wrote:
> On Fri, May 26, 2017 at 11:22:37AM -0500, Elliott Cable wrote:
> 
> > 1. Most simply, better documentation: mention `GIT_PROTOCOL_FROM_USER`
> >    explicitly in the documentation of/near `insteadOf`, most
> >    particularly in the README for `contrib/persistent-https`.
> 
> I agree that a hint in both places would be helpful.  The patch for that
> is below.
> 
> > 2. Possibly, special-case “higher-security” porcelain (like
> >    `git-submodule`, as described in 33cfccbbf3) to ignore `insteadOf`
> >    rewrite-rules without additional, special configuration. This way,
> >    `git-submodule` works for ignorant users (like me) out of the box,
> >    just as it previously did, and there's no possible security
> >    compramise.
> 
> I don't think we can do that. Rewrites of "git://" to "ssh://" are
> pretty common (and completely harmless). Besides, I think submodules are
> a case where you really would want persistent-https to kick in. IIRC,
> the original use case for that helper is Android development, where a
> user is likely to update a ton of repositories from the same server all
> at once. Right now the fetches are all done individually with the "repo"
> tool, but in theory the whole thing could be set up as submodules.

This right here is why Stefan and I have been working on submodules.

> 
> -- >8 --
> Subject: [PATCH] docs/config: mention protocol implications of url.insteadOf
> 
> If a URL rewrite switches the protocol to something
> nonstandard (like "persistent-https" for "https"), the user
> may be bitten by the fact that the default protocol
> restrictions are different between the two. Let's drop a
> note in insteadOf that points the user in the right
> direction.
> 
> It would be nice if we could make this work out of the box,
> but we can't without knowing the security implications of
> the user's rewrite. Only the documentation for a particular
> remote helper can advise one way or the other. Since we do
> include the persistent-https helper in contrib/ (and since
> it was the helper in the real-world case that inspired that
> patch), let's also drop a note there.

Documentation changes look sane to me.  Thanks for whipping this up!

> 
> Suggested-by: Elliott Cable <me@ell.io>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  Documentation/config.txt        |  7 +++++++
>  contrib/persistent-https/README | 10 ++++++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 43d830ee3..5218ecd37 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -3235,6 +3235,13 @@ url.<base>.insteadOf::
>  	the best alternative for the particular user, even for a
>  	never-before-seen repository on the site.  When more than one
>  	insteadOf strings match a given URL, the longest match is used.
> ++
> +Note that any protocol restrictions will be applied to the rewritten
> +URL. If the rewrite changes the URL to use a custom protocol or remote
> +helper, you may need to adjust the `protocol.*.allow` config to permit
> +the request.  In particular, protocols you expect to use for submodules
> +must be set to `always` rather than the default of `user`. See the
> +description of `protocol.allow` above.
>  
>  url.<base>.pushInsteadOf::
>  	Any URL that starts with this value will not be pushed to;
> diff --git a/contrib/persistent-https/README b/contrib/persistent-https/README
> index f784dd2e6..7c4cd8d25 100644
> --- a/contrib/persistent-https/README
> +++ b/contrib/persistent-https/README
> @@ -35,6 +35,16 @@ to use persistent-https:
>  [url "persistent-http"]
>  	insteadof = http
>  
> +You may also want to allow the use of the persistent-https helper for
> +submodule URLs (since any https URLs pointing to submodules will be
> +rewritten, and Git's out-of-the-box defaults forbid submodules from
> +using unknown remote helpers):
> +
> +[protocol "persistent-https"]
> +	allow = always
> +[protocol "persistent-http"]
> +	allow = always
> +
>  
>  #####################################################################
>  # BUILDING FROM SOURCE
> -- 
> 2.13.0.678.ga17378094
> 

-- 
Brandon Williams