| Message ID | 20190519051604.GC19434@sigill.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] transport_anonymize_url(): support retaining username | expand |
On Sun, May 19, 2019 at 2:42 PM Jeff King <peff@peff.net> wrote: > If the user clones with a URL containing a password and has no > credential helper configured, we're stuck. We don't want to write the > password into .git/config because that risks accidentally disclosing it. > But if we don't record it somewhere, subsequent fetches will fail unless > the user is there to input the password. > > But we can actually go a step further and enable the "store" helper for > them. [...] > > The biggest downside is that it's a bit magical from the user's > perspective, because now the password is off in some other file (usually > ~/.git-credentials, but sometimes in $XDG_CONFIG_HOME). Which > complicates things if they want to purge the repo and password, for > example, because now they can't just delete the repository directory. > > The file location is documented, though, and we point people to the > documentation. So perhaps it will be enough (and better still, may lead > to them configuring a more secure helper). I'm trying to decide how I feel about this based upon my own experience recently of having my password magically stored by Git for Windows without warning or consent on a computer which was not my own but on which I needed to access a private GitHub repository. Although the situation is not perfectly analogous, the concern of having one's password magically squirreled-away _somewhere_ unexpectedly is the same. Being unfamiliar with Git for Windows's credential helper or Windows credential management in general, I experienced more than a few minutes of consternation and alarm before finally figuring out where Git for Windows had stored my password and how to remove it. The sense of alarm and discomfort likely would have not arisen had the credential helper given me the opportunity to approve or deny the action. > static const char sanitized_url_advice[] = N_( > "The URL you provided to Git contains a password. It will be\n" > "used to clone the repository, but to avoid accidental disclosure\n" > +"the password will not be recorded in the repository config.\n" > +"Since you have no credential helper configured, the \"store\" helper\n" > +"has been enabled for this repository, and will provide the password\n" > +"for further fetches.\n" > +"\n" > +"Note that the password is still stored in plaintext in the filesystem;\n" > +"consider configuring a more secure helper. See \"git help gitcredentials\"\n" > +"and \"git help git-credential-store\" for details.\n" > ); Give the above experience, one way to mitigate such feelings of alarm might, at a minimum, be for this message to say where the password is being stored (and, possibly, how to remove it) so the user can do so immediately if desired. Prompting the user to approve or deny the action might also go a long way toward making this more palatable (assuming the session is interactive).
On Mon, May 20, 2019 at 07:28:08AM -0400, Eric Sunshine wrote: > > The biggest downside is that it's a bit magical from the user's > > perspective, because now the password is off in some other file (usually > > ~/.git-credentials, but sometimes in $XDG_CONFIG_HOME). Which > > complicates things if they want to purge the repo and password, for > > example, because now they can't just delete the repository directory. > > > > The file location is documented, though, and we point people to the > > documentation. So perhaps it will be enough (and better still, may lead > > to them configuring a more secure helper). > > I'm trying to decide how I feel about this based upon my own > experience recently of having my password magically stored by Git for > Windows without warning or consent on a computer which was not my own > but on which I needed to access a private GitHub repository. Although > the situation is not perfectly analogous, the concern of having one's > password magically squirreled-away _somewhere_ unexpectedly is the > same. Being unfamiliar with Git for Windows's credential helper or > Windows credential management in general, I experienced more than a > few minutes of consternation and alarm before finally figuring out > where Git for Windows had stored my password and how to remove it. The > sense of alarm and discomfort likely would have not arisen had the > credential helper given me the opportunity to approve or deny the > action. Thanks, that's a good elaboration of the uneasiness I was feeling. This patch is better than the status quo in that your password was already being squirreled away in plaintext, and now it's at least locked down with filesystem permissions. But it's clearly not as far as we could go. I was mostly just afraid to break existing workflows, but maybe we should be more opinionated. Or maybe we just need to give more specific details about how to back out the change. > > +"Note that the password is still stored in plaintext in the filesystem;\n" > > +"consider configuring a more secure helper. See \"git help gitcredentials\"\n" > > +"and \"git help git-credential-store\" for details.\n" > > ); > > Give the above experience, one way to mitigate such feelings of alarm > might, at a minimum, be for this message to say where the password is > being stored (and, possibly, how to remove it) so the user can do so > immediately if desired. Prompting the user to approve or deny the > action might also go a long way toward making this more palatable > (assuming the session is interactive). I actually thought about pointing to the file, but it's non-trivial to do so (there's a bunch of internal logic in credential-store to decide between $HOME and XDG locations). I think what we really need are better commands to manage credentials independent of helpers, and then we could recommend a simple command to clear a credential that you don't want to have stored. Right now I think the best you can do is: echo url=https://example.com | git credential reject but: - "reject" is a funny term; this comes from the C code, which thinks of it in terms of the server approving/rejecting (and that makes sense for scripts calling this command). But at the helper level, the operations are really store/erase. We probably ought to support those names, too. - piping the credential protocol is slightly awkward; we probably ought to allow a url on the command line, and avoid reading stdin if we get one. That would give us: git credential erase https://example.com which is really quite readable. :) Likewise, if we choose not to auto-enable the store helper, we'd probably want to give advice on seeding your password. Right now that is: echo url=https://example.com | git credential fill | git credential approve which is...not intuitive. It would make sense to me to have a "seed" operation which does a fill/approve together. Maybe that should just be what "store" does, which would allow: $ git credential store https://example.com Username for 'https://example.com': Password for 'https://user@example.com': (Of course you can also just "git fetch" to get prompted, but it seems like this shouldn't require a network operation if you don't want it to). -Peff
On Sun, May 19 2019, Jeff King wrote: > If the user clones with a URL containing a password and has no > credential helper configured, we're stuck. We don't want to write the > password into .git/config because that risks accidentally disclosing it. > But if we don't record it somewhere, subsequent fetches will fail unless > the user is there to input the password. > > The best advice we can give the user is to set up a credential helper. > But we can actually go a step further and enable the "store" helper for > them. This still records the password in plaintext, but: > > 1. It's not inside the repo directory, which makes it slightly less > likely to be disclosed. > > 2. The permissions on the storage file are tighter than what would be > on .git/config. > > So this is generally a security win over the old behavior of writing it > into .git/config. And it's a usability win over the more recent behavior > of just forgetting the password entirely. > > The biggest downside is that it's a bit magical from the user's > perspective, because now the password is off in some other file (usually > ~/.git-credentials, but sometimes in $XDG_CONFIG_HOME). Which > complicates things if they want to purge the repo and password, for > example, because now they can't just delete the repository directory. > > The file location is documented, though, and we point people to the > documentation. So perhaps it will be enough (and better still, may lead > to them configuring a more secure helper). > > Signed-off-by: Jeff King <peff@peff.net> > --- > If we do decide this is too magical, I think the best alternate path is > to advise them on credential helpers, and how to seed the password into > the helper (basically configure the helper and then "git fetch" > should work). > > One other thing I wondered: if they do have a helper configured this > patch will omit the advice entirely, but we'll still print the warning. > Is that useful (to remind them that passwords in URLs are a bad thing), > or is it just annoying? > > builtin/clone.c | 19 ++++++++++++++++--- > credential.c | 13 +++++++++++++ > credential.h | 6 ++++++ > t/t5550-http-fetch-dumb.sh | 2 +- > 4 files changed, 36 insertions(+), 4 deletions(-) > > diff --git a/builtin/clone.c b/builtin/clone.c > index 15fffc3268..94d2659154 100644 > --- a/builtin/clone.c > +++ b/builtin/clone.c > @@ -31,6 +31,7 @@ > #include "packfile.h" > #include "list-objects-filter-options.h" > #include "object-store.h" > +#include "credential.h" > > /* > * Overall FIXMEs: > @@ -894,8 +895,14 @@ static int dir_exists(const char *path) > static const char sanitized_url_advice[] = N_( > "The URL you provided to Git contains a password. It will be\n" > "used to clone the repository, but to avoid accidental disclosure\n" > -"the password will not be recorded. Further fetches from the remote\n" > -"may require you to provide the password interactively.\n" > +"the password will not be recorded in the repository config.\n" > +"Since you have no credential helper configured, the \"store\" helper\n" > +"has been enabled for this repository, and will provide the password\n" > +"for further fetches.\n" > +"\n" > +"Note that the password is still stored in plaintext in the filesystem;\n" > +"consider configuring a more secure helper. See \"git help gitcredentials\"\n" > +"and \"git help git-credential-store\" for details.\n" > ); > > int cmd_clone(int argc, const char **argv, const char *prefix) > @@ -1090,7 +1097,13 @@ int cmd_clone(int argc, const char **argv, const char *prefix) > sanitized_repo = transport_strip_url(repo, 0); > if (strcmp(repo, sanitized_repo)) { > warning(_("omitting password while storing URL in on-disk config")); > - advise(_(sanitized_url_advice)); > + if (!url_has_credential_helper(sanitized_repo)) { > + strbuf_addf(&key, "credential.%s.helper", > + sanitized_repo); > + git_config_set(key.buf, "store"); > + strbuf_reset(&key); > + advise(_(sanitized_url_advice)); > + } > } > strbuf_addf(&key, "remote.%s.url", option_origin); > git_config_set(key.buf, sanitized_repo); > diff --git a/credential.c b/credential.c > index 62be651b03..0a70edcee5 100644 > --- a/credential.c > +++ b/credential.c > @@ -372,3 +372,16 @@ void credential_from_url(struct credential *c, const char *url) > *p-- = '\0'; > } > } > + > +int url_has_credential_helper(const char *url) > +{ > + struct credential c = CREDENTIAL_INIT; > + int ret; > + > + credential_from_url(&c, url); > + credential_apply_config(&c); > + ret = c.helpers.nr > 0; > + > + credential_clear(&c); > + return ret; > +} > diff --git a/credential.h b/credential.h > index 6b0cd16be2..e6d6d6fa40 100644 > --- a/credential.h > +++ b/credential.h > @@ -32,4 +32,10 @@ void credential_from_url(struct credential *, const char *url); > int credential_match(const struct credential *have, > const struct credential *want); > > +/* > + * Return true if feeding "url" to the credential system would trigger one > + * or more helpers. > + */ > +int url_has_credential_helper(const char *url); > + > #endif /* CREDENTIAL_H */ > diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh > index d8c85f3126..2723e91ae0 100755 > --- a/t/t5550-http-fetch-dumb.sh > +++ b/t/t5550-http-fetch-dumb.sh > @@ -70,7 +70,7 @@ test_expect_success 'username is retained in URL, password is not' ' > ! grep pass url > ' > > -test_expect_failure 'fetch of password-URL clone uses stored auth' ' > +test_expect_success 'fetch of password-URL clone uses stored auth' ' > set_askpass wrong && > git -C clone-auth-none fetch && > expect_askpass none I've only looked at this very briefly, there's a regression here where you're assuming that having a configured credential helper means it works. I.e. I have a ~/.gitconfig where I point to some-gnome-thing-or-other what doesn't exist on my VPS in my ~/.gitconfig, cloning just warns about it being missing, but will store the password in the repo. With this you detect that I have the helper, don't store it, but then my helper doesn't work, whereas this worked before. That's an X-Y problem of config includes being rather limited (now I'm just putting up with the error), but I expect this'll apply to others.
On Mon, May 20, 2019 at 03:56:14PM +0200, Ævar Arnfjörð Bjarmason wrote: > > -test_expect_failure 'fetch of password-URL clone uses stored auth' ' > > +test_expect_success 'fetch of password-URL clone uses stored auth' ' > > set_askpass wrong && > > git -C clone-auth-none fetch && > > expect_askpass none > > I've only looked at this very briefly, there's a regression here where > you're assuming that having a configured credential helper means it > works. > > I.e. I have a ~/.gitconfig where I point to some-gnome-thing-or-other > what doesn't exist on my VPS in my ~/.gitconfig, cloning just warns > about it being missing, but will store the password in the repo. > > With this you detect that I have the helper, don't store it, but then my > helper doesn't work, whereas this worked before. There are more cases beyond that, too. You might have a helper defined which doesn't actually store passwords, but just sometimes tries to provide one. My thinking was that if you're clueful enough to have configured helpers, you can probably deal with the fallout. But you're right that it may still be a regression in the sense that the user may still have to actually _do_ something to get their fetch to work. I guess a more robust version of this is that _after_ the successful clone, we could ask the credential system "hey, do you have the credential for $URL?". And if it can't answer, then we can take action (whether that action is setting up credential-store and seeding it with the password, or just advising the user about the situation). -Peff
On Mon, May 20 2019, Jeff King wrote: > On Mon, May 20, 2019 at 03:56:14PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> > -test_expect_failure 'fetch of password-URL clone uses stored auth' ' >> > +test_expect_success 'fetch of password-URL clone uses stored auth' ' >> > set_askpass wrong && >> > git -C clone-auth-none fetch && >> > expect_askpass none >> >> I've only looked at this very briefly, there's a regression here where >> you're assuming that having a configured credential helper means it >> works. >> >> I.e. I have a ~/.gitconfig where I point to some-gnome-thing-or-other >> what doesn't exist on my VPS in my ~/.gitconfig, cloning just warns >> about it being missing, but will store the password in the repo. >> >> With this you detect that I have the helper, don't store it, but then my >> helper doesn't work, whereas this worked before. > > There are more cases beyond that, too. You might have a helper defined > which doesn't actually store passwords, but just sometimes tries to > provide one. My thinking was that if you're clueful enough to have > configured helpers, you can probably deal with the fallout. But you're > right that it may still be a regression in the sense that the user may > still have to actually _do_ something to get their fetch to work. > > I guess a more robust version of this is that _after_ the successful > clone, we could ask the credential system "hey, do you have the > credential for $URL?". And if it can't answer, then we can take action > (whether that action is setting up credential-store and seeding it with > the password, or just advising the user about the situation). > > -Peff Yeah I don't mean deal with some there-but-broken helper, but this: /usr/share/doc/git/contrib/credential/gnome-keyring/git-credential-gnome-keyring: not found Until then the observable effect of that has been to make the credential.helper config a noop, but now it's causing "we have a helper" behavior.
On Mon, May 20, 2019 at 05:17:20PM +0200, Ævar Arnfjörð Bjarmason wrote: > > There are more cases beyond that, too. You might have a helper defined > > which doesn't actually store passwords, but just sometimes tries to > > provide one. My thinking was that if you're clueful enough to have > > configured helpers, you can probably deal with the fallout. But you're > > right that it may still be a regression in the sense that the user may > > still have to actually _do_ something to get their fetch to work. > > > > I guess a more robust version of this is that _after_ the successful > > clone, we could ask the credential system "hey, do you have the > > credential for $URL?". And if it can't answer, then we can take action > > (whether that action is setting up credential-store and seeding it with > > the password, or just advising the user about the situation). > > > > -Peff > > Yeah I don't mean deal with some there-but-broken helper, but this: > > /usr/share/doc/git/contrib/credential/gnome-keyring/git-credential-gnome-keyring: > not found > > Until then the observable effect of that has been to make the > credential.helper config a noop, but now it's causing "we have a helper" > behavior. Right, I understood. The other case I mean is not one that's broken, but a helper that's designed to provide a password from a read-only store (which presumably doesn't have _this_ password, else why would they be providing it in the URL?). It is not going to help that the clone will feed the password to such a helper because it will (correctly, and by design) ignore any "store" requests. In other words, I am agreeing with you and indicating that there are even more cases where a non-empty helper config will mislead us. I'm going to try to re-work the patch to do this check-at-the-end technique, and probably try to make the UI for clearing and seeding passwords a bit more friendly, too. -Peff
Hi Peff, On Mon, 20 May 2019, Jeff King wrote: > On Mon, May 20, 2019 at 07:28:08AM -0400, Eric Sunshine wrote: > > > > The biggest downside is that it's a bit magical from the user's > > > perspective, because now the password is off in some other file > > > (usually ~/.git-credentials, but sometimes in $XDG_CONFIG_HOME). > > > Which complicates things if they want to purge the repo and > > > password, for example, because now they can't just delete the > > > repository directory. > > > > > > The file location is documented, though, and we point people to the > > > documentation. So perhaps it will be enough (and better still, may > > > lead to them configuring a more secure helper). > > > > I'm trying to decide how I feel about this based upon my own > > experience recently of having my password magically stored by Git for > > Windows without warning or consent on a computer which was not my own > > but on which I needed to access a private GitHub repository. Although > > the situation is not perfectly analogous, the concern of having one's > > password magically squirreled-away _somewhere_ unexpectedly is the > > same. Being unfamiliar with Git for Windows's credential helper or > > Windows credential management in general, I experienced more than a > > few minutes of consternation and alarm before finally figuring out > > where Git for Windows had stored my password and how to remove it. The > > sense of alarm and discomfort likely would have not arisen had the > > credential helper given me the opportunity to approve or deny the > > action. > > Thanks, that's a good elaboration of the uneasiness I was feeling. This > patch is better than the status quo in that your password was already > being squirreled away in plaintext, and now it's at least locked down > with filesystem permissions. But it's clearly not as far as we could go. > I was mostly just afraid to break existing workflows, but maybe we > should be more opinionated. > > Or maybe we just need to give more specific details about how to back > out the change. I think that would make the most sense. "If you did not mean for this password to be stored, call [...]". > > > +"Note that the password is still stored in plaintext in the filesystem;\n" > > > +"consider configuring a more secure helper. See \"git help gitcredentials\"\n" > > > +"and \"git help git-credential-store\" for details.\n" > > > ); > > > > Give the above experience, one way to mitigate such feelings of alarm > > might, at a minimum, be for this message to say where the password is > > being stored (and, possibly, how to remove it) so the user can do so > > immediately if desired. Prompting the user to approve or deny the > > action might also go a long way toward making this more palatable > > (assuming the session is interactive). > > I actually thought about pointing to the file, but it's non-trivial to > do so (there's a bunch of internal logic in credential-store to decide > between $HOME and XDG locations). > > I think what we really need are better commands to manage credentials > independent of helpers, and then we could recommend a simple command to > clear a credential that you don't want to have stored. Right now I think > the best you can do is: > > echo url=https://example.com | git credential reject > > but: > > - "reject" is a funny term; this comes from the C code, which thinks > of it in terms of the server approving/rejecting (and that makes > sense for scripts calling this command). But at the helper level, > the operations are really store/erase. We probably ought to support > those names, too. > > - piping the credential protocol is slightly awkward; we probably > ought to allow a url on the command line, and avoid reading stdin if > we get one. > > That would give us: > > git credential erase https://example.com > > which is really quite readable. :) > > Likewise, if we choose not to auto-enable the store helper, we'd > probably want to give advice on seeding your password. Right now that > is: > > echo url=https://example.com | git credential fill | git credential approve > > which is...not intuitive. It would make sense to me to have a "seed" > operation which does a fill/approve together. Maybe that should just be > what "store" does, which would allow: > > $ git credential store https://example.com > Username for 'https://example.com': > Password for 'https://user@example.com': > > (Of course you can also just "git fetch" to get prompted, but it seems > like this shouldn't require a network operation if you don't want it > to). There is nothing stopping us from adding a new command to `git credential`: `git credential forget <url>` where the URL may contain the user name for further differentiation (and maybe even a password that will be ignored, for copy/pasting convenience). Ciao, Dscho
On Sun, May 19 2019, Jeff King wrote: > If the user clones with a URL containing a password and has no > credential helper configured, we're stuck. We don't want to write the > password into .git/config because that risks accidentally disclosing it. > But if we don't record it somewhere, subsequent fetches will fail unless > the user is there to input the password. > > The best advice we can give the user is to set up a credential helper. > But we can actually go a step further and enable the "store" helper for > them. This still records the password in plaintext, but: > > 1. It's not inside the repo directory, which makes it slightly less > likely to be disclosed. > > 2. The permissions on the storage file are tighter than what would be > on .git/config. > > So this is generally a security win over the old behavior of writing it > into .git/config. And it's a usability win over the more recent behavior > of just forgetting the password entirely. > > The biggest downside is that it's a bit magical from the user's > perspective, because now the password is off in some other file (usually > ~/.git-credentials, but sometimes in $XDG_CONFIG_HOME). Which > complicates things if they want to purge the repo and password, for > example, because now they can't just delete the repository directory. > > The file location is documented, though, and we point people to the > documentation. So perhaps it will be enough (and better still, may lead > to them configuring a more secure helper). > > Signed-off-by: Jeff King <peff@peff.net> > --- > If we do decide this is too magical, I think the best alternate path is > to advise them on credential helpers, and how to seed the password into > the helper (basically configure the helper and then "git fetch" > should work). > > One other thing I wondered: if they do have a helper configured this > patch will omit the advice entirely, but we'll still print the warning. > Is that useful (to remind them that passwords in URLs are a bad thing), > or is it just annoying? A few things, in no particular order, sorry for the braindump. Please don't take this as "this all sucks" just trying to help by poking holes in it. 1. I'm a bit uneasy about us acting on an incident where the details of what happened / frequency haven't been released. I.e. GitHub supposedly has ~100 million repos, one source claims on the order of ~300 of these (public)[1], none of the sites said anything on that, that 300 number is third-party speculation. Let's say it's 100x that counting private repos. That's still a very small percentage of 100 million. But more importantly, it doesn't give us any insight on how to perhaps better solve this problem, e.g. maybe 80% of it is people using some build system (docker?) where we'd expose /home too. Not saying it is, just that we basically have a partial bug report here. I don't have a good sense for what the state is there. Is this all repos cloned in /var/www, then this is sufficient, or something else? 2. We're now making the trade-off of auto-storing the password in ~/, is that a worse default now in the wild? We're defaulting to storing for a longer time (e.g. on a shared *nix server) in workflows where you'd clone && rm -rf. So yeah, we might handle this *specific* incident, but are we just making it worse for others? 3. There's seemingly no way to just say "I want it the old way, make it so!" without creating an auth helper that does that. Storing passwords in config isn't per-se insecure, neither is having passwords in (https) URLs. Yeah in combination with *other* stuff the might be insecure, but often not (but may actually become insecure by auto-storing in ~/) 4. Re #3: We have existing users in the wild who do this, e.g. GitLab CI, the user/password token (lives for about an hour IIRC) is stored in .gitconfig, there's no writable /home there IIRC (or might not be in similar CI environments), and I daresay the people who set that up understand the security trade-offs. 5. Given #2 && #3 && #4 I'd be much more comfortable with something where we just make this fail outright, and force the user to eitiher say "Yeah I want passwords in .git/config here" or "oh thanks, I'll use a credential helper" via some core.* or credentials.* config. I.e. let's not make #3 impossible, and for users who are clueless about security we're doing them a disservice by auto-using a crappy fallback ~/ helper, whereas they might have an *actual* helper they can use that doesn't suck (i.e. OS pwd store) if it was an error. 6. We'll still happily let this new behavior pass by on init && config fetch. Should we care? I was going to say this categorically breaks core.sharedRepository=group with a password in the repo, it does in the 'git -c core.sharedRepository=group clone' case, but you can manually set it up still, you just can't use "clone" anymore. 7. We still accept passwords on the command-line for cloning, so if we're trying to help novice users we're still open to the shared server attack where someone just runs "ps auxf" in a loop to scrape passwords. To me this is another good argument for some variant of #3. We could provide ssh-like security by outright refusing passwords on the command-line (could take them interactively, or via some FD/file), then allow relaxing that to the level of this auto-helper, or all the way down to the old behavior. 1. https://hub.packtpub.com/atlassian-bitbucket-github-and-gitlab-take-collective-steps-against-the-git-ransomware-attack/
diff --git a/builtin/clone.c b/builtin/clone.c index 15fffc3268..94d2659154 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -31,6 +31,7 @@ #include "packfile.h" #include "list-objects-filter-options.h" #include "object-store.h" +#include "credential.h" /* * Overall FIXMEs: @@ -894,8 +895,14 @@ static int dir_exists(const char *path) static const char sanitized_url_advice[] = N_( "The URL you provided to Git contains a password. It will be\n" "used to clone the repository, but to avoid accidental disclosure\n" -"the password will not be recorded. Further fetches from the remote\n" -"may require you to provide the password interactively.\n" +"the password will not be recorded in the repository config.\n" +"Since you have no credential helper configured, the \"store\" helper\n" +"has been enabled for this repository, and will provide the password\n" +"for further fetches.\n" +"\n" +"Note that the password is still stored in plaintext in the filesystem;\n" +"consider configuring a more secure helper. See \"git help gitcredentials\"\n" +"and \"git help git-credential-store\" for details.\n" ); int cmd_clone(int argc, const char **argv, const char *prefix) @@ -1090,7 +1097,13 @@ int cmd_clone(int argc, const char **argv, const char *prefix) sanitized_repo = transport_strip_url(repo, 0); if (strcmp(repo, sanitized_repo)) { warning(_("omitting password while storing URL in on-disk config")); - advise(_(sanitized_url_advice)); + if (!url_has_credential_helper(sanitized_repo)) { + strbuf_addf(&key, "credential.%s.helper", + sanitized_repo); + git_config_set(key.buf, "store"); + strbuf_reset(&key); + advise(_(sanitized_url_advice)); + } } strbuf_addf(&key, "remote.%s.url", option_origin); git_config_set(key.buf, sanitized_repo); diff --git a/credential.c b/credential.c index 62be651b03..0a70edcee5 100644 --- a/credential.c +++ b/credential.c @@ -372,3 +372,16 @@ void credential_from_url(struct credential *c, const char *url) *p-- = '\0'; } } + +int url_has_credential_helper(const char *url) +{ + struct credential c = CREDENTIAL_INIT; + int ret; + + credential_from_url(&c, url); + credential_apply_config(&c); + ret = c.helpers.nr > 0; + + credential_clear(&c); + return ret; +} diff --git a/credential.h b/credential.h index 6b0cd16be2..e6d6d6fa40 100644 --- a/credential.h +++ b/credential.h @@ -32,4 +32,10 @@ void credential_from_url(struct credential *, const char *url); int credential_match(const struct credential *have, const struct credential *want); +/* + * Return true if feeding "url" to the credential system would trigger one + * or more helpers. + */ +int url_has_credential_helper(const char *url); + #endif /* CREDENTIAL_H */ diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh index d8c85f3126..2723e91ae0 100755 --- a/t/t5550-http-fetch-dumb.sh +++ b/t/t5550-http-fetch-dumb.sh @@ -70,7 +70,7 @@ test_expect_success 'username is retained in URL, password is not' ' ! grep pass url ' -test_expect_failure 'fetch of password-URL clone uses stored auth' ' +test_expect_success 'fetch of password-URL clone uses stored auth' ' set_askpass wrong && git -C clone-auth-none fetch && expect_askpass none
If the user clones with a URL containing a password and has no credential helper configured, we're stuck. We don't want to write the password into .git/config because that risks accidentally disclosing it. But if we don't record it somewhere, subsequent fetches will fail unless the user is there to input the password. The best advice we can give the user is to set up a credential helper. But we can actually go a step further and enable the "store" helper for them. This still records the password in plaintext, but: 1. It's not inside the repo directory, which makes it slightly less likely to be disclosed. 2. The permissions on the storage file are tighter than what would be on .git/config. So this is generally a security win over the old behavior of writing it into .git/config. And it's a usability win over the more recent behavior of just forgetting the password entirely. The biggest downside is that it's a bit magical from the user's perspective, because now the password is off in some other file (usually ~/.git-credentials, but sometimes in $XDG_CONFIG_HOME). Which complicates things if they want to purge the repo and password, for example, because now they can't just delete the repository directory. The file location is documented, though, and we point people to the documentation. So perhaps it will be enough (and better still, may lead to them configuring a more secure helper). Signed-off-by: Jeff King <peff@peff.net> --- If we do decide this is too magical, I think the best alternate path is to advise them on credential helpers, and how to seed the password into the helper (basically configure the helper and then "git fetch" should work). One other thing I wondered: if they do have a helper configured this patch will omit the advice entirely, but we'll still print the warning. Is that useful (to remind them that passwords in URLs are a bad thing), or is it just annoying? builtin/clone.c | 19 ++++++++++++++++--- credential.c | 13 +++++++++++++ credential.h | 6 ++++++ t/t5550-http-fetch-dumb.sh | 2 +- 4 files changed, 36 insertions(+), 4 deletions(-)