{"thread":{"id":"51440","subject":"[PATCH] credential: add nocache option to the credentials API","startedAt":"2019-07-07T05:51:38Z","lastAt":"2019-09-15T21:51:16Z","messageCount":6,"participants":["Masaya Suzuki","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"378663","messageId":"20190707055132.103736-1-masayasuzuki@google.com","threadId":"51440","inReplyTo":null,"subject":"[PATCH] credential: add nocache option to the credentials API","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2019-07-07T05:51:32Z","receivedAt":"2019-07-07T05:51:38Z","isPatch":true,"sender":{"key":"masayasuzuki@google.com","avatar":"https://avatars.githubusercontent.com/u/21878?v=4"},"body":"The credentials API calls credentials helpers in order. If a\nusername/password pair is returned the helpers and if it's used for\nauthentication successfully, it's announced to the helpers and they can\nstore it for later use.\n\nSome credentials are valid only for the limited time and should not be\ncached. In this case, because the credential is announced to all helpers\nand they can independently decide whether they will cache it or not,\nthose short-lived credentials can be cached.\n\nThis change adds an option that a credential helper can specify that the\ncredential returned by the helper should not be cached. If this is\nspecified, even after the credential is used successfully, it won't be\nannounced to other helpers for store.\n\nSigned-off-by: Masaya Suzuki <masayasuzuki@google.com>\n---\n Documentation/technical/api-credentials.txt | 4 +++-\n credential.c                                | 4 +++-\n credential.h                                | 3 ++-\n t/t0300-credentials.sh                      | 9 +++++++++\n 4 files changed, 17 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/technical/api-credentials.txt b/Documentation/technical/api-credentials.txt\nindex 75368f26ca..3db5841b40 100644\n--- a/Documentation/technical/api-credentials.txt\n+++ b/Documentation/technical/api-credentials.txt\n@@ -251,7 +251,9 @@ even no values at all if it has nothing useful to provide. Any provided\n attributes will overwrite those already known about by Git.  If a helper\n outputs a `quit` attribute with a value of `true` or `1`, no further\n helpers will be consulted, nor will the user be prompted (if no\n-credential has been provided, the operation will then fail).\n+credential has been provided, the operation will then fail). If a helper outputs\n+a `nocache` attribute with a value of `true` or `1`, `credential_approve` will\n+not be called even after the credential is used for authentication sucessfully.\n \n For a `store` or `erase` operation, the helper's output is ignored.\n If it fails to perform the requested operation, it may complain to\ndiff --git a/credential.c b/credential.c\nindex 62be651b03..db7b351447 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -179,6 +179,8 @@ int credential_read(struct credential *c, FILE *fp)\n \t\t\tcredential_from_url(c, value);\n \t\t} else if (!strcmp(key, \"quit\")) {\n \t\t\tc->quit = !!git_config_bool(\"quit\", value);\n+\t\t} else if (!strcmp(key, \"nocache\")) {\n+\t\t\tc->no_cache= !!git_config_bool(\"nocache\", value);\n \t\t}\n \t\t/*\n \t\t * Ignore other lines; we don't know what they mean, but\n@@ -296,7 +298,7 @@ void credential_approve(struct credential *c)\n {\n \tint i;\n \n-\tif (c->approved)\n+\tif (c->approved || c->no_cache)\n \t\treturn;\n \tif (!c->username || !c->password)\n \t\treturn;\ndiff --git a/credential.h b/credential.h\nindex 6b0cd16be2..be0f35d841 100644\n--- a/credential.h\n+++ b/credential.h\n@@ -8,7 +8,8 @@ struct credential {\n \tunsigned approved:1,\n \t\t configured:1,\n \t\t quit:1,\n-\t\t use_http_path:1;\n+\t\t use_http_path:1,\n+\t\t no_cache:1;\n \n \tchar *username;\n \tchar *password;\ndiff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\nindex 82eaaea0f4..ad06f6fe11 100755\n--- a/t/t0300-credentials.sh\n+++ b/t/t0300-credentials.sh\n@@ -118,6 +118,15 @@ test_expect_success 'do not bother storing password-less credential' '\n \tEOF\n '\n \n+test_expect_success 'credential_approve does not call helpers for nocache' '\n+\tcheck approve useless <<-\\EOF\n+\tusername=foo\n+\tpassword=bar\n+\tnocache=1\n+\t--\n+\t--\n+\tEOF\n+'\n \n test_expect_success 'credential_reject calls all helpers' '\n \tcheck reject useless \"verbatim one two\" <<-\\EOF\n-- \n2.22.0.410.gd8fdbe21b5-goog\n\n"},{"id":"378721","messageId":"20190709125620.GA18175@sigill.intra.peff.net","threadId":"51440","inReplyTo":"20190707055132.103736-1-masayasuzuki@google.com","subject":"Re: [PATCH] credential: add nocache option to the credentials API","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-09T12:56:20Z","receivedAt":"2019-07-09T12:56:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 06, 2019 at 10:51:32PM -0700, Masaya Suzuki wrote:\n\n> The credentials API calls credentials helpers in order. If a\n> username/password pair is returned the helpers and if it's used for\n> authentication successfully, it's announced to the helpers and they can\n> store it for later use.\n> \n> Some credentials are valid only for the limited time and should not be\n> cached. In this case, because the credential is announced to all helpers\n> and they can independently decide whether they will cache it or not,\n> those short-lived credentials can be cached.\n> \n> This change adds an option that a credential helper can specify that the\n> credential returned by the helper should not be cached. If this is\n> specified, even after the credential is used successfully, it won't be\n> announced to other helpers for store.\n\nI think this makes sense to do, though note that there's an old\ndiscussion which covers some alternatives:\n\n  https://public-inbox.org/git/20120407033417.GA13914@sigill.intra.peff.net/\n\nIn that patch, I essentially proposed making all gathered credentials as\nnocache. That's a more secure default (though in some cases less\nconvenient).\n\nIt did break a case Shawn had of caching the result of another helper. I\nshowed some options there for providing a mechanism to chain helpers\ntogether explicitly.\n\nWe also discussed helpers passing out an explicit ttl. That's a more\ngeneral case of your nocache flag (i.e., ttl=0 covers that case, but we\ncould additionally pass \"ttl\" to the cache helper to let it be smarter).\n\nGiven the age of that discussion and the fact that nobody has really\ncomplained much in the interim, I'm OK to go with your much simpler\napproach. But I think it's worth at least thinking for a few minutes on\nwhether there's anything to pull from that discussion. :)\n\n(As a side note, I've had all those patches on my \"to revisit and send\nupstream\" queue for 7 years; if we take yours, maybe I can finally let\nthem go. ;) ).\n\n>  Documentation/technical/api-credentials.txt | 4 +++-\n>  credential.c                                | 4 +++-\n>  credential.h                                | 3 ++-\n>  t/t0300-credentials.sh                      | 9 +++++++++\n>  4 files changed, 17 insertions(+), 3 deletions(-)\n\nThe patch itself looks good; two minor comments:\n\n> @@ -296,7 +298,7 @@ void credential_approve(struct credential *c)\n>  {\n>  \tint i;\n>  \n> -\tif (c->approved)\n> +\tif (c->approved || c->no_cache)\n>  \t\treturn;\n>  \tif (!c->username || !c->password)\n>  \t\treturn;\n\nHere we're disallowing a \"nocache\" credential from being passed to _any_\nhelper, whether it's caching or not. It could be storing permanently,\nthough perhaps that's semantic nitpicking (if it's not to be cached, it\nprobably shouldn't be stored permanently either). Other helpers could in\ntheory be doing something else with the data, though in practice I doubt\nhere are any uses beyond debugging.\n\nI dunno. I started writing that paragraph to suggest calling it\n\"nostore\" or something, but I think that is really no better than\n\"nocache\".\n\nIf we did have a ttl, I'd expect to see a check for \"ttl=0\" here, but\nthen otherwise pass the ttl along to the helper (and a matching change\nin credential-cache to use the minimum of the credential-specific ttl or\nthe global-configured one).\n\n> diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\n> index 82eaaea0f4..ad06f6fe11 100755\n> --- a/t/t0300-credentials.sh\n> +++ b/t/t0300-credentials.sh\n> @@ -118,6 +118,15 @@ test_expect_success 'do not bother storing password-less credential' '\n>  \tEOF\n>  '\n>  \n> +test_expect_success 'credential_approve does not call helpers for nocache' '\n> +\tcheck approve useless <<-\\EOF\n> +\tusername=foo\n> +\tpassword=bar\n> +\tnocache=1\n> +\t--\n> +\t--\n> +\tEOF\n> +'\n\nAt first I thought this test was doing nothing, since we don't generally\nexpect a helper to produce output for an \"approve\" request (and there\nare cases in lib-credential.sh that rely on that). But the \"useless\"\nhelper is intentionally chatty, so it produces output for all requests,\nand this would fail without your patch. Good.\n\nThe cases in lib-credential also omit the \"--\" for empty sections, but I\nsee the similar \"do not bother...\" test above includes them. It\nshouldn't matter either way.\n\nSo I think this looks good.\n\n-Peff\n"},{"id":"379147","messageId":"CAJB1erXRg4S-vzRZwA-Q5cXAPayRE0dAjFjjkNQ9CoKiXF=7EQ@mail.gmail.com","threadId":"51440","inReplyTo":"20190709125620.GA18175@sigill.intra.peff.net","subject":"Re: [PATCH] credential: add nocache option to the credentials API","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2019-07-22T17:30:52Z","receivedAt":"2019-07-22T17:31:08Z","isPatch":true,"sender":{"key":"masayasuzuki@google.com","avatar":"https://avatars.githubusercontent.com/u/21878?v=4"},"body":"On Tue, Jul 9, 2019 at 5:56 AM Jeff King <peff@peff.net> wrote:\n>\n> On Sat, Jul 06, 2019 at 10:51:32PM -0700, Masaya Suzuki wrote:\n>\n> > The credentials API calls credentials helpers in order. If a\n> > username/password pair is returned the helpers and if it's used for\n> > authentication successfully, it's announced to the helpers and they can\n> > store it for later use.\n> >\n> > Some credentials are valid only for the limited time and should not be\n> > cached. In this case, because the credential is announced to all helpers\n> > and they can independently decide whether they will cache it or not,\n> > those short-lived credentials can be cached.\n> >\n> > This change adds an option that a credential helper can specify that the\n> > credential returned by the helper should not be cached. If this is\n> > specified, even after the credential is used successfully, it won't be\n> > announced to other helpers for store.\n>\n> I think this makes sense to do, though note that there's an old\n> discussion which covers some alternatives:\n>\n>   https://public-inbox.org/git/20120407033417.GA13914@sigill.intra.peff.net/\n>\n> In that patch, I essentially proposed making all gathered credentials as\n> nocache. That's a more secure default (though in some cases less\n> convenient).\n>\n> It did break a case Shawn had of caching the result of another helper. I\n> showed some options there for providing a mechanism to chain helpers\n> together explicitly.\n\nI think that it's better to make it nocache by default. Having one\nhelper produce a credential and having another cache it looks storage.\nBut since this is the current behavior, I'm OK with just keeping\nnocache an option. It's backward compatible.\n\n> We also discussed helpers passing out an explicit ttl. That's a more\n> general case of your nocache flag (i.e., ttl=0 covers that case, but we\n> could additionally pass \"ttl\" to the cache helper to let it be smarter).\n\nTTL sounds like it's a generalized version. It might be a bit awkward\nbecause the existing credential helpers that don't support TTL would\nanyway cache the credentials. I think in practice the password saving\nfeature is mainly used by those password management software (like\ngit-credential-osxkeychain), and they wouldn't support a short-lived\ncredential. Just having nocache seems fine to me. As you said, if\nneeded, \"ttl\" can be added and \"nocache\" can be just a shorthand of\n\"ttl=0\".\n\n> Given the age of that discussion and the fact that nobody has really\n> complained much in the interim, I'm OK to go with your much simpler\n> approach. But I think it's worth at least thinking for a few minutes on\n> whether there's anything to pull from that discussion. :)\n>\n> (As a side note, I've had all those patches on my \"to revisit and send\n> upstream\" queue for 7 years; if we take yours, maybe I can finally let\n> them go. ;) ).\n>\n> >  Documentation/technical/api-credentials.txt | 4 +++-\n> >  credential.c                                | 4 +++-\n> >  credential.h                                | 3 ++-\n> >  t/t0300-credentials.sh                      | 9 +++++++++\n> >  4 files changed, 17 insertions(+), 3 deletions(-)\n>\n> The patch itself looks good; two minor comments:\n>\n> > @@ -296,7 +298,7 @@ void credential_approve(struct credential *c)\n> >  {\n> >       int i;\n> >\n> > -     if (c->approved)\n> > +     if (c->approved || c->no_cache)\n> >               return;\n> >       if (!c->username || !c->password)\n> >               return;\n>\n> Here we're disallowing a \"nocache\" credential from being passed to _any_\n> helper, whether it's caching or not. It could be storing permanently,\n> though perhaps that's semantic nitpicking (if it's not to be cached, it\n> probably shouldn't be stored permanently either). Other helpers could in\n> theory be doing something else with the data, though in practice I doubt\n> here are any uses beyond debugging.\n\nI cannot think of a usage either. If there's a good usage, I would\nchange this, but if it's for debugging, it's better to be done with\nthose debugging features (like GIT_TRACE_CURL). Note that this is\ncalled only when the credential is successfully used. We probably want\nto use such debugging feature for the credentials that are not\nsuccessfully used.\n"},{"id":"379158","messageId":"20190722210037.GA31664@sigill.intra.peff.net","threadId":"51440","inReplyTo":"CAJB1erXRg4S-vzRZwA-Q5cXAPayRE0dAjFjjkNQ9CoKiXF=7EQ@mail.gmail.com","subject":"Re: [PATCH] credential: add nocache option to the credentials API","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-22T21:00:38Z","receivedAt":"2019-07-22T21:00:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 22, 2019 at 10:30:52AM -0700, Masaya Suzuki wrote:\n\n> > In that patch, I essentially proposed making all gathered credentials as\n> > nocache. That's a more secure default (though in some cases less\n> > convenient).\n> >\n> > It did break a case Shawn had of caching the result of another helper. I\n> > showed some options there for providing a mechanism to chain helpers\n> > together explicitly.\n> \n> I think that it's better to make it nocache by default. Having one\n> helper produce a credential and having another cache it looks storage.\n> But since this is the current behavior, I'm OK with just keeping\n> nocache an option. It's backward compatible.\n\nYeah, I think that make sense.\n\n> > We also discussed helpers passing out an explicit ttl. That's a more\n> > general case of your nocache flag (i.e., ttl=0 covers that case, but we\n> > could additionally pass \"ttl\" to the cache helper to let it be smarter).\n> \n> TTL sounds like it's a generalized version. It might be a bit awkward\n> because the existing credential helpers that don't support TTL would\n> anyway cache the credentials. I think in practice the password saving\n> feature is mainly used by those password management software (like\n> git-credential-osxkeychain), and they wouldn't support a short-lived\n> credential. Just having nocache seems fine to me. As you said, if\n> needed, \"ttl\" can be added and \"nocache\" can be just a shorthand of\n> \"ttl=0\".\n\nI was thinking that Git itself could treat \"ttl=0\" specially, the same\nas your nocache, and avoid passing it along to any helpers during the\napprove stage. That would make it exactly equivalent to your patch\n(modulo the name change).\n\n> > Here we're disallowing a \"nocache\" credential from being passed to _any_\n> > helper, whether it's caching or not. It could be storing permanently,\n> > though perhaps that's semantic nitpicking (if it's not to be cached, it\n> > probably shouldn't be stored permanently either). Other helpers could in\n> > theory be doing something else with the data, though in practice I doubt\n> > here are any uses beyond debugging.\n> \n> I cannot think of a usage either. If there's a good usage, I would\n> change this, but if it's for debugging, it's better to be done with\n> those debugging features (like GIT_TRACE_CURL). Note that this is\n> called only when the credential is successfully used. We probably want\n> to use such debugging feature for the credentials that are not\n> successfully used.\n\nYeah, I don't think debugging is worth caring about here. As you say, we\ncan dump the data readily through other means. I was more wondering if\nthere was some legitimate use where a helper wanted to see (but not\nstore!) an existing credential. But again, I don't know of one.\n\nAnd as you noted above, if we don't suppress the helper calls inside\nGit, then every matching storage helper needs to learn about \"nocache\"\n(or \"ttl\") before it will do any good.\n\n-Peff\n"},{"id":"381204","messageId":"xmqqsgpnj3hv.fsf@gitster-ct.c.googlers.com","threadId":"51440","inReplyTo":"20190722210037.GA31664@sigill.intra.peff.net","subject":"Re: [PATCH] credential: add nocache option to the credentials API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-26T16:27:56Z","receivedAt":"2019-08-26T16:28:04Z","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> I was thinking that Git itself could treat \"ttl=0\" specially, the same\n> as your nocache, and avoid passing it along to any helpers during the\n> approve stage. That would make it exactly equivalent to your patch\n> (modulo the name change).\n> ...\n> And as you noted above, if we don't suppress the helper calls inside\n> Git, then every matching storage helper needs to learn about \"nocache\"\n> (or \"ttl\") before it will do any good.\n\nI was waiting for this discussion to settle and then the discussion\nseems to have petered out.  Any interest to following the \"ttl with\nspecial casing value 0 as 'nocache'\" idea thru from either two of\nyou, or should I take the patch as is in the meantime?\n\nThanks.\n"},{"id":"382401","messageId":"CAJB1erXHWKu35_7EJ7tK7X4jkzcWA6KR8LqNdR8Mf=hY+rWK+w@mail.gmail.com","threadId":"51440","inReplyTo":"xmqqsgpnj3hv.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] credential: add nocache option to the credentials API","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2019-09-15T21:50:59Z","receivedAt":"2019-09-15T21:51:16Z","isPatch":true,"sender":{"key":"masayasuzuki@google.com","avatar":"https://avatars.githubusercontent.com/u/21878?v=4"},"body":"On Mon, Aug 26, 2019 at 9:28 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Jeff King <peff@peff.net> writes:\n>\n> > I was thinking that Git itself could treat \"ttl=0\" specially, the same\n> > as your nocache, and avoid passing it along to any helpers during the\n> > approve stage. That would make it exactly equivalent to your patch\n> > (modulo the name change).\n> > ...\n> > And as you noted above, if we don't suppress the helper calls inside\n> > Git, then every matching storage helper needs to learn about \"nocache\"\n> > (or \"ttl\") before it will do any good.\n>\n> I was waiting for this discussion to settle and then the discussion\n> seems to have petered out.  Any interest to following the \"ttl with\n> special casing value 0 as 'nocache'\" idea thru from either two of\n> you, or should I take the patch as is in the meantime?\n\nSorry for the late reply. I think about this again. I imagine that, if\nI would like to have credentials with an expiration and I want to have\nthem managed by other helpers, it's probably better to use an absolute\ntimestamp instead of duration. The second call to the helpers is done\nafter the remote call. For the helpers that store TTL-ed credentials,\nthey cannot tell the start time of the TTL in the second call. This\nmakes it hard to cache the short-lived credentials safely because some\ntime has spent during the remote call and the actual TTL is shorter\nthan ttl=N option. From this, instead of adding ttl=DURATION, it might\nbe better to have expires_at=TIMESTAMP.\n\nMaybe my observation is not an issue. I don't know. For now, adding\nnocache seems a safer action for me, so I vote for taking nocache\npatch as-is in the meantime. If there's somebody who wants to receive\nTTL or expiration timestamp, they can decide what's actually needed\nlater.\n"}]}