{"thread":{"id":"48190","subject":"[PATCH] send-email: report host and port separately when calling git credential","startedAt":"2018-03-31T18:05:25Z","lastAt":"2018-04-07T10:08:41Z","messageCount":7,"participants":["Michal Nazarewicz","Junio C Hamano","Jeff King","Michał Nazarewicz"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"343540","messageId":"20180331180514.14628-1-mina86@mina86.com","threadId":"48190","inReplyTo":null,"subject":"[PATCH] send-email: report host and port separately when calling git credential","fromName":"Michal Nazarewicz","fromEmail":"mina86@mina86.com","sentAt":"2018-03-31T18:05:14Z","receivedAt":"2018-03-31T18:05:25Z","isPatch":true,"sender":{"key":"mina86@mina86.com","avatar":"https://avatars.githubusercontent.com/u/32383?v=4"},"body":"When git-send-email uses git-credential to get SMTP password, it will\ncommunicate SMTP host and port (if both are provided) as a single entry\n‘host=<host>:<port>’.  This trips the ‘git-credential-store’ helper\nwhich expects those values as separate keys (‘host’ and ‘port’).\n\nSend the values as separate pieces of information so things work\nsmoothly.\n\nSigned-off-by: Michał Nazarewicz <mina86@mina86.com>\n---\n git-send-email.perl | 11 ++---------\n 1 file changed, 2 insertions(+), 9 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 2fa7818ca..2a9f89a58 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1229,14 +1229,6 @@ sub maildomain {\n \treturn maildomain_net() || maildomain_mta() || 'localhost.localdomain';\n }\n \n-sub smtp_host_string {\n-\tif (defined $smtp_server_port) {\n-\t\treturn \"$smtp_server:$smtp_server_port\";\n-\t} else {\n-\t\treturn $smtp_server;\n-\t}\n-}\n-\n # Returns 1 if authentication succeeded or was not necessary\n # (smtp_user was not specified), and 0 otherwise.\n \n@@ -1263,7 +1255,8 @@ sub smtp_auth_maybe {\n \t# reject credentials.\n \t$auth = Git::credential({\n \t\t'protocol' => 'smtp',\n-\t\t'host' => smtp_host_string(),\n+\t\t'host' => $smtp_server,\n+\t\t'port' => $smtp_server_port,\n \t\t'username' => $smtp_authuser,\n \t\t# if there's no password, \"git credential fill\" will\n \t\t# give us one, otherwise it'll just pass this one.\n-- \n2.16.2\n\n"},{"id":"343632","messageId":"xmqqk1tpxo0g.fsf@gitster-ct.c.googlers.com","threadId":"48190","inReplyTo":"20180331180514.14628-1-mina86@mina86.com","subject":"Re: [PATCH] send-email: report host and port separately when calling git credential","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-02T22:05:35Z","receivedAt":"2018-04-02T22:05:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michal Nazarewicz <mina86@mina86.com> writes:\n\n> When git-send-email uses git-credential to get SMTP password, it will\n> communicate SMTP host and port (if both are provided) as a single entry\n> ‘host=<host>:<port>’.  This trips the ‘git-credential-store’ helper\n> which expects those values as separate keys (‘host’ and ‘port’).\n>\n> Send the values as separate pieces of information so things work\n> smoothly.\n>\n> Signed-off-by: Michał Nazarewicz <mina86@mina86.com>\n> ---\n>  git-send-email.perl | 11 ++---------\n>  1 file changed, 2 insertions(+), 9 deletions(-)\n\n\"git help credential\" mentions protocol, host, path, username and\npassword (and also url which is a short-hand for setting protocol\nand host), but not \"port\".  And common sense tells me, when a system\nallows setting host but not port, that it would expect host:port to\nbe given when the service is running a non-standard port, so from\nthat point of view, I suspect that the current code is working as\nexpected.  In fact, credential.h, which defines the API, does not\nhave any \"port\" field, either, so I am not sure how this is expected\nto change anything without touching the back-end that talks over the\npipe via _credential_run-->credential_write callchain.\n\nNow, it is a separate matter if it were a better design if the\ncredential API had 'host' and 'port' defined as separate keys to the\nauthentication information.  Such an alternative design would have\nmade certain things harder while some other things easier (e.g. \"use\nthis credential to the host no matter what port the service runs\"\nmay be easier to implement if 'host' and 'port' are separate).\n\n\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index 2fa7818ca..2a9f89a58 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -1229,14 +1229,6 @@ sub maildomain {\n>  \treturn maildomain_net() || maildomain_mta() || 'localhost.localdomain';\n>  }\n>  \n> -sub smtp_host_string {\n> -\tif (defined $smtp_server_port) {\n> -\t\treturn \"$smtp_server:$smtp_server_port\";\n> -\t} else {\n> -\t\treturn $smtp_server;\n> -\t}\n> -}\n> -\n>  # Returns 1 if authentication succeeded or was not necessary\n>  # (smtp_user was not specified), and 0 otherwise.\n>  \n> @@ -1263,7 +1255,8 @@ sub smtp_auth_maybe {\n>  \t# reject credentials.\n>  \t$auth = Git::credential({\n>  \t\t'protocol' => 'smtp',\n> -\t\t'host' => smtp_host_string(),\n> +\t\t'host' => $smtp_server,\n> +\t\t'port' => $smtp_server_port,\n>  \t\t'username' => $smtp_authuser,\n>  \t\t# if there's no password, \"git credential fill\" will\n>  \t\t# give us one, otherwise it'll just pass this one.\n"},{"id":"343645","messageId":"20180402232348.20293-1-mina86@mina86.com","threadId":"48190","inReplyTo":"xmqqk1tpxo0g.fsf@gitster-ct.c.googlers.com","subject":"[PATCH] send-email: fix docs regarding storing password with git credential","fromName":"Michal Nazarewicz","fromEmail":"mina86@mina86.com","sentAt":"2018-04-02T23:23:48Z","receivedAt":"2018-04-02T23:24:23Z","isPatch":true,"sender":{"key":"mina86@mina86.com","avatar":"https://avatars.githubusercontent.com/u/32383?v=4"},"body":"First of all, ‘git credential fill’ does not store credentials\nbut is used to *read* them.  The command which adds credentials\nto the helper’s store is ‘git credential approve’.\n\nSecond of all, git-send-email will include port number in host\nparameter when getting the password so it has to be set when\nstoring the password as well.\n\nApply the two above to fix the Gmail example in git-send-email\ndocumentation.\n\nSigned-off-by: Michał Nazarewicz <mina86@mina86.com>\n---\n Documentation/git-send-email.txt | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt\nindex 71ef97ba9..172c7b344 100644\n--- a/Documentation/git-send-email.txt\n+++ b/Documentation/git-send-email.txt\n@@ -477,13 +477,12 @@ https://security.google.com/settings/security/apppasswords to setup an\n app-specific password.  Once setup, you can store it with the credentials\n helper:\n \n-\t$ git credential fill\n+\t$ git credential approve\n \tprotocol=smtp\n-\thost=smtp.gmail.com\n+\thost=smtp.gmail.com:587\n \tusername=youname@gmail.com\n \tpassword=app-password\n \n-\n Once your commits are ready to be sent to the mailing list, run the\n following commands:\n \n-- \n2.16.2\n\n"},{"id":"343819","messageId":"20180404210719.GB15402@sigill.intra.peff.net","threadId":"48190","inReplyTo":"xmqqk1tpxo0g.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] send-email: report host and port separately when calling git credential","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-04-04T21:07:20Z","receivedAt":"2018-04-04T21:07:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 02, 2018 at 03:05:35PM -0700, Junio C Hamano wrote:\n\n> \"git help credential\" mentions protocol, host, path, username and\n> password (and also url which is a short-hand for setting protocol\n> and host), but not \"port\".  And common sense tells me, when a system\n> allows setting host but not port, that it would expect host:port to\n> be given when the service is running a non-standard port, so from\n> that point of view, I suspect that the current code is working as\n> expected.  In fact, credential.h, which defines the API, does not\n> have any \"port\" field, either, so I am not sure how this is expected\n> to change anything without touching the back-end that talks over the\n> pipe via _credential_run-->credential_write callchain.\n> \n> Now, it is a separate matter if it were a better design if the\n> credential API had 'host' and 'port' defined as separate keys to the\n> authentication information.  Such an alternative design would have\n> made certain things harder while some other things easier (e.g. \"use\n> this credential to the host no matter what port the service runs\"\n> may be easier to implement if 'host' and 'port' are separate).\n\nI don't recall giving a huge amount of thought to alternate ports when\nwriting the credential code. But at least the osxkeychain helper does\nparse \"host:port\" from the host field and feed it to the appropriate\nkeychain arguments. And I think more oblivious helpers like\ncredential-cache would just treat the \"host\" field as an opaque blob,\nmaking the port part of the matching.\n\nI suspect there are some corner cases, though. Reading the osxkeychain\ncode, I think that asking for \"http://example.com:80\" and\n\"http://example.com\" would probably not get you to the same key, as we\nfeed port==0 in the second case. In practice, it's probably not a _huge_\ndeal to be overly picky, as the worst case is that you get prompted and\nstore the credential in a second slot (which then works going forward).\n\nSo in general I think it's OK for the whole system to err on the side of\nbeing picky about whether two things are \"the same\" (which in this case\nis including the port). It usually works itself out in the long run, and\nwe would not surprise the user with \"example.com:8080 is the same as\nexample.com:80\".\n\n-Peff\n"},{"id":"343820","messageId":"20180404211446.GC15402@sigill.intra.peff.net","threadId":"48190","inReplyTo":"20180402232348.20293-1-mina86@mina86.com","subject":"Re: [PATCH] send-email: fix docs regarding storing password with git credential","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-04-04T21:14:46Z","receivedAt":"2018-04-04T21:14:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 03, 2018 at 12:23:48AM +0100, Michal Nazarewicz wrote:\n\n> First of all, ‘git credential fill’ does not store credentials\n> but is used to *read* them.  The command which adds credentials\n> to the helper’s store is ‘git credential approve’.\n\nYep, makes sense (I wish we had just called these consistently \"get\",\n\"store\", and \"erase\" as they are in the git<->helper interface).\n\n> Second of all, git-send-email will include port number in host\n> parameter when getting the password so it has to be set when\n> storing the password as well.\n> \n> Apply the two above to fix the Gmail example in git-send-email\n> documentation.\n\nMakes sense. This is an interesting counter-example to my earlier \"well,\nit usually works out in the long run\" statement. Because usually you're\nrelying on some part of Git to issue the \"fill\" and the \"approve\", so\nwhatever it uses, it will be the same. But here we're trying to\npre-seed, so we have to match what the tool will do.\n\nOn the other hand, I'm not sure why we need to pre-seed here. Wouldn't\nit be sufficient to just issue a \"git send-email\", which would then\nprompt for the password? And then you'd input your generated token,\nwhich would get saved via the approve mechanism?\n\n-Peff\n"},{"id":"344062","messageId":"20180407100723.2168-1-mina86@mina86.com","threadId":"48190","inReplyTo":"20180404211446.GC15402@sigill.intra.peff.net","subject":"[PATCH] send-email: simplify Gmail example in the documentation","fromName":"Michal Nazarewicz","fromEmail":"mina86@mina86.com","sentAt":"2018-04-07T10:07:23Z","receivedAt":"2018-04-07T10:07:47Z","isPatch":true,"sender":{"key":"mina86@mina86.com","avatar":"https://avatars.githubusercontent.com/u/32383?v=4"},"body":"There is no need for use to manually call ‘git credential’ especially\nas the interface isn’t super user-friendly and a bit confusing.  ‘git\nsend-email’ will do that for them at the first execution and if the\npassword matches, it will be saved in the store.\n\nSimplify the documentaion so it dosn’t include the ‘git credential’\ninvocation (which was incorrect anyway as it should use ‘approve’\ninstead of ‘fill’) and instead just mentions that credentials helper\nmust be set up.\n\nSigned-off-by: Michał Nazarewicz <mina86@mina86.com>\n---\n Documentation/git-send-email.txt | 16 ++++++----------\n 1 file changed, 6 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt\nindex 71ef97ba9..af07840b4 100644\n--- a/Documentation/git-send-email.txt\n+++ b/Documentation/git-send-email.txt\n@@ -473,16 +473,7 @@ edit ~/.gitconfig to specify your account settings:\n\n If you have multifactor authentication setup on your gmail account, you will\n need to generate an app-specific password for use with 'git send-email'. Visit\n-https://security.google.com/settings/security/apppasswords to setup an\n-app-specific password.  Once setup, you can store it with the credentials\n-helper:\n-\n-\t$ git credential fill\n-\tprotocol=smtp\n-\thost=smtp.gmail.com\n-\tusername=youname@gmail.com\n-\tpassword=app-password\n-\n+https://security.google.com/settings/security/apppasswords to create it.\n\n Once your commits are ready to be sent to the mailing list, run the\n following commands:\n@@ -491,7 +482,11 @@ following commands:\n \t$ edit outgoing/0000-*\n \t$ git send-email outgoing/*\n\n+The first time you run it, you will be prompted for your credentials.  Enter the\n+app-specific or your regular password as appropriate.  If you have credential\n+helper configured (see linkgit:git-credential[1]), the password will be saved in\n+the credential store so you won't have to type it the next time.\n+\n Note: the following perl modules are required\n       Net::SMTP::SSL, MIME::Base64 and Authen::SASL\n\n-- \n2.16.2\n"},{"id":"344063","messageId":"CA+pa1O1G+TQaKTCDEE_KBtMAMgG+GbsmeGv=mke_9aArL0US8Q@mail.gmail.com","threadId":"48190","inReplyTo":"20180404211446.GC15402@sigill.intra.peff.net","subject":"Re: [PATCH] send-email: fix docs regarding storing password with git credential","fromName":"Michał Nazarewicz","fromEmail":"mina86@mina86.com","sentAt":"2018-04-07T10:08:34Z","receivedAt":"2018-04-07T10:08:41Z","isPatch":true,"sender":{"key":"mina86@mina86.com","avatar":"https://avatars.githubusercontent.com/u/32383?v=4"},"body":"2018-04-04 22:14 GMT+01:00 Jeff King <peff@peff.net>:\n> On the other hand, I'm not sure why we need to pre-seed here. Wouldn't\n> it be sufficient to just issue a \"git send-email\", which would then\n> prompt for the password? And then you'd input your generated token,\n> which would get saved via the approve mechanism?\n\nYeah, this is precisely what I’ve figured as well.  As long as the credentials\nhelper is configured git-send-email will approve the password and it’ll be\nstored then. New patch sent.\n"}]}