[PATCH] Uses git-credential for git-imap-send

[PATCH] Uses git-credential for git-imap-send

[Dan Albert]

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

Re: [PATCH] Uses git-credential for git-imap-send

[Jeff King]

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

Re: [PATCH] Uses git-credential for git-imap-send

[Jeff King]

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 <danalbert@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

[PATCH] Uses git-credential for git-imap-send

[Dan Albert]

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 <danalbert@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

Re: [PATCH] Uses git-credential for git-imap-send

[Jeff King]

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 <danalbert@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

[PATCH] Uses git-credential for git-imap-send

[Dan Albert]

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 <danalbert@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

Re: [PATCH] Uses git-credential for git-imap-send

[Jeff King]

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 <peff@peff.net>

-Peff

Re: [PATCH] Uses git-credential for git-imap-send

[Junio C Hamano]

Jeff King <peff@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 <peff@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.