{"thread":{"id":"22825","subject":"[PATCH v2 2/3] git-core: Support retrieving passwords with GIT_ASKPASS","startedAt":"2010-02-26T00:12:34Z","lastAt":"2010-02-26T17:50:22Z","messageCount":8,"participants":["Frank Li","Miklos Vajna","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"135706","messageId":"1267143154-5020-1-git-send-email-lznuaa@gmail.com","threadId":"22825","inReplyTo":null,"subject":"[PATCH v2 2/3] git-core: Support retrieving passwords with GIT_ASKPASS","fromName":"Frank Li","fromEmail":"lznuaa@gmail.com","sentAt":"2010-02-26T00:12:34Z","receivedAt":"2010-02-26T00:12:34Z","isPatch":true,"sender":{"key":"lznuaa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40642?v=4"},"body":"imap-send and authority http connect reads passwords from an interactive\nterminal. This behavious cause GUIs to hang waiting for git complete.\n\nFix this problem by allowing a password-retrieving command\nto be specified in GIT_ASKPASS\n\nSigned-off-by: Frank Li <lznuaa@gmail.com>\n---\n connect.c   |   40 ++++++++++++++++++++++++++++++++++++++++\n http.c      |    4 ++--\n imap-send.c |    2 +-\n 3 files changed, 43 insertions(+), 3 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex a37cf6a..6a8e3ab 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -647,3 +647,43 @@ int finish_connect(struct child_process *conn)\n \tfree(conn);\n \treturn code;\n }\n+\n+char *git_getpass(char *prompt)\n+{\n+\tchar *askpass;\n+\tstruct child_process pass;\n+\tconst char *args[3];\n+\tstruct strbuf buffer = STRBUF_INIT;\n+\tint i = 0;\n+\n+\taskpass = getenv(\"GIT_ASKPASS\");\n+\tif (askpass && strlen(askpass) != 0) {\n+\t\targs[0] = getenv(\"GIT_ASKPASS\");\n+\t\targs[1]\t= prompt;\n+\t\targs[2] = NULL;\n+\n+\t\tmemset(&pass, 0, sizeof(pass));\n+\t\tpass.argv = args;\n+\t\tpass.out = -1;\n+\t\tpass.no_stdin = 1;\n+\t\tpass.no_stderr = 1;\n+\n+\t\tif (start_command(&pass)) {\n+\t\t\terror(\"could not run %s\\n\", askpass);\n+\t\t\treturn getpass(prompt);\n+\t\t}\n+\n+\t\tstrbuf_read(&buffer, pass.out, 20);\n+\t\tclose(pass.out);\n+\t\tfor (i = 0; i < buffer.len; i++)\n+\t\t\tif (buffer.buf[i] == '\\n' || buffer.buf[i] == '\\r') {\n+\t\t\t\tbuffer.buf[i] = '\\0';\n+\t\t\t\tbuffer.len = i;\n+\t\t}\n+\t\treturn strbuf_detach(&buffer, NULL);\n+\n+\t} else {\n+\t\treturn getpass(prompt);\n+\t}\n+\treturn NULL;\n+}\ndiff --git a/http.c b/http.c\nindex deab595..4814217 100644\n--- a/http.c\n+++ b/http.c\n@@ -204,7 +204,7 @@ static void init_curl_http_auth(CURL *result)\n \tif (user_name) {\n \t\tstruct strbuf up = STRBUF_INIT;\n \t\tif (!user_pass)\n-\t\t\tuser_pass = xstrdup(getpass(\"Password: \"));\n+\t\t\tuser_pass = xstrdup(git_getpass(\"Password: \"));\n \t\tstrbuf_addf(&up, \"%s:%s\", user_name, user_pass);\n \t\tcurl_easy_setopt(result, CURLOPT_USERPWD,\n \t\t\t\t strbuf_detach(&up, NULL));\n@@ -219,7 +219,7 @@ static int has_cert_password(void)\n \t\treturn 0;\n \t/* Only prompt the user once. */\n \tssl_cert_password_required = -1;\n-\tssl_cert_password = getpass(\"Certificate Password: \");\n+\tssl_cert_password = git_getpass(\"Certificate Password: \");\n \tif (ssl_cert_password != NULL) {\n \t\tssl_cert_password = xstrdup(ssl_cert_password);\n \t\treturn 1;\ndiff --git a/imap-send.c b/imap-send.c\nindex 5631930..5254b2a 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -1107,7 +1107,7 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)\n \t\tif (!srvc->pass) {\n \t\t\tchar prompt[80];\n \t\t\tsprintf(prompt, \"Password (%s@%s): \", srvc->user, srvc->host);\n-\t\t\targ = getpass(prompt);\n+\t\t\targ = git_getpass(prompt);\n \t\t\tif (!arg) {\n \t\t\t\tperror(\"getpass\");\n \t\t\t\texit(1);\n-- \n1.7.0.85.g37fda.dirty\n"},{"id":"135712","messageId":"20100226005020.GR12429@genesis.frugalware.org","threadId":"22825","inReplyTo":"1267143154-5020-1-git-send-email-lznuaa@gmail.com","subject":"Re: [PATCH v2 2/3] git-core: Support retrieving passwords with GIT_ASKPASS","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2010-02-26T00:50:20Z","receivedAt":"2010-02-26T00:50:20Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Fri, Feb 26, 2010 at 08:12:34AM +0800, Frank Li <lznuaa@gmail.com> wrote:\n> +\t\tfor (i = 0; i < buffer.len; i++)\n> +\t\t\tif (buffer.buf[i] == '\\n' || buffer.buf[i] == '\\r') {\n> +\t\t\t\tbuffer.buf[i] = '\\0';\n> +\t\t\t\tbuffer.len = i;\n> +\t\t}\n> +\t\treturn strbuf_detach(&buffer, NULL);\n> +\n> +\t} else {\n> +\t\treturn getpass(prompt);\n> +\t}\n\nAccording to Documentation/CodingGuidelines, this would be:\n\n\n\t\tfor (i = 0; i < buffer.len; i++)\n\t\t\tif (buffer.buf[i] == '\\n' || buffer.buf[i] == '\\r') {\n\t\t\t\tbuffer.buf[i] = '\\0';\n\t\t\t\tbuffer.len = i;\n\t\t\t}\n\t\treturn strbuf_detach(&buffer, NULL);\n\n\t} else\n\t\treturn getpass(prompt);\n"},{"id":"135714","messageId":"1976ea661002251817v12f04314vc32fe7924f31c070@mail.gmail.com","threadId":"22825","inReplyTo":"20100226005020.GR12429@genesis.frugalware.org","subject":"Re: [PATCH v2 2/3] git-core: Support retrieving passwords with GIT_ASKPASS","fromName":"Frank Li","fromEmail":"lznuaa@gmail.com","sentAt":"2010-02-26T02:17:44Z","receivedAt":"2010-02-26T02:17:44Z","isPatch":true,"sender":{"key":"lznuaa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40642?v=4"},"body":"> According to Documentation/CodingGuidelines, this would be:\n>\n>\n>                for (i = 0; i < buffer.len; i++)\n>                        if (buffer.buf[i] == '\\n' || buffer.buf[i] == '\\r') {\n>                                buffer.buf[i] = '\\0';\n>                                buffer.len = i;\n>                        }\n>                return strbuf_detach(&buffer, NULL);\n>\n>        } else\n>                return getpass(prompt);\n>\n\nOkay, I will change it.\n"},{"id":"135736","messageId":"4B87797D.7030905@viscovery.net","threadId":"22825","inReplyTo":"1267143154-5020-1-git-send-email-lznuaa@gmail.com","subject":"Re: [PATCH v2 2/3] git-core: Support retrieving passwords with GIT_ASKPASS","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-02-26T07:34:21Z","receivedAt":"2010-02-26T07:34:21Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Frank Li schrieb:\n>  connect.c   |   40 ++++++++++++++++++++++++++++++++++++++++\n>  http.c      |    4 ++--\n>  imap-send.c |    2 +-\n\nI don't see any header file changes. Don't you get warnings about an\nundeclared function git_getpass() at the call sites?\n\n> +char *git_getpass(char *prompt)\n\nchar *git_getpass(const char *prompt)\n\n> +\taskpass = getenv(\"GIT_ASKPASS\");\n> +\tif (askpass && strlen(askpass) != 0) {\n> +\t\targs[0] = getenv(\"GIT_ASKPASS\");\n\n\tif (askpass && *askpass) {\n\t\targs[0] = askpass;\n\nBTW, to save a level of indentation, you could handle the \"trivial\" case\nearly like this:\n\n\tif (!askpass || !*askpass)\n\t\treturn get_pass(prompt);\n\nand continue without an 'else' branch.\n\n> +\t\targs[1]\t= prompt;\n> +\t\targs[2] = NULL;\n> +\n> +\t\tmemset(&pass, 0, sizeof(pass));\n> +\t\tpass.argv = args;\n> +\t\tpass.out = -1;\n> +\t\tpass.no_stdin = 1;\n> +\t\tpass.no_stderr = 1;\n\nIs it such a good idea to redirect stdin and stderr to /dev/null? What if\nmy password prompt program depends on them? I think it should not matter\nfor your use-case, where a GUI is invoked, to just inherit all channels.\n\nOTOH, it may be worthwhile to set\n\n\t\tpass.use_shell = 1;\n\nto allow commands that are not just a single plain word. But perhaps this\nhas security implications - I don't know.\n\n> +\n> +\t\tif (start_command(&pass)) {\n> +\t\t\terror(\"could not run %s\\n\", askpass);\n> +\t\t\treturn getpass(prompt);\n\nI don't think this is a good idea. The user instructed to use GIT_ASKPASS,\nand you fall back to asking a password from the terminal. I think the most\nsensible thing to do here is to 'exit(1)' (start_command has already\nprinted an error message that included the command), because there are\ncallers that do not expect NULL.\n\n> +\t\t}\n> +\n> +\t\tstrbuf_read(&buffer, pass.out, 20);\n> +\t\tclose(pass.out);\n> +\t\tfor (i = 0; i < buffer.len; i++)\n> +\t\t\tif (buffer.buf[i] == '\\n' || buffer.buf[i] == '\\r') {\n> +\t\t\t\tbuffer.buf[i] = '\\0';\n> +\t\t\t\tbuffer.len = i;\n> +\t\t}\n> +\t\treturn strbuf_detach(&buffer, NULL);\n\nYou don't call finish_command() anywhere. Call it after the close() call.\n\n> +\n> +\t} else {\n> +\t\treturn getpass(prompt);\n\nYou handle the return value in different ways. getpass() returns a pointer\nto a static buffer, but in the 'then' branch you return an allocated\nbuffer. Not that it matters a lot, though. You could add a comment that\nyou are aware that the memory is leaked.\n\n> +\t}\n> +\treturn NULL;\n\nWhat is this good for?\n\n-- Hannes\n"},{"id":"135737","messageId":"7vr5o84erv.fsf@alter.siamese.dyndns.org","threadId":"22825","inReplyTo":"4B87797D.7030905@viscovery.net","subject":"Re: [PATCH v2 2/3] git-core: Support retrieving passwords with GIT_ASKPASS","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-26T07:50:12Z","receivedAt":"2010-02-26T07:50:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> writes:\n\n> BTW, to save a level of indentation, you could handle the \"trivial\" case\n> early like this:\n>\n> \tif (!askpass || !*askpass)\n> \t\treturn get_pass(prompt);\n>\n> and continue without an 'else' branch.\n\nThat is a good advice in general.\n\nAlso, when you have a way unbalanced if ... else ... where else clause is\nvery small, it usually is much easier to read if you invert the logic to\nmake if part smaller.\n\n> OTOH, it may be worthwhile to set\n>\n> \t\tpass.use_shell = 1;\n>\n> to allow commands that are not just a single plain word. But perhaps this\n> has security implications - I don't know.\n\nHow does SSH_ASKPASS gets interpreted by other programs?  I think we\nshould follow that example.\n\nOther than that, I agree with everything you said in your review.  Thanks.\n"},{"id":"135741","messageId":"4B87952F.1000902@viscovery.net","threadId":"22825","inReplyTo":"7vr5o84erv.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/3] git-core: Support retrieving passwords with GIT_ASKPASS","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-02-26T09:32:31Z","receivedAt":"2010-02-26T09:32:31Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Junio C Hamano schrieb:\n> Johannes Sixt <j.sixt@viscovery.net> writes:\n>> OTOH, it may be worthwhile to set\n>>\n>> \t\tpass.use_shell = 1;\n>>\n>> to allow commands that are not just a single plain word. But perhaps this\n>> has security implications - I don't know.\n> \n> How does SSH_ASKPASS gets interpreted by other programs?  I think we\n> should follow that example.\n\nopenssh treats SSH_ASKPASS as a command name and uses execlp, i.e., does a\nPATH search; no shell tricks are possible. Hence, we should *not* set\nuse_shell.\n\nhttp://www.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/readpass.c?rev=1.47\n\nOf course, we could define that GIT_ASKPASS is different from SSH_ASKPASS\nin this regard, but I haven't followed the discussion to know whether this\nis necessary.\n\n-- Hannes\n"},{"id":"135745","messageId":"1976ea661002260201s75ef6185x3f89742d58f12222@mail.gmail.com","threadId":"22825","inReplyTo":"4B87797D.7030905@viscovery.net","subject":"Re: [PATCH v2 2/3] git-core: Support retrieving passwords with GIT_ASKPASS","fromName":"Frank Li","fromEmail":"lznuaa@gmail.com","sentAt":"2010-02-26T10:01:10Z","receivedAt":"2010-02-26T10:01:10Z","isPatch":true,"sender":{"key":"lznuaa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40642?v=4"},"body":">\n> You handle the return value in different ways. getpass() returns a pointer\n> to a static buffer, but in the 'then' branch you return an allocated\n> buffer. Not that it matters a lot, though. You could add a comment that\n> you are aware that the memory is leaked.\n>\n\nHow about my branch also use static buffer, so there are not memory leak.\n\nbest regards\nFrank Li\n"},{"id":"135763","messageId":"7vmxyvx4wx.fsf@alter.siamese.dyndns.org","threadId":"22825","inReplyTo":"4B87952F.1000902@viscovery.net","subject":"Re: [PATCH v2 2/3] git-core: Support retrieving passwords with GIT_ASKPASS","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-26T17:50:22Z","receivedAt":"2010-02-26T17:50:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> writes:\n\n> openssh treats SSH_ASKPASS as a command name and uses execlp, i.e., does a\n> PATH search; no shell tricks are possible. Hence, we should *not* set\n> use_shell.\n>\n> http://www.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/readpass.c?rev=1.47\n>\n> Of course, we could define that GIT_ASKPASS is different from SSH_ASKPASS\n> in this regard, but I haven't followed the discussion to know whether this\n> is necessary.\n\nIt is sad that they do exec*p() on that one; it means that the users\ncannot supply leading set of options with CMD=\"cmd --initial-option\".\n\nBut we should do the same as others do.\n\nIt is doubly sad that I earlier was hoping that we can run external\nprograms specified via GIT_* uniformly via shell.\n"}]}