{"thread":{"id":"33946","subject":"[PATCH] credential-osxkeychain: support more protocols","startedAt":"2013-05-27T07:57:11Z","lastAt":"2013-05-28T02:36:21Z","messageCount":10,"participants":["Xidorn Quan","John Szakmeister","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"218592","messageId":"1369641431-44504-1-git-send-email-quanxunzhen@gmail.com","threadId":"33946","inReplyTo":null,"subject":"[PATCH] credential-osxkeychain: support more protocols","fromName":"Xidorn Quan","fromEmail":"quanxunzhen@gmail.com","sentAt":"2013-05-27T07:57:11Z","receivedAt":"2013-05-27T07:57:11Z","isPatch":true,"sender":{"key":"quanxunzhen@gmail.com","avatar":"https://avatars.githubusercontent.com/u/333750?v=4"},"body":"Add protocol ftp, smtp, and ssh for credential-osxkeychain.\n---\n contrib/credential/osxkeychain/git-credential-osxkeychain.c | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\nindex 3940202..4ddcfb3 100644\n--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n@@ -127,10 +127,16 @@ static void read_credential(void)\n \t\t*v++ = '\\0';\n \n \t\tif (!strcmp(buf, \"protocol\")) {\n-\t\t\tif (!strcmp(v, \"https\"))\n+\t\t\tif (!strcmp(v, \"ftp\"))\n+\t\t\t\tprotocol = kSecProtocolTypeFTP;\n+\t\t\telse if (!strcmp(v, \"https\"))\n \t\t\t\tprotocol = kSecProtocolTypeHTTPS;\n \t\t\telse if (!strcmp(v, \"http\"))\n \t\t\t\tprotocol = kSecProtocolTypeHTTP;\n+\t\t\telse if (!strcmp(v, \"smtp\"))\n+\t\t\t\tprotocol = kSecProtocolTypeSMTP;\n+\t\t\telse if (!strcmp(v, \"ssh\"))\n+\t\t\t\tprotocol = kSecProtocolTypeSSH;\n \t\t\telse /* we don't yet handle other protocols */\n \t\t\t\texit(0);\n \t\t}\n-- \n1.8.3\n"},{"id":"218597","messageId":"CAEBDL5W4sLB0R1ZOspb-yQzmyTCE7Y1HeC2KZ69F8R28fJY7_A@mail.gmail.com","threadId":"33946","inReplyTo":"1369641431-44504-1-git-send-email-quanxunzhen@gmail.com","subject":"Re: [PATCH] credential-osxkeychain: support more protocols","fromName":"John Szakmeister","fromEmail":"john@szakmeister.net","sentAt":"2013-05-27T10:27:47Z","receivedAt":"2013-05-27T10:27:47Z","isPatch":true,"sender":{"key":"john@szakmeister.net","avatar":"https://avatars.githubusercontent.com/u/448087?v=4"},"body":"On Mon, May 27, 2013 at 3:57 AM, Xidorn Quan wrote:\n> Add protocol ftp, smtp, and ssh for credential-osxkeychain.\n> ---\n> contrib/credential/osxkeychain/git-credential-osxkeychain.c | 8 +++++++-\n> 1 file changed, 7 insertions(+), 1 deletion(-)\n>\n> diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> index 3940202..4ddcfb3 100644\n> --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> @@ -127,10 +127,16 @@ static void read_credential(void)\n> *v++ = '\\0';\n>\n> if (!strcmp(buf, \"protocol\")) {\n> - if (!strcmp(v, \"https\"))\n> + if (!strcmp(v, \"ftp\"))\n> + protocol = kSecProtocolTypeFTP;\n> + else if (!strcmp(v, \"https\"))\n> protocol = kSecProtocolTypeHTTPS;\n> else if (!strcmp(v, \"http\"))\n> protocol = kSecProtocolTypeHTTP;\n> + else if (!strcmp(v, \"smtp\"))\n> + protocol = kSecProtocolTypeSMTP;\n> + else if (!strcmp(v, \"ssh\"))\n> + protocol = kSecProtocolTypeSSH;\n> else /* we don't yet handle other protocols */\n> exit(0);\n\nThis looks pretty good, except the last one raises a question. I'm\nusing Mac OS X, and ssh already interacts with keychain to get my SSH\nkey password. Is this mainly for password logins via SSH? Assuming\nthat's the case:\n\nSigned-off-by: John Szakmeister \n\n-John\n"},{"id":"218601","messageId":"CAMdq69_=21de4FvGkp0bFtZjNOttkwXOxjvhM0yZi5gyj0-V9g@mail.gmail.com","threadId":"33946","inReplyTo":"CAEBDL5W4sLB0R1ZOspb-yQzmyTCE7Y1HeC2KZ69F8R28fJY7_A@mail.gmail.com","subject":"Re: [PATCH] credential-osxkeychain: support more protocols","fromName":"Xidorn Quan","fromEmail":"quanxunzhen@gmail.com","sentAt":"2013-05-27T13:57:01Z","receivedAt":"2013-05-27T13:57:01Z","isPatch":true,"sender":{"key":"quanxunzhen@gmail.com","avatar":"https://avatars.githubusercontent.com/u/333750?v=4"},"body":"On Mon, May 27, 2013 at 6:27 PM, John Szakmeister wrote:\n>\n> On Mon, May 27, 2013 at 3:57 AM, Xidorn Quan wrote:\n> > Add protocol ftp, smtp, and ssh for credential-osxkeychain.\n> > ---\n> > contrib/credential/osxkeychain/git-credential-osxkeychain.c | 8 +++++++-\n> > 1 file changed, 7 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> > index 3940202..4ddcfb3 100644\n> > --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> > +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> > @@ -127,10 +127,16 @@ static void read_credential(void)\n> > *v++ = '\\0';\n> >\n> > if (!strcmp(buf, \"protocol\")) {\n> > - if (!strcmp(v, \"https\"))\n> > + if (!strcmp(v, \"ftp\"))\n> > + protocol = kSecProtocolTypeFTP;\n> > + else if (!strcmp(v, \"https\"))\n> > protocol = kSecProtocolTypeHTTPS;\n> > else if (!strcmp(v, \"http\"))\n> > protocol = kSecProtocolTypeHTTP;\n> > + else if (!strcmp(v, \"smtp\"))\n> > + protocol = kSecProtocolTypeSMTP;\n> > + else if (!strcmp(v, \"ssh\"))\n> > + protocol = kSecProtocolTypeSSH;\n> > else /* we don't yet handle other protocols */\n> > exit(0);\n>\n> This looks pretty good, except the last one raises a question. I'm\n> using Mac OS X, and ssh already interacts with keychain to get my SSH\n> key password. Is this mainly for password logins via SSH? Assuming\n> that's the case:\n>\n> Signed-off-by: John Szakmeister \n>\n> -John\n\nI thought that SSH password logins can benefit from it, but I just\nfound that it is wrong because it seems that SSH client is responsible\nfor authenticating. Consequently, supporting SSH here is useless.\nI will remove that lines and send this patch again.\n\nSince it is the first time I submit a patch to git, I am not very\nfamiliar with the convention here. Should I send the modified patch\nto the maintainer directly? And what information should I append to\nmy patch before it can get merged?\n\n--\nXidorn Quan\n"},{"id":"218606","messageId":"CAEBDL5XMamYoZFgFWeRr3KTZWZATKp=prFus_44UEMV8LEEtzw@mail.gmail.com","threadId":"33946","inReplyTo":"CAMdq6987TAj7f03mABkqu9v4wicarrZLYQypNUiOrP0fsLc4mQ@mail.gmail.com","subject":"Re: [PATCH] credential-osxkeychain: support more protocols","fromName":"John Szakmeister","fromEmail":"john@szakmeister.net","sentAt":"2013-05-27T14:28:40Z","receivedAt":"2013-05-27T14:28:40Z","isPatch":true,"sender":{"key":"john@szakmeister.net","avatar":"https://avatars.githubusercontent.com/u/448087?v=4"},"body":"On Mon, May 27, 2013 at 9:55 AM, Xidorn Quan wrote:\n[snip]\n> I thought that SSH password logins can benefit from it, but I just\n> found that it is wrong because it seems that SSH client is responsible\n> for authenticating. Consequently, supporting SSH here is useless.\n> I will remove that lines and send this patch again.\n>\n> Since it is the first time I submit a patch to git, I am not very\n> familiar with the convention here. Should I send the modified patch\n> to the maintainer directly? And what information should I append to\n> my patch before it can get merged?\n\nYou'll need to read Documentation/SubmittingPatches (here's a link to\na version online:\nhttps://github.com/git/git/blob/master/Documentation/SubmittingPatches).\n\nYou should resend this patch with the fix and change \"[PATCH]\" to\n\"[PATCH v2]\", so the folks involved know that this is the second\niteration. You also need to include a \"Signed-off-by\" line in your\npatch, which means you agree to the agreement set forth in the\n\"Developer's Certificate of Origin\" (which is in the SubmittingPatches\ndocumentation). You can easily include this line when you make the\ncommit by using the `-s` option on `git commit`.\n\nYou can also add an \"Acked-by\" line for me (since I reviewed and\napprove of the change):\n\n Acked-by: John Szakmeister \n\nHTH!\n\n-John\n"},{"id":"218607","messageId":"1369665359-48242-1-git-send-email-quanxunzhen@gmail.com","threadId":"33946","inReplyTo":"1369641431-44504-1-git-send-email-quanxunzhen@gmail.com","subject":"[PATCH v2] credential-osxkeychain: support more protocols","fromName":"Xidorn Quan","fromEmail":"quanxunzhen@gmail.com","sentAt":"2013-05-27T14:35:59Z","receivedAt":"2013-05-27T14:35:59Z","isPatch":true,"sender":{"key":"quanxunzhen@gmail.com","avatar":"https://avatars.githubusercontent.com/u/333750?v=4"},"body":"Add protocol ftp and smtp for credential-osxkeychain.\n\nAcked-by: John Szakmeister \n\nSigned-off-by: Xidorn Quan \n---\n contrib/credential/osxkeychain/git-credential-osxkeychain.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\nindex 3940202..648fadd 100644\n--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n@@ -127,10 +127,14 @@ static void read_credential(void)\n \t\t*v++ = '\\0';\n \n \t\tif (!strcmp(buf, \"protocol\")) {\n-\t\t\tif (!strcmp(v, \"https\"))\n+\t\t\tif (!strcmp(v, \"ftp\"))\n+\t\t\t\tprotocol = kSecProtocolTypeFTP;\n+\t\t\telse if (!strcmp(v, \"https\"))\n \t\t\t\tprotocol = kSecProtocolTypeHTTPS;\n \t\t\telse if (!strcmp(v, \"http\"))\n \t\t\t\tprotocol = kSecProtocolTypeHTTP;\n+\t\t\telse if (!strcmp(v, \"smtp\"))\n+\t\t\t\tprotocol = kSecProtocolTypeSMTP;\n \t\t\telse /* we don't yet handle other protocols */\n \t\t\t\texit(0);\n \t\t}\n-- \n1.8.3\n"},{"id":"218609","messageId":"CAMdq699G4bZSEQmZorRfOR87RZ7YGdNnkh_oP4rDZXXauOpDnQ@mail.gmail.com","threadId":"33946","inReplyTo":"CAEBDL5XMamYoZFgFWeRr3KTZWZATKp=prFus_44UEMV8LEEtzw@mail.gmail.com","subject":"Re: [PATCH] credential-osxkeychain: support more protocols","fromName":"Xidorn Quan","fromEmail":"quanxunzhen@gmail.com","sentAt":"2013-05-27T15:02:09Z","receivedAt":"2013-05-27T15:02:09Z","isPatch":true,"sender":{"key":"quanxunzhen@gmail.com","avatar":"https://avatars.githubusercontent.com/u/333750?v=4"},"body":"On Mon, May 27, 2013 at 10:28 PM, John Szakmeister wrote:\n[snip]\n>\n> You'll need to read Documentation/SubmittingPatches (here's a link to\n> a version online:\n> https://github.com/git/git/blob/master/Documentation/SubmittingPatches).\n>\n> You should resend this patch with the fix and change \"[PATCH]\" to\n> \"[PATCH v2]\", so the folks involved know that this is the second\n> iteration. You also need to include a \"Signed-off-by\" line in your\n> patch, which means you agree to the agreement set forth in the\n> \"Developer's Certificate of Origin\" (which is in the SubmittingPatches\n> documentation). You can easily include this line when you make the\n> commit by using the `-s` option on `git commit`.\n>\n> You can also add an \"Acked-by\" line for me (since I reviewed and\n> approve of the change):\n>\n> Acked-by: John Szakmeister \n>\n> HTH!\n\nThx for your explaining, it helps a lot!\n\nAfter reading the doc, I have a question that when can I affirm that\nthe list reaches a consensus?\n\n--\nXidorn Quan\n"},{"id":"218610","messageId":"20130527150855.GA8250@sigill.intra.peff.net","threadId":"33946","inReplyTo":"1369665359-48242-1-git-send-email-quanxunzhen@gmail.com","subject":"Re: [PATCH v2] credential-osxkeychain: support more protocols","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-27T15:08:56Z","receivedAt":"2013-05-27T15:08:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 27, 2013 at 10:35:59PM +0800, Xidorn Quan wrote:\n\n> diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> index 3940202..648fadd 100644\n> --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> @@ -127,10 +127,14 @@ static void read_credential(void)\n> \t\t*v++ = '\\0';\n> \n> \t\tif (!strcmp(buf, \"protocol\")) {\n> -\t\t\tif (!strcmp(v, \"https\"))\n> +\t\t\tif (!strcmp(v, \"ftp\"))\n> +\t\t\t\tprotocol = kSecProtocolTypeFTP;\n> +\t\t\telse if (!strcmp(v, \"https\"))\n> \t\t\t\tprotocol = kSecProtocolTypeHTTPS;\n> \t\t\telse if (!strcmp(v, \"http\"))\n> \t\t\t\tprotocol = kSecProtocolTypeHTTP;\n> +\t\t\telse if (!strcmp(v, \"smtp\"))\n> +\t\t\t\tprotocol = kSecProtocolTypeSMTP;\n> \t\t\telse /* we don't yet handle other protocols */\n> \t\t\t\texit(0);\n\nThis looks good to me. Git will ask for \"protocol=ftp\" when\naccessing the dumb protocol over ftp. And it will ask for smtp via\ngit-send-email since 4d31a44 (git-send-email: use git credential to\nobtain password, 2013-02-12).\n\nWhile we are in the area it may be worth thinking if there are other\nschemes we would want to support. Git might feed any URL scheme that\ncurl accepts, so I think we would want to handle FTPS alongside FTP, no?\n\nWe may also eventually want IMAP for git-imap-send, but we have not yet\nimplemented credential-helper support there. We may also want http/socks\nproxy authentication, but we also have not implemented the git side of\nthat yet. So I think both of those can wait for now.\n\n-Peff\n"},{"id":"218614","messageId":"CAMdq69_RarEXDdR+xzvSb9YPCR2hE=0tM3o-mF3d=cgDMWSq8A@mail.gmail.com","threadId":"33946","inReplyTo":"20130527150855.GA8250@sigill.intra.peff.net","subject":"Re: [PATCH v2] credential-osxkeychain: support more protocols","fromName":"Xidorn Quan","fromEmail":"quanxunzhen@gmail.com","sentAt":"2013-05-27T15:46:24Z","receivedAt":"2013-05-27T15:46:24Z","isPatch":true,"sender":{"key":"quanxunzhen@gmail.com","avatar":"https://avatars.githubusercontent.com/u/333750?v=4"},"body":"On Mon, May 27, 2013 at 11:08 PM, Jeff King wrote:\n[snip]\n>\n> This looks good to me. Git will ask for \"protocol=ftp\" when\n> accessing the dumb protocol over ftp. And it will ask for smtp via\n> git-send-email since 4d31a44 (git-send-email: use git credential to\n> obtain password, 2013-02-12).\n>\n> While we are in the area it may be worth thinking if there are other\n> schemes we would want to support. Git might feed any URL scheme that\n> curl accepts, so I think we would want to handle FTPS alongside FTP, no?\n\nGood point, I'll add that.\n\n> We may also eventually want IMAP for git-imap-send, but we have not yet\n> implemented credential-helper support there. We may also want http/socks\n> proxy authentication, but we also have not implemented the git side of\n> that yet. So I think both of those can wait for now.\n\nHope the helpers will be implemented soon. IMO, we can add IMAP and\nSOCKS for now since the protocol names are clear, while it is unclear\nwhat protocol name will be used for HTTP/HTTPS proxy. I guess that\nsome may prefer using http/https as protocol name instead of something\nspecific. What do you think?\n\n--\nXidorn Quan\n"},{"id":"218616","messageId":"20130527160511.GA9235@sigill.intra.peff.net","threadId":"33946","inReplyTo":"CAMdq69_RarEXDdR+xzvSb9YPCR2hE=0tM3o-mF3d=cgDMWSq8A@mail.gmail.com","subject":"Re: [PATCH v2] credential-osxkeychain: support more protocols","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-27T16:05:11Z","receivedAt":"2013-05-27T16:05:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 27, 2013 at 11:46:24PM +0800, Xidorn Quan wrote:\n\n> > We may also eventually want IMAP for git-imap-send, but we have not yet\n> > implemented credential-helper support there. We may also want http/socks\n> > proxy authentication, but we also have not implemented the git side of\n> > that yet. So I think both of those can wait for now.\n> \n> Hope the helpers will be implemented soon. IMO, we can add IMAP and\n> SOCKS for now since the protocol names are clear, while it is unclear\n> what protocol name will be used for HTTP/HTTPS proxy. I guess that\n> some may prefer using http/https as protocol name instead of something\n> specific. What do you think?\n\nYes, I think git will need to munge the protocol to indicate to the\nhelper that it is a proxy (or alternatively, add a new key \"proxy=1\" or\nsomething). So it would make sense to wait until the git side of the\ncode materializes so that the helpers knows what to expect.\n\nThere was some work done in this area last May, but there are a lot of\ncorner cases with knowing exactly what the proxy URL is (because in some\ncases, curl pulls it from the environment without git's knowledge).\nThere was a patch series under consideration for libcurl that would let\nus supply an \"authentication callback\" to curl that would be called when\ncredentials were needed. But I have not kept track of the state of that\npatch and whether it ever got merged upstream into libcurl.\n\n-Peff\n"},{"id":"218631","messageId":"1369708581-4104-1-git-send-email-quanxunzhen@gmail.com","threadId":"33946","inReplyTo":"1369665359-48242-1-git-send-email-quanxunzhen@gmail.com","subject":"[PATCH v3] credential-osxkeychain: support more protocols","fromName":"Xidorn Quan","fromEmail":"quanxunzhen@gmail.com","sentAt":"2013-05-28T02:36:21Z","receivedAt":"2013-05-28T02:36:21Z","isPatch":true,"sender":{"key":"quanxunzhen@gmail.com","avatar":"https://avatars.githubusercontent.com/u/333750?v=4"},"body":"Add protocol imap, imaps, ftp and smtp for credential-osxkeychain.\n\nSigned-off-by: Xidorn Quan \nAcked-by: John Szakmeister \nAcked-by: Jeff King \n---\n contrib/credential/osxkeychain/git-credential-osxkeychain.c | 12 +++++++++++-\n 1 file changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\nindex 3940202..bcd3f57 100644\n--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n@@ -127,10 +127,20 @@ static void read_credential(void)\n \t\t*v++ = '\\0';\n \n \t\tif (!strcmp(buf, \"protocol\")) {\n-\t\t\tif (!strcmp(v, \"https\"))\n+\t\t\tif (!strcmp(v, \"imap\"))\n+\t\t\t\tprotocol = kSecProtocolTypeIMAP;\n+\t\t\telse if (!strcmp(v, \"imaps\"))\n+\t\t\t\tprotocol = kSecProtocolTypeIMAPS;\n+\t\t\telse if (!strcmp(v, \"ftp\"))\n+\t\t\t\tprotocol = kSecProtocolTypeFTP;\n+\t\t\telse if (!strcmp(v, \"ftps\"))\n+\t\t\t\tprotocol = kSecProtocolTypeFTPS;\n+\t\t\telse if (!strcmp(v, \"https\"))\n \t\t\t\tprotocol = kSecProtocolTypeHTTPS;\n \t\t\telse if (!strcmp(v, \"http\"))\n \t\t\t\tprotocol = kSecProtocolTypeHTTP;\n+\t\t\telse if (!strcmp(v, \"smtp\"))\n+\t\t\t\tprotocol = kSecProtocolTypeSMTP;\n \t\t\telse /* we don't yet handle other protocols */\n \t\t\t\texit(0);\n \t\t}\n-- \n1.8.3\n"}]}