threads / patch / 36506

patchUses git-credential for git-imap-send

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

## tl;dr

8 messages between Apr 26, 2014 and Apr 29, 2014. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Dan Albert· Apr 26, 2014, 17:50 UTC · lore

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(-)
Show changes to imap-send.c +15 −14
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
Jeff King· Apr 26, 2014, 18:08 UTC · re: Dan Albert · lore

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).

Show 5 quoted lines
> + 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
Dan Albert· Apr 27, 2014, 17:58 UTC · re: Jeff King · lore

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>
---
Show 23 quoted lines
>> 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(-)
Show changes to imap-send.c +15 −14
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
Jeff King· Apr 28, 2014, 19:23 UTC · re: Dan Albert · lore

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

On Sun, Apr 27, 2014 at 10:58:51AM -0700, Dan Albert wrote:
Show 7 quoted lines
> 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
Dan Albert· Apr 29, 2014, 03:00 UTC · re: Jeff King · lore

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>
---
Show 18 quoted lines
> 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(-)
Show changes to imap-send.c +26 −19
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
Jeff King· Apr 29, 2014, 03:05 UTC · re: Dan Albert · lore

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

On Mon, Apr 28, 2014 at 08:00:04PM -0700, Dan Albert wrote:
Show 14 quoted lines
> > 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
Junio C Hamano· Apr 29, 2014, 17:19 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 20 quoted lines
> 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.

Jeff King· Apr 27, 2014, 07:51 UTC · lore

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).

Show 6 quoted lines
> 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.

Show 15 quoted lines
> ---
> 
> 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

← back to recent threads