* [PATCH] credential-osxkeychain: support more protocols
@ 2013-05-27 7:57 Xidorn Quan
2013-05-27 10:27 ` John Szakmeister
2013-05-27 14:35 ` [PATCH v2] " Xidorn Quan
0 siblings, 2 replies; 10+ messages in thread
From: Xidorn Quan @ 2013-05-27 7:57 UTC (permalink / raw)
To: git; +Cc: Xidorn Quan, Jeff King
Add protocol ftp, smtp, and ssh for credential-osxkeychain.
---
contrib/credential/osxkeychain/git-credential-osxkeychain.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
index 3940202..4ddcfb3 100644
--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
@@ -127,10 +127,16 @@ static void read_credential(void)
*v++ = '\0';
if (!strcmp(buf, "protocol")) {
- if (!strcmp(v, "https"))
+ if (!strcmp(v, "ftp"))
+ protocol = kSecProtocolTypeFTP;
+ else if (!strcmp(v, "https"))
protocol = kSecProtocolTypeHTTPS;
else if (!strcmp(v, "http"))
protocol = kSecProtocolTypeHTTP;
+ else if (!strcmp(v, "smtp"))
+ protocol = kSecProtocolTypeSMTP;
+ else if (!strcmp(v, "ssh"))
+ protocol = kSecProtocolTypeSSH;
else /* we don't yet handle other protocols */
exit(0);
}
--
1.8.3
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] credential-osxkeychain: support more protocols
2013-05-27 7:57 [PATCH] credential-osxkeychain: support more protocols Xidorn Quan
@ 2013-05-27 10:27 ` John Szakmeister
2013-05-27 13:57 ` Xidorn Quan
[not found] ` <CAMdq6987TAj7f03mABkqu9v4wicarrZLYQypNUiOrP0fsLc4mQ@mail.gmail.com>
2013-05-27 14:35 ` [PATCH v2] " Xidorn Quan
1 sibling, 2 replies; 10+ messages in thread
From: John Szakmeister @ 2013-05-27 10:27 UTC (permalink / raw)
To: Xidorn Quan; +Cc: git, Jeff King
On Mon, May 27, 2013 at 3:57 AM, Xidorn Quan <quanxunzhen@gmail.com> wrote:
> Add protocol ftp, smtp, and ssh for credential-osxkeychain.
> ---
> contrib/credential/osxkeychain/git-credential-osxkeychain.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> index 3940202..4ddcfb3 100644
> --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> @@ -127,10 +127,16 @@ static void read_credential(void)
> *v++ = '\0';
>
> if (!strcmp(buf, "protocol")) {
> - if (!strcmp(v, "https"))
> + if (!strcmp(v, "ftp"))
> + protocol = kSecProtocolTypeFTP;
> + else if (!strcmp(v, "https"))
> protocol = kSecProtocolTypeHTTPS;
> else if (!strcmp(v, "http"))
> protocol = kSecProtocolTypeHTTP;
> + else if (!strcmp(v, "smtp"))
> + protocol = kSecProtocolTypeSMTP;
> + else if (!strcmp(v, "ssh"))
> + protocol = kSecProtocolTypeSSH;
> else /* we don't yet handle other protocols */
> exit(0);
This looks pretty good, except the last one raises a question. I'm
using Mac OS X, and ssh already interacts with keychain to get my SSH
key password. Is this mainly for password logins via SSH? Assuming
that's the case:
Signed-off-by: John Szakmeister <john@szakmeister.net>
-John
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] credential-osxkeychain: support more protocols
2013-05-27 10:27 ` John Szakmeister
@ 2013-05-27 13:57 ` Xidorn Quan
[not found] ` <CAMdq6987TAj7f03mABkqu9v4wicarrZLYQypNUiOrP0fsLc4mQ@mail.gmail.com>
1 sibling, 0 replies; 10+ messages in thread
From: Xidorn Quan @ 2013-05-27 13:57 UTC (permalink / raw)
To: John Szakmeister; +Cc: git, Jeff King
On Mon, May 27, 2013 at 6:27 PM, John Szakmeister <john@szakmeister.net> wrote:
>
> On Mon, May 27, 2013 at 3:57 AM, Xidorn Quan <quanxunzhen@gmail.com> wrote:
> > Add protocol ftp, smtp, and ssh for credential-osxkeychain.
> > ---
> > contrib/credential/osxkeychain/git-credential-osxkeychain.c | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> > index 3940202..4ddcfb3 100644
> > --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> > +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> > @@ -127,10 +127,16 @@ static void read_credential(void)
> > *v++ = '\0';
> >
> > if (!strcmp(buf, "protocol")) {
> > - if (!strcmp(v, "https"))
> > + if (!strcmp(v, "ftp"))
> > + protocol = kSecProtocolTypeFTP;
> > + else if (!strcmp(v, "https"))
> > protocol = kSecProtocolTypeHTTPS;
> > else if (!strcmp(v, "http"))
> > protocol = kSecProtocolTypeHTTP;
> > + else if (!strcmp(v, "smtp"))
> > + protocol = kSecProtocolTypeSMTP;
> > + else if (!strcmp(v, "ssh"))
> > + protocol = kSecProtocolTypeSSH;
> > else /* we don't yet handle other protocols */
> > exit(0);
>
> This looks pretty good, except the last one raises a question. I'm
> using Mac OS X, and ssh already interacts with keychain to get my SSH
> key password. Is this mainly for password logins via SSH? Assuming
> that's the case:
>
> Signed-off-by: John Szakmeister <john@szakmeister.net>
>
> -John
I thought that SSH password logins can benefit from it, but I just
found that it is wrong because it seems that SSH client is responsible
for authenticating. Consequently, supporting SSH here is useless.
I will remove that lines and send this patch again.
Since it is the first time I submit a patch to git, I am not very
familiar with the convention here. Should I send the modified patch
to the maintainer directly? And what information should I append to
my patch before it can get merged?
--
Xidorn Quan
^ permalink raw reply [flat|nested] 10+ messages in thread[parent not found: <CAMdq6987TAj7f03mABkqu9v4wicarrZLYQypNUiOrP0fsLc4mQ@mail.gmail.com>]
* Re: [PATCH] credential-osxkeychain: support more protocols
[not found] ` <CAMdq6987TAj7f03mABkqu9v4wicarrZLYQypNUiOrP0fsLc4mQ@mail.gmail.com>
@ 2013-05-27 14:28 ` John Szakmeister
2013-05-27 15:02 ` Xidorn Quan
0 siblings, 1 reply; 10+ messages in thread
From: John Szakmeister @ 2013-05-27 14:28 UTC (permalink / raw)
To: Xidorn Quan; +Cc: git, Jeff King
On Mon, May 27, 2013 at 9:55 AM, Xidorn Quan <quanxunzhen@gmail.com> wrote:
[snip]
> I thought that SSH password logins can benefit from it, but I just
> found that it is wrong because it seems that SSH client is responsible
> for authenticating. Consequently, supporting SSH here is useless.
> I will remove that lines and send this patch again.
>
> Since it is the first time I submit a patch to git, I am not very
> familiar with the convention here. Should I send the modified patch
> to the maintainer directly? And what information should I append to
> my patch before it can get merged?
You'll need to read Documentation/SubmittingPatches (here's a link to
a version online:
https://github.com/git/git/blob/master/Documentation/SubmittingPatches).
You should resend this patch with the fix and change "[PATCH]" to
"[PATCH v2]", so the folks involved know that this is the second
iteration. You also need to include a "Signed-off-by" line in your
patch, which means you agree to the agreement set forth in the
"Developer's Certificate of Origin" (which is in the SubmittingPatches
documentation). You can easily include this line when you make the
commit by using the `-s` option on `git commit`.
You can also add an "Acked-by" line for me (since I reviewed and
approve of the change):
Acked-by: John Szakmeister <john@szakmeister.net>
HTH!
-John
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] credential-osxkeychain: support more protocols
2013-05-27 14:28 ` John Szakmeister
@ 2013-05-27 15:02 ` Xidorn Quan
0 siblings, 0 replies; 10+ messages in thread
From: Xidorn Quan @ 2013-05-27 15:02 UTC (permalink / raw)
To: John Szakmeister; +Cc: git
On Mon, May 27, 2013 at 10:28 PM, John Szakmeister <john@szakmeister.net> wrote:
[snip]
>
> You'll need to read Documentation/SubmittingPatches (here's a link to
> a version online:
> https://github.com/git/git/blob/master/Documentation/SubmittingPatches).
>
> You should resend this patch with the fix and change "[PATCH]" to
> "[PATCH v2]", so the folks involved know that this is the second
> iteration. You also need to include a "Signed-off-by" line in your
> patch, which means you agree to the agreement set forth in the
> "Developer's Certificate of Origin" (which is in the SubmittingPatches
> documentation). You can easily include this line when you make the
> commit by using the `-s` option on `git commit`.
>
> You can also add an "Acked-by" line for me (since I reviewed and
> approve of the change):
>
> Acked-by: John Szakmeister <john@szakmeister.net>
>
> HTH!
Thx for your explaining, it helps a lot!
After reading the doc, I have a question that when can I affirm that
the list reaches a consensus?
--
Xidorn Quan
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2] credential-osxkeychain: support more protocols
2013-05-27 7:57 [PATCH] credential-osxkeychain: support more protocols Xidorn Quan
2013-05-27 10:27 ` John Szakmeister
@ 2013-05-27 14:35 ` Xidorn Quan
2013-05-27 15:08 ` Jeff King
2013-05-28 2:36 ` [PATCH v3] " Xidorn Quan
1 sibling, 2 replies; 10+ messages in thread
From: Xidorn Quan @ 2013-05-27 14:35 UTC (permalink / raw)
To: git; +Cc: Xidorn Quan, John Szakmeister, Jeff King
Add protocol ftp and smtp for credential-osxkeychain.
Acked-by: John Szakmeister <john@szakmeister.net>
Signed-off-by: Xidorn Quan <quanxunzhen@gmail.com>
---
contrib/credential/osxkeychain/git-credential-osxkeychain.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
index 3940202..648fadd 100644
--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
@@ -127,10 +127,14 @@ static void read_credential(void)
*v++ = '\0';
if (!strcmp(buf, "protocol")) {
- if (!strcmp(v, "https"))
+ if (!strcmp(v, "ftp"))
+ protocol = kSecProtocolTypeFTP;
+ else if (!strcmp(v, "https"))
protocol = kSecProtocolTypeHTTPS;
else if (!strcmp(v, "http"))
protocol = kSecProtocolTypeHTTP;
+ else if (!strcmp(v, "smtp"))
+ protocol = kSecProtocolTypeSMTP;
else /* we don't yet handle other protocols */
exit(0);
}
--
1.8.3
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v2] credential-osxkeychain: support more protocols
2013-05-27 14:35 ` [PATCH v2] " Xidorn Quan
@ 2013-05-27 15:08 ` Jeff King
2013-05-27 15:46 ` Xidorn Quan
2013-05-28 2:36 ` [PATCH v3] " Xidorn Quan
1 sibling, 1 reply; 10+ messages in thread
From: Jeff King @ 2013-05-27 15:08 UTC (permalink / raw)
To: Xidorn Quan; +Cc: git, John Szakmeister
On Mon, May 27, 2013 at 10:35:59PM +0800, Xidorn Quan wrote:
> diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> index 3940202..648fadd 100644
> --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> @@ -127,10 +127,14 @@ static void read_credential(void)
> *v++ = '\0';
>
> if (!strcmp(buf, "protocol")) {
> - if (!strcmp(v, "https"))
> + if (!strcmp(v, "ftp"))
> + protocol = kSecProtocolTypeFTP;
> + else if (!strcmp(v, "https"))
> protocol = kSecProtocolTypeHTTPS;
> else if (!strcmp(v, "http"))
> protocol = kSecProtocolTypeHTTP;
> + else if (!strcmp(v, "smtp"))
> + protocol = kSecProtocolTypeSMTP;
> else /* we don't yet handle other protocols */
> exit(0);
This looks good to me. Git will ask for "protocol=ftp" when
accessing the dumb protocol over ftp. And it will ask for smtp via
git-send-email since 4d31a44 (git-send-email: use git credential to
obtain password, 2013-02-12).
While we are in the area it may be worth thinking if there are other
schemes we would want to support. Git might feed any URL scheme that
curl accepts, so I think we would want to handle FTPS alongside FTP, no?
We may also eventually want IMAP for git-imap-send, but we have not yet
implemented credential-helper support there. We may also want http/socks
proxy authentication, but we also have not implemented the git side of
that yet. So I think both of those can wait for now.
-Peff
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] credential-osxkeychain: support more protocols
2013-05-27 15:08 ` Jeff King
@ 2013-05-27 15:46 ` Xidorn Quan
2013-05-27 16:05 ` Jeff King
0 siblings, 1 reply; 10+ messages in thread
From: Xidorn Quan @ 2013-05-27 15:46 UTC (permalink / raw)
To: Jeff King; +Cc: git, John Szakmeister
On Mon, May 27, 2013 at 11:08 PM, Jeff King <peff@peff.net> wrote:
[snip]
>
> This looks good to me. Git will ask for "protocol=ftp" when
> accessing the dumb protocol over ftp. And it will ask for smtp via
> git-send-email since 4d31a44 (git-send-email: use git credential to
> obtain password, 2013-02-12).
>
> While we are in the area it may be worth thinking if there are other
> schemes we would want to support. Git might feed any URL scheme that
> curl accepts, so I think we would want to handle FTPS alongside FTP, no?
Good point, I'll add that.
> We may also eventually want IMAP for git-imap-send, but we have not yet
> implemented credential-helper support there. We may also want http/socks
> proxy authentication, but we also have not implemented the git side of
> that yet. So I think both of those can wait for now.
Hope the helpers will be implemented soon. IMO, we can add IMAP and
SOCKS for now since the protocol names are clear, while it is unclear
what protocol name will be used for HTTP/HTTPS proxy. I guess that
some may prefer using http/https as protocol name instead of something
specific. What do you think?
--
Xidorn Quan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] credential-osxkeychain: support more protocols
2013-05-27 15:46 ` Xidorn Quan
@ 2013-05-27 16:05 ` Jeff King
0 siblings, 0 replies; 10+ messages in thread
From: Jeff King @ 2013-05-27 16:05 UTC (permalink / raw)
To: Xidorn Quan; +Cc: git, John Szakmeister
On Mon, May 27, 2013 at 11:46:24PM +0800, Xidorn Quan wrote:
> > We may also eventually want IMAP for git-imap-send, but we have not yet
> > implemented credential-helper support there. We may also want http/socks
> > proxy authentication, but we also have not implemented the git side of
> > that yet. So I think both of those can wait for now.
>
> Hope the helpers will be implemented soon. IMO, we can add IMAP and
> SOCKS for now since the protocol names are clear, while it is unclear
> what protocol name will be used for HTTP/HTTPS proxy. I guess that
> some may prefer using http/https as protocol name instead of something
> specific. What do you think?
Yes, I think git will need to munge the protocol to indicate to the
helper that it is a proxy (or alternatively, add a new key "proxy=1" or
something). So it would make sense to wait until the git side of the
code materializes so that the helpers knows what to expect.
There was some work done in this area last May, but there are a lot of
corner cases with knowing exactly what the proxy URL is (because in some
cases, curl pulls it from the environment without git's knowledge).
There was a patch series under consideration for libcurl that would let
us supply an "authentication callback" to curl that would be called when
credentials were needed. But I have not kept track of the state of that
patch and whether it ever got merged upstream into libcurl.
-Peff
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3] credential-osxkeychain: support more protocols
2013-05-27 14:35 ` [PATCH v2] " Xidorn Quan
2013-05-27 15:08 ` Jeff King
@ 2013-05-28 2:36 ` Xidorn Quan
1 sibling, 0 replies; 10+ messages in thread
From: Xidorn Quan @ 2013-05-28 2:36 UTC (permalink / raw)
To: git; +Cc: Xidorn Quan, Jeff King, John Szakmeister
Add protocol imap, imaps, ftp and smtp for credential-osxkeychain.
Signed-off-by: Xidorn Quan <quanxunzhen@gmail.com>
Acked-by: John Szakmeister <john@szakmeister.net>
Acked-by: Jeff King <peff@peff.net>
---
contrib/credential/osxkeychain/git-credential-osxkeychain.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
index 3940202..bcd3f57 100644
--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
@@ -127,10 +127,20 @@ static void read_credential(void)
*v++ = '\0';
if (!strcmp(buf, "protocol")) {
- if (!strcmp(v, "https"))
+ if (!strcmp(v, "imap"))
+ protocol = kSecProtocolTypeIMAP;
+ else if (!strcmp(v, "imaps"))
+ protocol = kSecProtocolTypeIMAPS;
+ else if (!strcmp(v, "ftp"))
+ protocol = kSecProtocolTypeFTP;
+ else if (!strcmp(v, "ftps"))
+ protocol = kSecProtocolTypeFTPS;
+ else if (!strcmp(v, "https"))
protocol = kSecProtocolTypeHTTPS;
else if (!strcmp(v, "http"))
protocol = kSecProtocolTypeHTTP;
+ else if (!strcmp(v, "smtp"))
+ protocol = kSecProtocolTypeSMTP;
else /* we don't yet handle other protocols */
exit(0);
}
--
1.8.3
^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-05-28 2:36 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-05-27 7:57 [PATCH] credential-osxkeychain: support more protocols Xidorn Quan
2013-05-27 10:27 ` John Szakmeister
2013-05-27 13:57 ` Xidorn Quan
[not found] ` <CAMdq6987TAj7f03mABkqu9v4wicarrZLYQypNUiOrP0fsLc4mQ@mail.gmail.com>
2013-05-27 14:28 ` John Szakmeister
2013-05-27 15:02 ` Xidorn Quan
2013-05-27 14:35 ` [PATCH v2] " Xidorn Quan
2013-05-27 15:08 ` Jeff King
2013-05-27 15:46 ` Xidorn Quan
2013-05-27 16:05 ` Jeff King
2013-05-28 2:36 ` [PATCH v3] " Xidorn Quan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).