Re: [PATCH] Uses git-credential for git-imap-send
Jeff King p...@peff.net writes:
On Mon, Apr 28, 2014 at 08:00:04PM -0700, Dan Albert wrote:
I noticed that we are just filling in the password here, since we'll
always fill cred.username from srvc-user. The lines directly above are:
if (!srvc-user) {
fprintf(stderr, Skipping server %s, no user\n,
srvc-host);
goto bail;
}
That comes from the imap.user config variable. I wonder if we should
just pass it off to credential_fill() in this case, too, which will fill
in the username if necessary.
[...]
Yeah, doubtful anyone cares, but it's simple enough to do.
Thanks, I think the result looks good.
Acked-by: Jeff King p...@peff.net
OK, luckily I haven't merged the one from the yesterday yet, so I'll
replace ;-)
Thanks for working on this, Dan, and as always thanks for reviewing,
Peff.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Uses git-credential for git-imap-send
On Sun, Apr 27, 2014 at 10:58:51AM -0700, Dan Albert wrote:
git-imap-send was directly prompting for a password rather than using
git-credential. git-send-email, on the other hand, supports git-credential.
This is a necessary improvement for users that use two factor authentication,
as
they should not be expected to remember all of their app specific passwords.
Signed-off-by: Dan Albert danalb...@google.com
Thanks, this version looks good to me.
I noticed that we are just filling in the password here, since we'll
always fill cred.username from srvc-user. The lines directly above are:
if (!srvc-user) {
fprintf(stderr, Skipping server %s, no user\n, srvc-host);
goto bail;
}
That comes from the imap.user config variable. I wonder if we should
just pass it off to credential_fill() in this case, too, which will fill
in the username if necessary.
It probably doesn't matter much, though, as nobody is complaining. And
if we were designing from scratch, I would say that imap.user and
imap.pass would not need to exist, as you can configure
credential.imaps://host/.* for the same purpose. But since we would
have to keep supporting them anyway for compatibility, it's not worth
trying to transition.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Uses git-credential for git-imap-send
git-imap-send was directly prompting for a password rather than using
git-credential. git-send-email, on the other hand, supports git-credential.
This is a necessary improvement for users that use two factor authentication, as
they should not be expected to remember all of their app specific passwords.
Signed-off-by: Dan Albert danalb...@google.com
---
I noticed that we are just filling in the password here, since we'll
always fill cred.username from srvc-user. The lines directly above are:
if (!srvc-user) {
fprintf(stderr, Skipping server %s, no user\n, srvc-host);
goto bail;
}
That comes from the imap.user config variable. I wonder if we should
just pass it off to credential_fill() in this case, too, which will fill
in the username if necessary.
It probably doesn't matter much, though, as nobody is complaining. And
if we were designing from scratch, I would say that imap.user and
imap.pass would not need to exist, as you can configure
credential.imaps://host/.* for the same purpose. But since we would
have to keep supporting them anyway for compatibility, it's not worth
trying to transition.
Yeah, doubtful anyone cares, but it's simple enough to do.
imap-send.c | 45 ++---
1 file changed, 26 insertions(+), 19 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index 0bc6f7f..5c4f336 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -23,9 +23,9 @@
*/
#include cache.h
+#include credential.h
#include exec_cmd.h
#include run-command.h
-#include prompt.h
#ifdef NO_OPENSSL
typedef void *SSL;
#endif
@@ -946,6 +946,7 @@ static int auth_cram_md5(struct imap_store *ctx, struct
imap_cmd *cmd, const cha
static struct imap_store *imap_open_store(struct imap_server_conf *srvc)
{
+ struct credential cred = CREDENTIAL_INIT;
struct imap_store *ctx;
struct imap *imap;
char *arg, *rsp;
@@ -1096,25 +1097,23 @@ static struct imap_store *imap_open_store(struct
imap_server_conf *srvc)
}
#endif
imap_info(Logging in...\n);
- if (!srvc-user) {
- fprintf(stderr, Skipping server %s, no user\n,
srvc-host);
- goto bail;
- }
- if (!srvc-pass) {
- struct strbuf prompt = STRBUF_INIT;
- strbuf_addf(prompt, Password (%s@%s): , srvc-user,
srvc-host);
- arg = git_getpass(prompt.buf);
- strbuf_release(prompt);
- if (!*arg) {
- fprintf(stderr, Skipping account %s@%s, no
password\n, srvc-user, srvc-host);
- goto bail;
- }
- /*
-* getpass() returns a pointer to a static buffer.
make a copy
-* for long term storage.
-*/
- srvc-pass = xstrdup(arg);
+ if (!srvc-user || !srvc-pass) {
+ cred.protocol = xstrdup(srvc-use_ssl ? imaps :
imap);
+ cred.host = xstrdup(srvc-host);
+
+ if (srvc-user)
+ cred.username = xstrdup(srvc-user);
+ if (srvc-pass)
+ cred.password = xstrdup(srvc-pass);
+
+ credential_fill(cred);
+
+ if (!srvc-user)
+ srvc-user = xstrdup(cred.username);
+ if (!srvc-pass)
+ srvc-pass = xstrdup(cred.password);
}
+
if (CAP(NOLOGIN)) {
fprintf(stderr, Skipping account %s@%s, server forbids
LOGIN\n, srvc-user, srvc-host);
goto bail;
@@ -1153,10 +1152,18 @@ static struct imap_store *imap_open_store(struct
imap_server_conf *srvc)
}
} /* !preauth */
+ if (cred.username)
+ credential_approve(cred);
+ credential_clear(cred);
+
ctx-prefix = ;
return ctx;
bail:
+ if (cred.username)
+ credential_reject(cred);
+ credential_clear(cred);
+
imap_close_store(ctx);
return NULL;
}
--
2.0.0.rc1.1.gce060f5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Uses git-credential for git-imap-send
On Mon, Apr 28, 2014 at 08:00:04PM -0700, Dan Albert wrote:
I noticed that we are just filling in the password here, since we'll
always fill cred.username from srvc-user. The lines directly above are:
if (!srvc-user) {
fprintf(stderr, Skipping server %s, no user\n,
srvc-host);
goto bail;
}
That comes from the imap.user config variable. I wonder if we should
just pass it off to credential_fill() in this case, too, which will fill
in the username if necessary.
[...]
Yeah, doubtful anyone cares, but it's simple enough to do.
Thanks, I think the result looks good.
Acked-by: Jeff King p...@peff.net
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Uses git-credential for git-imap-send
On Sat, Apr 26, 2014 at 11:30:11AM -0700, Dan Albert wrote: I had resent a less broken patch after I noticed the tabs, but it seems to have gotten lost. Better formatted patch at the bottom of this message. Your emails (including this one) are multipart/alternatives with an html part, which will cause the mailing list software to reject them. This email also still seems whitespace-damaged to me (the leading tabs are collapsed to a single space). It looks like you're using gmail to send; you might try using git send-email (the example at the end of git help send-email can walk you through it). About imap vs. imaps: I actually had your exact line in before, but decided that as long as its for the same host the user probably wants to use the same credentials for both imap and imaps (if they for some reason had both configured). Hard coding imap allows them to use either protocol with only one keychain entry. The use case is a stretch, but it doesn't do any harm to implement it this way. My concerns with conflating the two are: 1. The system helper might care about the distinction and prefer imaps (e.g., it might already have the credential stored for your regular mail client, which uses imaps). But osxkeychain is the only helper that makes the distinction, and I don't really know how OS X's keychain code handles the distinction. 2. With http and https, we are careful to make the distinction, because we would not want to accidentally share a credential over http that was stored via https. But it's pretty easy to use an http URL rather than an https one. It's probably pretty rare to accidentally turn off imap SSL. So I'd be OK with leaving it as imap for now, and waiting for somebody to actually come up with a real case where the distinction matters. --- Uses git-credential for git-imap-send git-imap-send was directly prompting for a password rather than using git-credential. git-send-email, on the other hand, supports git-credential. This is a necessary improvement for users that use two factor authentication, as they should not be expected to remember all of their app specific passwords. Signed-off-by: Dan Albert danalb...@google.com --- imap-send.c | 29 +++-- 1 file changed, 15 insertions(+), 14 deletions(-) A side note on formatting your commit message: The maintainer picks up patches from the list with git am, which will take everything up to the first --- as the commit message, and discard everything after up to the start of the diff. So in this case it would take your cover-letter material as the commit message, and drop your actual commit message. The usual formats are either to put the cover letter material after the ---, like: $COMMIT_MESSAGE Signed-off-by: You --- $COVER_LETTER $DIFFSTAT $DIFF or to use a scissors-line -- 8 -- instead of three-dash: $COVER_LETTER -- 8 -- $COMMIT_MESSAGE --- $DIFFSTAT $DIFF -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Uses git-credential for git-imap-send
git-imap-send was directly prompting for a password rather than using
git-credential. git-send-email, on the other hand, supports git-credential.
This is a necessary improvement for users that use two factor authentication, as
they should not be expected to remember all of their app specific passwords.
Signed-off-by: Dan Albert danalb...@google.com
---
About imap vs. imaps: I actually had your exact line in before, but decided
that as long as its for the same host the user probably wants to use the
same credentials for both imap and imaps (if they for some reason had both
configured). Hard coding imap allows them to use either protocol with
only one keychain entry. The use case is a stretch, but it doesn't do any
harm to implement it this way.
My concerns with conflating the two are:
1. The system helper might care about the distinction and prefer imaps
(e.g., it might already have the credential stored for your regular
mail client, which uses imaps). But osxkeychain is the only helper
that makes the distinction, and I don't really know how OS X's
keychain code handles the distinction.
2. With http and https, we are careful to make the distinction,
because we would not want to accidentally share a credential over http
that was stored via https. But it's pretty easy to use an http URL
rather than an https one. It's probably pretty rare to accidentally
turn off imap SSL.
So I'd be OK with leaving it as imap for now, and waiting for somebody
to actually come up with a real case where the distinction matters.
These are good points. I've made the change.
imap-send.c | 29 +++--
1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index 0bc6f7f..112fc83 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -23,9 +23,9 @@
*/
#include cache.h
+#include credential.h
#include exec_cmd.h
#include run-command.h
-#include prompt.h
#ifdef NO_OPENSSL
typedef void *SSL;
#endif
@@ -946,6 +946,7 @@ static int auth_cram_md5(struct imap_store *ctx, struct
imap_cmd *cmd, const cha
static struct imap_store *imap_open_store(struct imap_server_conf *srvc)
{
+ struct credential cred = CREDENTIAL_INIT;
struct imap_store *ctx;
struct imap *imap;
char *arg, *rsp;
@@ -1101,19 +1102,11 @@ static struct imap_store *imap_open_store(struct
imap_server_conf *srvc)
goto bail;
}
if (!srvc-pass) {
- struct strbuf prompt = STRBUF_INIT;
- strbuf_addf(prompt, Password (%s@%s): , srvc-user,
srvc-host);
- arg = git_getpass(prompt.buf);
- strbuf_release(prompt);
- if (!*arg) {
- fprintf(stderr, Skipping account %s@%s, no
password\n, srvc-user, srvc-host);
- goto bail;
- }
- /*
-* getpass() returns a pointer to a static buffer.
make a copy
-* for long term storage.
-*/
- srvc-pass = xstrdup(arg);
+ cred.username = xstrdup(srvc-user);
+ cred.protocol = xstrdup(srvc-use_ssl ? imaps :
imap);
+ cred.host = xstrdup(srvc-host);
+ credential_fill(cred);
+ srvc-pass = xstrdup(cred.password);
}
if (CAP(NOLOGIN)) {
fprintf(stderr, Skipping account %s@%s, server forbids
LOGIN\n, srvc-user, srvc-host);
@@ -1153,10 +1146,18 @@ static struct imap_store *imap_open_store(struct
imap_server_conf *srvc)
}
} /* !preauth */
+ if (cred.username)
+ credential_approve(cred);
+ credential_clear(cred);
+
ctx-prefix = ;
return ctx;
bail:
+ if (cred.username)
+ credential_reject(cred);
+ credential_clear(cred);
+
imap_close_store(ctx);
return NULL;
}
--
2.0.0.rc1.1.gce060f5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Uses git-credential for git-imap-send
git-imap-send was directly prompting for a password rather than using
git-credential. git-send-email, on the other hand, supports git-credential.
This is a necessary improvement for users that use two factor authentication, as
they should not be expected to remember all of their app specific passwords.
---
imap-send.c | 29 +++--
1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index 0bc6f7f..262fb9a 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -23,9 +23,9 @@
*/
#include cache.h
+#include credential.h
#include exec_cmd.h
#include run-command.h
-#include prompt.h
#ifdef NO_OPENSSL
typedef void *SSL;
#endif
@@ -946,6 +946,7 @@ static int auth_cram_md5(struct imap_store *ctx,
struct imap_cmd *cmd, const cha
static struct imap_store *imap_open_store(struct imap_server_conf *srvc)
{
+ struct credential cred = CREDENTIAL_INIT;
struct imap_store *ctx;
struct imap *imap;
char *arg, *rsp;
@@ -1101,19 +1102,11 @@ static struct imap_store
*imap_open_store(struct imap_server_conf *srvc)
goto bail;
}
if (!srvc-pass) {
- struct strbuf prompt = STRBUF_INIT;
- strbuf_addf(prompt, Password (%s@%s): , srvc-user, srvc-host);
- arg = git_getpass(prompt.buf);
- strbuf_release(prompt);
- if (!*arg) {
- fprintf(stderr, Skipping account %s@%s, no password\n, srvc-user,
srvc-host);
- goto bail;
- }
- /*
- * getpass() returns a pointer to a static buffer. make a copy
- * for long term storage.
- */
- srvc-pass = xstrdup(arg);
+ cred.username = xstrdup(srvc-user);
+ cred.protocol = xstrdup(imap);
+ cred.host = xstrdup(srvc-host);
+ credential_fill(cred);
+ srvc-pass = xstrdup(cred.password);
}
if (CAP(NOLOGIN)) {
fprintf(stderr, Skipping account %s@%s, server forbids LOGIN\n,
srvc-user, srvc-host);
@@ -1153,10 +1146,18 @@ static struct imap_store
*imap_open_store(struct imap_server_conf *srvc)
}
} /* !preauth */
+ if (cred.username)
+ credential_approve(cred);
+ credential_clear(cred);
+
ctx-prefix = ;
return ctx;
bail:
+ if (cred.username)
+ credential_reject(cred);
+ credential_clear(cred);
+
imap_close_store(ctx);
return NULL;
}
--
2.0.0.rc1.1.gce060f5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Uses git-credential for git-imap-send
On Sat, Apr 26, 2014 at 10:50:26AM -0700, Dan Albert wrote:
git-imap-send was directly prompting for a password rather than using
git-credential. git-send-email, on the other hand, supports
git-credential.
Yay. These sorts of conversions were definitely on my mind when I did
the original credential work, and have mostly been waiting on somebody
who cares enough about specific tools to switch them over.
static struct imap_store *imap_open_store(struct imap_server_conf *srvc)
{
+ struct credential cred = CREDENTIAL_INIT;
Your patch seems to have been whitespace-damaged in transit (there are
hints for gmail in the format-patch and send-email manpages).
+ cred.username = xstrdup(srvc-user);
+ cred.protocol = xstrdup(imap);
+ cred.host = xstrdup(srvc-host);
+ credential_fill(cred);
+ srvc-pass = xstrdup(cred.password);
The imap protocol will get passed along to any helpers. I think the
only one which actually cares about the specific content is the
osxkeychain helper, which differentiates between imap and imaps. I
think we can know which is which at this point and do:
cred.protocol = xstrdup(srvc-use_ssl ? imaps : imap);
Other than that, the patch looks good. Tests would be wonderful, but we
have no infrastructure at all, so it would involve writing a mock IMAP
server. Having implemented both IMAP servers and clients in a previous
life, I could not ask anyone to go through that pain. :)
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
