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

[Jeff King <peff@peff.net>]

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.
And as a bonus, it makes configuration easier on the users (they don't
have to configure the cache helper separately, and they don't have to
set the timeout on it manually).

-Peff