Re: [PATCH] Improvement to only call Git Credential Helper once

From: Jeff King <peff@peff.net>
To: Kyle Hubert <khubert@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] Improvement to only call Git Credential Helper once
Date: Sat, 29 Sep 2018 04:17:05 -0400	[thread overview]
Message-ID: <20180929081705.GI2174@sigill.intra.peff.net> (raw)
In-Reply-To: <20180928163716.29947-1-khubert@gmail.com>

On Fri, Sep 28, 2018 at 12:37:16PM -0400, Kyle Hubert wrote:

> When calling the Git Credential Helper that is set in the git config,
> the get command can return a credential. Git immediately turns around
> and calls the store command, even though that credential was just
> retrieved by the Helper. This creates two side effects. First of all,
> if the Helper requires a passphrase, the user has to type it in
> twice. Secondly, if the user has a number of helpers, this retrieves
> the credential from one service and writes it to all services.
> 
> This commit introduces a new field in the credential struct that
> detects when the credential was retrieved using the Helper, and early
> exits when called to store the credential.

Wow, what's old is new again. Here's more or less the same patch from
2012:

  https://public-inbox.org/git/20120407033417.GA13914@sigill.intra.peff.net/

Unfortunately, some people seem to rely on this multi-helper behavior. I
recommend reading the whole thread, as there are some interesting bits
in it (that I had always meant to return to, but somehow 6 years went
by).

I'm not entirely opposed to breaking the current behavior in the name of
better security (namely not unexpectedly propagating credentials), but
it would be nice if we provided better tools for people to let their
helpers interact (like the credential-wrap thing I showed in that
thread).

> ---
>  credential.c | 8 +++++++-
>  credential.h | 3 ++-
>  2 files changed, 9 insertions(+), 2 deletions(-)

I know your patch is right, because it's almost identical to mine. :)
(Mine didn't use the "retrieved" flag in the middle, but just set
"approved" directly).

If we do go this route, though, we might want to steal the test from
that earlier round.

-Peff