# [PATCH v2 2/3] git-core: Support retrieving passwords with GIT_ASKPASS 8 messages from 2010-02-26 to 2010-02-26. Participants: Frank Li, Miklos Vajna, Johannes Sixt, Junio C Hamano. Thread: https://gitlist.dev/t/22825 ## Frank Li, 2010-02-26 00:12 Subject: [PATCH v2 2/3] git-core: Support retrieving passwords with GIT_ASKPASS Message-ID: <1267143154-5020-1-git-send-email-lznuaa@gmail.com> URL: https://gitlist.dev/e/1267143154-5020-1-git-send-email-lznuaa%40gmail.com ``` imap-send and authority http connect reads passwords from an interactive terminal. This behavious cause GUIs to hang waiting for git complete. Fix this problem by allowing a password-retrieving command to be specified in GIT_ASKPASS Signed-off-by: Frank Li --- connect.c | 40 ++++++++++++++++++++++++++++++++++++++++ http.c | 4 ++-- imap-send.c | 2 +- 3 files changed, 43 insertions(+), 3 deletions(-) diff --git a/connect.c b/connect.c index a37cf6a..6a8e3ab 100644 --- a/connect.c +++ b/connect.c @@ -647,3 +647,43 @@ int finish_connect(struct child_process *conn) free(conn); return code; } + +char *git_getpass(char *prompt) +{ + char *askpass; + struct child_process pass; + const char *args[3]; + struct strbuf buffer = STRBUF_INIT; + int i = 0; + + askpass = getenv("GIT_ASKPASS"); + if (askpass && strlen(askpass) != 0) { + args[0] = getenv("GIT_ASKPASS"); + args[1] = prompt; + args[2] = NULL; + + memset(&pass, 0, sizeof(pass)); + pass.argv = args; + pass.out = -1; + pass.no_stdin = 1; + pass.no_stderr = 1; + + if (start_command(&pass)) { + error("could not run %s\n", askpass); + return getpass(prompt); + } + + strbuf_read(&buffer, pass.out, 20); + close(pass.out); + for (i = 0; i < buffer.len; i++) + if (buffer.buf[i] == '\n' || buffer.buf[i] == '\r') { + buffer.buf[i] = '\0'; + buffer.len = i; + } + return strbuf_detach(&buffer, NULL); + + } else { + return getpass(prompt); + } + return NULL; +} diff --git a/http.c b/http.c index deab595..4814217 100644 --- a/http.c +++ b/http.c @@ -204,7 +204,7 @@ static void init_curl_http_auth(CURL *result) if (user_name) { struct strbuf up = STRBUF_INIT; if (!user_pass) - user_pass = xstrdup(getpass("Password: ")); + user_pass = xstrdup(git_getpass("Password: ")); strbuf_addf(&up, "%s:%s", user_name, user_pass); curl_easy_setopt(result, CURLOPT_USERPWD, strbuf_detach(&up, NULL)); @@ -219,7 +219,7 @@ static int has_cert_password(void) return 0; /* Only prompt the user once. */ ssl_cert_password_required = -1; - ssl_cert_password = getpass("Certificate Password: "); + ssl_cert_password = git_getpass("Certificate Password: "); if (ssl_cert_password != NULL) { ssl_cert_password = xstrdup(ssl_cert_password); return 1; diff --git a/imap-send.c b/imap-send.c index 5631930..5254b2a 100644 --- a/imap-send.c +++ b/imap-send.c @@ -1107,7 +1107,7 @@ static struct store *imap_open_store(struct imap_server_conf *srvc) if (!srvc->pass) { char prompt[80]; sprintf(prompt, "Password (%s@%s): ", srvc->user, srvc->host); - arg = getpass(prompt); + arg = git_getpass(prompt); if (!arg) { perror("getpass"); exit(1); -- 1.7.0.85.g37fda.dirty ``` ## Miklos Vajna, 2010-02-26 00:50 Subject: Re: [PATCH v2 2/3] git-core: Support retrieving passwords with GIT_ASKPASS Message-ID: <20100226005020.GR12429@genesis.frugalware.org> URL: https://gitlist.dev/e/20100226005020.GR12429%40genesis.frugalware.org In-Reply-To: <1267143154-5020-1-git-send-email-lznuaa@gmail.com> ``` On Fri, Feb 26, 2010 at 08:12:34AM +0800, Frank Li wrote: > + for (i = 0; i < buffer.len; i++) > + if (buffer.buf[i] == '\n' || buffer.buf[i] == '\r') { > + buffer.buf[i] = '\0'; > + buffer.len = i; > + } > + return strbuf_detach(&buffer, NULL); > + > + } else { > + return getpass(prompt); > + } According to Documentation/CodingGuidelines, this would be: for (i = 0; i < buffer.len; i++) if (buffer.buf[i] == '\n' || buffer.buf[i] == '\r') { buffer.buf[i] = '\0'; buffer.len = i; } return strbuf_detach(&buffer, NULL); } else return getpass(prompt); ``` ## Frank Li, 2010-02-26 02:17 Subject: Re: [PATCH v2 2/3] git-core: Support retrieving passwords with GIT_ASKPASS Message-ID: <1976ea661002251817v12f04314vc32fe7924f31c070@mail.gmail.com> URL: https://gitlist.dev/e/1976ea661002251817v12f04314vc32fe7924f31c070%40mail.gmail.com In-Reply-To: <20100226005020.GR12429@genesis.frugalware.org> ``` > According to Documentation/CodingGuidelines, this would be: > > >                for (i = 0; i < buffer.len; i++) >                        if (buffer.buf[i] == '\n' || buffer.buf[i] == '\r') { >                                buffer.buf[i] = '\0'; >                                buffer.len = i; >                        } >                return strbuf_detach(&buffer, NULL); > >        } else >                return getpass(prompt); > Okay, I will change it. ``` ## Johannes Sixt, 2010-02-26 07:34 Subject: Re: [PATCH v2 2/3] git-core: Support retrieving passwords with GIT_ASKPASS Message-ID: <4B87797D.7030905@viscovery.net> URL: https://gitlist.dev/e/4B87797D.7030905%40viscovery.net In-Reply-To: <1267143154-5020-1-git-send-email-lznuaa@gmail.com> ``` Frank Li schrieb: > connect.c | 40 ++++++++++++++++++++++++++++++++++++++++ > http.c | 4 ++-- > imap-send.c | 2 +- I don't see any header file changes. Don't you get warnings about an undeclared function git_getpass() at the call sites? > +char *git_getpass(char *prompt) char *git_getpass(const char *prompt) > + askpass = getenv("GIT_ASKPASS"); > + if (askpass && strlen(askpass) != 0) { > + args[0] = getenv("GIT_ASKPASS"); if (askpass && *askpass) { args[0] = askpass; BTW, to save a level of indentation, you could handle the "trivial" case early like this: if (!askpass || !*askpass) return get_pass(prompt); and continue without an 'else' branch. > + args[1] = prompt; > + args[2] = NULL; > + > + memset(&pass, 0, sizeof(pass)); > + pass.argv = args; > + pass.out = -1; > + pass.no_stdin = 1; > + pass.no_stderr = 1; Is it such a good idea to redirect stdin and stderr to /dev/null? What if my password prompt program depends on them? I think it should not matter for your use-case, where a GUI is invoked, to just inherit all channels. OTOH, it may be worthwhile to set pass.use_shell = 1; to allow commands that are not just a single plain word. But perhaps this has security implications - I don't know. > + > + if (start_command(&pass)) { > + error("could not run %s\n", askpass); > + return getpass(prompt); I don't think this is a good idea. The user instructed to use GIT_ASKPASS, and you fall back to asking a password from the terminal. I think the most sensible thing to do here is to 'exit(1)' (start_command has already printed an error message that included the command), because there are callers that do not expect NULL. > + } > + > + strbuf_read(&buffer, pass.out, 20); > + close(pass.out); > + for (i = 0; i < buffer.len; i++) > + if (buffer.buf[i] == '\n' || buffer.buf[i] == '\r') { > + buffer.buf[i] = '\0'; > + buffer.len = i; > + } > + return strbuf_detach(&buffer, NULL); You don't call finish_command() anywhere. Call it after the close() call. > + > + } else { > + return getpass(prompt); You handle the return value in different ways. getpass() returns a pointer to a static buffer, but in the 'then' branch you return an allocated buffer. Not that it matters a lot, though. You could add a comment that you are aware that the memory is leaked. > + } > + return NULL; What is this good for? -- Hannes ``` ## Junio C Hamano, 2010-02-26 07:50 Subject: Re: [PATCH v2 2/3] git-core: Support retrieving passwords with GIT_ASKPASS Message-ID: <7vr5o84erv.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vr5o84erv.fsf%40alter.siamese.dyndns.org In-Reply-To: <4B87797D.7030905@viscovery.net> ``` Johannes Sixt writes: > BTW, to save a level of indentation, you could handle the "trivial" case > early like this: > > if (!askpass || !*askpass) > return get_pass(prompt); > > and continue without an 'else' branch. That is a good advice in general. Also, when you have a way unbalanced if ... else ... where else clause is very small, it usually is much easier to read if you invert the logic to make if part smaller. > OTOH, it may be worthwhile to set > > pass.use_shell = 1; > > to allow commands that are not just a single plain word. But perhaps this > has security implications - I don't know. How does SSH_ASKPASS gets interpreted by other programs? I think we should follow that example. Other than that, I agree with everything you said in your review. Thanks. ``` ## Johannes Sixt, 2010-02-26 09:32 Subject: Re: [PATCH v2 2/3] git-core: Support retrieving passwords with GIT_ASKPASS Message-ID: <4B87952F.1000902@viscovery.net> URL: https://gitlist.dev/e/4B87952F.1000902%40viscovery.net In-Reply-To: <7vr5o84erv.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano schrieb: > Johannes Sixt writes: >> OTOH, it may be worthwhile to set >> >> pass.use_shell = 1; >> >> to allow commands that are not just a single plain word. But perhaps this >> has security implications - I don't know. > > How does SSH_ASKPASS gets interpreted by other programs? I think we > should follow that example. openssh treats SSH_ASKPASS as a command name and uses execlp, i.e., does a PATH search; no shell tricks are possible. Hence, we should *not* set use_shell. http://www.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/readpass.c?rev=1.47 Of course, we could define that GIT_ASKPASS is different from SSH_ASKPASS in this regard, but I haven't followed the discussion to know whether this is necessary. -- Hannes ``` ## Frank Li, 2010-02-26 10:01 Subject: Re: [PATCH v2 2/3] git-core: Support retrieving passwords with GIT_ASKPASS Message-ID: <1976ea661002260201s75ef6185x3f89742d58f12222@mail.gmail.com> URL: https://gitlist.dev/e/1976ea661002260201s75ef6185x3f89742d58f12222%40mail.gmail.com In-Reply-To: <4B87797D.7030905@viscovery.net> ``` > > You handle the return value in different ways. getpass() returns a pointer > to a static buffer, but in the 'then' branch you return an allocated > buffer. Not that it matters a lot, though. You could add a comment that > you are aware that the memory is leaked. > How about my branch also use static buffer, so there are not memory leak. best regards Frank Li ``` ## Junio C Hamano, 2010-02-26 17:50 Subject: Re: [PATCH v2 2/3] git-core: Support retrieving passwords with GIT_ASKPASS Message-ID: <7vmxyvx4wx.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vmxyvx4wx.fsf%40alter.siamese.dyndns.org In-Reply-To: <4B87952F.1000902@viscovery.net> ``` Johannes Sixt writes: > openssh treats SSH_ASKPASS as a command name and uses execlp, i.e., does a > PATH search; no shell tricks are possible. Hence, we should *not* set > use_shell. > > http://www.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/readpass.c?rev=1.47 > > Of course, we could define that GIT_ASKPASS is different from SSH_ASKPASS > in this regard, but I haven't followed the discussion to know whether this > is necessary. It is sad that they do exec*p() on that one; it means that the users cannot supply leading set of options with CMD="cmd --initial-option". But we should do the same as others do. It is doubly sad that I earlier was hoping that we can run external programs specified via GIT_* uniformly via shell. ```