Re: [PATCH] credential: do not store credentials received from helpers

Jeff King [off-list ref] writes:

On Fri, Apr 06, 2012 at 09:56:18PM -0700, Junio C Hamano wrote:

quoted

I am afraid that "do not trigger the cache helper" might be throwing the
baby with bathwater to solve the real problem the patch tries to address,
which is:

Peff>   2. If you use a time-based storage helper like
Peff>      "git-credential-cache", every time you run a git
Peff>      command which uses the credential, it will also
Peff>      re-insert the credential after use, freshening the
Peff>      cache timestamp. So the cache will eventually expire N
Peff>      time units after the last _use_, not after the time the
Peff>      user actually typed the password. This is probably not

Actually, that was not the real problem. The real problem I had was the
leakage between helpers. I just noticed this one while thinking about
it. So the very thing that is useful to Shawn is also potentially
dangerous to people who are doing something less clever.

quoted

Shouldn't the memory cache based helper already have enough clue to tell
when a new entry is first inserted vs when the existing entry it supplied
came back from the network layer after use?  If there is not enough clue
with the current network-layer-to-helper protocol, then wouldn't it be a
better approach to add that, so that the memory cache helper can make more
intelligent management of its timer?

You can approximate it. The daemon sees that somebody is inserting the
same thing that it already there, and can guess that it probably came
from the daemon in the first place. There are some corner cases with
expiration boundaries (we get the credential, the daemon expires it,
then the http request succeeds, and we tell the daemon "hey, store
this").

quoted

Once that is fixed, I would imagine that you can tell your users to use 
two helpers (yours and generic caching one) and configure them so that (1)
the caching one is asked first and then fall back to ask yours, and (2)
the expiration time of the caching one is set close to $X.

Yeah, in my other response to Shawn, I mentioned that we could add a
flag to do the "leaking" behavior if that's what the user wants. But it
would have the side effect of refreshing his timestamp on each use, so
his $X would not expire (although that is also the case now, and he
hasn't complained).

So I actually do think he would be better to implement the caching
inside his helper, even if it is by calling out to git-credential-cache.

While I'm for keeping the interface simple, at the same time, "I have this
credential obtained and it is valid for $X time duration" sounds like a
very common thing, and it is somewhat a shame for the API to force its
users (i.e. helpers) to reimplement the caching logic over and over.

The network layer (i.e. the one that gets the authentication material from
helpers and uses it to talk with a remote git service on the other end)
could be thought of as a proxy server that gets information from its true
source (i.e. helpers) and relays it to the browser (i.e. the remote git
service). In that context, it would be natural for the API to allow the
source of the truth (i.e. the helpers) to pass its information to the
proxy with validity duration, so that the proxy can handle the expiration
in a way hidden from the information sources and the browser.  That proxy
(i.e. the network layer) could use git-credential-cache as a non-volatile
memory but that will become an implementation detail hidden from both the
remote service and the helpers.

So,... I dunno.

On Sat, Apr 07, 2012 at 10:05:00PM -0700, Junio C Hamano wrote:

quoted

So I actually do think he would be better to implement the caching
inside his helper, even if it is by calling out to git-credential-cache.

While I'm for keeping the interface simple, at the same time, "I have this
credential obtained and it is valid for $X time duration" sounds like a
very common thing, and it is somewhat a shame for the API to force its
users (i.e. helpers) to reimplement the caching logic over and over.

But the caching _must_ be part of a helper, because the parent git
process is not long-lived. If I understand you correctly, you are
advocating adding a "ttl" field, then transparently chaining to the
cache helper when it exists.

We can do that, but the whole point of implementing a generic helper
protocol was to let people decide to do things like that themselves
(which is exactly what Shawn has done). There's no reason that git needs
to lock you into one particular implementation of caching or storage;
that's why we have configurable helpers.  There's no reason that git
should only cache when there is a ttl field (you still may want to cache
for 15 minutes, even if the credential will last longer). But nor should
git say "you always must cache".  Perhaps you don't like the complexity
or the security implications.

So I'd much rather leave it up to the user to configure their helpers
rather than trying to run credential-cache behind the scenes.

There are basically two problems to solve. The first is getting data
from one helper to the other.

One way to implement that is by just wrapping the real helper inside a
caching layer. That can even be generic. In fact, my initial version of
the credential helpers had a "chain" parameter, so you could ask
credential-cache to run a sub-helper and cache its response.
The original use was that "getpass" would be its own helper, so you
could plug in a custom version. Since nobody seemed to want to do that,
I dropped it in the name of simplicity.

You can easily write a generic wrapper like this:

  #!/bin/sh
  cache=$1
  chain=$2
  case "$3" in
  get)
          cat >request
          eval "$cache get" <request >response
          if test -s response; then
            cat response
          else
            eval "$chain" >response
            cat request response | eval "$cache store"
            cat response
          fi
          ;;
  *)
          eval "$cache $3"
          ;;
  esac

though obviously you would write it in a real language that doesn't
involve storing the intermediate states on disk (and rather than eval,
you'd take the usual helper specification). And we could easily provide
such a wrapper, and Shawn's config would look like:

  [credential]
  helper = "wrapper cache real-helper"

Another way to do it is by listing the helpers sequentially, and feeding
the output of one to the input of another during a "store". As I said,
I'm uncomfortable with the security implications of doing that by
default. Your proposal side steps it by implying that cache is special,
and does not have these security implications.  Which is mostly true
(though it also is limiting). So the obvious thing would be to mark
helpers as either "yes, it's OK to share my output with others" or "yes,
it's OK for me to see the output of others". And then Shawn's config
looks like:

  [credential]
  helper = cache
  helper = real-helper

The second issue is that of communicating the ttl or expiration between
helpers. That's easy enough. The protocol allows arbitrary key/value
pairs. We typically just drop ones we don't care about, but we could
retain them and pass them along.

-Peff

On Sun, Apr 08, 2012 at 02:40:59AM -0400, Jeff King wrote:

One way to implement that is by just wrapping the real helper inside a
caching layer. That can even be generic.

Here's a C implementation of the shell sketch I posted earlier.
Obviously missing documentation, and only lightly tested, but just to
give a sense of what it would look like. You can exercise it manually
with:

  {
    # simulate git's input
    echo protocol=https
    echo host=example.com
  } |
  git credential-wrap cache '!f() {
    # note whether we ran or not
    echo >&2 Generating...
    # and simulate output
    echo username=fake.username
    echo password=fake.password
  }; f' get

or configure it with:

  git config credential.helper 'wrap cache your-real-helper'

---
 Makefile          |    1 +
 credential-wrap.c |   32 ++++++++++++++++++++++++++++++++
 credential.c      |    4 ++--
 credential.h      |    3 +++
 4 files changed, 38 insertions(+), 2 deletions(-)
 create mode 100644 credential-wrap.c

diff --git a/Makefile b/Makefile
index be1957a..c91bb23 100644
--- a/Makefile
+++ b/Makefile

@@ -463,6 +463,7 @@ PROGRAM_OBJS += upload-pack.o
 PROGRAM_OBJS += http-backend.o
 PROGRAM_OBJS += sh-i18n--envsubst.o
 PROGRAM_OBJS += credential-store.o
+PROGRAM_OBJS += credential-wrap.o
 
 # Binary suffix, set to .exe for Windows builds
 X =

diff --git a/credential-wrap.c b/credential-wrap.c
new file mode 100644
index 0000000..f4aadc4
--- /dev/null
+++ b/credential-wrap.c

@@ -0,0 +1,32 @@
+#include "cache.h"
+#include "credential.h"
+
+int main(int argc, const char **argv)
+{
+	struct credential c = CREDENTIAL_INIT;
+	const char *storage, *source, *action;
+
+	if (argc != 4)
+		usage("git credential-wrap <storage> <source> <action>");
+	storage = argv[1];
+	source = argv[2];
+	action = argv[3];
+
+	if (credential_read(&c, stdin) < 0)
+		die("unable to read input credential");
+
+	if (!strcmp(action, "get")) {
+		credential_do(&c, storage, "get");
+		if (!c.username || !c.password) {
+			credential_do(&c, source, "get");
+			if (!c.username || !c.password)
+				return 0;
+			credential_do(&c, storage, "store");
+		}
+		credential_write(&c, stdout);
+	}
+	else
+		credential_do(&c, storage, action);
+
+	return 0;
+}

diff --git a/credential.c b/credential.c
index 813e77a..13409e1 100644
--- a/credential.c
+++ b/credential.c

@@ -191,7 +191,7 @@ static void credential_write_item(FILE *fp, const char *key, const char *value)
 	fprintf(fp, "%s=%s\n", key, value);
 }
 
-static void credential_write(const struct credential *c, FILE *fp)
+void credential_write(const struct credential *c, FILE *fp)
 {
 	credential_write_item(fp, "protocol", c->protocol);
 	credential_write_item(fp, "host", c->host);

@@ -241,7 +241,7 @@ static int run_credential_helper(struct credential *c,
 	return 0;
 }
 
-static int credential_do(struct credential *c, const char *helper,
+int credential_do(struct credential *c, const char *helper,
 			 const char *operation)
 {
 	struct strbuf cmd = STRBUF_INIT;

diff --git a/credential.h b/credential.h
index 96ea41b..daf3e81 100644
--- a/credential.h
+++ b/credential.h

@@ -30,4 +30,7 @@ void credential_from_url(struct credential *, const char *url);
 int credential_match(const struct credential *have,
 		     const struct credential *want);
 
+int credential_do(struct credential *, const char *helper, const char *action);
+void credential_write(const struct credential *, FILE *);
+
 #endif /* CREDENTIAL_H */

-- 
1.7.10.11.g901cee

On Sun, Apr 08, 2012 at 02:40:59AM -0400, Jeff King wrote:

The second issue is that of communicating the ttl or expiration between
helpers. That's easy enough. The protocol allows arbitrary key/value
pairs. We typically just drop ones we don't care about, but we could
retain them and pass them along.

And here's a rough patch for that. This is just to get an idea of the
scale, and which parts of the code need changed. I'd probably use a
key/value store instead of a string_list. On top of this,
credential-cache would have to learn to respect a TTL variable in the
input (actually, it does already respect "timeout" which is added on the
way from the cache client to the cache daemon, but the parsing around
that would have to be cleaned up a bit).

---
 credential.c |   14 +++++++++++---
 credential.h |    3 ++-
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/credential.c b/credential.c
index 13409e1..2237e7e 100644
--- a/credential.c
+++ b/credential.c

@@ -9,6 +9,7 @@ void credential_init(struct credential *c)
 {
 	memset(c, 0, sizeof(*c));
 	c->helpers.strdup_strings = 1;
+	c->extra.strdup_strings = 1;
 }
 
 void credential_clear(struct credential *c)

@@ -19,6 +20,7 @@ void credential_clear(struct credential *c)
 	free(c->username);
 	free(c->password);
 	string_list_clear(&c->helpers, 0);
+	string_list_clear(&c->extra, 0);
 
 	credential_init(c);
 }

@@ -174,10 +176,11 @@ int credential_read(struct credential *c, FILE *fp)
 			c->path = xstrdup(value);
 		}
 		/*
-		 * Ignore other lines; we don't know what they mean, but
-		 * this future-proofs us when later versions of git do
-		 * learn new lines, and the helpers are updated to match.
+		 * Save other lines so they can be fed back to the helper or
+		 * transported to other helpers.
 		 */
+		*(value-1) = '=';
+		string_list_append(&c->extra, line.buf);
 	}
 
 	strbuf_release(&line);

@@ -193,11 +196,16 @@ static void credential_write_item(FILE *fp, const char *key, const char *value)
 
 void credential_write(const struct credential *c, FILE *fp)
 {
+	int i;
+
 	credential_write_item(fp, "protocol", c->protocol);
 	credential_write_item(fp, "host", c->host);
 	credential_write_item(fp, "path", c->path);
 	credential_write_item(fp, "username", c->username);
 	credential_write_item(fp, "password", c->password);
+
+	for (i = 0; i < c->extra.nr; i++)
+		fprintf(fp, "%s\n", c->extra.items[i].string);
 }
 
 static int run_credential_helper(struct credential *c,

diff --git a/credential.h b/credential.h
index daf3e81..5f98527 100644
--- a/credential.h
+++ b/credential.h

@@ -5,6 +5,7 @@
 
 struct credential {
 	struct string_list helpers;
+	struct string_list extra;
 	unsigned approved:1,
 		 configured:1,
 		 use_http_path:1;

@@ -16,7 +17,7 @@ struct credential {
 	char *path;
 };
 
-#define CREDENTIAL_INIT { STRING_LIST_INIT_DUP }
+#define CREDENTIAL_INIT { STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP }
 
 void credential_init(struct credential *);
 void credential_clear(struct credential *);

-- 
1.7.10.11.g901cee