persistent-https, url insteadof, and `git submodule`

persistent-https, url insteadof, and `git submodule`

[Elliott Cable]

Set up `persistent-https` as described in the [README][]; including the
‘rewrite https urls’ feature in `.gitconfig`:

    [url "persistent-https"]
        insteadof = https
    [url "persistent-http"]
        insteadof = http

Unfortunately, this breaks `git submodule add`:

    > git submodule add https://github.com/nodenv/nodenv.git \
        ./Vendor/nodenv
    Cloning into '/Users/ec/Library/System Repo/Vendor/nodenv'...
    fatal: transport 'persistent-https' not allowed
    fatal: clone of 'https://github.com/nodenv/nodenv.git' into
submodule path '/Users/ec/Library/System Repo/Vendor/nodenv' failed

Presumably this isn't intended behaviour?

   [README]: <https://github.com/git/git/tree/master/contrib/persistent-https#readme>


⁓ ELLIOTTCABLE — fly safe.
  http://ell.io/tt

Re: persistent-https, url insteadof, and `git submodule`

[Dennis Kaarsemaker]

On Fri, 2017-05-19 at 14:57 -0500, Elliott Cable wrote:
> Set up `persistent-https` as described in the [README][]; including the
> ‘rewrite https urls’ feature in `.gitconfig`:
> 
>     [url "persistent-https"]
>         insteadof = https
>     [url "persistent-http"]
>         insteadof = http
> 
> Unfortunately, this breaks `git submodule add`:
> 
>     > git submodule add https://github.com/nodenv/nodenv.git \
>         ./Vendor/nodenv
>     Cloning into '/Users/ec/Library/System Repo/Vendor/nodenv'...
>     fatal: transport 'persistent-https' not allowed
>     fatal: clone of 'https://github.com/nodenv/nodenv.git' into
> submodule path '/Users/ec/Library/System Repo/Vendor/nodenv' failed
> 
> Presumably this isn't intended behaviour?

It actually is. git-submodule sets GIT_PROTOCOL_FROM_USER to 0, which
makes git not trust any urls except http(s), git, ssh and file urls
unless you explicitely configure git to allow it. See the
GIT_ALLOW_PROTOCOL section in man git and the git-config section it
links to.

D.

Re: persistent-https, url insteadof, and `git submodule`

[Dennis Kaarsemaker]

On Fri, 2017-05-19 at 23:43 +0200, Dennis Kaarsemaker wrote:
> On Fri, 2017-05-19 at 14:57 -0500, Elliott Cable wrote:
> > Set up `persistent-https` as described in the [README][]; including the
> > ‘rewrite https urls’ feature in `.gitconfig`:
> > 
> >     [url "persistent-https"]
> >         insteadof = https
> >     [url "persistent-http"]
> >         insteadof = http
> > 
> > Unfortunately, this breaks `git submodule add`:
> > 
> >     > git submodule add https://github.com/nodenv/nodenv.git \
> >         ./Vendor/nodenv
> >     Cloning into '/Users/ec/Library/System Repo/Vendor/nodenv'...
> >     fatal: transport 'persistent-https' not allowed
> >     fatal: clone of 'https://github.com/nodenv/nodenv.git' into
> > submodule path '/Users/ec/Library/System Repo/Vendor/nodenv' failed
> > 
> > Presumably this isn't intended behaviour?
> 
> It actually is. git-submodule sets GIT_PROTOCOL_FROM_USER to 0, which
> makes git not trust any urls except http(s), git, ssh and file urls
> unless you explicitely configure git to allow it. See the
> GIT_ALLOW_PROTOCOL section in man git and the git-config section it
> links to.

33cfccbbf3 (submodule: allow only certain protocols for submodule
fetches, 2015-09-16) says:

    submodule: allow only certain protocols for submodule fetches
    
    Some protocols (like git-remote-ext) can execute arbitrary
    code found in the URL. The URLs that submodules use may come
    from arbitrary sources (e.g., .gitmodules files in a remote
    repository). Let's restrict submodules to fetching from a
    known-good subset of protocols.
    
    Note that we apply this restriction to all submodule
    commands, whether the URL comes from .gitmodules or not.
    This is more restrictive than we need to be; for example, in
    the tests we run:
    
      git submodule add ext::...
    
    which should be trusted, as the URL comes directly from the
    command line provided by the user. But doing it this way is
    simpler, and makes it much less likely that we would miss a
    case. And since such protocols should be an exception
    (especially because nobody who clones from them will be able
    to update the submodules!), it's not likely to inconvenience
    anyone in practice.


D.

Re: persistent-https, url insteadof, and `git submodule`

[Jeff King]

On Fri, May 19, 2017 at 11:55:34PM +0200, Dennis Kaarsemaker wrote:

> > > Presumably this isn't intended behaviour?
> > 
> > It actually is. git-submodule sets GIT_PROTOCOL_FROM_USER to 0, which
> > makes git not trust any urls except http(s), git, ssh and file urls
> > unless you explicitely configure git to allow it. See the
> > GIT_ALLOW_PROTOCOL section in man git and the git-config section it
> > links to.
> 
> 33cfccbbf3 (submodule: allow only certain protocols for submodule
> fetches, 2015-09-16) says:
> [...]
>     But doing it this way is
>     simpler, and makes it much less likely that we would miss a
>     case. And since such protocols should be an exception
>     (especially because nobody who clones from them will be able
>     to update the submodules!), it's not likely to inconvenience
>     anyone in practice.

Yeah, I think the use of "insteadOf" here is a good counter-example to
the reasoning in that commit message. The submodule itself has a vanilla
protocol, so most users wouldn't need to configure anything. But
somebody who has done a blanket insteadOf now needs to explicitly allow
the protocol, too.

So one workaround is just adding:

  [protocol "persistent-https"]
  allow = always

next to the insteadOf config. And maybe that's enough. It's a little
inconvenient, but it the user has to configure something either way. And
it does give you some flexibility in deciding whether submodules get
access to their special remote helper.

The other approach is to declare that a url rewrite resets the
protocol-from-user flag to 1. IOW, since the "persistent-https" protocol
comes from our local config, it's not dangerous and we should behave as
if the user themselves gave it to us. That makes Elliott's case work out
of the box.

-Peff

Re: persistent-https, url insteadof, and `git submodule`

[Elliott Cable]

Hi! Thanks for the responses (I hope reply-all isn't bad mailing-list
etiquette? Feel free to yell at with a direct reply!). For whatever it's
worth, as a random user, here's my thoughts:

On Sat, May 20, 2017 at 2:07 AM, Jeff King <peff@peff.net> wrote:
> On Fri, May 19, 2017 at 11:55:34PM +0200, Dennis Kaarsemaker wrote:
>> > On Fri, 2017-05-19 at 14:57 -0500, Elliott Cable wrote:
>> > > Presumably this isn't intended behaviour?
>> >
>> > It actually is. git-submodule sets GIT_PROTOCOL_FROM_USER to 0, which
>> > makes git not trust any urls except http(s), git, ssh and file urls
>> > unless you explicitely configure git to allow it. See the
>> > GIT_ALLOW_PROTOCOL section in man git and the git-config section it
>> > links to.
>>
>> 33cfccbbf3 (submodule: allow only certain protocols for submodule
>> fetches, 2015-09-16) says:
>> [...]
>>     But doing it this way is
>>     simpler, and makes it much less likely that we would miss a
>>     case. And since such protocols should be an exception
>>     (especially because nobody who clones from them will be able
>>     to update the submodules!), it's not likely to inconvenience
>>     anyone in practice.
>
> The other approach is to declare that a url rewrite resets the
> protocol-from-user flag to 1. IOW, since the "persistent-https" protocol
> comes from our local config, it's not dangerous and we should behave as
> if the user themselves gave it to us. That makes Elliott's case work out
> of the box.

Well, now that I'm aware of security concerns, `GIT_PROTOCOL_FROM_USER`
and `GIT_ALLOW_PROTOCOL`, and so on, I wouldn't *at all* expect
`insteadOf` to disable that behaviour. Instead, one of two things seems
like a more ideal solution:

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

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.

Just my 2¢ — thanks for your tireless contributions, loves. <3


⁓ ELLIOTTCABLE — fly safe.
  http://ell.io/tt

Re: persistent-https, url insteadof, and `git submodule`

[Jeff King]

On Fri, May 26, 2017 at 11:22:37AM -0500, Elliott Cable wrote:

> Hi! Thanks for the responses (I hope reply-all isn't bad mailing-list
> etiquette? Feel free to yell at with a direct reply!). For whatever it's
> worth, as a random user, here's my thoughts:

No, reply-all is the preferred method on this list.

> > The other approach is to declare that a url rewrite resets the
> > protocol-from-user flag to 1. IOW, since the "persistent-https" protocol
> > comes from our local config, it's not dangerous and we should behave as
> > if the user themselves gave it to us. That makes Elliott's case work out
> > of the box.
> 
> Well, now that I'm aware of security concerns, `GIT_PROTOCOL_FROM_USER`
> and `GIT_ALLOW_PROTOCOL`, and so on, I wouldn't *at all* expect
> `insteadOf` to disable that behaviour. Instead, one of two things seems
> like a more ideal solution:
> 
> 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`.
> 
> 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.

After my other email, I was all set to write a patch to set
"from_user=1" when we rewrite a URL. But I think it actually is a bit
risky, because we don't know which parts of the URL are
security-sensitive versus which parts were rewritten. A modification of
a tainted string doesn't necessarily untaint it (but sometimes it does,
as in your case).

We could actually have a flag as part of the rewrite config, like:

  [url "persistent-https"]
  insteadOf = "https"
  untaint = true

but I don't think that really buys anything. If you know about the
problem, you could just as easily do:

  [url "persistent-https"]
  insteadOf = "https"
  [protocol "persistent-https"]
  allow = always

It really is an issue of the user knowing about the problem (and how to
solve it), and I don't think we can get around that securely. So better
documentation probably is the right solution.

I'll see if I can cook something up.

-Peff

Re: persistent-https, url insteadof, and `git submodule`

[Ævar Arnfjörð Bjarmason]

On Wed, May 31, 2017 at 6:50 AM, Jeff King <peff@peff.net> wrote:
> On Fri, May 26, 2017 at 11:22:37AM -0500, Elliott Cable wrote:
>
>> Hi! Thanks for the responses (I hope reply-all isn't bad mailing-list
>> etiquette? Feel free to yell at with a direct reply!). For whatever it's
>> worth, as a random user, here's my thoughts:
>
> No, reply-all is the preferred method on this list.
>
>> > The other approach is to declare that a url rewrite resets the
>> > protocol-from-user flag to 1. IOW, since the "persistent-https" protocol
>> > comes from our local config, it's not dangerous and we should behave as
>> > if the user themselves gave it to us. That makes Elliott's case work out
>> > of the box.
>>
>> Well, now that I'm aware of security concerns, `GIT_PROTOCOL_FROM_USER`
>> and `GIT_ALLOW_PROTOCOL`, and so on, I wouldn't *at all* expect
>> `insteadOf` to disable that behaviour. Instead, one of two things seems
>> like a more ideal solution:
>>
>> 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`.
>>
>> 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.
>
> After my other email, I was all set to write a patch to set
> "from_user=1" when we rewrite a URL. But I think it actually is a bit
> risky, because we don't know which parts of the URL are
> security-sensitive versus which parts were rewritten. A modification of
> a tainted string doesn't necessarily untaint it (but sometimes it does,
> as in your case).
>
> We could actually have a flag as part of the rewrite config, like:
>
>   [url "persistent-https"]
>   insteadOf = "https"
>   untaint = true
>
> but I don't think that really buys anything. If you know about the
> problem, you could just as easily do:
>
>   [url "persistent-https"]
>   insteadOf = "https"
>   [protocol "persistent-https"]
>   allow = always
>
> It really is an issue of the user knowing about the problem (and how to
> solve it), and I don't think we can get around that securely. So better
> documentation probably is the right solution.
>
> I'll see if I can cook something up.

I was going to say: A way to have our cake & eat it too here would be
to just support *.insteadOfRegex, i.e.
"url.persistent-https://.insteadOfRegex="^https://".

But in this case, even if we can just do un-anchored string
replacement, isn't a way around this just to do
"url.persistent-https://.insteadOf=https://" & untaint & document that
you should do that?

Although that would potentially leave people who have existing
protocol rewrites without :// open to rewrites they didn't intend for
submodules...

Re: persistent-https, url insteadof, and `git submodule`

[Jeff King]

On Wed, May 31, 2017 at 04:23:49PM +0200, Ævar Arnfjörð Bjarmason wrote:

> > It really is an issue of the user knowing about the problem (and how to
> > solve it), and I don't think we can get around that securely. So better
> > documentation probably is the right solution.
> >
> > I'll see if I can cook something up.
> 
> I was going to say: A way to have our cake & eat it too here would be
> to just support *.insteadOfRegex, i.e.
> "url.persistent-https://.insteadOfRegex="^https://".
> 
> But in this case, even if we can just do un-anchored string
> replacement, isn't a way around this just to do
> "url.persistent-https://.insteadOf=https://" & untaint & document that
> you should do that?

I think we already do the second form, and that's what Elliott ran into.
The problem is that it is not clear if "persistent-https" is safe to use
for submodules. _We_ know that it is because we know what that remote
does, but Git doesn't know that. You would not necessarily want:

  [url "ext::ssh-wrapper "]
  insteadOf  = "ssh://"

to kick in for a submodule. That's a fairly insane thing to be doing,
but the point is that we don't know if the rewritten protocol is ready
to handle "tainted" URLs that come from an untrusted submodule file.

-Peff

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

[Jeff King]

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.

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

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

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

[Brandon Williams]

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