Re: [PATCH] credential: add nocache option to the credentials API

From: Masaya Suzuki <masayasuzuki@google.com>
Date: Jul 22, 2019, 17:30 UTC
Message-ID: <CAJB1erXRg4S-vzRZwA-Q5cXAPayRE0dAjFjjkNQ9CoKiXF=7EQ@mail.gmail.com>
In-Reply-To: <20190709125620.GA18175@sigill.intra.peff.net>

On Tue, Jul 9, 2019 at 5:56 AM Jeff King <peff@peff.net> wrote:

Show 30 quoted lines

>
> 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@sigill.intra.peff.net/
>
> 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".

Show 33 quoted lines

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