threads / discuss / 46004

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

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

## tl;dr

10 messages between May 19, 2017 and Jun 1, 2017.

replies: 9people: 6as markdown or json

Elliott Cable· May 19, 2017, 19:57 UTC · lore

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
Dennis Kaarsemaker· May 19, 2017, 21:43 UTC · re: Elliott Cable · lore

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

On Fri, 2017-05-19 at 14:57 -0500, Elliott Cable wrote:
Show 18 quoted lines
> 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.
Dennis Kaarsemaker· May 19, 2017, 21:55 UTC · re: Dennis Kaarsemaker · lore

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

On Fri, 2017-05-19 at 23:43 +0200, Dennis Kaarsemaker wrote:
Show 25 quoted lines
> 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.
Jeff King· May 20, 2017, 07:07 UTC · re: Dennis Kaarsemaker · lore

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

On Fri, May 19, 2017 at 11:55:34PM +0200, Dennis Kaarsemaker wrote:
Show 17 quoted lines
> > > 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
Elliott Cable· May 26, 2017, 16:22 UTC · re: Jeff King · lore

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

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:
Show 25 quoted lines
> 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
Jeff King· May 31, 2017, 04:50 UTC · re: Elliott Cable · lore

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

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.
Show 21 quoted lines
> > 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
Ævar Arnfjörð Bjarmason· May 31, 2017, 14:23 UTC · re: Jeff King · lore

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

On Wed, May 31, 2017 at 6:50 AM, Jeff King <peff@peff.net> wrote:
Show 56 quoted lines
> 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...

Jeff King· May 31, 2017, 21:22 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

On Wed, May 31, 2017 at 04:23:49PM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 14 quoted lines
> > 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
Jeff King· May 31, 2017, 05:18 UTC · re: Elliott Cable · lore

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

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.

Show 6 quoted lines
> 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
Brandon Williams· Jun 1, 2017, 00:15 UTC · re: Jeff King · lore

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

On 05/31, Jeff King wrote:
Show 23 quoted lines
> 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.
Show 18 quoted lines
> 
> -- >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!
Show 50 quoted lines
> 
> 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

← back to recent threads