Re: [PATCH] credential: warn about git-credential-store [RFC]

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

> -	if (!c->password)
> +	if (!c->password) {
> +		if (c->helpers.nr >= 1 && starts_with(c->helpers.items[0].string, "store"))
> +			warning("git-credential-store saves passwords unencrypted on disk. For alternatives, see gitcredentials(7).");

I have no strong opinion on the details of how the detection of use
of the "store" helper should be implemented, but I recall reading
somewhere that users can configure more than one helpers and they
are used in casdading fashion?  Insecure helpers may be configured
to come later on the list, so [0] might not be sufficient.  A few
other things are that git-credential-store could be installed in an
unusual place and credential.c:credential_do() may find it from its
absolute path.  Also the end-users can use third-party helpers,
whose names we do not control, but presumably they will not name
theirs exactly the same as the one we ship, so starts_with() may
want to get a bit tightened.  If somebody writes a custom helper
"git-credential-store-securely" and installs the binary in a
directory where "git" can find via the usual GIT_EXEC_PATH mechanism
as "git credential-store-securely", helpers.items[].string would say
"store-securely".

I agree with you that it is a rather unfortunate layering violation
that you need to know what helper would see the result from this
function, because you want to warn before the user gives the
password to us.

Warning immediately before the bits hits the disk platter (i.e., the
result of _fill() is passed to the helper) is not as secure because
there is no way to say "ah, was I using an insecure backend?  Then
please stop and do not store it there" later, so I do not think of a
strong reason to claim that it is a wrong place to give the warning.

Regarding the warning message, you may want to consider using the
advice mechanism for a thing like this, perhaps?  If somebody has a
legitimate reason why they need to use and cannot move away from the
backend, it does not help them at all to keep giving the same
warning() they are already aware of, without a way to say "Yes, I
know, I've seen it enough times, go shut up, please".

Thanks.