Re: [PATCH 0/6] echo usernames as they are typed
- From
Erik Faye-Lund <kusmabite@gmail.com>
- Date
- Nov 28, 2011, 09:36 UTC
- Message-ID
- <CABPQNSbLvWh-ivaqBk-Du+kwZvV3t+ajEJhHATRzyGZbHYyM=Q@mail.gmail.com>
- In-Reply-To
- <20111128035321.GA15640@sigill.intra.peff.net>
On Mon, Nov 28, 2011 at 4:53 AM, Jeff King <peff@peff.net> wrote:
Show 21 quoted lines
> On Sun, Nov 27, 2011 at 10:17:26AM +0100, Erik Faye-Lund wrote: > >> > And here's something I've been meaning to do on top: actually echo >> > characters at the username prompt. We can't do this portably, but we can >> > at least stub out a compatibility layer and let each system do something >> > sensible. >> >> Interesting, I've been working on something pretty similar: getting >> rid of getpass usage all together: >> >> https://github.com/kusma/git/tree/work/askpass >> >> My reason to write a getpass replacement was to avoid capping input to >> PASS_MAX, which can be as low as 8 characters (and AFAIK is just that >> on Solaris)... > > Yeah, if there are really bad getpass implementations, we would want to > work around them. If we are going to do so, it might make sense to > combine the effort with my getpass_echo wrapper, as they are really the > same function, modulo tweaking the echo settings. >
My thinking exactly ;)
Show 9 quoted lines
> It would also be nice to make getpass a little more predictable. If > /dev/tty can't be opened, glibc's getpass will fall back to writing the > prompt to stderr and reading the password from stdin. But we definitely > don't want to do that in git-remote-curl, where stdin is already talking > a special protocol with the parent fetch process. > > So I think it might be best to just write our own getpass. However, > your implementation looks wrong to me: >
It probably is, yes. It was a very naive attempt ;)
Show 37 quoted lines
>> diff --git a/password.c b/password.c
>> new file mode 100644
>> index 0000000..727ed84
>> --- /dev/null
>> +++ b/password.c
>> @@ -0,0 +1,94 @@
>> +#include "cache.h"
>> +#include "git-compat-util.h"
>> +#include "strbuf.h"
>> +#include "run-command.h"
>> +
>> +#ifndef WIN32
>> +
>> +static void ask_password(struct strbuf *sb, const char *prompt)
>> +{
>> + struct termios attr;
>> + tcflag_t c_lflag;
>> +
>> + if (tcgetattr(1, &attr) < 0)
>> + die("tcgetattr failed!");
>> + c_lflag = attr.c_lflag;
>
> This is getting the terminal attributes for stdout. But in many
> cases, stdout will not be connected to the terminal (in particular,
> remote-curl, as I mentioned above, will have its stdio connected to the
> parent fetch process). Stderr is a better guess, as you do here:
>
>> + fputs(prompt, stderr);
>
> but even that is not foolproof. With getpass(), this should work:
>
> git clone ... 2>errors
>
> with the prompt going to the terminal. But it doesn't with your patch.
>
> You really want to open "/dev/tty" on most Unix systems (which is what
> getpass() does).Yes, you're right. Opening "/dev/tty" is much better. But what happens for processes started by GUI applications (with no easily observable tty, if any)? Does open simply fail? If so, is it desirable for us to fail in that case?
> I have no idea what would be appropriate on Windows. >
It's pretty similar, but not exactly: CreateFile("CONIN$", ...) or CreateFile("CONOUT$", ...), depending on if you want the read-handle or the write-handle... I can probably cook up something a bit more concrete, though.
But _getch() that we already use always reads from the console (according to MSDN, I haven't actually tested this myself: http://msdn.microsoft.com/en-us/library/078sfkak%28v=VS.80%29.aspx). But I don't think this allows us to fail when no console is attached. Question is, should we fail in such cases? Windows does have an API to prompt for passwords in a GUI window... Perhaps fallbacking to those are the way to go? Something like:
if (GetConsoleWindow()) {
/* normal console-stuff */
} else {
/* call CredUIPromptForWindowsCredentials(...) instead */
}This might be nicer towards GUI tools, but it requires us to explicitly ask for a password (and possibly a username at the same time)...
Show 12 quoted lines
>> + for (;;) {
>> + int c = getchar();
>> + if (c == EOF || c == '\n')
>> + break;
>> + strbuf_addch(sb, c);
>> + }
>
> And this is even worse. You're reading from stdin, which will get
> whatever cruft is in the pipe coming from the parent process (or may
> even cause a hang, as the parent is probably blocking waiting to read
> from the child helper).
>Yeah, thanks for pointing these glitches out ;)