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

[Junio C Hamano <gitster@pobox.com>]

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.