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