{"thread":{"id":"51103","subject":"Git ransom campaign incident report - May 2019","startedAt":"2019-05-15T17:49:27Z","lastAt":"2019-05-20T17:08:41Z","messageCount":23,"participants":["Martin Langhoff","Ævar Arnfjörð Bjarmason","Jeff King","Johannes Schindelin","Eric Sunshine","René Scharfe"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"375634","messageId":"CACPiFCJdXsrywra8qPU3ebiiGQP3YPC6g-_Eohbfwu_bQgfyVg@mail.gmail.com","threadId":"51103","inReplyTo":null,"subject":"Git ransom campaign incident report - May 2019","fromName":"Martin Langhoff","fromEmail":"martin.langhoff@gmail.com","sentAt":"2019-05-15T17:49:11Z","receivedAt":"2019-05-15T17:49:27Z","isPatch":false,"sender":{"key":"martin.langhoff@gmail.com","avatar":"https://gravatar.com/avatar/1e3f311b6c4c15836501901ca58f8c0b0667246488084ba524d8bc9867e22fd9?d=mp&s=160"},"body":"Spotted this on the internet...\n\nhttps://github.blog/2019-05-14-git-ransom-campaign-incident-report/\n\nHaven't hacked on git for a while, and I am not affiliated with any of\nthe stakeholders. However, reading it, I wanted to slam my head on the\ndesk.\n\nIIRC, git will sanely store a password elsewhere if it gets to prompt\nfor it. Should we be trying to unpack usernames/passwords from HTTP\nurls, and DTRT with them?\n\nAre there other ways this could be made better?\n\ncheers,\n\n\nmartin\n-- \n martin.langhoff@gmail.com\n - ask interesting questions  ~  http://linkedin.com/in/martinlanghoff\n - don't be distracted        ~  http://github.com/martin-langhoff\n   by shiny stuff\n"},{"id":"375639","messageId":"8736lfwnks.fsf@evledraar.gmail.com","threadId":"51103","inReplyTo":"CACPiFCJdXsrywra8qPU3ebiiGQP3YPC6g-_Eohbfwu_bQgfyVg@mail.gmail.com","subject":"Re: Git ransom campaign incident report - May 2019","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-05-15T18:59:47Z","receivedAt":"2019-05-15T18:59:52Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, May 15 2019, Martin Langhoff wrote:\n\n> Spotted this on the internet...\n>\n> https://github.blog/2019-05-14-git-ransom-campaign-incident-report/\n>\n> Haven't hacked on git for a while, and I am not affiliated with any of\n> the stakeholders. However, reading it, I wanted to slam my head on the\n> desk.\n>\n> IIRC, git will sanely store a password elsewhere if it gets to prompt\n> for it. Should we be trying to unpack usernames/passwords from HTTP\n> urls, and DTRT with them?\n>\n> Are there other ways this could be made better?\n\nI think we should do nothing.\n\nThe linked blog post really manages to bury the lead. I guess you'll get\nthat when PR at three different companies gets a say. For those looking\nfor a brief summary, here's mine:\n\n    Some people using git hosting sites \"git clone\"'d https URLs to\n    their repos with username/passwords in them. They then pointed a\n    webserver at their checked-out directory, and got pwned by someone\n    scraping \"/.git/config\" from public websites looking for\n    credentials.\n\nTrying to mitigate this in git is just going to annoy users who are\ndoing this in the context of an otherwise secure workflow. The users who\nwere affected by this are probably also the sort of users who are\nhardcoding their AWS password in some JavaScript checked into their\nproject or whatever, there's only so much you can do.\n\nIt's probably more productive to say convince whoever maintains the\ndefault nginx/apache etc. docker image to default to some Fisher-Price\nmode where dotfiles aren't served up by default.\n\nOr, for GitLab/GitHub etc. to discourage use of https API tokens in\nfavor of SSH deploy keys. OpenSSH goes out of its way to not allow you\nto provide paswords in URLs, on the command-line etc. in anticipation of\nexactly this sort of scenario. Even then I've seen users write say\ndocker images where they manage to hardcode an SSH private key in a\npublic image out of convenience or lazyness (say needing \"git clone\"\nsomething during the image build).\n"},{"id":"375697","messageId":"20190516042739.GH4596@sigill.intra.peff.net","threadId":"51103","inReplyTo":"8736lfwnks.fsf@evledraar.gmail.com","subject":"Re: Git ransom campaign incident report - May 2019","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-16T04:27:39Z","receivedAt":"2019-05-16T04:31:56Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 15, 2019 at 08:59:47PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> \n> On Wed, May 15 2019, Martin Langhoff wrote:\n> \n> > Spotted this on the internet...\n> >\n> > https://github.blog/2019-05-14-git-ransom-campaign-incident-report/\n> >\n> > Haven't hacked on git for a while, and I am not affiliated with any of\n> > the stakeholders. However, reading it, I wanted to slam my head on the\n> > desk.\n> >\n> > IIRC, git will sanely store a password elsewhere if it gets to prompt\n> > for it. Should we be trying to unpack usernames/passwords from HTTP\n> > urls, and DTRT with them?\n> >\n> > Are there other ways this could be made better?\n> \n> I think we should do nothing.\n\nI think so, too.\n\nBut just brainstorming, one thing we _could_ do is issue a warning when\nwe see a password in a URL and say \"hey, what you're doing isn't\nfantastic; considering using a credential helper\".\n\nOf course I suspect there are many cases where people _do_ need to store\nthe password in plaintext, because an automated system needs to fetch\nwith it. They can use the plaintext git-credential-store, but it's\nslightly more hassle. And it doesn't really _solve_ the problem (though\nperhaps it would be harder to accidentally expose it with your web\nserver!).\n\n> Or, for GitLab/GitHub etc. to discourage use of https API tokens in\n> favor of SSH deploy keys. OpenSSH goes out of its way to not allow you\n> to provide paswords in URLs, on the command-line etc. in anticipation of\n> exactly this sort of scenario. Even then I've seen users write say\n> docker images where they manage to hardcode an SSH private key in a\n> public image out of convenience or lazyness (say needing \"git clone\"\n> something during the image build).\n\nYou can have limited-access https tokens, too. But I don't think those\nor deploy keys fix the fundamental problem, because that read access is\nsufficient. The ransom is not \"give us bitcoin or we will delete your\ncode, lock you out of your account, etc\". Those things can easily be\nfixed by complaining to the service provider, who has backups. The\nransom is \"give me bitcoin or I will share your code with the world\".\nSo whatever token the server has to do a fetch is enough to steal the\ncode (though I think deploy keys can be repo-specific, and I'm not sure\nif http tokens are; that expands the attack to _all_ of the user's\nrepos, not just the one the server cares about).\n\nI don't know how many people who git by this actually even want to fetch\nto the server, though. They might just have blindly rsync'd up their\nlocal checkout, and the deploy server actually never needed to know\nabout git in the first place.\n\n-Peff\n"},{"id":"375819","messageId":"nycvar.QRO.7.76.6.1905172121130.46@tvgsbejvaqbjf.bet","threadId":"51103","inReplyTo":"20190516042739.GH4596@sigill.intra.peff.net","subject":"Re: Git ransom campaign incident report - May 2019","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-05-17T19:39:55Z","receivedAt":"2019-05-17T19:39:55Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 16 May 2019, Jeff King wrote:\n\n> On Wed, May 15, 2019 at 08:59:47PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n> >\n> > On Wed, May 15 2019, Martin Langhoff wrote:\n> >\n> > > Spotted this on the internet...\n> > >\n> > > https://github.blog/2019-05-14-git-ransom-campaign-incident-report/\n> > >\n> > > Haven't hacked on git for a while, and I am not affiliated with any of\n> > > the stakeholders. However, reading it, I wanted to slam my head on the\n> > > desk.\n> > >\n> > > IIRC, git will sanely store a password elsewhere if it gets to prompt\n> > > for it. Should we be trying to unpack usernames/passwords from HTTP\n> > > urls, and DTRT with them?\n> > >\n> > > Are there other ways this could be made better?\n> >\n> > I think we should do nothing.\n>\n> I think so, too.\n>\n> But just brainstorming, one thing we _could_ do is issue a warning when\n> we see a password in a URL and say \"hey, what you're doing isn't\n> fantastic; considering using a credential helper\".\n>\n> Of course I suspect there are many cases where people _do_ need to store\n> the password in plaintext, because an automated system needs to fetch\n> with it. They can use the plaintext git-credential-store, but it's\n> slightly more hassle. And it doesn't really _solve_ the problem (though\n> perhaps it would be harder to accidentally expose it with your web\n> server!).\n\nOne thing that we actually *could* do here is to anonymize the URLs stored\nunder remote.origin.url when cloning. In no other circumstance that I can\nthink of do we take an URL from some command-line parameter that is not\n*explicitly* intended for storing in the config.\n\nCombined with that warning \"You cloned via a URL that contains\ncredentials; for security reasons, the credentials were scrubbed before\nstoring this in your Git config. Please consider using a credential\nmanager instead of storing secrets in your Git config.\" this should\nprovide a reasonable compromise.\n\nJudging from looking at my own automated jobs, it does not appear that you\nwould *ever* need to store such credentials in the Git config, anyway. If\nyou need to, say, push to a repository, you can always store the full URL\n(or the credentials) in a secret variable.\n\nCiao,\nDscho\n"},{"id":"375834","messageId":"20190517222031.GA17966@sigill.intra.peff.net","threadId":"51103","inReplyTo":"nycvar.QRO.7.76.6.1905172121130.46@tvgsbejvaqbjf.bet","subject":"Re: Git ransom campaign incident report - May 2019","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-17T22:20:31Z","receivedAt":"2019-05-17T22:20:35Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 17, 2019 at 09:39:55PM +0200, Johannes Schindelin wrote:\n\n> > Of course I suspect there are many cases where people _do_ need to store\n> > the password in plaintext, because an automated system needs to fetch\n> > with it. They can use the plaintext git-credential-store, but it's\n> > slightly more hassle. And it doesn't really _solve_ the problem (though\n> > perhaps it would be harder to accidentally expose it with your web\n> > server!).\n> \n> One thing that we actually *could* do here is to anonymize the URLs stored\n> under remote.origin.url when cloning. In no other circumstance that I can\n> think of do we take an URL from some command-line parameter that is not\n> *explicitly* intended for storing in the config.\n\nGood point. That's a plausible way that these end up in config even when\nthey're not needed and it wasn't intended by the user.\n\n> Combined with that warning \"You cloned via a URL that contains\n> credentials; for security reasons, the credentials were scrubbed before\n> storing this in your Git config. Please consider using a credential\n> manager instead of storing secrets in your Git config.\" this should\n> provide a reasonable compromise.\n\nYeah, we'd definitely want to warn the user. Some people might find it\nirritating because a follow-up fetch would require an interactive\npassword. We might need to make sure the escape hatch is pretty turnkey.\nThe simplest thing is just a \"--save-password\" option to override this,\nbut it would be nice if there's an out-of-the-box solution with a\ncredential helper.\n\nWe can use credential-store as a general solution. That's still\nsticking the password in the filesystem in plaintext, but the file is\n0700 by default, and it's not in the repo directory).\n\nUnfortunately converting:\n\n  git clone https://user@pass:example.com/repo.git\n\ninto a safer use of credential-store is a little tricky, because seeding\nthe store is generally only done interactively. What if we did this:\n\n  1. Do not ever write the password part of a URL into config.\n\n  2. When we extract the user/pass out of a URL, put them into the\n     credential struct, so that when we successfully authenticate, we\n     trigger storage. This _might_ actually happen already, but we\n     should definitely confirm it.\n\n  3. If the user has no credential helper defined, then do one of:\n\n     a. Warn them that the credential was not recorded, but that they\n        can use \"git clone -c credential.helper=store\" as a fallback\n\t(but probably worded in a way to recommend using something\n\tstronger if possible).\n\n\tThis is slightly annoying because following the advice requires\n\tre-cloning. Fixing it up from there is more like:\n\n\t  git config credential.helper store\n\t  git fetch\n\t  [interactively input password]\n\n     b. Just use credential-store. Optionally notify them of what\n         happened (and that they might want to choose a better helper).\n\nI hate the magical-ness of 3b, because credential-store really _isn't_\nthe best choice. It's just better than the current behavior. At the same\ntime, by doing it automatically, the existing flow they were using just\nworks, and is moderately better.\n\n> Judging from looking at my own automated jobs, it does not appear that you\n> would *ever* need to store such credentials in the Git config, anyway. If\n> you need to, say, push to a repository, you can always store the full URL\n> (or the credentials) in a secret variable.\n\nYes, that's definitely the way you _should_ do it. I think the problem\nis that handling secret storage is tricky and system-specific, and the\npeople who are affected here are ones who just didn't think about it. If\nwe can make even a moderate improvement without the user having to do\nanything differently, it's worth considering.\n\n-Peff\n"},{"id":"375835","messageId":"CACPiFC+=co2-yGwoiKqr1qSu8dLmVQZ5NxfbdwOr7xz=a7xpdA@mail.gmail.com","threadId":"51103","inReplyTo":"20190517222031.GA17966@sigill.intra.peff.net","subject":"Re: Git ransom campaign incident report - May 2019","fromName":"Martin Langhoff","fromEmail":"martin.langhoff@gmail.com","sentAt":"2019-05-17T23:13:17Z","receivedAt":"2019-05-17T23:13:31Z","isPatch":false,"sender":{"key":"martin.langhoff@gmail.com","avatar":"https://gravatar.com/avatar/1e3f311b6c4c15836501901ca58f8c0b0667246488084ba524d8bc9867e22fd9?d=mp&s=160"},"body":"On Fri, May 17, 2019 at 6:20 PM Jeff King <peff@peff.net> wrote:\n> I hate the magical-ness of 3b, because credential-store really _isn't_\n> the best choice. It's just better than the current behavior. At the same\n> time, by doing it automatically, the existing flow they were using just\n> works, and is moderately better.\n\nQuite a bit better. It sits in a different directory, and with tight\npermissions.\n\nOverall -- thank you! That's the process I was picturing. Even just\nscrubbing the credentials -- your \"step 1\" -- would be a significant\nimprovement, if a bit unfriendly.\n\n> > Judging from looking at my own automated jobs, it does not appear that you\n> > would *ever* need to store such credentials in the Git config, anyway. If\n> > you need to, say, push to a repository, you can always store the full URL\n> > (or the credentials) in a secret variable.\n>\n> Yes, that's definitely the way you _should_ do it. I think the problem\n\nThe key thing are the credentials, and there are much better solutions\nfor this -- ssh keys, etc.\n\nThis isn't for thoughtful users, this is to save unaware users from\nthemselves. Maybe they'll and hurt themselves with something else, but\nthat's part of removing sharp edges from a product.\n\ncheers,\n\n\nm\n\n\n\n\n--\n martin.langhoff@gmail.com\n - ask interesting questions  ~  http://linkedin.com/in/martinlanghoff\n - don't be distracted        ~  http://github.com/martin-langhoff\n   by shiny stuff\n"},{"id":"375886","messageId":"20190519050724.GA26179@sigill.intra.peff.net","threadId":"51103","inReplyTo":"20190517222031.GA17966@sigill.intra.peff.net","subject":"Re: Git ransom campaign incident report - May 2019","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-19T05:07:24Z","receivedAt":"2019-05-19T18:34:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 17, 2019 at 06:20:31PM -0400, Jeff King wrote:\n\n> What if we did this:\n> \n>   1. Do not ever write the password part of a URL into config.\n> \n>   2. When we extract the user/pass out of a URL, put them into the\n>      credential struct, so that when we successfully authenticate, we\n>      trigger storage. This _might_ actually happen already, but we\n>      should definitely confirm it.\n> \n>   3. If the user has no credential helper defined, then do one of:\n> \n>      a. Warn them that the credential was not recorded, but that they\n>         can use \"git clone -c credential.helper=store\" as a fallback\n> \t(but probably worded in a way to recommend using something\n> \tstronger if possible).\n> \n> \tThis is slightly annoying because following the advice requires\n> \tre-cloning. Fixing it up from there is more like:\n> \n> \t  git config credential.helper store\n> \t  git fetch\n> \t  [interactively input password]\n> \n>      b. Just use credential-store. Optionally notify them of what\n>          happened (and that they might want to choose a better helper).\n\nSo here are patches to do that. Step 2 is indeed how things work\nalready, so nothing was needed there (there are still 3 patches because\nthere was a bit of prep-work ;) ).\n\nI did 3b here: automagically enabling credential-store. I'm still on the\nfence on whether that's a good idea or not.\n\nI don't have any data on how many victims of this ransom campaign might\nhave been helped by this. But it certainly seems like a decent best\npractice. I'd hope that _most_ people have moved on to using a\ncredential helper and supplying the initial password interactively these\ndays; this is really just aimed at people who don't know better. So the\ngoal is making them a bit more secure, but also educating them about the\nother options. Hopefully without breaking any workflows. :)\n\n-Peff\n\n  [1/3]: transport_anonymize_url(): support retaining username\n  [2/3]: clone: avoid storing URL passwords in config\n  [3/3]: clone: auto-enable git-credential-store when necessary\n\n builtin/clone.c            | 37 +++++++++++++++++++++++++++++++++++--\n credential.c               | 13 +++++++++++++\n credential.h               |  6 ++++++\n t/t5550-http-fetch-dumb.sh | 12 ++++++++++++\n transport.c                | 21 ++++++++++++++-------\n transport.h                | 11 ++++++++++-\n 6 files changed, 90 insertions(+), 10 deletions(-)\n"},{"id":"375887","messageId":"20190519051031.GA19434@sigill.intra.peff.net","threadId":"51103","inReplyTo":"20190519050724.GA26179@sigill.intra.peff.net","subject":"[PATCH 1/3] transport_anonymize_url(): support retaining username","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-19T05:10:32Z","receivedAt":"2019-05-19T18:37:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we anonymize URLs to show in messages, we strip out both the\nusername and password (if any). But there are also contexts where we\nshould strip out the password (to avoid leaking it) but retain the\nusername.\n\nLet's generalize transport_anonymize_url() to support both cases. We'll\ngive it a new name since the password-only mode isn't really\n\"anonymizing\", but keep the old name as a synonym to avoid disrupting\nexisting callers.\n\nNote that there are actually three places we parse URLs, and this\nfunctionality _could_ go into any of them:\n\n  - transport_anonymize_url(), which we modify here\n\n  - the urlmatch.c code parses a URL into its constituent parts, from\n    which we could easily remove the elements we want to drop and\n    re-format it as a single URL. But its parsing also normalizes\n    elements (e.g., downcasing hostnames).  This isn't wrong, but it's\n    more friendly if we can leave the rest of the URL untouched.\n\n  - credential_form_url() parses a URL and decodes the specific\n    elements, but it's hard to convert it back into a regular URL. It\n    treats \"host:port\" as a single unit, meaning it needs to be\n    re-encoded specially (since a colon would otherwise end\n    percent-encoded).\n\nSince transport_anonymize_url() seemed closest to what we want here, I\nused that as the base.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI think it would be beneficial to unify these three cases under a single\nparser, but it seemed like too big a rabbit hole for this topic. Of the\nthree, the urlmatch one seems the most mature. I think if we could\nsimply separate the normalization from the parsing/decoding, the others\ncould build on top of it. It might also require some careful thinking\nabout how pseudo-urls like ssh \"host:path\" interact.\n\nI won't call that a #leftoverbits, because it's more of a feast. :)\n\n transport.c | 21 ++++++++++++++-------\n transport.h | 11 ++++++++++-\n 2 files changed, 24 insertions(+), 8 deletions(-)\n\ndiff --git a/transport.c b/transport.c\nindex f1fcd2c4b0..ba61e57295 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1335,11 +1335,7 @@ int transport_disconnect(struct transport *transport)\n \treturn ret;\n }\n \n-/*\n- * Strip username (and password) from a URL and return\n- * it in a newly allocated string.\n- */\n-char *transport_anonymize_url(const char *url)\n+char *transport_strip_url(const char *url, int strip_user)\n {\n \tchar *scheme_prefix, *anon_part;\n \tsize_t anon_len, prefix_len = 0;\n@@ -1348,7 +1344,10 @@ char *transport_anonymize_url(const char *url)\n \tif (url_is_local_not_ssh(url) || !anon_part)\n \t\tgoto literal_copy;\n \n-\tanon_len = strlen(++anon_part);\n+\tanon_len = strlen(anon_part);\n+\tif (strip_user)\n+\t\tanon_part++;\n+\n \tscheme_prefix = strstr(url, \"://\");\n \tif (!scheme_prefix) {\n \t\tif (!strchr(anon_part, ':'))\n@@ -1373,7 +1372,15 @@ char *transport_anonymize_url(const char *url)\n \t\tcp = strchr(scheme_prefix + 3, '/');\n \t\tif (cp && cp < anon_part)\n \t\t\tgoto literal_copy;\n-\t\tprefix_len = scheme_prefix - url + 3;\n+\n+\t\tif (strip_user)\n+\t\t\tprefix_len = scheme_prefix - url + 3;\n+\t\telse {\n+\t\t\tcp = strchr(scheme_prefix + 3, ':');\n+\t\t\tif (cp && cp > anon_part)\n+\t\t\t\tgoto literal_copy; /* username only */\n+\t\t\tprefix_len = cp - url;\n+\t\t}\n \t}\n \treturn xstrfmt(\"%.*s%.*s\", (int)prefix_len, url,\n \t\t       (int)anon_len, anon_part);\ndiff --git a/transport.h b/transport.h\nindex 06e06d3d89..6d8c99ac91 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -243,10 +243,19 @@ const struct ref *transport_get_remote_refs(struct transport *transport,\n int transport_fetch_refs(struct transport *transport, struct ref *refs);\n void transport_unlock_pack(struct transport *transport);\n int transport_disconnect(struct transport *transport);\n-char *transport_anonymize_url(const char *url);\n void transport_take_over(struct transport *transport,\n \t\t\t struct child_process *child);\n \n+/*\n+ * Strip password and optionally username from a URL and return\n+ * it in a newly allocated string (even if nothing was stripped).\n+ */\n+char *transport_strip_url(const char *url, int strip_username);\n+static inline char *transport_anonymize_url(const char *url)\n+{\n+\treturn transport_strip_url(url, 1);\n+}\n+\n int transport_connect(struct transport *transport, const char *name,\n \t\t      const char *exec, int fd[2]);\n \n-- \n2.22.0.rc0.583.g23d90da2b3\n\n"},{"id":"375888","messageId":"20190519051218.GB19434@sigill.intra.peff.net","threadId":"51103","inReplyTo":"20190519050724.GA26179@sigill.intra.peff.net","subject":"[PATCH 2/3] clone: avoid storing URL passwords in config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-19T05:12:19Z","receivedAt":"2019-05-19T18:39:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If you clone a URL with a password, like:\n\n  git clone https://user:pass@example.com/repo.git\n\nwe'll write that literal URL, including the password into .git/config.\nThis can lead to accidentally disclosing it, since .git/config isn't\ngenerally assumed to be a secret. In particular, it's very prone to\naccidentally exposing by a webserver:\n\n  1. It's actually _in_ the repo directory, and it's not uncommon for\n     people to clone (or copy) the contents to a web-accessible\n     directory.\n\n  2. The filesystem permissions are not restrictive. So cloning as user\n     \"bob\" would usually produce a config file readable by user \"httpd\".\n\nLet's strip the password out before storing it. There are two things to\nnote:\n\n  - we must (and do) retain the username here. Both as a convenience so\n    the user does not have to input it again, but also because\n    credential helpers use it as part of the lookup key (which matters\n    if you use two different usernames with the same host).\n\n  - since we don't record the password anywhere, a follow-up fetch will\n    fail. This is a regression for some workflows, of course, but one\n    we'll fix in a future commit. For now we'll warn the user about\n    what's happening and add a failing test to show the problem.\n\nIdea-stolen-from: Johannes Schindelin <Johannes.Schindelin@gmx.de>\nSigned-off-by: Jeff King <peff@peff.net>\n---\nIf we stop here, we probably ought to be pointing users at credential\nhelpers in the advice. I didn't bother here, because the next commit\nends up rewriting most of this advice anyway.\n\n builtin/clone.c            | 24 ++++++++++++++++++++++--\n t/t5550-http-fetch-dumb.sh | 12 ++++++++++++\n 2 files changed, 34 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex ffdd94e8f6..15fffc3268 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -891,10 +891,18 @@ static int dir_exists(const char *path)\n \treturn !stat(path, &sb);\n }\n \n+static const char sanitized_url_advice[] = N_(\n+\"The URL you provided to Git contains a password. It will be\\n\"\n+\"used to clone the repository, but to avoid accidental disclosure\\n\"\n+\"the password will not be recorded. Further fetches from the remote\\n\"\n+\"may require you to provide the password interactively.\\n\"\n+);\n+\n int cmd_clone(int argc, const char **argv, const char *prefix)\n {\n \tint is_bundle = 0, is_local;\n \tconst char *repo_name, *repo, *work_tree, *git_dir;\n+\tchar *sanitized_repo;\n \tchar *path, *dir;\n \tint dest_exists;\n \tconst struct ref *refs, *remote_head;\n@@ -1079,9 +1087,15 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tstrbuf_addf(&branch_top, \"refs/remotes/%s/\", option_origin);\n \t}\n \n+\tsanitized_repo = transport_strip_url(repo, 0);\n+\tif (strcmp(repo, sanitized_repo)) {\n+\t\twarning(_(\"omitting password while storing URL in on-disk config\"));\n+\t\tadvise(_(sanitized_url_advice));\n+\t}\n \tstrbuf_addf(&key, \"remote.%s.url\", option_origin);\n-\tgit_config_set(key.buf, repo);\n+\tgit_config_set(key.buf, sanitized_repo);\n \tstrbuf_reset(&key);\n+\tFREE_AND_NULL(sanitized_repo);\n \n \tif (option_no_tags) {\n \t\tstrbuf_addf(&key, \"remote.%s.tagOpt\", option_origin);\n@@ -1098,7 +1112,13 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t    branch_top.buf);\n \trefspec_append(&remote->fetch, default_refspec.buf);\n \n-\ttransport = transport_get(remote, remote->url[0]);\n+\t/*\n+\t * Override remote->url here, since that will be the sanitized version\n+\t * we wrote to the config. If there was a password, we need to use the\n+\t * version that has it (and if there isn't, the two are identical\n+\t * anyway).\n+\t */\n+\ttransport = transport_get(remote, repo);\n \ttransport_set_verbosity(transport, option_verbosity, option_progress);\n \ttransport->family = family;\n \ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex b811d89cfd..d8c85f3126 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -64,6 +64,18 @@ test_expect_success 'http auth can use user/pass in URL' '\n \texpect_askpass none\n '\n \n+test_expect_success 'username is retained in URL, password is not' '\n+\tgit -C clone-auth-none config remote.origin.url >url &&\n+\tgrep user url &&\n+\t! grep pass url\n+'\n+\n+test_expect_failure 'fetch of password-URL clone uses stored auth' '\n+\tset_askpass wrong &&\n+\tgit -C clone-auth-none fetch &&\n+\texpect_askpass none\n+'\n+\n test_expect_success 'http auth can use just user in URL' '\n \tset_askpass wrong pass@host &&\n \tgit clone \"$HTTPD_URL_USER/auth/dumb/repo.git\" clone-auth-pass &&\n-- \n2.22.0.rc0.583.g23d90da2b3\n\n"},{"id":"375889","messageId":"20190519051604.GC19434@sigill.intra.peff.net","threadId":"51103","inReplyTo":"20190519050724.GA26179@sigill.intra.peff.net","subject":"[PATCH 3/3] clone: auto-enable git-credential-store when necessary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-19T05:16:04Z","receivedAt":"2019-05-19T18:42:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If the user clones with a URL containing a password and has no\ncredential helper configured, we're stuck. We don't want to write the\npassword into .git/config because that risks accidentally disclosing it.\nBut if we don't record it somewhere, subsequent fetches will fail unless\nthe user is there to input the password.\n\nThe best advice we can give the user is to set up a credential helper.\nBut we can actually go a step further and enable the \"store\" helper for\nthem. This still records the password in plaintext, but:\n\n  1. It's not inside the repo directory, which makes it slightly less\n     likely to be disclosed.\n\n  2. The permissions on the storage file are tighter than what would be\n     on .git/config.\n\nSo this is generally a security win over the old behavior of writing it\ninto .git/config. And it's a usability win over the more recent behavior\nof just forgetting the password entirely.\n\nThe biggest downside is that it's a bit magical from the user's\nperspective, because now the password is off in some other file (usually\n~/.git-credentials, but sometimes in $XDG_CONFIG_HOME). Which\ncomplicates things if they want to purge the repo and password, for\nexample, because now they can't just delete the repository directory.\n\nThe file location is documented, though, and we point people to the\ndocumentation. So perhaps it will be enough (and better still, may lead\nto them configuring a more secure helper).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nIf we do decide this is too magical, I think the best alternate path is\nto advise them on credential helpers, and how to seed the password into\nthe helper (basically configure the helper and then \"git fetch\"\nshould work).\n\nOne other thing I wondered: if they do have a helper configured this\npatch will omit the advice entirely, but we'll still print the warning.\nIs that useful (to remind them that passwords in URLs are a bad thing),\nor is it just annoying?\n\n builtin/clone.c            | 19 ++++++++++++++++---\n credential.c               | 13 +++++++++++++\n credential.h               |  6 ++++++\n t/t5550-http-fetch-dumb.sh |  2 +-\n 4 files changed, 36 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 15fffc3268..94d2659154 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -31,6 +31,7 @@\n #include \"packfile.h\"\n #include \"list-objects-filter-options.h\"\n #include \"object-store.h\"\n+#include \"credential.h\"\n \n /*\n  * Overall FIXMEs:\n@@ -894,8 +895,14 @@ static int dir_exists(const char *path)\n static const char sanitized_url_advice[] = N_(\n \"The URL you provided to Git contains a password. It will be\\n\"\n \"used to clone the repository, but to avoid accidental disclosure\\n\"\n-\"the password will not be recorded. Further fetches from the remote\\n\"\n-\"may require you to provide the password interactively.\\n\"\n+\"the password will not be recorded in the repository config.\\n\"\n+\"Since you have no credential helper configured, the \\\"store\\\" helper\\n\"\n+\"has been enabled for this repository, and will provide the password\\n\"\n+\"for further fetches.\\n\"\n+\"\\n\"\n+\"Note that the password is still stored in plaintext in the filesystem;\\n\"\n+\"consider configuring a more secure helper. See \\\"git help gitcredentials\\\"\\n\"\n+\"and \\\"git help git-credential-store\\\" for details.\\n\"\n );\n \n int cmd_clone(int argc, const char **argv, const char *prefix)\n@@ -1090,7 +1097,13 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tsanitized_repo = transport_strip_url(repo, 0);\n \tif (strcmp(repo, sanitized_repo)) {\n \t\twarning(_(\"omitting password while storing URL in on-disk config\"));\n-\t\tadvise(_(sanitized_url_advice));\n+\t\tif (!url_has_credential_helper(sanitized_repo)) {\n+\t\t\tstrbuf_addf(&key, \"credential.%s.helper\",\n+\t\t\t\t    sanitized_repo);\n+\t\t\tgit_config_set(key.buf, \"store\");\n+\t\t\tstrbuf_reset(&key);\n+\t\t\tadvise(_(sanitized_url_advice));\n+\t\t}\n \t}\n \tstrbuf_addf(&key, \"remote.%s.url\", option_origin);\n \tgit_config_set(key.buf, sanitized_repo);\ndiff --git a/credential.c b/credential.c\nindex 62be651b03..0a70edcee5 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -372,3 +372,16 @@ void credential_from_url(struct credential *c, const char *url)\n \t\t\t*p-- = '\\0';\n \t}\n }\n+\n+int url_has_credential_helper(const char *url)\n+{\n+\tstruct credential c = CREDENTIAL_INIT;\n+\tint ret;\n+\n+\tcredential_from_url(&c, url);\n+\tcredential_apply_config(&c);\n+\tret = c.helpers.nr > 0;\n+\n+\tcredential_clear(&c);\n+\treturn ret;\n+}\ndiff --git a/credential.h b/credential.h\nindex 6b0cd16be2..e6d6d6fa40 100644\n--- a/credential.h\n+++ b/credential.h\n@@ -32,4 +32,10 @@ void credential_from_url(struct credential *, const char *url);\n int credential_match(const struct credential *have,\n \t\t     const struct credential *want);\n \n+/*\n+ * Return true if feeding \"url\" to the credential system would trigger one\n+ * or more helpers.\n+ */\n+int url_has_credential_helper(const char *url);\n+\n #endif /* CREDENTIAL_H */\ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex d8c85f3126..2723e91ae0 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -70,7 +70,7 @@ test_expect_success 'username is retained in URL, password is not' '\n \t! grep pass url\n '\n \n-test_expect_failure 'fetch of password-URL clone uses stored auth' '\n+test_expect_success 'fetch of password-URL clone uses stored auth' '\n \tset_askpass wrong &&\n \tgit -C clone-auth-none fetch &&\n \texpect_askpass none\n-- \n2.22.0.rc0.583.g23d90da2b3\n"},{"id":"375895","messageId":"CAPig+cTDzhaRD8MMqFp2EwGmhsR3u=t9en4+JkU2Vkz-S4Bu5A@mail.gmail.com","threadId":"51103","inReplyTo":"20190519051031.GA19434@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] transport_anonymize_url(): support retaining username","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-05-19T23:28:37Z","receivedAt":"2019-05-19T23:28:52Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, May 19, 2019 at 2:37 PM Jeff King <peff@peff.net> wrote:\n> [...]\n>   - credential_form_url() parses a URL and decodes the specific\n\ns/form/from/\n\n>     elements, but it's hard to convert it back into a regular URL. It\n>     treats \"host:port\" as a single unit, meaning it needs to be\n>     re-encoded specially (since a colon would otherwise end\n>     percent-encoded).\n> [...]\n> Signed-off-by: Jeff King <peff@peff.net>\n"},{"id":"375910","messageId":"CAPig+cTkjJjuyrDOUh92B16an+wy9OnZgyKY0-bihWnzyWsoKg@mail.gmail.com","threadId":"51103","inReplyTo":"20190519051604.GC19434@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] clone: auto-enable git-credential-store when necessary","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-05-20T11:28:08Z","receivedAt":"2019-05-20T11:28:23Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, May 19, 2019 at 2:42 PM Jeff King <peff@peff.net> wrote:\n> If the user clones with a URL containing a password and has no\n> credential helper configured, we're stuck. We don't want to write the\n> password into .git/config because that risks accidentally disclosing it.\n> But if we don't record it somewhere, subsequent fetches will fail unless\n> the user is there to input the password.\n>\n> But we can actually go a step further and enable the \"store\" helper for\n> them. [...]\n>\n> The biggest downside is that it's a bit magical from the user's\n> perspective, because now the password is off in some other file (usually\n> ~/.git-credentials, but sometimes in $XDG_CONFIG_HOME). Which\n> complicates things if they want to purge the repo and password, for\n> example, because now they can't just delete the repository directory.\n>\n> The file location is documented, though, and we point people to the\n> documentation. So perhaps it will be enough (and better still, may lead\n> to them configuring a more secure helper).\n\nI'm trying to decide how I feel about this based upon my own\nexperience recently of having my password magically stored by Git for\nWindows without warning or consent on a computer which was not my own\nbut on which I needed to access a private GitHub repository. Although\nthe situation is not perfectly analogous, the concern of having one's\npassword magically squirreled-away _somewhere_ unexpectedly is the\nsame. Being unfamiliar with Git for Windows's credential helper or\nWindows credential management in general, I experienced more than a\nfew minutes of consternation and alarm before finally figuring out\nwhere Git for Windows had stored my password and how to remove it. The\nsense of alarm and discomfort likely would have not arisen had the\ncredential helper given me the opportunity to approve or deny the\naction.\n\n>  static const char sanitized_url_advice[] = N_(\n>  \"The URL you provided to Git contains a password. It will be\\n\"\n>  \"used to clone the repository, but to avoid accidental disclosure\\n\"\n> +\"the password will not be recorded in the repository config.\\n\"\n> +\"Since you have no credential helper configured, the \\\"store\\\" helper\\n\"\n> +\"has been enabled for this repository, and will provide the password\\n\"\n> +\"for further fetches.\\n\"\n> +\"\\n\"\n> +\"Note that the password is still stored in plaintext in the filesystem;\\n\"\n> +\"consider configuring a more secure helper. See \\\"git help gitcredentials\\\"\\n\"\n> +\"and \\\"git help git-credential-store\\\" for details.\\n\"\n>  );\n\nGive the above experience, one way to mitigate such feelings of alarm\nmight, at a minimum, be for this message to say where the password is\nbeing stored (and, possibly, how to remove it) so the user can do so\nimmediately if desired. Prompting the user to approve or deny the\naction might also go a long way toward making this more palatable\n(assuming the session is interactive).\n"},{"id":"375917","messageId":"20190520123110.GE11212@sigill.intra.peff.net","threadId":"51103","inReplyTo":"CAPig+cTkjJjuyrDOUh92B16an+wy9OnZgyKY0-bihWnzyWsoKg@mail.gmail.com","subject":"Re: [PATCH 3/3] clone: auto-enable git-credential-store when necessary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-20T12:31:10Z","receivedAt":"2019-05-20T12:31:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 20, 2019 at 07:28:08AM -0400, Eric Sunshine wrote:\n\n> > The biggest downside is that it's a bit magical from the user's\n> > perspective, because now the password is off in some other file (usually\n> > ~/.git-credentials, but sometimes in $XDG_CONFIG_HOME). Which\n> > complicates things if they want to purge the repo and password, for\n> > example, because now they can't just delete the repository directory.\n> >\n> > The file location is documented, though, and we point people to the\n> > documentation. So perhaps it will be enough (and better still, may lead\n> > to them configuring a more secure helper).\n> \n> I'm trying to decide how I feel about this based upon my own\n> experience recently of having my password magically stored by Git for\n> Windows without warning or consent on a computer which was not my own\n> but on which I needed to access a private GitHub repository. Although\n> the situation is not perfectly analogous, the concern of having one's\n> password magically squirreled-away _somewhere_ unexpectedly is the\n> same. Being unfamiliar with Git for Windows's credential helper or\n> Windows credential management in general, I experienced more than a\n> few minutes of consternation and alarm before finally figuring out\n> where Git for Windows had stored my password and how to remove it. The\n> sense of alarm and discomfort likely would have not arisen had the\n> credential helper given me the opportunity to approve or deny the\n> action.\n\nThanks, that's a good elaboration of the uneasiness I was feeling. This\npatch is better than the status quo in that your password was already\nbeing squirreled away in plaintext, and now it's at least locked down\nwith filesystem permissions. But it's clearly not as far as we could go.\nI was mostly just afraid to break existing workflows, but maybe we\nshould be more opinionated.\n\nOr maybe we just need to give more specific details about how to back\nout the change.\n\n> > +\"Note that the password is still stored in plaintext in the filesystem;\\n\"\n> > +\"consider configuring a more secure helper. See \\\"git help gitcredentials\\\"\\n\"\n> > +\"and \\\"git help git-credential-store\\\" for details.\\n\"\n> >  );\n> \n> Give the above experience, one way to mitigate such feelings of alarm\n> might, at a minimum, be for this message to say where the password is\n> being stored (and, possibly, how to remove it) so the user can do so\n> immediately if desired. Prompting the user to approve or deny the\n> action might also go a long way toward making this more palatable\n> (assuming the session is interactive).\n\nI actually thought about pointing to the file, but it's non-trivial to\ndo so (there's a bunch of internal logic in credential-store to decide\nbetween $HOME and XDG locations).\n\nI think what we really need are better commands to manage credentials\nindependent of helpers, and then we could recommend a simple command to\nclear a credential that you don't want to have stored. Right now I think\nthe best you can do is:\n\n  echo url=https://example.com | git credential reject\n\nbut:\n\n  - \"reject\" is a funny term; this comes from the C code, which thinks\n    of it in terms of the server approving/rejecting (and that makes\n    sense for scripts calling this command). But at the helper level,\n    the operations are really store/erase. We probably ought to support\n    those names, too.\n\n  - piping the credential protocol is slightly awkward; we probably\n    ought to allow a url on the command line, and avoid reading stdin if\n    we get one.\n\nThat would give us:\n\n  git credential erase https://example.com\n\nwhich is really quite readable. :)\n\nLikewise, if we choose not to auto-enable the store helper, we'd\nprobably want to give advice on seeding your password. Right now that\nis:\n\n  echo url=https://example.com | git credential fill | git credential approve\n\nwhich is...not intuitive. It would make sense to me to have a \"seed\"\noperation which does a fill/approve together. Maybe that should just be\nwhat \"store\" does, which would allow:\n\n  $ git credential store https://example.com\n  Username for 'https://example.com':\n  Password for 'https://user@example.com':\n\n(Of course you can also just \"git fetch\" to get prompted, but it seems\nlike this shouldn't require a network operation if you don't want it\nto).\n\n-Peff\n"},{"id":"375923","messageId":"87imu5ut4x.fsf@evledraar.gmail.com","threadId":"51103","inReplyTo":"20190519051604.GC19434@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] clone: auto-enable git-credential-store when necessary","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-05-20T13:56:14Z","receivedAt":"2019-05-20T13:56:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, May 19 2019, Jeff King wrote:\n\n> If the user clones with a URL containing a password and has no\n> credential helper configured, we're stuck. We don't want to write the\n> password into .git/config because that risks accidentally disclosing it.\n> But if we don't record it somewhere, subsequent fetches will fail unless\n> the user is there to input the password.\n>\n> The best advice we can give the user is to set up a credential helper.\n> But we can actually go a step further and enable the \"store\" helper for\n> them. This still records the password in plaintext, but:\n>\n>   1. It's not inside the repo directory, which makes it slightly less\n>      likely to be disclosed.\n>\n>   2. The permissions on the storage file are tighter than what would be\n>      on .git/config.\n>\n> So this is generally a security win over the old behavior of writing it\n> into .git/config. And it's a usability win over the more recent behavior\n> of just forgetting the password entirely.\n>\n> The biggest downside is that it's a bit magical from the user's\n> perspective, because now the password is off in some other file (usually\n> ~/.git-credentials, but sometimes in $XDG_CONFIG_HOME). Which\n> complicates things if they want to purge the repo and password, for\n> example, because now they can't just delete the repository directory.\n>\n> The file location is documented, though, and we point people to the\n> documentation. So perhaps it will be enough (and better still, may lead\n> to them configuring a more secure helper).\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> If we do decide this is too magical, I think the best alternate path is\n> to advise them on credential helpers, and how to seed the password into\n> the helper (basically configure the helper and then \"git fetch\"\n> should work).\n>\n> One other thing I wondered: if they do have a helper configured this\n> patch will omit the advice entirely, but we'll still print the warning.\n> Is that useful (to remind them that passwords in URLs are a bad thing),\n> or is it just annoying?\n>\n>  builtin/clone.c            | 19 ++++++++++++++++---\n>  credential.c               | 13 +++++++++++++\n>  credential.h               |  6 ++++++\n>  t/t5550-http-fetch-dumb.sh |  2 +-\n>  4 files changed, 36 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 15fffc3268..94d2659154 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -31,6 +31,7 @@\n>  #include \"packfile.h\"\n>  #include \"list-objects-filter-options.h\"\n>  #include \"object-store.h\"\n> +#include \"credential.h\"\n>\n>  /*\n>   * Overall FIXMEs:\n> @@ -894,8 +895,14 @@ static int dir_exists(const char *path)\n>  static const char sanitized_url_advice[] = N_(\n>  \"The URL you provided to Git contains a password. It will be\\n\"\n>  \"used to clone the repository, but to avoid accidental disclosure\\n\"\n> -\"the password will not be recorded. Further fetches from the remote\\n\"\n> -\"may require you to provide the password interactively.\\n\"\n> +\"the password will not be recorded in the repository config.\\n\"\n> +\"Since you have no credential helper configured, the \\\"store\\\" helper\\n\"\n> +\"has been enabled for this repository, and will provide the password\\n\"\n> +\"for further fetches.\\n\"\n> +\"\\n\"\n> +\"Note that the password is still stored in plaintext in the filesystem;\\n\"\n> +\"consider configuring a more secure helper. See \\\"git help gitcredentials\\\"\\n\"\n> +\"and \\\"git help git-credential-store\\\" for details.\\n\"\n>  );\n>\n>  int cmd_clone(int argc, const char **argv, const char *prefix)\n> @@ -1090,7 +1097,13 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \tsanitized_repo = transport_strip_url(repo, 0);\n>  \tif (strcmp(repo, sanitized_repo)) {\n>  \t\twarning(_(\"omitting password while storing URL in on-disk config\"));\n> -\t\tadvise(_(sanitized_url_advice));\n> +\t\tif (!url_has_credential_helper(sanitized_repo)) {\n> +\t\t\tstrbuf_addf(&key, \"credential.%s.helper\",\n> +\t\t\t\t    sanitized_repo);\n> +\t\t\tgit_config_set(key.buf, \"store\");\n> +\t\t\tstrbuf_reset(&key);\n> +\t\t\tadvise(_(sanitized_url_advice));\n> +\t\t}\n>  \t}\n>  \tstrbuf_addf(&key, \"remote.%s.url\", option_origin);\n>  \tgit_config_set(key.buf, sanitized_repo);\n> diff --git a/credential.c b/credential.c\n> index 62be651b03..0a70edcee5 100644\n> --- a/credential.c\n> +++ b/credential.c\n> @@ -372,3 +372,16 @@ void credential_from_url(struct credential *c, const char *url)\n>  \t\t\t*p-- = '\\0';\n>  \t}\n>  }\n> +\n> +int url_has_credential_helper(const char *url)\n> +{\n> +\tstruct credential c = CREDENTIAL_INIT;\n> +\tint ret;\n> +\n> +\tcredential_from_url(&c, url);\n> +\tcredential_apply_config(&c);\n> +\tret = c.helpers.nr > 0;\n> +\n> +\tcredential_clear(&c);\n> +\treturn ret;\n> +}\n> diff --git a/credential.h b/credential.h\n> index 6b0cd16be2..e6d6d6fa40 100644\n> --- a/credential.h\n> +++ b/credential.h\n> @@ -32,4 +32,10 @@ void credential_from_url(struct credential *, const char *url);\n>  int credential_match(const struct credential *have,\n>  \t\t     const struct credential *want);\n>\n> +/*\n> + * Return true if feeding \"url\" to the credential system would trigger one\n> + * or more helpers.\n> + */\n> +int url_has_credential_helper(const char *url);\n> +\n>  #endif /* CREDENTIAL_H */\n> diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\n> index d8c85f3126..2723e91ae0 100755\n> --- a/t/t5550-http-fetch-dumb.sh\n> +++ b/t/t5550-http-fetch-dumb.sh\n> @@ -70,7 +70,7 @@ test_expect_success 'username is retained in URL, password is not' '\n>  \t! grep pass url\n>  '\n>\n> -test_expect_failure 'fetch of password-URL clone uses stored auth' '\n> +test_expect_success 'fetch of password-URL clone uses stored auth' '\n>  \tset_askpass wrong &&\n>  \tgit -C clone-auth-none fetch &&\n>  \texpect_askpass none\n\nI've only looked at this very briefly, there's a regression here where\nyou're assuming that having a configured credential helper means it\nworks.\n\nI.e. I have a ~/.gitconfig where I point to some-gnome-thing-or-other\nwhat doesn't exist on my VPS in my ~/.gitconfig, cloning just warns\nabout it being missing, but will store the password in the repo.\n\nWith this you detect that I have the helper, don't store it, but then my\nhelper doesn't work, whereas this worked before.\n\nThat's an X-Y problem of config includes being rather limited (now I'm\njust putting up with the error), but I expect this'll apply to others.\n"},{"id":"375924","messageId":"20190520140825.GA31580@sigill.intra.peff.net","threadId":"51103","inReplyTo":"87imu5ut4x.fsf@evledraar.gmail.com","subject":"Re: [PATCH 3/3] clone: auto-enable git-credential-store when necessary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-20T14:08:25Z","receivedAt":"2019-05-20T14:08:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 20, 2019 at 03:56:14PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > -test_expect_failure 'fetch of password-URL clone uses stored auth' '\n> > +test_expect_success 'fetch of password-URL clone uses stored auth' '\n> >  \tset_askpass wrong &&\n> >  \tgit -C clone-auth-none fetch &&\n> >  \texpect_askpass none\n> \n> I've only looked at this very briefly, there's a regression here where\n> you're assuming that having a configured credential helper means it\n> works.\n> \n> I.e. I have a ~/.gitconfig where I point to some-gnome-thing-or-other\n> what doesn't exist on my VPS in my ~/.gitconfig, cloning just warns\n> about it being missing, but will store the password in the repo.\n> \n> With this you detect that I have the helper, don't store it, but then my\n> helper doesn't work, whereas this worked before.\n\nThere are more cases beyond that, too. You might have a helper defined\nwhich doesn't actually store passwords, but just sometimes tries to\nprovide one. My thinking was that if you're clueful enough to have\nconfigured helpers, you can probably deal with the fallout. But you're\nright that it may still be a regression in the sense that the user may\nstill have to actually _do_ something to get their fetch to work.\n\nI guess a more robust version of this is that _after_ the successful\nclone, we could ask the credential system \"hey, do you have the\ncredential for $URL?\". And if it can't answer, then we can take action\n(whether that action is setting up credential-store and seeding it with\nthe password, or just advising the user about the situation).\n\n-Peff\n"},{"id":"375927","messageId":"nycvar.QRO.7.76.6.1905201339540.46@tvgsbejvaqbjf.bet","threadId":"51103","inReplyTo":"20190519050724.GA26179@sigill.intra.peff.net","subject":"Re: Git ransom campaign incident report - May 2019","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-05-20T14:43:31Z","receivedAt":"2019-05-20T14:43:57Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Sun, 19 May 2019, Jeff King wrote:\n\n> On Fri, May 17, 2019 at 06:20:31PM -0400, Jeff King wrote:\n>\n> > What if we did this:\n> >\n> >   1. Do not ever write the password part of a URL into config.\n> >\n> >   2. When we extract the user/pass out of a URL, put them into the\n> >      credential struct, so that when we successfully authenticate, we\n> >      trigger storage. This _might_ actually happen already, but we\n> >      should definitely confirm it.\n> >\n> >   3. If the user has no credential helper defined, then do one of:\n> >\n> >      a. Warn them that the credential was not recorded, but that they\n> >         can use \"git clone -c credential.helper=store\" as a fallback\n> > \t(but probably worded in a way to recommend using something\n> > \tstronger if possible).\n> >\n> > \tThis is slightly annoying because following the advice requires\n> > \tre-cloning. Fixing it up from there is more like:\n> >\n> > \t  git config credential.helper store\n> > \t  git fetch\n> > \t  [interactively input password]\n> >\n> >      b. Just use credential-store. Optionally notify them of what\n> >          happened (and that they might want to choose a better helper).\n>\n> So here are patches to do that. Step 2 is indeed how things work\n> already, so nothing was needed there (there are still 3 patches because\n> there was a bit of prep-work ;) ).\n\nThank you for working on this!!!\n\n> I did 3b here: automagically enabling credential-store. I'm still on the\n> fence on whether that's a good idea or not.\n\nI think you're right, it is a good idea. It is built by default (because\nit has no 3rd-party dependencies), and it should be \"safe enough\".\n\nI'll just have to look into mapping the Unix-y `chmod()` to a\nWindows-appropriate ACL action, I guess. AFAICT we don't do that yet.\n\n> I don't have any data on how many victims of this ransom campaign might\n> have been helped by this. But it certainly seems like a decent best\n> practice. I'd hope that _most_ people have moved on to using a\n> credential helper and supplying the initial password interactively these\n> days; this is really just aimed at people who don't know better. So the\n> goal is making them a bit more secure, but also educating them about the\n> other options. Hopefully without breaking any workflows. :)\n\nWell, let's also not underestimate the side effect of educating people\n(even transitively) by the mere action of doing something about it and\ntalking about that.\n\nCiao,\nDscho\n\n>\n> -Peff\n>\n>   [1/3]: transport_anonymize_url(): support retaining username\n>   [2/3]: clone: avoid storing URL passwords in config\n>   [3/3]: clone: auto-enable git-credential-store when necessary\n>\n>  builtin/clone.c            | 37 +++++++++++++++++++++++++++++++++++--\n>  credential.c               | 13 +++++++++++++\n>  credential.h               |  6 ++++++\n>  t/t5550-http-fetch-dumb.sh | 12 ++++++++++++\n>  transport.c                | 21 ++++++++++++++-------\n>  transport.h                | 11 ++++++++++-\n>  6 files changed, 90 insertions(+), 10 deletions(-)\n>\n"},{"id":"375929","messageId":"87h89pupdr.fsf@evledraar.gmail.com","threadId":"51103","inReplyTo":"20190520140825.GA31580@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] clone: auto-enable git-credential-store when necessary","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-05-20T15:17:20Z","receivedAt":"2019-05-20T15:17:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, May 20 2019, Jeff King wrote:\n\n> On Mon, May 20, 2019 at 03:56:14PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> > -test_expect_failure 'fetch of password-URL clone uses stored auth' '\n>> > +test_expect_success 'fetch of password-URL clone uses stored auth' '\n>> >  \tset_askpass wrong &&\n>> >  \tgit -C clone-auth-none fetch &&\n>> >  \texpect_askpass none\n>>\n>> I've only looked at this very briefly, there's a regression here where\n>> you're assuming that having a configured credential helper means it\n>> works.\n>>\n>> I.e. I have a ~/.gitconfig where I point to some-gnome-thing-or-other\n>> what doesn't exist on my VPS in my ~/.gitconfig, cloning just warns\n>> about it being missing, but will store the password in the repo.\n>>\n>> With this you detect that I have the helper, don't store it, but then my\n>> helper doesn't work, whereas this worked before.\n>\n> There are more cases beyond that, too. You might have a helper defined\n> which doesn't actually store passwords, but just sometimes tries to\n> provide one. My thinking was that if you're clueful enough to have\n> configured helpers, you can probably deal with the fallout. But you're\n> right that it may still be a regression in the sense that the user may\n> still have to actually _do_ something to get their fetch to work.\n>\n> I guess a more robust version of this is that _after_ the successful\n> clone, we could ask the credential system \"hey, do you have the\n> credential for $URL?\". And if it can't answer, then we can take action\n> (whether that action is setting up credential-store and seeding it with\n> the password, or just advising the user about the situation).\n>\n> -Peff\n\nYeah I don't mean deal with some there-but-broken helper, but this:\n\n    /usr/share/doc/git/contrib/credential/gnome-keyring/git-credential-gnome-keyring:\n    not found\n\nUntil then the observable effect of that has been to make the\ncredential.helper config a noop, but now it's causing \"we have a helper\"\nbehavior.\n"},{"id":"375930","messageId":"20190520152433.GB4583@sigill.intra.peff.net","threadId":"51103","inReplyTo":"87h89pupdr.fsf@evledraar.gmail.com","subject":"Re: [PATCH 3/3] clone: auto-enable git-credential-store when necessary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-20T15:24:34Z","receivedAt":"2019-05-20T15:24:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 20, 2019 at 05:17:20PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > There are more cases beyond that, too. You might have a helper defined\n> > which doesn't actually store passwords, but just sometimes tries to\n> > provide one. My thinking was that if you're clueful enough to have\n> > configured helpers, you can probably deal with the fallout. But you're\n> > right that it may still be a regression in the sense that the user may\n> > still have to actually _do_ something to get their fetch to work.\n> >\n> > I guess a more robust version of this is that _after_ the successful\n> > clone, we could ask the credential system \"hey, do you have the\n> > credential for $URL?\". And if it can't answer, then we can take action\n> > (whether that action is setting up credential-store and seeding it with\n> > the password, or just advising the user about the situation).\n> >\n> > -Peff\n> \n> Yeah I don't mean deal with some there-but-broken helper, but this:\n> \n>     /usr/share/doc/git/contrib/credential/gnome-keyring/git-credential-gnome-keyring:\n>     not found\n> \n> Until then the observable effect of that has been to make the\n> credential.helper config a noop, but now it's causing \"we have a helper\"\n> behavior.\n\nRight, I understood. The other case I mean is not one that's broken, but\na helper that's designed to provide a password from a read-only store\n(which presumably doesn't have _this_ password, else why would they be\nproviding it in the URL?).\n\nIt is not going to help that the clone will feed the password to such a\nhelper because it will (correctly, and by design) ignore any \"store\"\nrequests.\n\nIn other words, I am agreeing with you and indicating that there are\neven more cases where a non-empty helper config will mislead us.\n\nI'm going to try to re-work the patch to do this check-at-the-end\ntechnique, and probably try to make the UI for clearing and seeding\npasswords a bit more friendly, too.\n\n-Peff\n"},{"id":"375931","messageId":"29276251-0dd4-d22c-4479-7de52efc35b2@web.de","threadId":"51103","inReplyTo":"20190519051031.GA19434@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] transport_anonymize_url(): support retaining username","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-05-20T16:14:37Z","receivedAt":"2019-05-20T16:15:00Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 19.05.19 um 07:10 schrieb Jeff King:\n> diff --git a/transport.c b/transport.c\n> index f1fcd2c4b0..ba61e57295 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -1373,7 +1372,15 @@ char *transport_anonymize_url(const char *url)\n>  \t\tcp = strchr(scheme_prefix + 3, '/');\n>  \t\tif (cp && cp < anon_part)\n>  \t\t\tgoto literal_copy;\n> -\t\tprefix_len = scheme_prefix - url + 3;\n> +\n> +\t\tif (strip_user)\n> +\t\t\tprefix_len = scheme_prefix - url + 3;\n> +\t\telse {\n> +\t\t\tcp = strchr(scheme_prefix + 3, ':');\n> +\t\t\tif (cp && cp > anon_part)\n\nDon't you mean this?\n\n\t\t\tif (!cp || cp > anon_part)\n\nOr the search could stop at anon_part in the first place:\n\n\t\t\tassert(scheme_prefix + 3 < anon_part);\n\t\t\tcp = memchr(schema_prefix + 3, ':', anon_part - schema_prefix - 3);\n\t\t\tif (!cp)\n\nThat whole thing looks fragile.  I wonder if using the official regex\n(https://tools.ietf.org/html/rfc3986#appendix-B) would make it easier\nand more robust.\n\n> +\t\t\t\tgoto literal_copy; /* username only */\n> +\t\t\tprefix_len = cp - url;\n\nAnyway, you don't want cp == NULL here.\n\n> +\t\t}\n>  \t}\n>  \treturn xstrfmt(\"%.*s%.*s\", (int)prefix_len, url,\n>  \t\t       (int)anon_len, anon_part);\n"},{"id":"375933","messageId":"nycvar.QRO.7.76.6.1905201834450.46@tvgsbejvaqbjf.bet","threadId":"51103","inReplyTo":"20190519051031.GA19434@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] transport_anonymize_url(): support retaining username","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-05-20T16:36:13Z","receivedAt":"2019-05-20T16:36:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Sun, 19 May 2019, Jeff King wrote:\n\n> When we anonymize URLs to show in messages, we strip out both the\n> username and password (if any). But there are also contexts where we\n> should strip out the password (to avoid leaking it) but retain the\n> username.\n>\n> Let's generalize transport_anonymize_url() to support both cases. We'll\n> give it a new name since the password-only mode isn't really\n> \"anonymizing\", but keep the old name as a synonym to avoid disrupting\n> existing callers.\n>\n> Note that there are actually three places we parse URLs, and this\n> functionality _could_ go into any of them:\n>\n>   - transport_anonymize_url(), which we modify here\n>\n>   - the urlmatch.c code parses a URL into its constituent parts, from\n>     which we could easily remove the elements we want to drop and\n>     re-format it as a single URL. But its parsing also normalizes\n>     elements (e.g., downcasing hostnames).  This isn't wrong, but it's\n>     more friendly if we can leave the rest of the URL untouched.\n\nI have not looked into it at all, but I seem to vaguely remember that the\nresult of this code might be used to look up `url.<url>.insteadOf`\nsettings, where the middle part *is* case-sensitive.\n\n>   - credential_form_url() parses a URL and decodes the specific\n>     elements, but it's hard to convert it back into a regular URL. It\n>     treats \"host:port\" as a single unit, meaning it needs to be\n>     re-encoded specially (since a colon would otherwise end\n>     percent-encoded).\n>\n> Since transport_anonymize_url() seemed closest to what we want here, I\n> used that as the base.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> I think it would be beneficial to unify these three cases under a single\n> parser, but it seemed like too big a rabbit hole for this topic. Of the\n> three, the urlmatch one seems the most mature. I think if we could\n> simply separate the normalization from the parsing/decoding, the others\n> could build on top of it. It might also require some careful thinking\n> about how pseudo-urls like ssh \"host:path\" interact.\n\nIn light of what I mentioned above, I am not sure that we should go there\nin the first place...\n\nThanks,\nDscho\n\n> I won't call that a #leftoverbits, because it's more of a feast. :)\n>\n>  transport.c | 21 ++++++++++++++-------\n>  transport.h | 11 ++++++++++-\n>  2 files changed, 24 insertions(+), 8 deletions(-)\n>\n> diff --git a/transport.c b/transport.c\n> index f1fcd2c4b0..ba61e57295 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -1335,11 +1335,7 @@ int transport_disconnect(struct transport *transport)\n>  \treturn ret;\n>  }\n>\n> -/*\n> - * Strip username (and password) from a URL and return\n> - * it in a newly allocated string.\n> - */\n> -char *transport_anonymize_url(const char *url)\n> +char *transport_strip_url(const char *url, int strip_user)\n>  {\n>  \tchar *scheme_prefix, *anon_part;\n>  \tsize_t anon_len, prefix_len = 0;\n> @@ -1348,7 +1344,10 @@ char *transport_anonymize_url(const char *url)\n>  \tif (url_is_local_not_ssh(url) || !anon_part)\n>  \t\tgoto literal_copy;\n>\n> -\tanon_len = strlen(++anon_part);\n> +\tanon_len = strlen(anon_part);\n> +\tif (strip_user)\n> +\t\tanon_part++;\n> +\n>  \tscheme_prefix = strstr(url, \"://\");\n>  \tif (!scheme_prefix) {\n>  \t\tif (!strchr(anon_part, ':'))\n> @@ -1373,7 +1372,15 @@ char *transport_anonymize_url(const char *url)\n>  \t\tcp = strchr(scheme_prefix + 3, '/');\n>  \t\tif (cp && cp < anon_part)\n>  \t\t\tgoto literal_copy;\n> -\t\tprefix_len = scheme_prefix - url + 3;\n> +\n> +\t\tif (strip_user)\n> +\t\t\tprefix_len = scheme_prefix - url + 3;\n> +\t\telse {\n> +\t\t\tcp = strchr(scheme_prefix + 3, ':');\n> +\t\t\tif (cp && cp > anon_part)\n> +\t\t\t\tgoto literal_copy; /* username only */\n> +\t\t\tprefix_len = cp - url;\n> +\t\t}\n>  \t}\n>  \treturn xstrfmt(\"%.*s%.*s\", (int)prefix_len, url,\n>  \t\t       (int)anon_len, anon_part);\n> diff --git a/transport.h b/transport.h\n> index 06e06d3d89..6d8c99ac91 100644\n> --- a/transport.h\n> +++ b/transport.h\n> @@ -243,10 +243,19 @@ const struct ref *transport_get_remote_refs(struct transport *transport,\n>  int transport_fetch_refs(struct transport *transport, struct ref *refs);\n>  void transport_unlock_pack(struct transport *transport);\n>  int transport_disconnect(struct transport *transport);\n> -char *transport_anonymize_url(const char *url);\n>  void transport_take_over(struct transport *transport,\n>  \t\t\t struct child_process *child);\n>\n> +/*\n> + * Strip password and optionally username from a URL and return\n> + * it in a newly allocated string (even if nothing was stripped).\n> + */\n> +char *transport_strip_url(const char *url, int strip_username);\n> +static inline char *transport_anonymize_url(const char *url)\n> +{\n> +\treturn transport_strip_url(url, 1);\n> +}\n> +\n>  int transport_connect(struct transport *transport, const char *name,\n>  \t\t      const char *exec, int fd[2]);\n>\n> --\n> 2.22.0.rc0.583.g23d90da2b3\n>\n>\n"},{"id":"375934","messageId":"nycvar.QRO.7.76.6.1905201836380.46@tvgsbejvaqbjf.bet","threadId":"51103","inReplyTo":"20190519051031.GA19434@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] transport_anonymize_url(): support retaining username","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-05-20T16:43:07Z","receivedAt":"2019-05-20T16:43:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Whoops. Meant to comment on this:\n\nOn Sun, 19 May 2019, Jeff King wrote:\n\n> diff --git a/transport.c b/transport.c\n> index f1fcd2c4b0..ba61e57295 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -1335,11 +1335,7 @@ int transport_disconnect(struct transport *transport)\n>  \treturn ret;\n>  }\n>\n> -/*\n> - * Strip username (and password) from a URL and return\n> - * it in a newly allocated string.\n> - */\n> -char *transport_anonymize_url(const char *url)\n> +char *transport_strip_url(const char *url, int strip_user)\n\nIt might make more sense to skip the \"transport_\" prefix here, and maybe\nuse a slightly more descriptive name? My current favorite would be\n`strip_credentials_from_url(const char *url, int keep_user)`.\n\n>  {\n>  \tchar *scheme_prefix, *anon_part;\n>  \tsize_t anon_len, prefix_len = 0;\n> @@ -1348,7 +1344,10 @@ char *transport_anonymize_url(const char *url)\n>  \tif (url_is_local_not_ssh(url) || !anon_part)\n>  \t\tgoto literal_copy;\n>\n> -\tanon_len = strlen(++anon_part);\n> +\tanon_len = strlen(anon_part);\n> +\tif (strip_user)\n> +\t\tanon_part++;\n> +\n>  \tscheme_prefix = strstr(url, \"://\");\n>  \tif (!scheme_prefix) {\n>  \t\tif (!strchr(anon_part, ':'))\n> @@ -1373,7 +1372,15 @@ char *transport_anonymize_url(const char *url)\n>  \t\tcp = strchr(scheme_prefix + 3, '/');\n>  \t\tif (cp && cp < anon_part)\n>  \t\t\tgoto literal_copy;\n> -\t\tprefix_len = scheme_prefix - url + 3;\n> +\n> +\t\tif (strip_user)\n> +\t\t\tprefix_len = scheme_prefix - url + 3;\n> +\t\telse {\n> +\t\t\tcp = strchr(scheme_prefix + 3, ':');\n\nHow about `scheme_prefix += 3;` (actually quite a bit earlier than here),\nfollowed by `memchr(scheme_prefix, ':', anon_part - scheme_prefix)`?\n\nAh, I see you just copied that part from above...\n\nThanks,\nDscho\n\n> +\t\t\tif (cp && cp > anon_part)\n> +\t\t\t\tgoto literal_copy; /* username only */\n> +\t\t\tprefix_len = cp - url;\n> +\t\t}\n>  \t}\n>  \treturn xstrfmt(\"%.*s%.*s\", (int)prefix_len, url,\n>  \t\t       (int)anon_len, anon_part);\n> diff --git a/transport.h b/transport.h\n> index 06e06d3d89..6d8c99ac91 100644\n> --- a/transport.h\n> +++ b/transport.h\n> @@ -243,10 +243,19 @@ const struct ref *transport_get_remote_refs(struct transport *transport,\n>  int transport_fetch_refs(struct transport *transport, struct ref *refs);\n>  void transport_unlock_pack(struct transport *transport);\n>  int transport_disconnect(struct transport *transport);\n> -char *transport_anonymize_url(const char *url);\n>  void transport_take_over(struct transport *transport,\n>  \t\t\t struct child_process *child);\n>\n> +/*\n> + * Strip password and optionally username from a URL and return\n> + * it in a newly allocated string (even if nothing was stripped).\n> + */\n> +char *transport_strip_url(const char *url, int strip_username);\n> +static inline char *transport_anonymize_url(const char *url)\n> +{\n> +\treturn transport_strip_url(url, 1);\n> +}\n> +\n>  int transport_connect(struct transport *transport, const char *name,\n>  \t\t      const char *exec, int fd[2]);\n>\n> --\n> 2.22.0.rc0.583.g23d90da2b3\n>\n>\n"},{"id":"375935","messageId":"nycvar.QRO.7.76.6.1905201845550.46@tvgsbejvaqbjf.bet","threadId":"51103","inReplyTo":"20190520123110.GE11212@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] clone: auto-enable git-credential-store when necessary","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-05-20T16:48:47Z","receivedAt":"2019-05-20T16:49:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Mon, 20 May 2019, Jeff King wrote:\n\n> On Mon, May 20, 2019 at 07:28:08AM -0400, Eric Sunshine wrote:\n>\n> > > The biggest downside is that it's a bit magical from the user's\n> > > perspective, because now the password is off in some other file\n> > > (usually ~/.git-credentials, but sometimes in $XDG_CONFIG_HOME).\n> > > Which complicates things if they want to purge the repo and\n> > > password, for example, because now they can't just delete the\n> > > repository directory.\n> > >\n> > > The file location is documented, though, and we point people to the\n> > > documentation. So perhaps it will be enough (and better still, may\n> > > lead to them configuring a more secure helper).\n> >\n> > I'm trying to decide how I feel about this based upon my own\n> > experience recently of having my password magically stored by Git for\n> > Windows without warning or consent on a computer which was not my own\n> > but on which I needed to access a private GitHub repository. Although\n> > the situation is not perfectly analogous, the concern of having one's\n> > password magically squirreled-away _somewhere_ unexpectedly is the\n> > same. Being unfamiliar with Git for Windows's credential helper or\n> > Windows credential management in general, I experienced more than a\n> > few minutes of consternation and alarm before finally figuring out\n> > where Git for Windows had stored my password and how to remove it. The\n> > sense of alarm and discomfort likely would have not arisen had the\n> > credential helper given me the opportunity to approve or deny the\n> > action.\n>\n> Thanks, that's a good elaboration of the uneasiness I was feeling. This\n> patch is better than the status quo in that your password was already\n> being squirreled away in plaintext, and now it's at least locked down\n> with filesystem permissions. But it's clearly not as far as we could go.\n> I was mostly just afraid to break existing workflows, but maybe we\n> should be more opinionated.\n>\n> Or maybe we just need to give more specific details about how to back\n> out the change.\n\nI think that would make the most sense. \"If you did not mean for this\npassword to be stored, call [...]\".\n\n> > > +\"Note that the password is still stored in plaintext in the filesystem;\\n\"\n> > > +\"consider configuring a more secure helper. See \\\"git help gitcredentials\\\"\\n\"\n> > > +\"and \\\"git help git-credential-store\\\" for details.\\n\"\n> > >  );\n> >\n> > Give the above experience, one way to mitigate such feelings of alarm\n> > might, at a minimum, be for this message to say where the password is\n> > being stored (and, possibly, how to remove it) so the user can do so\n> > immediately if desired. Prompting the user to approve or deny the\n> > action might also go a long way toward making this more palatable\n> > (assuming the session is interactive).\n>\n> I actually thought about pointing to the file, but it's non-trivial to\n> do so (there's a bunch of internal logic in credential-store to decide\n> between $HOME and XDG locations).\n>\n> I think what we really need are better commands to manage credentials\n> independent of helpers, and then we could recommend a simple command to\n> clear a credential that you don't want to have stored. Right now I think\n> the best you can do is:\n>\n>   echo url=https://example.com | git credential reject\n>\n> but:\n>\n>   - \"reject\" is a funny term; this comes from the C code, which thinks\n>     of it in terms of the server approving/rejecting (and that makes\n>     sense for scripts calling this command). But at the helper level,\n>     the operations are really store/erase. We probably ought to support\n>     those names, too.\n>\n>   - piping the credential protocol is slightly awkward; we probably\n>     ought to allow a url on the command line, and avoid reading stdin if\n>     we get one.\n>\n> That would give us:\n>\n>   git credential erase https://example.com\n>\n> which is really quite readable. :)\n>\n> Likewise, if we choose not to auto-enable the store helper, we'd\n> probably want to give advice on seeding your password. Right now that\n> is:\n>\n>   echo url=https://example.com | git credential fill | git credential approve\n>\n> which is...not intuitive. It would make sense to me to have a \"seed\"\n> operation which does a fill/approve together. Maybe that should just be\n> what \"store\" does, which would allow:\n>\n>   $ git credential store https://example.com\n>   Username for 'https://example.com':\n>   Password for 'https://user@example.com':\n>\n> (Of course you can also just \"git fetch\" to get prompted, but it seems\n> like this shouldn't require a network operation if you don't want it\n> to).\n\nThere is nothing stopping us from adding a new command to `git\ncredential`: `git credential forget <url>` where the URL may contain the\nuser name for further differentiation (and maybe even a password that will\nbe ignored, for copy/pasting convenience).\n\nCiao,\nDscho\n"},{"id":"375939","messageId":"87ftp9uk8d.fsf@evledraar.gmail.com","threadId":"51103","inReplyTo":"20190519051604.GC19434@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] clone: auto-enable git-credential-store when necessary","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-05-20T17:08:34Z","receivedAt":"2019-05-20T17:08:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, May 19 2019, Jeff King wrote:\n\n> If the user clones with a URL containing a password and has no\n> credential helper configured, we're stuck. We don't want to write the\n> password into .git/config because that risks accidentally disclosing it.\n> But if we don't record it somewhere, subsequent fetches will fail unless\n> the user is there to input the password.\n>\n> The best advice we can give the user is to set up a credential helper.\n> But we can actually go a step further and enable the \"store\" helper for\n> them. This still records the password in plaintext, but:\n>\n>   1. It's not inside the repo directory, which makes it slightly less\n>      likely to be disclosed.\n>\n>   2. The permissions on the storage file are tighter than what would be\n>      on .git/config.\n>\n> So this is generally a security win over the old behavior of writing it\n> into .git/config. And it's a usability win over the more recent behavior\n> of just forgetting the password entirely.\n>\n> The biggest downside is that it's a bit magical from the user's\n> perspective, because now the password is off in some other file (usually\n> ~/.git-credentials, but sometimes in $XDG_CONFIG_HOME). Which\n> complicates things if they want to purge the repo and password, for\n> example, because now they can't just delete the repository directory.\n>\n> The file location is documented, though, and we point people to the\n> documentation. So perhaps it will be enough (and better still, may lead\n> to them configuring a more secure helper).\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> If we do decide this is too magical, I think the best alternate path is\n> to advise them on credential helpers, and how to seed the password into\n> the helper (basically configure the helper and then \"git fetch\"\n> should work).\n>\n> One other thing I wondered: if they do have a helper configured this\n> patch will omit the advice entirely, but we'll still print the warning.\n> Is that useful (to remind them that passwords in URLs are a bad thing),\n> or is it just annoying?\n\nA few things, in no particular order, sorry for the braindump. Please\ndon't take this as \"this all sucks\" just trying to help by poking holes\nin it.\n\n 1. I'm a bit uneasy about us acting on an incident where the details of\n    what happened / frequency haven't been released. I.e. GitHub\n    supposedly has ~100 million repos, one source claims on the order of\n    ~300 of these (public)[1], none of the sites said anything on that,\n    that 300 number is third-party speculation.\n\n    Let's say it's 100x that counting private repos. That's still a very\n    small percentage of 100 million.\n\n    But more importantly, it doesn't give us any insight on how to\n    perhaps better solve this problem, e.g. maybe 80% of it is people\n    using some build system (docker?) where we'd expose /home too. Not\n    saying it is, just that we basically have a partial bug report here.\n\n    I don't have a good sense for what the state is there. Is this all\n    repos cloned in /var/www, then this is sufficient, or something\n    else?\n\n 2. We're now making the trade-off of auto-storing the password in ~/,\n    is that a worse default now in the wild? We're defaulting to storing\n    for a longer time (e.g. on a shared *nix server) in workflows where\n    you'd clone && rm -rf.\n\n    So yeah, we might handle this *specific* incident, but are we just\n    making it worse for others?\n\n 3. There's seemingly no way to just say \"I want it the old way, make it\n    so!\" without creating an auth helper that does that.\n\n    Storing passwords in config isn't per-se insecure, neither is having\n    passwords in (https) URLs. Yeah in combination with *other* stuff\n    the might be insecure, but often not (but may actually become\n    insecure by auto-storing in ~/)\n\n 4. Re #3: We have existing users in the wild who do this, e.g. GitLab\n    CI, the user/password token (lives for about an hour IIRC) is stored\n    in .gitconfig, there's no writable /home there IIRC (or might not be\n    in similar CI environments), and I daresay the people who set that\n    up understand the security trade-offs.\n\n 5. Given #2 && #3 && #4 I'd be much more comfortable with something\n    where we just make this fail outright, and force the user to eitiher\n    say \"Yeah I want passwords in .git/config here\" or \"oh thanks, I'll\n    use a credential helper\" via some core.* or credentials.* config.\n\n    I.e. let's not make #3 impossible, and for users who are clueless\n    about security we're doing them a disservice by auto-using a crappy\n    fallback ~/ helper, whereas they might have an *actual* helper they\n    can use that doesn't suck (i.e. OS pwd store) if it was an error.\n\n 6. We'll still happily let this new behavior pass by on init && config\n    fetch. Should we care? I was going to say this categorically breaks\n    core.sharedRepository=group with a password in the repo, it does in\n    the 'git -c core.sharedRepository=group clone' case, but you can\n    manually set it up still, you just can't use \"clone\" anymore.\n\n 7. We still accept passwords on the command-line for cloning, so if\n    we're trying to help novice users we're still open to the shared\n    server attack where someone just runs \"ps auxf\" in a loop to scrape\n    passwords.\n\n    To me this is another good argument for some variant of #3. We could\n    provide ssh-like security by outright refusing passwords on the\n    command-line (could take them interactively, or via some FD/file),\n    then allow relaxing that to the level of this auto-helper, or all\n    the way down to the old behavior.\n\n1. https://hub.packtpub.com/atlassian-bitbucket-github-and-gitlab-take-collective-steps-against-the-git-ransomware-attack/\n"}]}