{"thread":{"id":"46004","subject":"persistent-https, url insteadof, and `git submodule`","startedAt":"2017-05-19T19:57:38Z","lastAt":"2017-06-01T00:15:29Z","messageCount":10,"participants":["Elliott Cable","Dennis Kaarsemaker","Jeff King","Ævar Arnfjörð Bjarmason","Brandon Williams"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"320283","messageId":"CAPZ477MCsBsfbqKzp69MT_brwz-0aes6twJofQrhizUBV7ZoeA@mail.gmail.com","threadId":"46004","inReplyTo":null,"subject":"persistent-https, url insteadof, and `git submodule`","fromName":"Elliott Cable","fromEmail":"me@ell.io","sentAt":"2017-05-19T19:57:12Z","receivedAt":"2017-05-19T19:57:38Z","isPatch":false,"sender":{"key":"me@ell.io","avatar":"https://gravatar.com/avatar/0d35bb7d7c29b6bbcaafdf13ec70745573d31cb222130cc754a064e707c08d63?d=mp&s=160"},"body":"Set up `persistent-https` as described in the [README][]; including the\n‘rewrite https urls’ feature in `.gitconfig`:\n\n [url \"persistent-https\"]\n insteadof = https\n [url \"persistent-http\"]\n insteadof = http\n\nUnfortunately, this breaks `git submodule add`:\n\n > git submodule add https://github.com/nodenv/nodenv.git \\\n ./Vendor/nodenv\n Cloning into '/Users/ec/Library/System Repo/Vendor/nodenv'...\n fatal: transport 'persistent-https' not allowed\n fatal: clone of 'https://github.com/nodenv/nodenv.git' into\nsubmodule path '/Users/ec/Library/System Repo/Vendor/nodenv' failed\n\nPresumably this isn't intended behaviour?\n\n [README]: \n\n\n⁓ ELLIOTTCABLE — fly safe.\n  http://ell.io/tt\n"},{"id":"320285","messageId":"1495230186.19473.7.camel@kaarsemaker.net","threadId":"46004","inReplyTo":"CAPZ477MCsBsfbqKzp69MT_brwz-0aes6twJofQrhizUBV7ZoeA@mail.gmail.com","subject":"Re: persistent-https, url insteadof, and `git submodule`","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2017-05-19T21:43:06Z","receivedAt":"2017-05-19T21:50:48Z","isPatch":false,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On Fri, 2017-05-19 at 14:57 -0500, Elliott Cable wrote:\n> Set up `persistent-https` as described in the [README][]; including the\n> ‘rewrite https urls’ feature in `.gitconfig`:\n> \n> [url \"persistent-https\"]\n> insteadof = https\n> [url \"persistent-http\"]\n> insteadof = http\n> \n> Unfortunately, this breaks `git submodule add`:\n> \n> > git submodule add https://github.com/nodenv/nodenv.git \\\n> ./Vendor/nodenv\n> Cloning into '/Users/ec/Library/System Repo/Vendor/nodenv'...\n> fatal: transport 'persistent-https' not allowed\n> fatal: clone of 'https://github.com/nodenv/nodenv.git' into\n> submodule path '/Users/ec/Library/System Repo/Vendor/nodenv' failed\n> \n> Presumably this isn't intended behaviour?\n\nIt actually is. git-submodule sets GIT_PROTOCOL_FROM_USER to 0, which\nmakes git not trust any urls except http(s), git, ssh and file urls\nunless you explicitely configure git to allow it. See the\nGIT_ALLOW_PROTOCOL section in man git and the git-config section it\nlinks to.\n\nD.\n"},{"id":"320288","messageId":"1495230934.19473.10.camel@booking.com","threadId":"46004","inReplyTo":"1495230186.19473.7.camel@kaarsemaker.net","subject":"Re: persistent-https, url insteadof, and `git submodule`","fromName":"Dennis Kaarsemaker","fromEmail":"dennis.kaarsemaker@booking.com","sentAt":"2017-05-19T21:55:34Z","receivedAt":"2017-05-19T22:27:46Z","isPatch":false,"sender":{"key":"dennis.kaarsemaker@booking.com","avatar":null},"body":"On Fri, 2017-05-19 at 23:43 +0200, Dennis Kaarsemaker wrote:\n> On Fri, 2017-05-19 at 14:57 -0500, Elliott Cable wrote:\n> > Set up `persistent-https` as described in the [README][]; including the\n> > ‘rewrite https urls’ feature in `.gitconfig`:\n> > \n> > [url \"persistent-https\"]\n> > insteadof = https\n> > [url \"persistent-http\"]\n> > insteadof = http\n> > \n> > Unfortunately, this breaks `git submodule add`:\n> > \n> > > git submodule add https://github.com/nodenv/nodenv.git \\\n> > ./Vendor/nodenv\n> > Cloning into '/Users/ec/Library/System Repo/Vendor/nodenv'...\n> > fatal: transport 'persistent-https' not allowed\n> > fatal: clone of 'https://github.com/nodenv/nodenv.git' into\n> > submodule path '/Users/ec/Library/System Repo/Vendor/nodenv' failed\n> > \n> > Presumably this isn't intended behaviour?\n> \n> It actually is. git-submodule sets GIT_PROTOCOL_FROM_USER to 0, which\n> makes git not trust any urls except http(s), git, ssh and file urls\n> unless you explicitely configure git to allow it. See the\n> GIT_ALLOW_PROTOCOL section in man git and the git-config section it\n> links to.\n\n33cfccbbf3 (submodule: allow only certain protocols for submodule\nfetches, 2015-09-16) says:\n\n    submodule: allow only certain protocols for submodule fetches\n    \n    Some protocols (like git-remote-ext) can execute arbitrary\n    code found in the URL. The URLs that submodules use may come\n    from arbitrary sources (e.g., .gitmodules files in a remote\n    repository). Let's restrict submodules to fetching from a\n    known-good subset of protocols.\n    \n    Note that we apply this restriction to all submodule\n    commands, whether the URL comes from .gitmodules or not.\n    This is more restrictive than we need to be; for example, in\n    the tests we run:\n    \n      git submodule add ext::...\n    \n    which should be trusted, as the URL comes directly from the\n    command line provided by the user. But doing it this way is\n    simpler, and makes it much less likely that we would miss a\n    case. And since such protocols should be an exception\n    (especially because nobody who clones from them will be able\n    to update the submodules!), it's not likely to inconvenience\n    anyone in practice.\n\n\nD.\n\n"},{"id":"320302","messageId":"20170520070757.jekykxagzze3t2wy@sigill.intra.peff.net","threadId":"46004","inReplyTo":"1495230934.19473.10.camel@booking.com","subject":"Re: persistent-https, url insteadof, and `git submodule`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-20T07:07:57Z","receivedAt":"2017-05-20T07:08:08Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 19, 2017 at 11:55:34PM +0200, Dennis Kaarsemaker wrote:\n\n> > > Presumably this isn't intended behaviour?\n> > \n> > It actually is. git-submodule sets GIT_PROTOCOL_FROM_USER to 0, which\n> > makes git not trust any urls except http(s), git, ssh and file urls\n> > unless you explicitely configure git to allow it. See the\n> > GIT_ALLOW_PROTOCOL section in man git and the git-config section it\n> > links to.\n> \n> 33cfccbbf3 (submodule: allow only certain protocols for submodule\n> fetches, 2015-09-16) says:\n> [...]\n>     But doing it this way is\n>     simpler, and makes it much less likely that we would miss a\n>     case. And since such protocols should be an exception\n>     (especially because nobody who clones from them will be able\n>     to update the submodules!), it's not likely to inconvenience\n>     anyone in practice.\n\nYeah, I think the use of \"insteadOf\" here is a good counter-example to\nthe reasoning in that commit message. The submodule itself has a vanilla\nprotocol, so most users wouldn't need to configure anything. But\nsomebody who has done a blanket insteadOf now needs to explicitly allow\nthe protocol, too.\n\nSo one workaround is just adding:\n\n [protocol \"persistent-https\"]\n allow = always\n\nnext to the insteadOf config. And maybe that's enough. It's a little\ninconvenient, but it the user has to configure something either way. And\nit does give you some flexibility in deciding whether submodules get\naccess to their special remote helper.\n\nThe other approach is to declare that a url rewrite resets the\nprotocol-from-user flag to 1. IOW, since the \"persistent-https\" protocol\ncomes from our local config, it's not dangerous and we should behave as\nif the user themselves gave it to us. That makes Elliott's case work out\nof the box.\n\n-Peff\n"},{"id":"320823","messageId":"CAPZ477PoSXqahxaQVpO+m==vng==o4vQahrg_WA8Oeh7wmoW0w@mail.gmail.com","threadId":"46004","inReplyTo":"20170520070757.jekykxagzze3t2wy@sigill.intra.peff.net","subject":"Re: persistent-https, url insteadof, and `git submodule`","fromName":"Elliott Cable","fromEmail":"me@ell.io","sentAt":"2017-05-26T16:22:37Z","receivedAt":"2017-05-26T16:23:05Z","isPatch":false,"sender":{"key":"me@ell.io","avatar":"https://gravatar.com/avatar/0d35bb7d7c29b6bbcaafdf13ec70745573d31cb222130cc754a064e707c08d63?d=mp&s=160"},"body":"Hi! Thanks for the responses (I hope reply-all isn't bad mailing-list\netiquette? Feel free to yell at with a direct reply!). For whatever it's\nworth, as a random user, here's my thoughts:\n\nOn Sat, May 20, 2017 at 2:07 AM, Jeff King wrote:\n> On Fri, May 19, 2017 at 11:55:34PM +0200, Dennis Kaarsemaker wrote:\n>> > On Fri, 2017-05-19 at 14:57 -0500, Elliott Cable wrote:\n>> > > Presumably this isn't intended behaviour?\n>> >\n>> > It actually is. git-submodule sets GIT_PROTOCOL_FROM_USER to 0, which\n>> > makes git not trust any urls except http(s), git, ssh and file urls\n>> > unless you explicitely configure git to allow it. See the\n>> > GIT_ALLOW_PROTOCOL section in man git and the git-config section it\n>> > links to.\n>>\n>> 33cfccbbf3 (submodule: allow only certain protocols for submodule\n>> fetches, 2015-09-16) says:\n>> [...]\n>> But doing it this way is\n>> simpler, and makes it much less likely that we would miss a\n>> case. And since such protocols should be an exception\n>> (especially because nobody who clones from them will be able\n>> to update the submodules!), it's not likely to inconvenience\n>> anyone in practice.\n>\n> The other approach is to declare that a url rewrite resets the\n> protocol-from-user flag to 1. IOW, since the \"persistent-https\" protocol\n> comes from our local config, it's not dangerous and we should behave as\n> if the user themselves gave it to us. That makes Elliott's case work out\n> of the box.\n\nWell, now that I'm aware of security concerns, `GIT_PROTOCOL_FROM_USER`\nand `GIT_ALLOW_PROTOCOL`, and so on, I wouldn't *at all* expect\n`insteadOf` to disable that behaviour. Instead, one of two things seems\nlike a more ideal solution:\n\n1. Most simply, better documentation: mention `GIT_PROTOCOL_FROM_USER`\n explicitly in the documentation of/near `insteadOf`, most\n particularly in the README for `contrib/persistent-https`.\n\n2. Possibly, special-case “higher-security” porcelain (like\n `git-submodule`, as described in 33cfccbbf3) to ignore `insteadOf`\n rewrite-rules without additional, special configuration. This way,\n `git-submodule` works for ignorant users (like me) out of the box,\n just as it previously did, and there's no possible security\n compramise.\n\nJust my 2¢ — thanks for your tireless contributions, loves. <3\n\n\n⁓ ELLIOTTCABLE — fly safe.\n  http://ell.io/tt\n"},{"id":"321111","messageId":"20170531045051.ctoo7sv3f66xurdf@sigill.intra.peff.net","threadId":"46004","inReplyTo":"CAPZ477PoSXqahxaQVpO+m==vng==o4vQahrg_WA8Oeh7wmoW0w@mail.gmail.com","subject":"Re: persistent-https, url insteadof, and `git submodule`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-31T04:50:52Z","receivedAt":"2017-05-31T04:50:57Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 26, 2017 at 11:22:37AM -0500, Elliott Cable wrote:\n\n> Hi! Thanks for the responses (I hope reply-all isn't bad mailing-list\n> etiquette? Feel free to yell at with a direct reply!). For whatever it's\n> worth, as a random user, here's my thoughts:\n\nNo, reply-all is the preferred method on this list.\n\n> > The other approach is to declare that a url rewrite resets the\n> > protocol-from-user flag to 1. IOW, since the \"persistent-https\" protocol\n> > comes from our local config, it's not dangerous and we should behave as\n> > if the user themselves gave it to us. That makes Elliott's case work out\n> > of the box.\n> \n> Well, now that I'm aware of security concerns, `GIT_PROTOCOL_FROM_USER`\n> and `GIT_ALLOW_PROTOCOL`, and so on, I wouldn't *at all* expect\n> `insteadOf` to disable that behaviour. Instead, one of two things seems\n> like a more ideal solution:\n> \n> 1. Most simply, better documentation: mention `GIT_PROTOCOL_FROM_USER`\n> explicitly in the documentation of/near `insteadOf`, most\n> particularly in the README for `contrib/persistent-https`.\n> \n> 2. Possibly, special-case “higher-security” porcelain (like\n> `git-submodule`, as described in 33cfccbbf3) to ignore `insteadOf`\n> rewrite-rules without additional, special configuration. This way,\n> `git-submodule` works for ignorant users (like me) out of the box,\n> just as it previously did, and there's no possible security\n> compramise.\n\nAfter my other email, I was all set to write a patch to set\n\"from_user=1\" when we rewrite a URL. But I think it actually is a bit\nrisky, because we don't know which parts of the URL are\nsecurity-sensitive versus which parts were rewritten. A modification of\na tainted string doesn't necessarily untaint it (but sometimes it does,\nas in your case).\n\nWe could actually have a flag as part of the rewrite config, like:\n\n [url \"persistent-https\"]\n insteadOf = \"https\"\n untaint = true\n\nbut I don't think that really buys anything. If you know about the\nproblem, you could just as easily do:\n\n [url \"persistent-https\"]\n insteadOf = \"https\"\n [protocol \"persistent-https\"]\n allow = always\n\nIt really is an issue of the user knowing about the problem (and how to\nsolve it), and I don't think we can get around that securely. So better\ndocumentation probably is the right solution.\n\nI'll see if I can cook something up.\n\n-Peff\n"},{"id":"321112","messageId":"20170531051804.w6f7yvz4k5wkrwvc@sigill.intra.peff.net","threadId":"46004","inReplyTo":"CAPZ477PoSXqahxaQVpO+m==vng==o4vQahrg_WA8Oeh7wmoW0w@mail.gmail.com","subject":"[PATCH] docs/config: mention protocol implications of url.insteadOf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-31T05:18:04Z","receivedAt":"2017-05-31T05:18:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 26, 2017 at 11:22:37AM -0500, Elliott Cable wrote:\n\n> 1. Most simply, better documentation: mention `GIT_PROTOCOL_FROM_USER`\n> explicitly in the documentation of/near `insteadOf`, most\n> particularly in the README for `contrib/persistent-https`.\n\nI agree that a hint in both places would be helpful. The patch for that\nis below.\n\n> 2. Possibly, special-case “higher-security” porcelain (like\n> `git-submodule`, as described in 33cfccbbf3) to ignore `insteadOf`\n> rewrite-rules without additional, special configuration. This way,\n> `git-submodule` works for ignorant users (like me) out of the box,\n> just as it previously did, and there's no possible security\n> compramise.\n\nI don't think we can do that. Rewrites of \"git://\" to \"ssh://\" are\npretty common (and completely harmless). Besides, I think submodules are\na case where you really would want persistent-https to kick in. IIRC,\nthe original use case for that helper is Android development, where a\nuser is likely to update a ton of repositories from the same server all\nat once. Right now the fetches are all done individually with the \"repo\"\ntool, but in theory the whole thing could be set up as submodules.\n\n-- >8 --\nSubject: [PATCH] docs/config: mention protocol implications of url.insteadOf\n\nIf a URL rewrite switches the protocol to something\nnonstandard (like \"persistent-https\" for \"https\"), the user\nmay be bitten by the fact that the default protocol\nrestrictions are different between the two. Let's drop a\nnote in insteadOf that points the user in the right\ndirection.\n\nIt would be nice if we could make this work out of the box,\nbut we can't without knowing the security implications of\nthe user's rewrite. Only the documentation for a particular\nremote helper can advise one way or the other. Since we do\ninclude the persistent-https helper in contrib/ (and since\nit was the helper in the real-world case that inspired that\npatch), let's also drop a note there.\n\nSuggested-by: Elliott Cable \nSigned-off-by: Jeff King \n---\n Documentation/config.txt | 7 +++++++\n contrib/persistent-https/README | 10 ++++++++++\n 2 files changed, 17 insertions(+)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 43d830ee3..5218ecd37 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -3235,6 +3235,13 @@ url..insteadOf::\n \tthe best alternative for the particular user, even for a\n \tnever-before-seen repository on the site. When more than one\n \tinsteadOf strings match a given URL, the longest match is used.\n++\n+Note that any protocol restrictions will be applied to the rewritten\n+URL. If the rewrite changes the URL to use a custom protocol or remote\n+helper, you may need to adjust the `protocol.*.allow` config to permit\n+the request. In particular, protocols you expect to use for submodules\n+must be set to `always` rather than the default of `user`. See the\n+description of `protocol.allow` above.\n \n url..pushInsteadOf::\n \tAny URL that starts with this value will not be pushed to;\ndiff --git a/contrib/persistent-https/README b/contrib/persistent-https/README\nindex f784dd2e6..7c4cd8d25 100644\n--- a/contrib/persistent-https/README\n+++ b/contrib/persistent-https/README\n@@ -35,6 +35,16 @@ to use persistent-https:\n [url \"persistent-http\"]\n \tinsteadof = http\n \n+You may also want to allow the use of the persistent-https helper for\n+submodule URLs (since any https URLs pointing to submodules will be\n+rewritten, and Git's out-of-the-box defaults forbid submodules from\n+using unknown remote helpers):\n+\n+[protocol \"persistent-https\"]\n+\tallow = always\n+[protocol \"persistent-http\"]\n+\tallow = always\n+\n \n #####################################################################\n # BUILDING FROM SOURCE\n-- \n2.13.0.678.ga17378094\n\n"},{"id":"321131","messageId":"CACBZZX6LQRW=78R-rkeUKmDCRUmN52SjkShSjDC5AgV5o7T6iQ@mail.gmail.com","threadId":"46004","inReplyTo":"20170531045051.ctoo7sv3f66xurdf@sigill.intra.peff.net","subject":"Re: persistent-https, url insteadof, and `git submodule`","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-31T14:23:49Z","receivedAt":"2017-05-31T14:24:21Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Wed, May 31, 2017 at 6:50 AM, Jeff King wrote:\n> On Fri, May 26, 2017 at 11:22:37AM -0500, Elliott Cable wrote:\n>\n>> Hi! Thanks for the responses (I hope reply-all isn't bad mailing-list\n>> etiquette? Feel free to yell at with a direct reply!). For whatever it's\n>> worth, as a random user, here's my thoughts:\n>\n> No, reply-all is the preferred method on this list.\n>\n>> > The other approach is to declare that a url rewrite resets the\n>> > protocol-from-user flag to 1. IOW, since the \"persistent-https\" protocol\n>> > comes from our local config, it's not dangerous and we should behave as\n>> > if the user themselves gave it to us. That makes Elliott's case work out\n>> > of the box.\n>>\n>> Well, now that I'm aware of security concerns, `GIT_PROTOCOL_FROM_USER`\n>> and `GIT_ALLOW_PROTOCOL`, and so on, I wouldn't *at all* expect\n>> `insteadOf` to disable that behaviour. Instead, one of two things seems\n>> like a more ideal solution:\n>>\n>> 1. Most simply, better documentation: mention `GIT_PROTOCOL_FROM_USER`\n>> explicitly in the documentation of/near `insteadOf`, most\n>> particularly in the README for `contrib/persistent-https`.\n>>\n>> 2. Possibly, special-case “higher-security” porcelain (like\n>> `git-submodule`, as described in 33cfccbbf3) to ignore `insteadOf`\n>> rewrite-rules without additional, special configuration. This way,\n>> `git-submodule` works for ignorant users (like me) out of the box,\n>> just as it previously did, and there's no possible security\n>> compramise.\n>\n> After my other email, I was all set to write a patch to set\n> \"from_user=1\" when we rewrite a URL. But I think it actually is a bit\n> risky, because we don't know which parts of the URL are\n> security-sensitive versus which parts were rewritten. A modification of\n> a tainted string doesn't necessarily untaint it (but sometimes it does,\n> as in your case).\n>\n> We could actually have a flag as part of the rewrite config, like:\n>\n> [url \"persistent-https\"]\n> insteadOf = \"https\"\n> untaint = true\n>\n> but I don't think that really buys anything. If you know about the\n> problem, you could just as easily do:\n>\n> [url \"persistent-https\"]\n> insteadOf = \"https\"\n> [protocol \"persistent-https\"]\n> allow = always\n>\n> It really is an issue of the user knowing about the problem (and how to\n> solve it), and I don't think we can get around that securely. So better\n> documentation probably is the right solution.\n>\n> I'll see if I can cook something up.\n\nI was going to say: A way to have our cake & eat it too here would be\nto just support *.insteadOfRegex, i.e.\n\"url.persistent-https://.insteadOfRegex=\"^https://\".\n\nBut in this case, even if we can just do un-anchored string\nreplacement, isn't a way around this just to do\n\"url.persistent-https://.insteadOf=https://\" & untaint & document that\nyou should do that?\n\nAlthough that would potentially leave people who have existing\nprotocol rewrites without :// open to rewrites they didn't intend for\nsubmodules...\n"},{"id":"321165","messageId":"20170531212224.bhn36sa4g5ns54aj@sigill.intra.peff.net","threadId":"46004","inReplyTo":"CACBZZX6LQRW=78R-rkeUKmDCRUmN52SjkShSjDC5AgV5o7T6iQ@mail.gmail.com","subject":"Re: persistent-https, url insteadof, and `git submodule`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-31T21:22:24Z","receivedAt":"2017-05-31T21:22:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 31, 2017 at 04:23:49PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > It really is an issue of the user knowing about the problem (and how to\n> > solve it), and I don't think we can get around that securely. So better\n> > documentation probably is the right solution.\n> >\n> > I'll see if I can cook something up.\n> \n> I was going to say: A way to have our cake & eat it too here would be\n> to just support *.insteadOfRegex, i.e.\n> \"url.persistent-https://.insteadOfRegex=\"^https://\".\n> \n> But in this case, even if we can just do un-anchored string\n> replacement, isn't a way around this just to do\n> \"url.persistent-https://.insteadOf=https://\" & untaint & document that\n> you should do that?\n\nI think we already do the second form, and that's what Elliott ran into.\nThe problem is that it is not clear if \"persistent-https\" is safe to use\nfor submodules. _We_ know that it is because we know what that remote\ndoes, but Git doesn't know that. You would not necessarily want:\n\n [url \"ext::ssh-wrapper \"]\n insteadOf = \"ssh://\"\n\nto kick in for a submodule. That's a fairly insane thing to be doing,\nbut the point is that we don't know if the rewritten protocol is ready\nto handle \"tainted\" URLs that come from an untrusted submodule file.\n\n-Peff\n"},{"id":"321225","messageId":"20170601001520.GB43421@google.com","threadId":"46004","inReplyTo":"20170531051804.w6f7yvz4k5wkrwvc@sigill.intra.peff.net","subject":"Re: [PATCH] docs/config: mention protocol implications of url.insteadOf","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-06-01T00:15:20Z","receivedAt":"2017-06-01T00:15:29Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/31, Jeff King wrote:\n> On Fri, May 26, 2017 at 11:22:37AM -0500, Elliott Cable wrote:\n> \n> > 1. Most simply, better documentation: mention `GIT_PROTOCOL_FROM_USER`\n> > explicitly in the documentation of/near `insteadOf`, most\n> > particularly in the README for `contrib/persistent-https`.\n> \n> I agree that a hint in both places would be helpful. The patch for that\n> is below.\n> \n> > 2. Possibly, special-case “higher-security” porcelain (like\n> > `git-submodule`, as described in 33cfccbbf3) to ignore `insteadOf`\n> > rewrite-rules without additional, special configuration. This way,\n> > `git-submodule` works for ignorant users (like me) out of the box,\n> > just as it previously did, and there's no possible security\n> > compramise.\n> \n> I don't think we can do that. Rewrites of \"git://\" to \"ssh://\" are\n> pretty common (and completely harmless). Besides, I think submodules are\n> a case where you really would want persistent-https to kick in. IIRC,\n> the original use case for that helper is Android development, where a\n> user is likely to update a ton of repositories from the same server all\n> at once. Right now the fetches are all done individually with the \"repo\"\n> tool, but in theory the whole thing could be set up as submodules.\n\nThis right here is why Stefan and I have been working on submodules.\n\n> \n> -- >8 --\n> Subject: [PATCH] docs/config: mention protocol implications of url.insteadOf\n> \n> If a URL rewrite switches the protocol to something\n> nonstandard (like \"persistent-https\" for \"https\"), the user\n> may be bitten by the fact that the default protocol\n> restrictions are different between the two. Let's drop a\n> note in insteadOf that points the user in the right\n> direction.\n> \n> It would be nice if we could make this work out of the box,\n> but we can't without knowing the security implications of\n> the user's rewrite. Only the documentation for a particular\n> remote helper can advise one way or the other. Since we do\n> include the persistent-https helper in contrib/ (and since\n> it was the helper in the real-world case that inspired that\n> patch), let's also drop a note there.\n\nDocumentation changes look sane to me. Thanks for whipping this up!\n\n> \n> Suggested-by: Elliott Cable \n> Signed-off-by: Jeff King \n> ---\n> Documentation/config.txt | 7 +++++++\n> contrib/persistent-https/README | 10 ++++++++++\n> 2 files changed, 17 insertions(+)\n> \n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 43d830ee3..5218ecd37 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -3235,6 +3235,13 @@ url..insteadOf::\n> \tthe best alternative for the particular user, even for a\n> \tnever-before-seen repository on the site. When more than one\n> \tinsteadOf strings match a given URL, the longest match is used.\n> ++\n> +Note that any protocol restrictions will be applied to the rewritten\n> +URL. If the rewrite changes the URL to use a custom protocol or remote\n> +helper, you may need to adjust the `protocol.*.allow` config to permit\n> +the request. In particular, protocols you expect to use for submodules\n> +must be set to `always` rather than the default of `user`. See the\n> +description of `protocol.allow` above.\n> \n> url..pushInsteadOf::\n> \tAny URL that starts with this value will not be pushed to;\n> diff --git a/contrib/persistent-https/README b/contrib/persistent-https/README\n> index f784dd2e6..7c4cd8d25 100644\n> --- a/contrib/persistent-https/README\n> +++ b/contrib/persistent-https/README\n> @@ -35,6 +35,16 @@ to use persistent-https:\n> [url \"persistent-http\"]\n> \tinsteadof = http\n> \n> +You may also want to allow the use of the persistent-https helper for\n> +submodule URLs (since any https URLs pointing to submodules will be\n> +rewritten, and Git's out-of-the-box defaults forbid submodules from\n> +using unknown remote helpers):\n> +\n> +[protocol \"persistent-https\"]\n> +\tallow = always\n> +[protocol \"persistent-http\"]\n> +\tallow = always\n> +\n> \n> #####################################################################\n> # BUILDING FROM SOURCE\n> -- \n> 2.13.0.678.ga17378094\n> \n\n-- \nBrandon Williams\n"}]}