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

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

[Jeff King]

The typical call flow for the credential API is something
like:

  1. Network code (like http.c) wants a credential. It calls
     credential_fill() to get it, and one of the two happens:

     a. We call out to any helpers with a "get" request,
	one of which provides the credential.

     b. No helper provides us the credential, so we ask the
        user.

  2. The network code uses the credential. Let's imagine
     that the request completes successfully.

  3. The network code informs the credential subsystem of
     success by calling credential_approve(). The credential
     code then calls out to any helpers with a "store"
     request, so they can optionally store it if they want.

In the case of (1b), this is a good thing. The credential
comes from the user, gets used, and then gets put into
storage for later use again. But in the case of (1a), we end
up feeding the helper with a credential that came from
itself already (or possibly from another helper).

Most of the time, this is harmless and slightly inefficient.
But it has two user-visible impacts:

  1. If you have two helpers, you end up propagating data
     between them. For instance, imagine you have a helper
     "git-credential-wallet" which pulls data from a a
     read-only password wallet. You might configure git like
     this:

       [credential]
		helper = wallet
		helper = cache

     to check the wallet first, and then fall back to asking
     the user and caching the result. But when we do get
     something out of the wallet, git will then ask for it
     to be stored in the wallet (which is a no-op), and the
     cache. So your credential migrates from the wallet into
     the cache, which may not be what you want (e.g.,
     because the wallet implements more strict security
     policies than the cache).

  2. If you use a time-based storage helper like
     "git-credential-cache", every time you run a git
     command which uses the credential, it will also
     re-insert the credential after use, freshening the
     cache timestamp. So the cache will eventually expire N
     time units after the last _use_, not after the time the
     user actually typed the password. This is probably not
     what most users expect or want (and if they do, we
     should do it explicitly by providing an option to
     refresh the timestamp on use).

We can solve this by marking a credential that comes from a
helper, so we don't bother feeding it back to the helpers.
The credential struct already has an "approved" flag so
that we try to store it only once, rather than for each
successful http request. We can use the same flag to
"pre-approve" a credential which comes from a helper, and
enver try to store it at all.

Signed-off-by: Jeff King <peff@peff.net>
---
Whew, that was a long explanation. Fortunately the patch text itself is
pleasantly simple.

 credential.c          |    4 +++-
 t/t5550-http-fetch.sh |    6 +++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/credential.c b/credential.c
index 62d1c56..813e77a 100644
--- a/credential.c
+++ b/credential.c
@@ -272,8 +272,10 @@ void credential_fill(struct credential *c)
 
 	for (i = 0; i < c->helpers.nr; i++) {
 		credential_do(c, c->helpers.items[i].string, "get");
-		if (c->username && c->password)
+		if (c->username && c->password) {
+			c->approved = 1;
 			return;
+		}
 	}
 
 	credential_getpass(c);
diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index e5e6b8f..82e0d37 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -104,13 +104,17 @@ test_expect_success 'http auth can request both user and pass' '
 test_expect_success 'http auth respects credential helper config' '
 	test_config_global credential.helper "!f() {
 		cat >/dev/null
+		echo helper: \$* >>helper-trace
 		echo username=user@host
 		echo password=user@host
 	}; f" &&
+	>helper-trace &&
 	>askpass-query &&
 	echo wrong >askpass-response &&
 	git clone "$HTTPD_URL/auth/repo.git" clone-auth-helper &&
-	expect_askpass none
+	expect_askpass none &&
+	echo "helper: get" >helper-expect &&
+	test_cmp helper-expect helper-trace
 '
 
 test_expect_success 'http auth can get username from config' '
-- 
1.7.10.rc4.31.g355de

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

[Shawn Pearce]

On Fri, Apr 6, 2012 at 20:34, Jeff King <peff@peff.net> wrote:
>  2. If you use a time-based storage helper like
>     "git-credential-cache", every time you run a git
>     command which uses the credential, it will also
>     re-insert the credential after use, freshening the
>     cache timestamp. So the cache will eventually expire N
>     time units after the last _use_, not after the time the
>     user actually typed the password. This is probably not
>     what most users expect or want (and if they do, we
>     should do it explicitly by providing an option to
>     refresh the timestamp on use).

So if I use the cache helper, and its set to expire at the default of
15 minutes, I have to type my password in every 15 minutes, even if I
am doing a Git operation roughly every 8 minutes during a work day?

> We can solve this by marking a credential that comes from a
> helper, so we don't bother feeding it back to the helpers.
> The credential struct already has an "approved" flag so
> that we try to store it only once, rather than for each
> successful http request. We can use the same flag to
> "pre-approve" a credential which comes from a helper, and
> enver try to store it at all.

This breaks one of my credential helpers.

I have a helper that generates a password by asking a remote system to
generate a short lived password based on other authentication systems
that I can't describe. Once I have that password, its good for $X
time.

The helper just dumps it out to Git, and Git turns around and stores
it into the cache for me. This means later requests will keep that
credential in the cache, and avoid making that remote system call
every time I do a Git network command. I guess I now need to change my
helper to cache git credential-cache itself and store the password
into the cache if it wants to use the cache?

Should we update the credential helper documentation at the same time
as this change to make it clear Git won't cache passwords returned
from helpers, but a helper could call the credential-cache itself if
it wanted to reuse the existing cache service?

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

[Jeff King]

On Fri, Apr 06, 2012 at 09:12:39PM -0700, Shawn O. Pearce wrote:

> On Fri, Apr 6, 2012 at 20:34, Jeff King <peff@peff.net> wrote:
> >  2. If you use a time-based storage helper like
> >     "git-credential-cache", every time you run a git
> >     command which uses the credential, it will also
> >     re-insert the credential after use, freshening the
> >     cache timestamp. So the cache will eventually expire N
> >     time units after the last _use_, not after the time the
> >     user actually typed the password. This is probably not
> >     what most users expect or want (and if they do, we
> >     should do it explicitly by providing an option to
> >     refresh the timestamp on use).
> 
> So if I use the cache helper, and its set to expire at the default of
> 15 minutes, I have to type my password in every 15 minutes, even if I
> am doing a Git operation roughly every 8 minutes during a work day?

Yes. It's less convenient, but safer and more predictable (you put your
password in at 2:30, it's gone at 2:45). Keep in mind that you can also
bump the cache time. And like I said, if we do want have it behave the
other way, that's OK, but it should be explicit (and it can be optional,
even if it defaults to auto-refresh on use).

Or you could even do something more complex. gpg-agent will refresh the
timestamp on use, but still has a "max ttl" that drops a credential N
seconds after it was input, even if it was used recently. But making any
decision like that means that the daemon needs to actually know whether
the timestamp came from the user, or whether it was simply accessed and
regurgitated to us.

> This breaks one of my credential helpers.
> 
> I have a helper that generates a password by asking a remote system to
> generate a short lived password based on other authentication systems
> that I can't describe. Once I have that password, its good for $X
> time.
> 
> The helper just dumps it out to Git, and Git turns around and stores
> it into the cache for me. This means later requests will keep that
> credential in the cache, and avoid making that remote system call
> every time I do a Git network command.

I considered your use case when writing the patch and thought "no,
that's too insane. Nobody would want that." But leave it to you to prove
me wrong. :)

I'm torn. What you are doing is totally reasonable for your case. At the
same time, having it happen without the user's knowledge could have
surprising security implications, because you're leaking passwords from
one helper to the other (in the example in the commit message, I
mentioned a high-security helper followed by "cache". Which is probably
not that bad. But imagine if it were "store").

> I guess I now need to change my helper to cache git credential-cache
> itself and store the password into the cache if it wants to use the
> cache?

That's one option, though it is a little bit of a pain to implement (not
hard, but not one line of code). It would be very easy to add a
"credential.storeHelperCredentials" option that defaulted to off (that
name is horrible, but you get the point).

> Should we update the credential helper documentation at the same time
> as this change to make it clear Git won't cache passwords returned
> from helpers, but a helper could call the credential-cache itself if
> it wanted to reuse the existing cache service?

Yeah, I'll add a documentation update when I re-roll; but we first need
to figure out what the recommended course of action is.

One thing I don't like about having your helper call credential-cache is
that it is a layering violation; it is only guessing that putting the
data into credential-cache will be useful in the first place. But that
depends on the user's config. It could have be using an alternative
caching helper (or none at all).

So I think an explicit "it's OK to leak credentials between helpers"
flag is our best bet. And it's dirt simple to code.

-Peff

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

[Junio C Hamano]

Shawn Pearce <spearce@spearce.org> writes:

> On Fri, Apr 6, 2012 at 20:34, Jeff King <peff@peff.net> wrote:
>>  2. If you use a time-based storage helper like
>>     "git-credential-cache", every time you run a git
>>     command which uses the credential, it will also
>>     re-insert the credential after use, freshening the
>>     cache timestamp. So the cache will eventually expire N
>>     time units after the last _use_, not after the time the
>>     user actually typed the password. This is probably not
>>     what most users expect or want (and if they do, we
>>     should do it explicitly by providing an option to
>>     refresh the timestamp on use).
>
> So if I use the cache helper, and its set to expire at the default of
> 15 minutes, I have to type my password in every 15 minutes, even if I
> am doing a Git operation roughly every 8 minutes during a work day?

I think what Peff meant was to set the expiration time of the cache helper
a lot longer (say 20 hours, as opposed to 15 minutes) but once it learns a
credential material from your helper, leave the timer running without
resetting it every time the user uses the credential (your 8 minutes).

So I do not see a fundamental problem in this part.

>> We can solve this by marking a credential that comes from a
>> helper, so we don't bother feeding it back to the helpers.
>> The credential struct already has an "approved" flag so
>> that we try to store it only once, rather than for each
>> successful http request. We can use the same flag to
>> "pre-approve" a credential which comes from a helper, and
>> enver try to store it at all.
>
> This breaks one of my credential helpers.
>
> I have a helper that generates a password by asking a remote system to
> generate a short lived password based on other authentication systems
> that I can't describe. Once I have that password, its good for $X
> time.

Well, $X is something your helper knows but other helpers don't, so...

> The helper just dumps it out to Git, and Git turns around and stores
> it into the cache for me. This means later requests will keep that
> credential in the cache, and avoid making that remote system call
> every time I do a Git network command. I guess I now need to change my
> helper to cache git credential-cache itself and store the password
> into the cache if it wants to use the cache?

... logically the knowledge of cache expiration time belongs to your
helper, but "after the first use, it is OK to keep using it for N hours"
is so common that the two-helper workaround is very tempting.

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

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?

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.

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

[Jeff King]

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

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

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

> 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.
And as a bonus, it makes configuration easier on the users (they don't
have to configure the cache helper separately, and they don't have to
set the timeout on it manually).

-Peff

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

[Jeff King]

On Sat, Apr 07, 2012 at 12:56:12AM -0400, Jeff King wrote:

> > So if I use the cache helper, and its set to expire at the default of
> > 15 minutes, I have to type my password in every 15 minutes, even if I
> > am doing a Git operation roughly every 8 minutes during a work day?
> 
> Yes. It's less convenient, but safer and more predictable (you put your
> password in at 2:30, it's gone at 2:45). Keep in mind that you can also
> bump the cache time. And like I said, if we do want have it behave the
> other way, that's OK, but it should be explicit (and it can be optional,
> even if it defaults to auto-refresh on use).

And here's what the optional version looks like:

diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c
index 390f194..1f801f7 100644
--- a/credential-cache--daemon.c
+++ b/credential-cache--daemon.c
@@ -136,6 +136,9 @@ static void serve_one_client(FILE *in, FILE *out)
 	else if (!strcmp(action.buf, "get")) {
 		struct credential_cache_entry *e = lookup_credential(&c);
 		if (e) {
+			int new_expiration = time(NULL) + timeout;
+			if (new_expiration > e->expiration)
+				e->expiration = new_expiration;
 			fprintf(out, "username=%s\n", e->item.username);
 			fprintf(out, "password=%s\n", e->item.password);
 		}
diff --git a/credential-cache.c b/credential-cache.c
index 9a03792..5751b48 100644
--- a/credential-cache.c
+++ b/credential-cache.c
@@ -87,6 +87,7 @@ int main(int argc, const char **argv)
 {
 	char *socket_path = NULL;
 	int timeout = 900;
+	int refresh = 0;
 	const char *op;
 	const char * const usage[] = {
 		"git credential-cache [options] <action>",
@@ -97,6 +98,8 @@ int main(int argc, const char **argv)
 			    "number of seconds to cache credentials"),
 		OPT_STRING(0, "socket", &socket_path, "path",
 			   "path of cache-daemon socket"),
+		OPT_BOOL(0, "refresh-on-use", &refresh,
+			   "refresh timestamp when credential is accessed"),
 		OPT_END()
 	};
 
@@ -112,7 +115,9 @@ int main(int argc, const char **argv)
 
 	if (!strcmp(op, "exit"))
 		do_cache(socket_path, op, timeout, 0);
-	else if (!strcmp(op, "get") || !strcmp(op, "erase"))
+	else if (!strcmp(op, "get"))
+		do_cache(socket_path, op, refresh ? timeout : 0, FLAG_RELAY);
+	else if(!strcmp(op, "erase"))
 		do_cache(socket_path, op, timeout, FLAG_RELAY);
 	else if (!strcmp(op, "store"))
 		do_cache(socket_path, op, timeout, FLAG_RELAY|FLAG_SPAWN);

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Apr 06, 2012 at 09:56:18PM -0700, Junio C Hamano wrote:
>
>> 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.
>
>> 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").
>
>> 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.

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

[Jeff King]

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

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

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

[Jeff King]

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

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

[Jeff King]

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