On Tue, Jul 9, 2019 at 5:56 AM Jeff King <peff@xxxxxxxx> wrote: > > On Sat, Jul 06, 2019 at 10:51:32PM -0700, Masaya Suzuki wrote: > > > The credentials API calls credentials helpers in order. If a > > username/password pair is returned the helpers and if it's used for > > authentication successfully, it's announced to the helpers and they can > > store it for later use. > > > > Some credentials are valid only for the limited time and should not be > > cached. In this case, because the credential is announced to all helpers > > and they can independently decide whether they will cache it or not, > > those short-lived credentials can be cached. > > > > This change adds an option that a credential helper can specify that the > > credential returned by the helper should not be cached. If this is > > specified, even after the credential is used successfully, it won't be > > announced to other helpers for store. > > I think this makes sense to do, though note that there's an old > discussion which covers some alternatives: > > https://public-inbox.org/git/20120407033417.GA13914@xxxxxxxxxxxxxxxxxxxxx/ > > In that patch, I essentially proposed making all gathered credentials as > nocache. That's a more secure default (though in some cases less > convenient). > > It did break a case Shawn had of caching the result of another helper. I > showed some options there for providing a mechanism to chain helpers > together explicitly. I think that it's better to make it nocache by default. Having one helper produce a credential and having another cache it looks storage. But since this is the current behavior, I'm OK with just keeping nocache an option. It's backward compatible. > We also discussed helpers passing out an explicit ttl. That's a more > general case of your nocache flag (i.e., ttl=0 covers that case, but we > could additionally pass "ttl" to the cache helper to let it be smarter). TTL sounds like it's a generalized version. It might be a bit awkward because the existing credential helpers that don't support TTL would anyway cache the credentials. I think in practice the password saving feature is mainly used by those password management software (like git-credential-osxkeychain), and they wouldn't support a short-lived credential. Just having nocache seems fine to me. As you said, if needed, "ttl" can be added and "nocache" can be just a shorthand of "ttl=0". > Given the age of that discussion and the fact that nobody has really > complained much in the interim, I'm OK to go with your much simpler > approach. But I think it's worth at least thinking for a few minutes on > whether there's anything to pull from that discussion. :) > > (As a side note, I've had all those patches on my "to revisit and send > upstream" queue for 7 years; if we take yours, maybe I can finally let > them go. ;) ). > > > Documentation/technical/api-credentials.txt | 4 +++- > > credential.c | 4 +++- > > credential.h | 3 ++- > > t/t0300-credentials.sh | 9 +++++++++ > > 4 files changed, 17 insertions(+), 3 deletions(-) > > The patch itself looks good; two minor comments: > > > @@ -296,7 +298,7 @@ void credential_approve(struct credential *c) > > { > > int i; > > > > - if (c->approved) > > + if (c->approved || c->no_cache) > > return; > > if (!c->username || !c->password) > > return; > > Here we're disallowing a "nocache" credential from being passed to _any_ > helper, whether it's caching or not. It could be storing permanently, > though perhaps that's semantic nitpicking (if it's not to be cached, it > probably shouldn't be stored permanently either). Other helpers could in > theory be doing something else with the data, though in practice I doubt > here are any uses beyond debugging. I cannot think of a usage either. If there's a good usage, I would change this, but if it's for debugging, it's better to be done with those debugging features (like GIT_TRACE_CURL). Note that this is called only when the credential is successfully used. We probably want to use such debugging feature for the credentials that are not successfully used.
