{"thread":{"id":"30177","subject":"[PATCH] credential: do not store credentials received from helpers","startedAt":"2012-04-07T03:34:17Z","lastAt":"2012-04-08T07:13:00Z","messageCount":10,"participants":["Jeff King","Shawn Pearce","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"188713","messageId":"20120407033417.GA13914@sigill.intra.peff.net","threadId":"30177","inReplyTo":null,"subject":"[PATCH] credential: do not store credentials received from helpers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-07T03:34:17Z","receivedAt":"2012-04-07T03:34:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The typical call flow for the credential API is something\nlike:\n\n  1. Network code (like http.c) wants a credential. It calls\n     credential_fill() to get it, and one of the two happens:\n\n     a. We call out to any helpers with a \"get\" request,\n\tone of which provides the credential.\n\n     b. No helper provides us the credential, so we ask the\n        user.\n\n  2. The network code uses the credential. Let's imagine\n     that the request completes successfully.\n\n  3. The network code informs the credential subsystem of\n     success by calling credential_approve(). The credential\n     code then calls out to any helpers with a \"store\"\n     request, so they can optionally store it if they want.\n\nIn the case of (1b), this is a good thing. The credential\ncomes from the user, gets used, and then gets put into\nstorage for later use again. But in the case of (1a), we end\nup feeding the helper with a credential that came from\nitself already (or possibly from another helper).\n\nMost of the time, this is harmless and slightly inefficient.\nBut it has two user-visible impacts:\n\n  1. If you have two helpers, you end up propagating data\n     between them. For instance, imagine you have a helper\n     \"git-credential-wallet\" which pulls data from a a\n     read-only password wallet. You might configure git like\n     this:\n\n       [credential]\n\t\thelper = wallet\n\t\thelper = cache\n\n     to check the wallet first, and then fall back to asking\n     the user and caching the result. But when we do get\n     something out of the wallet, git will then ask for it\n     to be stored in the wallet (which is a no-op), and the\n     cache. So your credential migrates from the wallet into\n     the cache, which may not be what you want (e.g.,\n     because the wallet implements more strict security\n     policies than the cache).\n\n  2. If you use a time-based storage helper like\n     \"git-credential-cache\", every time you run a git\n     command which uses the credential, it will also\n     re-insert the credential after use, freshening the\n     cache timestamp. So the cache will eventually expire N\n     time units after the last _use_, not after the time the\n     user actually typed the password. This is probably not\n     what most users expect or want (and if they do, we\n     should do it explicitly by providing an option to\n     refresh the timestamp on use).\n\nWe can solve this by marking a credential that comes from a\nhelper, so we don't bother feeding it back to the helpers.\nThe credential struct already has an \"approved\" flag so\nthat we try to store it only once, rather than for each\nsuccessful http request. We can use the same flag to\n\"pre-approve\" a credential which comes from a helper, and\nenver try to store it at all.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nWhew, that was a long explanation. Fortunately the patch text itself is\npleasantly simple.\n\n credential.c          |    4 +++-\n t/t5550-http-fetch.sh |    6 +++++-\n 2 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/credential.c b/credential.c\nindex 62d1c56..813e77a 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -272,8 +272,10 @@ void credential_fill(struct credential *c)\n \n \tfor (i = 0; i < c->helpers.nr; i++) {\n \t\tcredential_do(c, c->helpers.items[i].string, \"get\");\n-\t\tif (c->username && c->password)\n+\t\tif (c->username && c->password) {\n+\t\t\tc->approved = 1;\n \t\t\treturn;\n+\t\t}\n \t}\n \n \tcredential_getpass(c);\ndiff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh\nindex e5e6b8f..82e0d37 100755\n--- a/t/t5550-http-fetch.sh\n+++ b/t/t5550-http-fetch.sh\n@@ -104,13 +104,17 @@ test_expect_success 'http auth can request both user and pass' '\n test_expect_success 'http auth respects credential helper config' '\n \ttest_config_global credential.helper \"!f() {\n \t\tcat >/dev/null\n+\t\techo helper: \\$* >>helper-trace\n \t\techo username=user@host\n \t\techo password=user@host\n \t}; f\" &&\n+\t>helper-trace &&\n \t>askpass-query &&\n \techo wrong >askpass-response &&\n \tgit clone \"$HTTPD_URL/auth/repo.git\" clone-auth-helper &&\n-\texpect_askpass none\n+\texpect_askpass none &&\n+\techo \"helper: get\" >helper-expect &&\n+\ttest_cmp helper-expect helper-trace\n '\n \n test_expect_success 'http auth can get username from config' '\n-- \n1.7.10.rc4.31.g355de\n"},{"id":"188714","messageId":"CAJo=hJvqQ0CgCga4va3ZX+XV5DWc1kWS5v4vYWkEzRYT5+p+cg@mail.gmail.com","threadId":"30177","inReplyTo":"20120407033417.GA13914@sigill.intra.peff.net","subject":"Re: [PATCH] credential: do not store credentials received from helpers","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2012-04-07T04:12:39Z","receivedAt":"2012-04-07T04:12:39Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Fri, Apr 6, 2012 at 20:34, Jeff King <peff@peff.net> wrote:\n>  2. If you use a time-based storage helper like\n>     \"git-credential-cache\", every time you run a git\n>     command which uses the credential, it will also\n>     re-insert the credential after use, freshening the\n>     cache timestamp. So the cache will eventually expire N\n>     time units after the last _use_, not after the time the\n>     user actually typed the password. This is probably not\n>     what most users expect or want (and if they do, we\n>     should do it explicitly by providing an option to\n>     refresh the timestamp on use).\n\nSo if I use the cache helper, and its set to expire at the default of\n15 minutes, I have to type my password in every 15 minutes, even if I\nam doing a Git operation roughly every 8 minutes during a work day?\n\n> We can solve this by marking a credential that comes from a\n> helper, so we don't bother feeding it back to the helpers.\n> The credential struct already has an \"approved\" flag so\n> that we try to store it only once, rather than for each\n> successful http request. We can use the same flag to\n> \"pre-approve\" a credential which comes from a helper, and\n> enver try to store it at all.\n\nThis breaks one of my credential helpers.\n\nI have a helper that generates a password by asking a remote system to\ngenerate a short lived password based on other authentication systems\nthat I can't describe. Once I have that password, its good for $X\ntime.\n\nThe helper just dumps it out to Git, and Git turns around and stores\nit into the cache for me. This means later requests will keep that\ncredential in the cache, and avoid making that remote system call\nevery time I do a Git network command. I guess I now need to change my\nhelper to cache git credential-cache itself and store the password\ninto the cache if it wants to use the cache?\n\nShould we update the credential helper documentation at the same time\nas this change to make it clear Git won't cache passwords returned\nfrom helpers, but a helper could call the credential-cache itself if\nit wanted to reuse the existing cache service?\n"},{"id":"188716","messageId":"20120407045612.GA965@sigill.intra.peff.net","threadId":"30177","inReplyTo":"CAJo=hJvqQ0CgCga4va3ZX+XV5DWc1kWS5v4vYWkEzRYT5+p+cg@mail.gmail.com","subject":"Re: [PATCH] credential: do not store credentials received from helpers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-07T04:56:12Z","receivedAt":"2012-04-07T04:56:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 06, 2012 at 09:12:39PM -0700, Shawn O. Pearce wrote:\n\n> On Fri, Apr 6, 2012 at 20:34, Jeff King <peff@peff.net> wrote:\n> >  2. If you use a time-based storage helper like\n> >     \"git-credential-cache\", every time you run a git\n> >     command which uses the credential, it will also\n> >     re-insert the credential after use, freshening the\n> >     cache timestamp. So the cache will eventually expire N\n> >     time units after the last _use_, not after the time the\n> >     user actually typed the password. This is probably not\n> >     what most users expect or want (and if they do, we\n> >     should do it explicitly by providing an option to\n> >     refresh the timestamp on use).\n> \n> So if I use the cache helper, and its set to expire at the default of\n> 15 minutes, I have to type my password in every 15 minutes, even if I\n> am doing a Git operation roughly every 8 minutes during a work day?\n\nYes. It's less convenient, but safer and more predictable (you put your\npassword in at 2:30, it's gone at 2:45). Keep in mind that you can also\nbump the cache time. And like I said, if we do want have it behave the\nother way, that's OK, but it should be explicit (and it can be optional,\neven if it defaults to auto-refresh on use).\n\nOr you could even do something more complex. gpg-agent will refresh the\ntimestamp on use, but still has a \"max ttl\" that drops a credential N\nseconds after it was input, even if it was used recently. But making any\ndecision like that means that the daemon needs to actually know whether\nthe timestamp came from the user, or whether it was simply accessed and\nregurgitated to us.\n\n> This breaks one of my credential helpers.\n> \n> I have a helper that generates a password by asking a remote system to\n> generate a short lived password based on other authentication systems\n> that I can't describe. Once I have that password, its good for $X\n> time.\n> \n> The helper just dumps it out to Git, and Git turns around and stores\n> it into the cache for me. This means later requests will keep that\n> credential in the cache, and avoid making that remote system call\n> every time I do a Git network command.\n\nI considered your use case when writing the patch and thought \"no,\nthat's too insane. Nobody would want that.\" But leave it to you to prove\nme wrong. :)\n\nI'm torn. What you are doing is totally reasonable for your case. At the\nsame time, having it happen without the user's knowledge could have\nsurprising security implications, because you're leaking passwords from\none helper to the other (in the example in the commit message, I\nmentioned a high-security helper followed by \"cache\". Which is probably\nnot that bad. But imagine if it were \"store\").\n\n> I guess I now need to change my helper to cache git credential-cache\n> itself and store the password into the cache if it wants to use the\n> cache?\n\nThat's one option, though it is a little bit of a pain to implement (not\nhard, but not one line of code). It would be very easy to add a\n\"credential.storeHelperCredentials\" option that defaulted to off (that\nname is horrible, but you get the point).\n\n> Should we update the credential helper documentation at the same time\n> as this change to make it clear Git won't cache passwords returned\n> from helpers, but a helper could call the credential-cache itself if\n> it wanted to reuse the existing cache service?\n\nYeah, I'll add a documentation update when I re-roll; but we first need\nto figure out what the recommended course of action is.\n\nOne thing I don't like about having your helper call credential-cache is\nthat it is a layering violation; it is only guessing that putting the\ndata into credential-cache will be useful in the first place. But that\ndepends on the user's config. It could have be using an alternative\ncaching helper (or none at all).\n\nSo I think an explicit \"it's OK to leak credentials between helpers\"\nflag is our best bet. And it's dirt simple to code.\n\n-Peff\n"},{"id":"188717","messageId":"7v398gywb1.fsf@alter.siamese.dyndns.org","threadId":"30177","inReplyTo":"CAJo=hJvqQ0CgCga4va3ZX+XV5DWc1kWS5v4vYWkEzRYT5+p+cg@mail.gmail.com","subject":"Re: [PATCH] credential: do not store credentials received from helpers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-07T04:56:18Z","receivedAt":"2012-04-07T04:56:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shawn Pearce <spearce@spearce.org> writes:\n\n> On Fri, Apr 6, 2012 at 20:34, Jeff King <peff@peff.net> wrote:\n>>  2. If you use a time-based storage helper like\n>>     \"git-credential-cache\", every time you run a git\n>>     command which uses the credential, it will also\n>>     re-insert the credential after use, freshening the\n>>     cache timestamp. So the cache will eventually expire N\n>>     time units after the last _use_, not after the time the\n>>     user actually typed the password. This is probably not\n>>     what most users expect or want (and if they do, we\n>>     should do it explicitly by providing an option to\n>>     refresh the timestamp on use).\n>\n> So if I use the cache helper, and its set to expire at the default of\n> 15 minutes, I have to type my password in every 15 minutes, even if I\n> am doing a Git operation roughly every 8 minutes during a work day?\n\nI think what Peff meant was to set the expiration time of the cache helper\na lot longer (say 20 hours, as opposed to 15 minutes) but once it learns a\ncredential material from your helper, leave the timer running without\nresetting it every time the user uses the credential (your 8 minutes).\n\nSo I do not see a fundamental problem in this part.\n\n>> We can solve this by marking a credential that comes from a\n>> helper, so we don't bother feeding it back to the helpers.\n>> The credential struct already has an \"approved\" flag so\n>> that we try to store it only once, rather than for each\n>> successful http request. We can use the same flag to\n>> \"pre-approve\" a credential which comes from a helper, and\n>> enver try to store it at all.\n>\n> This breaks one of my credential helpers.\n>\n> I have a helper that generates a password by asking a remote system to\n> generate a short lived password based on other authentication systems\n> that I can't describe. Once I have that password, its good for $X\n> time.\n\nWell, $X is something your helper knows but other helpers don't, so...\n\n> The helper just dumps it out to Git, and Git turns around and stores\n> it into the cache for me. This means later requests will keep that\n> credential in the cache, and avoid making that remote system call\n> every time I do a Git network command. I guess I now need to change my\n> helper to cache git credential-cache itself and store the password\n> into the cache if it wants to use the cache?\n\n... logically the knowledge of cache expiration time belongs to your\nhelper, but \"after the first use, it is OK to keep using it for N hours\"\nis so common that the two-helper workaround is very tempting.\n\nI am afraid that \"do not trigger the cache helper\" might be throwing the\nbaby with bathwater to solve the real problem the patch tries to address,\nwhich is:\n\nPeff>   2. If you use a time-based storage helper like\nPeff>      \"git-credential-cache\", every time you run a git\nPeff>      command which uses the credential, it will also\nPeff>      re-insert the credential after use, freshening the\nPeff>      cache timestamp. So the cache will eventually expire N\nPeff>      time units after the last _use_, not after the time the\nPeff>      user actually typed the password. This is probably not\n\nShouldn't the memory cache based helper already have enough clue to tell\nwhen a new entry is first inserted vs when the existing entry it supplied\ncame back from the network layer after use?  If there is not enough clue\nwith the current network-layer-to-helper protocol, then wouldn't it be a\nbetter approach to add that, so that the memory cache helper can make more\nintelligent management of its timer?\n\nOnce that is fixed, I would imagine that you can tell your users to use\ntwo helpers (yours and generic caching one) and configure them so that (1)\nthe caching one is asked first and then fall back to ask yours, and (2)\nthe expiration time of the caching one is set close to $X.\n"},{"id":"188719","messageId":"20120407050913.GA4211@sigill.intra.peff.net","threadId":"30177","inReplyTo":"7v398gywb1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] credential: do not store credentials received from helpers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-07T05:09:13Z","receivedAt":"2012-04-07T05:09:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 06, 2012 at 09:56:18PM -0700, Junio C Hamano wrote:\n\n> I am afraid that \"do not trigger the cache helper\" might be throwing the\n> baby with bathwater to solve the real problem the patch tries to address,\n> which is:\n> \n> Peff>   2. If you use a time-based storage helper like\n> Peff>      \"git-credential-cache\", every time you run a git\n> Peff>      command which uses the credential, it will also\n> Peff>      re-insert the credential after use, freshening the\n> Peff>      cache timestamp. So the cache will eventually expire N\n> Peff>      time units after the last _use_, not after the time the\n> Peff>      user actually typed the password. This is probably not\n\nActually, that was not the real problem. The real problem I had was the\nleakage between helpers. I just noticed this one while thinking about\nit. So the very thing that is useful to Shawn is also potentially\ndangerous to people who are doing something less clever.\n\n> Shouldn't the memory cache based helper already have enough clue to tell\n> when a new entry is first inserted vs when the existing entry it supplied\n> came back from the network layer after use?  If there is not enough clue\n> with the current network-layer-to-helper protocol, then wouldn't it be a\n> better approach to add that, so that the memory cache helper can make more\n> intelligent management of its timer?\n\nYou can approximate it. The daemon sees that somebody is inserting the\nsame thing that it already there, and can guess that it probably came\nfrom the daemon in the first place. There are some corner cases with\nexpiration boundaries (we get the credential, the daemon expires it,\nthen the http request succeeds, and we tell the daemon \"hey, store\nthis\").\n\n> Once that is fixed, I would imagine that you can tell your users to use \n> two helpers (yours and generic caching one) and configure them so that (1)\n> the caching one is asked first and then fall back to ask yours, and (2)\n> the expiration time of the caching one is set close to $X.\n\nYeah, in my other response to Shawn, I mentioned that we could add a\nflag to do the \"leaking\" behavior if that's what the user wants. But it\nwould have the side effect of refreshing his timestamp on each use, so\nhis $X would not expire (although that is also the case now, and he\nhasn't complained).\n\nSo I actually do think he would be better to implement the caching\ninside his helper, even if it is by calling out to git-credential-cache.\nAnd as a bonus, it makes configuration easier on the users (they don't\nhave to configure the cache helper separately, and they don't have to\nset the timeout on it manually).\n\n-Peff\n"},{"id":"188720","messageId":"20120407052110.GA12164@sigill.intra.peff.net","threadId":"30177","inReplyTo":"20120407045612.GA965@sigill.intra.peff.net","subject":"Re: [PATCH] credential: do not store credentials received from helpers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-07T05:21:10Z","receivedAt":"2012-04-07T05:21:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 07, 2012 at 12:56:12AM -0400, Jeff King wrote:\n\n> > So if I use the cache helper, and its set to expire at the default of\n> > 15 minutes, I have to type my password in every 15 minutes, even if I\n> > am doing a Git operation roughly every 8 minutes during a work day?\n> \n> Yes. It's less convenient, but safer and more predictable (you put your\n> password in at 2:30, it's gone at 2:45). Keep in mind that you can also\n> bump the cache time. And like I said, if we do want have it behave the\n> other way, that's OK, but it should be explicit (and it can be optional,\n> even if it defaults to auto-refresh on use).\n\nAnd here's what the optional version looks like:\n\ndiff --git a/credential-cache--daemon.c b/credential-cache--daemon.c\nindex 390f194..1f801f7 100644\n--- a/credential-cache--daemon.c\n+++ b/credential-cache--daemon.c\n@@ -136,6 +136,9 @@ static void serve_one_client(FILE *in, FILE *out)\n \telse if (!strcmp(action.buf, \"get\")) {\n \t\tstruct credential_cache_entry *e = lookup_credential(&c);\n \t\tif (e) {\n+\t\t\tint new_expiration = time(NULL) + timeout;\n+\t\t\tif (new_expiration > e->expiration)\n+\t\t\t\te->expiration = new_expiration;\n \t\t\tfprintf(out, \"username=%s\\n\", e->item.username);\n \t\t\tfprintf(out, \"password=%s\\n\", e->item.password);\n \t\t}\ndiff --git a/credential-cache.c b/credential-cache.c\nindex 9a03792..5751b48 100644\n--- a/credential-cache.c\n+++ b/credential-cache.c\n@@ -87,6 +87,7 @@ int main(int argc, const char **argv)\n {\n \tchar *socket_path = NULL;\n \tint timeout = 900;\n+\tint refresh = 0;\n \tconst char *op;\n \tconst char * const usage[] = {\n \t\t\"git credential-cache [options] <action>\",\n@@ -97,6 +98,8 @@ int main(int argc, const char **argv)\n \t\t\t    \"number of seconds to cache credentials\"),\n \t\tOPT_STRING(0, \"socket\", &socket_path, \"path\",\n \t\t\t   \"path of cache-daemon socket\"),\n+\t\tOPT_BOOL(0, \"refresh-on-use\", &refresh,\n+\t\t\t   \"refresh timestamp when credential is accessed\"),\n \t\tOPT_END()\n \t};\n \n@@ -112,7 +115,9 @@ int main(int argc, const char **argv)\n \n \tif (!strcmp(op, \"exit\"))\n \t\tdo_cache(socket_path, op, timeout, 0);\n-\telse if (!strcmp(op, \"get\") || !strcmp(op, \"erase\"))\n+\telse if (!strcmp(op, \"get\"))\n+\t\tdo_cache(socket_path, op, refresh ? timeout : 0, FLAG_RELAY);\n+\telse if(!strcmp(op, \"erase\"))\n \t\tdo_cache(socket_path, op, timeout, FLAG_RELAY);\n \telse if (!strcmp(op, \"store\"))\n \t\tdo_cache(socket_path, op, timeout, FLAG_RELAY|FLAG_SPAWN);\n"},{"id":"188761","messageId":"7vaa2myfsz.fsf@alter.siamese.dyndns.org","threadId":"30177","inReplyTo":"20120407050913.GA4211@sigill.intra.peff.net","subject":"Re: [PATCH] credential: do not store credentials received from helpers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-08T05:05:00Z","receivedAt":"2012-04-08T05:05:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Apr 06, 2012 at 09:56:18PM -0700, Junio C Hamano wrote:\n>\n>> I am afraid that \"do not trigger the cache helper\" might be throwing the\n>> baby with bathwater to solve the real problem the patch tries to address,\n>> which is:\n>> \n>> Peff>   2. If you use a time-based storage helper like\n>> Peff>      \"git-credential-cache\", every time you run a git\n>> Peff>      command which uses the credential, it will also\n>> Peff>      re-insert the credential after use, freshening the\n>> Peff>      cache timestamp. So the cache will eventually expire N\n>> Peff>      time units after the last _use_, not after the time the\n>> Peff>      user actually typed the password. This is probably not\n>\n> Actually, that was not the real problem. The real problem I had was the\n> leakage between helpers. I just noticed this one while thinking about\n> it. So the very thing that is useful to Shawn is also potentially\n> dangerous to people who are doing something less clever.\n>\n>> Shouldn't the memory cache based helper already have enough clue to tell\n>> when a new entry is first inserted vs when the existing entry it supplied\n>> came back from the network layer after use?  If there is not enough clue\n>> with the current network-layer-to-helper protocol, then wouldn't it be a\n>> better approach to add that, so that the memory cache helper can make more\n>> intelligent management of its timer?\n>\n> You can approximate it. The daemon sees that somebody is inserting the\n> same thing that it already there, and can guess that it probably came\n> from the daemon in the first place. There are some corner cases with\n> expiration boundaries (we get the credential, the daemon expires it,\n> then the http request succeeds, and we tell the daemon \"hey, store\n> this\").\n>\n>> Once that is fixed, I would imagine that you can tell your users to use \n>> two helpers (yours and generic caching one) and configure them so that (1)\n>> the caching one is asked first and then fall back to ask yours, and (2)\n>> the expiration time of the caching one is set close to $X.\n>\n> Yeah, in my other response to Shawn, I mentioned that we could add a\n> flag to do the \"leaking\" behavior if that's what the user wants. But it\n> would have the side effect of refreshing his timestamp on each use, so\n> his $X would not expire (although that is also the case now, and he\n> hasn't complained).\n>\n> So I actually do think he would be better to implement the caching\n> inside his helper, even if it is by calling out to git-credential-cache.\n\nWhile I'm for keeping the interface simple, at the same time, \"I have this\ncredential obtained and it is valid for $X time duration\" sounds like a\nvery common thing, and it is somewhat a shame for the API to force its\nusers (i.e. helpers) to reimplement the caching logic over and over.\n\nThe network layer (i.e. the one that gets the authentication material from\nhelpers and uses it to talk with a remote git service on the other end)\ncould be thought of as a proxy server that gets information from its true\nsource (i.e. helpers) and relays it to the browser (i.e. the remote git\nservice). In that context, it would be natural for the API to allow the\nsource of the truth (i.e. the helpers) to pass its information to the\nproxy with validity duration, so that the proxy can handle the expiration\nin a way hidden from the information sources and the browser.  That proxy\n(i.e. the network layer) could use git-credential-cache as a non-volatile\nmemory but that will become an implementation detail hidden from both the\nremote service and the helpers.\n\nSo,... I dunno.\n"},{"id":"188764","messageId":"20120408064059.GA6727@sigill.intra.peff.net","threadId":"30177","inReplyTo":"7vaa2myfsz.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] credential: do not store credentials received from helpers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-08T06:40:59Z","receivedAt":"2012-04-08T06:40:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 07, 2012 at 10:05:00PM -0700, Junio C Hamano wrote:\n\n> > So I actually do think he would be better to implement the caching\n> > inside his helper, even if it is by calling out to git-credential-cache.\n> \n> While I'm for keeping the interface simple, at the same time, \"I have this\n> credential obtained and it is valid for $X time duration\" sounds like a\n> very common thing, and it is somewhat a shame for the API to force its\n> users (i.e. helpers) to reimplement the caching logic over and over.\n\nBut the caching _must_ be part of a helper, because the parent git\nprocess is not long-lived. If I understand you correctly, you are\nadvocating adding a \"ttl\" field, then transparently chaining to the\ncache helper when it exists.\n\nWe can do that, but the whole point of implementing a generic helper\nprotocol was to let people decide to do things like that themselves\n(which is exactly what Shawn has done). There's no reason that git needs\nto lock you into one particular implementation of caching or storage;\nthat's why we have configurable helpers.  There's no reason that git\nshould only cache when there is a ttl field (you still may want to cache\nfor 15 minutes, even if the credential will last longer). But nor should\ngit say \"you always must cache\".  Perhaps you don't like the complexity\nor the security implications.\n\nSo I'd much rather leave it up to the user to configure their helpers\nrather than trying to run credential-cache behind the scenes.\n\nThere are basically two problems to solve. The first is getting data\nfrom one helper to the other.\n\nOne way to implement that is by just wrapping the real helper inside a\ncaching layer. That can even be generic. In fact, my initial version of\nthe credential helpers had a \"chain\" parameter, so you could ask\ncredential-cache to run a sub-helper and cache its response.\nThe original use was that \"getpass\" would be its own helper, so you\ncould plug in a custom version. Since nobody seemed to want to do that,\nI dropped it in the name of simplicity.\n\nYou can easily write a generic wrapper like this:\n\n  #!/bin/sh\n  cache=$1\n  chain=$2\n  case \"$3\" in\n  get)\n          cat >request\n          eval \"$cache get\" <request >response\n          if test -s response; then\n            cat response\n          else\n            eval \"$chain\" >response\n            cat request response | eval \"$cache store\"\n            cat response\n          fi\n          ;;\n  *)\n          eval \"$cache $3\"\n          ;;\n  esac\n\nthough obviously you would write it in a real language that doesn't\ninvolve storing the intermediate states on disk (and rather than eval,\nyou'd take the usual helper specification). And we could easily provide\nsuch a wrapper, and Shawn's config would look like:\n\n  [credential]\n  helper = \"wrapper cache real-helper\"\n\nAnother way to do it is by listing the helpers sequentially, and feeding\nthe output of one to the input of another during a \"store\". As I said,\nI'm uncomfortable with the security implications of doing that by\ndefault. Your proposal side steps it by implying that cache is special,\nand does not have these security implications.  Which is mostly true\n(though it also is limiting). So the obvious thing would be to mark\nhelpers as either \"yes, it's OK to share my output with others\" or \"yes,\nit's OK for me to see the output of others\". And then Shawn's config\nlooks like:\n\n  [credential]\n  helper = cache\n  helper = real-helper\n\nThe second issue is that of communicating the ttl or expiration between\nhelpers. That's easy enough. The protocol allows arbitrary key/value\npairs. We typically just drop ones we don't care about, but we could\nretain them and pass them along.\n\n-Peff\n"},{"id":"188765","messageId":"20120408070751.GA13662@sigill.intra.peff.net","threadId":"30177","inReplyTo":"20120408064059.GA6727@sigill.intra.peff.net","subject":"Re: [PATCH] credential: do not store credentials received from helpers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-08T07:07:51Z","receivedAt":"2012-04-08T07:07:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 08, 2012 at 02:40:59AM -0400, Jeff King wrote:\n\n> One way to implement that is by just wrapping the real helper inside a\n> caching layer. That can even be generic.\n\nHere's a C implementation of the shell sketch I posted earlier.\nObviously missing documentation, and only lightly tested, but just to\ngive a sense of what it would look like. You can exercise it manually\nwith:\n\n  {\n    # simulate git's input\n    echo protocol=https\n    echo host=example.com\n  } |\n  git credential-wrap cache '!f() {\n    # note whether we ran or not\n    echo >&2 Generating...\n    # and simulate output\n    echo username=fake.username\n    echo password=fake.password\n  }; f' get\n\nor configure it with:\n\n  git config credential.helper 'wrap cache your-real-helper'\n\n---\n Makefile          |    1 +\n credential-wrap.c |   32 ++++++++++++++++++++++++++++++++\n credential.c      |    4 ++--\n credential.h      |    3 +++\n 4 files changed, 38 insertions(+), 2 deletions(-)\n create mode 100644 credential-wrap.c\n\ndiff --git a/Makefile b/Makefile\nindex be1957a..c91bb23 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -463,6 +463,7 @@ PROGRAM_OBJS += upload-pack.o\n PROGRAM_OBJS += http-backend.o\n PROGRAM_OBJS += sh-i18n--envsubst.o\n PROGRAM_OBJS += credential-store.o\n+PROGRAM_OBJS += credential-wrap.o\n \n # Binary suffix, set to .exe for Windows builds\n X =\ndiff --git a/credential-wrap.c b/credential-wrap.c\nnew file mode 100644\nindex 0000000..f4aadc4\n--- /dev/null\n+++ b/credential-wrap.c\n@@ -0,0 +1,32 @@\n+#include \"cache.h\"\n+#include \"credential.h\"\n+\n+int main(int argc, const char **argv)\n+{\n+\tstruct credential c = CREDENTIAL_INIT;\n+\tconst char *storage, *source, *action;\n+\n+\tif (argc != 4)\n+\t\tusage(\"git credential-wrap <storage> <source> <action>\");\n+\tstorage = argv[1];\n+\tsource = argv[2];\n+\taction = argv[3];\n+\n+\tif (credential_read(&c, stdin) < 0)\n+\t\tdie(\"unable to read input credential\");\n+\n+\tif (!strcmp(action, \"get\")) {\n+\t\tcredential_do(&c, storage, \"get\");\n+\t\tif (!c.username || !c.password) {\n+\t\t\tcredential_do(&c, source, \"get\");\n+\t\t\tif (!c.username || !c.password)\n+\t\t\t\treturn 0;\n+\t\t\tcredential_do(&c, storage, \"store\");\n+\t\t}\n+\t\tcredential_write(&c, stdout);\n+\t}\n+\telse\n+\t\tcredential_do(&c, storage, action);\n+\n+\treturn 0;\n+}\ndiff --git a/credential.c b/credential.c\nindex 813e77a..13409e1 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -191,7 +191,7 @@ static void credential_write_item(FILE *fp, const char *key, const char *value)\n \tfprintf(fp, \"%s=%s\\n\", key, value);\n }\n \n-static void credential_write(const struct credential *c, FILE *fp)\n+void credential_write(const struct credential *c, FILE *fp)\n {\n \tcredential_write_item(fp, \"protocol\", c->protocol);\n \tcredential_write_item(fp, \"host\", c->host);\n@@ -241,7 +241,7 @@ static int run_credential_helper(struct credential *c,\n \treturn 0;\n }\n \n-static int credential_do(struct credential *c, const char *helper,\n+int credential_do(struct credential *c, const char *helper,\n \t\t\t const char *operation)\n {\n \tstruct strbuf cmd = STRBUF_INIT;\ndiff --git a/credential.h b/credential.h\nindex 96ea41b..daf3e81 100644\n--- a/credential.h\n+++ b/credential.h\n@@ -30,4 +30,7 @@ void credential_from_url(struct credential *, const char *url);\n int credential_match(const struct credential *have,\n \t\t     const struct credential *want);\n \n+int credential_do(struct credential *, const char *helper, const char *action);\n+void credential_write(const struct credential *, FILE *);\n+\n #endif /* CREDENTIAL_H */\n-- \n1.7.10.11.g901cee\n"},{"id":"188766","messageId":"20120408071300.GB13662@sigill.intra.peff.net","threadId":"30177","inReplyTo":"20120408064059.GA6727@sigill.intra.peff.net","subject":"Re: [PATCH] credential: do not store credentials received from helpers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-08T07:13:00Z","receivedAt":"2012-04-08T07:13:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 08, 2012 at 02:40:59AM -0400, Jeff King wrote:\n\n> The second issue is that of communicating the ttl or expiration between\n> helpers. That's easy enough. The protocol allows arbitrary key/value\n> pairs. We typically just drop ones we don't care about, but we could\n> retain them and pass them along.\n\nAnd here's a rough patch for that. This is just to get an idea of the\nscale, and which parts of the code need changed. I'd probably use a\nkey/value store instead of a string_list. On top of this,\ncredential-cache would have to learn to respect a TTL variable in the\ninput (actually, it does already respect \"timeout\" which is added on the\nway from the cache client to the cache daemon, but the parsing around\nthat would have to be cleaned up a bit).\n\n---\n credential.c |   14 +++++++++++---\n credential.h |    3 ++-\n 2 files changed, 13 insertions(+), 4 deletions(-)\n\ndiff --git a/credential.c b/credential.c\nindex 13409e1..2237e7e 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -9,6 +9,7 @@ void credential_init(struct credential *c)\n {\n \tmemset(c, 0, sizeof(*c));\n \tc->helpers.strdup_strings = 1;\n+\tc->extra.strdup_strings = 1;\n }\n \n void credential_clear(struct credential *c)\n@@ -19,6 +20,7 @@ void credential_clear(struct credential *c)\n \tfree(c->username);\n \tfree(c->password);\n \tstring_list_clear(&c->helpers, 0);\n+\tstring_list_clear(&c->extra, 0);\n \n \tcredential_init(c);\n }\n@@ -174,10 +176,11 @@ int credential_read(struct credential *c, FILE *fp)\n \t\t\tc->path = xstrdup(value);\n \t\t}\n \t\t/*\n-\t\t * Ignore other lines; we don't know what they mean, but\n-\t\t * this future-proofs us when later versions of git do\n-\t\t * learn new lines, and the helpers are updated to match.\n+\t\t * Save other lines so they can be fed back to the helper or\n+\t\t * transported to other helpers.\n \t\t */\n+\t\t*(value-1) = '=';\n+\t\tstring_list_append(&c->extra, line.buf);\n \t}\n \n \tstrbuf_release(&line);\n@@ -193,11 +196,16 @@ static void credential_write_item(FILE *fp, const char *key, const char *value)\n \n void credential_write(const struct credential *c, FILE *fp)\n {\n+\tint i;\n+\n \tcredential_write_item(fp, \"protocol\", c->protocol);\n \tcredential_write_item(fp, \"host\", c->host);\n \tcredential_write_item(fp, \"path\", c->path);\n \tcredential_write_item(fp, \"username\", c->username);\n \tcredential_write_item(fp, \"password\", c->password);\n+\n+\tfor (i = 0; i < c->extra.nr; i++)\n+\t\tfprintf(fp, \"%s\\n\", c->extra.items[i].string);\n }\n \n static int run_credential_helper(struct credential *c,\ndiff --git a/credential.h b/credential.h\nindex daf3e81..5f98527 100644\n--- a/credential.h\n+++ b/credential.h\n@@ -5,6 +5,7 @@\n \n struct credential {\n \tstruct string_list helpers;\n+\tstruct string_list extra;\n \tunsigned approved:1,\n \t\t configured:1,\n \t\t use_http_path:1;\n@@ -16,7 +17,7 @@ struct credential {\n \tchar *path;\n };\n \n-#define CREDENTIAL_INIT { STRING_LIST_INIT_DUP }\n+#define CREDENTIAL_INIT { STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP }\n \n void credential_init(struct credential *);\n void credential_clear(struct credential *);\n-- \n1.7.10.11.g901cee\n"}]}