{"thread":{"id":"51011","subject":"[PATCH 0/1] credential: do not mask the username","startedAt":"2019-04-29T22:06:14Z","lastAt":"2019-05-03T09:09:17Z","messageCount":8,"participants":["Johannes Schindelin via GitGitGadget","Jarosław Honkis via GitGitGadget","Eric Sunshine","Johannes Schindelin","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"374677","messageId":"pull.138.git.gitgitgadget@gmail.com","threadId":"51011","inReplyTo":null,"subject":"[PATCH 0/1] credential: do not mask the username","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-04-29T22:06:10Z","receivedAt":"2019-04-29T22:06:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This is yet another patch that finally makes it from Git for Windows into\ncore Git.\n\nJarosław Honkis (1):\n credential: do not mask the username\n\n credential.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\n\nbase-commit: 8104ec994ea3849a968b4667d072fedd1e688642\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-138%2Fdscho%2Funmask-credentials-username-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-138/dscho/unmask-credentials-username-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/138\n-- \ngitgitgadget\n"},{"id":"374679","messageId":"e459e487d3848ae1b7f37676bd9d2a2f9c967430.1556575570.git.gitgitgadget@gmail.com","threadId":"51011","inReplyTo":"pull.138.git.gitgitgadget@gmail.com","subject":"[PATCH 1/1] credential: do not mask the username","fromName":"Jarosław Honkis via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-04-29T22:06:11Z","receivedAt":"2019-04-29T22:06:17Z","isPatch":true,"sender":{"key":"name:Jarosław Honkis","avatar":null},"body":"From: =?UTF-8?q?Jaros=C5=82aw=20Honkis?= \n\nWhen a user is asked for credentials there is no need to mask the\nusername, so the PROMPT_ASKPASS flag on calling credential_ask_one for\nlogin is unnecessary.\n\nThe `credential_ask_one()` function internally uses `git_prompt()` which\nin case it is given the flag PROMPT_ASKPASS uses masked input method\ninstead of git_terminal_prompt, which does not mask user input.\n\nThis fixes https://github.com/git-for-windows/git/issue/675\n\nSigned-off-by: Jarosław Honkis \nSigned-off-by: Johannes Schindelin \n---\n credential.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/credential.c b/credential.c\nindex 62be651b03..e9108a9e8a 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -136,7 +136,9 @@ static void credential_getpass(struct credential *c)\n {\n \tif (!c->username)\n \t\tc->username = credential_ask_one(\"Username\", c,\n-\t\t\t\t\t\t PROMPT_ASKPASS|PROMPT_ECHO);\n+\t\t\t\t\t\t (getenv(\"GIT_ASKPASS\") ?\n+\t\t\t\t\t\t PROMPT_ASKPASS : 0) |\n+\t\t\t\t\t\t PROMPT_ECHO);\n \tif (!c->password)\n \t\tc->password = credential_ask_one(\"Password\", c,\n \t\t\t\t\t\t PROMPT_ASKPASS);\n-- \ngitgitgadget\n"},{"id":"374681","messageId":"CAPig+cSiv+4xh6bWDTggBiApywAmAx6eQLq+Dh116kO2+cf_kg@mail.gmail.com","threadId":"51011","inReplyTo":"e459e487d3848ae1b7f37676bd9d2a2f9c967430.1556575570.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] credential: do not mask the username","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-04-29T22:12:54Z","receivedAt":"2019-04-29T22:13:07Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 29, 2019 at 6:06 PM Jarosław Honkis via GitGitGadget\n wrote:\n> When a user is asked for credentials there is no need to mask the\n> username, so the PROMPT_ASKPASS flag on calling credential_ask_one for\n> login is unnecessary.\n>\n> The `credential_ask_one()` function internally uses `git_prompt()` which\n> in case it is given the flag PROMPT_ASKPASS uses masked input method\n> instead of git_terminal_prompt, which does not mask user input.\n>\n> This fixes https://github.com/git-for-windows/git/issue/675\n\nThis link is dead. Is there a more modern URL to which this can point?\n\n> Signed-off-by: Jarosław Honkis \n> Signed-off-by: Johannes Schindelin \n"},{"id":"374685","messageId":"nycvar.QRO.7.76.6.1904291904560.45@tvgsbejvaqbjf.bet","threadId":"51011","inReplyTo":"CAPig+cSiv+4xh6bWDTggBiApywAmAx6eQLq+Dh116kO2+cf_kg@mail.gmail.com","subject":"Re: [PATCH 1/1] credential: do not mask the username","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-04-29T23:06:05Z","receivedAt":"2019-04-29T23:06:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\n\nOn Mon, 29 Apr 2019, Eric Sunshine wrote:\n\n> On Mon, Apr 29, 2019 at 6:06 PM Jarosław Honkis via GitGitGadget\n> wrote:\n> > When a user is asked for credentials there is no need to mask the\n> > username, so the PROMPT_ASKPASS flag on calling credential_ask_one for\n> > login is unnecessary.\n> >\n> > The `credential_ask_one()` function internally uses `git_prompt()` which\n> > in case it is given the flag PROMPT_ASKPASS uses masked input method\n> > instead of git_terminal_prompt, which does not mask user input.\n> >\n> > This fixes https://github.com/git-for-windows/git/issue/675\n>\n> This link is dead. Is there a more modern URL to which this can point?\n\nOops. I should have looked more carefully, right? It is missing an \"s\" at\nthe end of \"issues\":\n\n\thttps://github.com/git-for-windows/git/issues/675\n\nThanks,\nDscho\n\n> > Signed-off-by: Jarosław Honkis \n> > Signed-off-by: Johannes Schindelin \n>\n>\n"},{"id":"374688","messageId":"20190429234028.GA24069@sigill.intra.peff.net","threadId":"51011","inReplyTo":"e459e487d3848ae1b7f37676bd9d2a2f9c967430.1556575570.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] credential: do not mask the username","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-29T23:40:28Z","receivedAt":"2019-04-29T23:40:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 29, 2019 at 03:06:11PM -0700, Jarosław Honkis via GitGitGadget wrote:\n\n> From: =?UTF-8?q?Jaros=C5=82aw=20Honkis?= \n> \n> When a user is asked for credentials there is no need to mask the\n> username, so the PROMPT_ASKPASS flag on calling credential_ask_one for\n> login is unnecessary.\n> \n> The `credential_ask_one()` function internally uses `git_prompt()` which\n> in case it is given the flag PROMPT_ASKPASS uses masked input method\n> instead of git_terminal_prompt, which does not mask user input.\n\nThis description (and the patch) doesn't make sense to me. The\nPROMPT_ASKPASS flag is just about whether we would trigger the askpass\ntool (e.g., if the user does not have a terminal).\n\nThe PROMPT_ECHO flag is what you want to tell the underlying code that\nthe value can be shown to the user. And that's already set.\n\nNow there is a slight issue, which is that the askpass tool has no way\nfor us to tell it to show the contents to the user. There's no way\naround that without disabling askpass entirely, which I guess is the\nstrategy this patch is trying to do. But in doing so it's going to break\nanybody who _doesn't_ have a terminal, because now we have no way to\nprompt there for their username!\n\nSo I think our options are:\n\n 1. Leave it. If people don't want askpass to prompt them, they should\n not set up askpass.\n\n 2. Use another tool besides askpass. I don't know of any askpass\n implementations that take something like our ECHO flag, but there\n are lots of other tools. I doubt there's any easy portable\n solution, though. And anyway, credential helpers are a much more\n advanced version of this anyway, so I'd probably steer people in\n that direction.\n\n 3. If we really want to try to try to avoid using askpass for\n usernames we can, but I don't think the logic in this patch is\n right.\n\n If we want to avoid regressing existing cases, then we'd have to\n first check if there's a usable terminal. And only if _that_ fails,\n try askpass. And then give up if neither work. I.e., invert the\n order in git_prompt() when both ASKPASS and ECHO are set.\n\n I think I'd still favor option 1 over this, though. Configuring\n askpass has always overridden the terminal for usernames, and this\n would change that. I come back to: if you don't want to use\n askpass, then why are you configuring it?\n\n> \t\tc->username = credential_ask_one(\"Username\", c,\n> -\t\t\t\t\t\t PROMPT_ASKPASS|PROMPT_ECHO);\n> +\t\t\t\t\t\t (getenv(\"GIT_ASKPASS\") ?\n> +\t\t\t\t\t\t PROMPT_ASKPASS : 0) |\n> +\t\t\t\t\t\t PROMPT_ECHO);\n\nThis logic is weird. It still uses askpass if GIT_ASKPASS isn't set. But\n_doesn't_ if it's set elsewhere, e.g. in core.askPass. Which makes\nlittle sense to me.\n\nMaybe the intent was that the original user has SSH_ASKPASS set, and\nthat is kicking in (which would also explain why \"if you don't like it,\ndon't configure it\" didn't work). You can get around that by setting\nGIT_ASKPASS (or core.askPass) to the empty string (I don't think that's\ndocumented anywhere, and I don't recall it being intentional, but it\ndoes look like that's how the code works).\n\n-Peff\n"},{"id":"374720","messageId":"nycvar.QRO.7.76.6.1904301753030.45@tvgsbejvaqbjf.bet","threadId":"51011","inReplyTo":"20190429234028.GA24069@sigill.intra.peff.net","subject":"Re: [PATCH 1/1] credential: do not mask the username","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-04-30T22:07:15Z","receivedAt":"2019-04-30T22:07:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Mon, 29 Apr 2019, Jeff King wrote:\n\n> On Mon, Apr 29, 2019 at 03:06:11PM -0700, Jarosław Honkis via GitGitGadget wrote:\n>\n> > From: =?UTF-8?q?Jaros=C5=82aw=20Honkis?= \n> >\n> > When a user is asked for credentials there is no need to mask the\n> > username, so the PROMPT_ASKPASS flag on calling credential_ask_one for\n> > login is unnecessary.\n> >\n> > The `credential_ask_one()` function internally uses `git_prompt()` which\n> > in case it is given the flag PROMPT_ASKPASS uses masked input method\n> > instead of git_terminal_prompt, which does not mask user input.\n>\n> This description (and the patch) doesn't make sense to me. The\n> PROMPT_ASKPASS flag is just about whether we would trigger the askpass\n> tool (e.g., if the user does not have a terminal).\n\nThanks for calling this out. It is a bit of a problem and you're right, I\nshould improve the commit message.\n\n> The PROMPT_ECHO flag is what you want to tell the underlying code that\n> the value can be shown to the user. And that's already set.\n>\n> Now there is a slight issue, which is that the askpass tool has no way\n> for us to tell it to show the contents to the user. There's no way\n> around that without disabling askpass entirely, which I guess is the\n> strategy this patch is trying to do.\n\nPrecisely.\n\n> But in doing so it's going to break anybody who _doesn't_ have a\n> terminal, because now we have no way to prompt there for their username!\n>\n> So I think our options are:\n>\n> 1. Leave it. If people don't want askpass to prompt them, they should\n> not set up askpass.\n>\n> 2. Use another tool besides askpass. I don't know of any askpass\n> implementations that take something like our ECHO flag, but there\n> are lots of other tools. I doubt there's any easy portable\n> solution, though. And anyway, credential helpers are a much more\n> advanced version of this anyway, so I'd probably steer people in\n> that direction.\n>\n> 3. If we really want to try to try to avoid using askpass for\n> usernames we can, but I don't think the logic in this patch is\n> right.\n>\n> If we want to avoid regressing existing cases, then we'd have to\n> first check if there's a usable terminal. And only if _that_ fails,\n> try askpass. And then give up if neither work. I.e., invert the\n> order in git_prompt() when both ASKPASS and ECHO are set.\n>\n> I think I'd still favor option 1 over this, though. Configuring\n> askpass has always overridden the terminal for usernames, and this\n> would change that. I come back to: if you don't want to use\n> askpass, then why are you configuring it?\n\nHow about inventing a new convention: we could set `GIT_ASKPASS_FLAGS`\nas an environment variable that contains whitespace-delimited flags, in\nthe current instance just \"echo\"?\n\nWe could then teach `git-gui--askpass` to heed that.\n\nMind you, these days, this patch is a lot less necessary because Git for\nWindows (which is *the* platform where you'd expect no terminal to be\navailable when fetching/pushing, right?) defaults to using the Git\nCredential Manager [*1*].\n\n> > \t\tc->username = credential_ask_one(\"Username\", c,\n> > -\t\t\t\t\t\t PROMPT_ASKPASS|PROMPT_ECHO);\n> > +\t\t\t\t\t\t (getenv(\"GIT_ASKPASS\") ?\n> > +\t\t\t\t\t\t PROMPT_ASKPASS : 0) |\n> > +\t\t\t\t\t\t PROMPT_ECHO);\n>\n> This logic is weird. It still uses askpass if GIT_ASKPASS isn't set. But\n> _doesn't_ if it's set elsewhere, e.g. in core.askPass. Which makes\n> little sense to me.\n\nTrue. I guess this is my fault, as that patch is so old that core.askPass\nmight not have existed when it was written.\n\n> Maybe the intent was that the original user has SSH_ASKPASS set, and\n> that is kicking in (which would also explain why \"if you don't like it,\n> don't configure it\" didn't work). You can get around that by setting\n> GIT_ASKPASS (or core.askPass) to the empty string (I don't think that's\n> documented anywhere, and I don't recall it being intentional, but it\n> does look like that's how the code works).\n\nI agree with your concerns.\n\nWhat do you think about the `GIT_ASKPASS_FLAGS` thing? It is not perfect,\nbut it could be used at least in conjunction with `git-gui--askpass` (or\nwith other helpers that would make use of this side channel to receive\nmore information about the way they should present the question to the\nuser)?\n\nAlternatively, I could simply drop that patch from Git for Windows, as it\nreally only concerns users who override the default, opting out of using\nGit Credential Manager.\n\nCiao,\nDscho\n\nFootnote *1*: The Git Credential Manager for Windows\n(https://github.com/Microsoft/Git-Credential-Manager-for-Windows/) is a C#\nproject that requires .NET Framework, which older Windows versions might\nnot have, but I think most of Git for Windows' users do. In the interest\nof keeping users' privacy intact, we have no way of knowing, of course.\n\nThe Git Credential Manager is preferable over the `git-gui--askpass`\nmethod in more ways than just unmasking the username: it also supports\nTwo-Factor-Authorization, most notably with GitHub, Azure Repos and\nBitBucket.\n"},{"id":"374724","messageId":"20190430222146.GA19492@sigill.intra.peff.net","threadId":"51011","inReplyTo":"nycvar.QRO.7.76.6.1904301753030.45@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/1] credential: do not mask the username","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-30T22:21:46Z","receivedAt":"2019-04-30T22:21:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 30, 2019 at 06:07:15PM -0400, Johannes Schindelin wrote:\n\n> > 2. Use another tool besides askpass. I don't know of any askpass\n> > implementations that take something like our ECHO flag, but there\n> > are lots of other tools. I doubt there's any easy portable\n> > solution, though. And anyway, credential helpers are a much more\n> > advanced version of this anyway, so I'd probably steer people in\n> > that direction.\n> [...]\n> \n> How about inventing a new convention: we could set `GIT_ASKPASS_FLAGS`\n> as an environment variable that contains whitespace-delimited flags, in\n> the current instance just \"echo\"?\n> \n> We could then teach `git-gui--askpass` to heed that.\n\nYeah, I almost suggested something like that for my (2) above, but I\ncouldn't actually find an askpass implementation that supported this\n(though there are lots of general dialog tools that do).\n\nBut today I learned about git-gui--askpass. :)\n\nSo yeah, I think that is a solution in the right direction, but it may\nnot be helping that many people if it is specific to git-gui--askpass.\n\n> Mind you, these days, this patch is a lot less necessary because Git for\n> Windows (which is *the* platform where you'd expect no terminal to be\n> available when fetching/pushing, right?) defaults to using the Git\n> Credential Manager [*1*].\n\nThe no-terminal thing isn't specific to Windows. You can imagine\nsomebody triggering \"git fetch\" from any GUI on Linux that no longer has\na controlling terminal (because the GUI was spawned by a desktop manager\nor some such).\n\nBut I definitely agree that credential helpers are the right way to do\nfancier prompting. AFAIK your GCM is the only one which actually does\nprompting (most of the rest are just key-store interfaces), but it's\ndefinitely inbounds and was part of the original intent. For usernames,\nI personally just stick mine in credential.$URL.username config, since\nthey're obviously not a secret. But then my habits may not match most\nusers. :)\n\n> What do you think about the `GIT_ASKPASS_FLAGS` thing? It is not perfect,\n> but it could be used at least in conjunction with `git-gui--askpass` (or\n> with other helpers that would make use of this side channel to receive\n> more information about the way they should present the question to the\n> user)?\n\nIt seems perfectly reasonable to me, but if it were me, I think this:\n\n> Alternatively, I could simply drop that patch from Git for Windows, as it\n> really only concerns users who override the default, opting out of using\n> Git Credential Manager.\n\nwould be the path I would take.\n\n-Peff\n"},{"id":"374863","messageId":"nycvar.QRO.7.76.6.1905031108260.45@tvgsbejvaqbjf.bet","threadId":"51011","inReplyTo":"20190430222146.GA19492@sigill.intra.peff.net","subject":"Re: [PATCH 1/1] credential: do not mask the username","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-05-03T09:09:05Z","receivedAt":"2019-05-03T09:09:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Tue, 30 Apr 2019, Jeff King wrote:\n\n> On Tue, Apr 30, 2019 at 06:07:15PM -0400, Johannes Schindelin wrote:\n>\n> > Alternatively, I could simply drop that patch from Git for Windows, as\n> > it really only concerns users who override the default, opting out of\n> > using Git Credential Manager.\n>\n> would be the path I would take.\n\nOkay, let's drop it, then.\n\nThanks,\nDscho\n"}]}