Re: [PATCH v5] credential-store: warn instead of fatal for bogus lines from store
- From
Carlo Marcelo Arenas Belón <carenas@gmail.com>
- Date
- Apr 29, 2020, 07:31 UTC
- Message-ID
- <20200429073116.GA94208@Carlos-MBP>
- In-Reply-To
- <xmqqd07qzz67.fsf@gitster.c.googlers.com>
On Tue, Apr 28, 2020 at 09:36:00PM -0700, Junio C Hamano wrote:
Show 18 quoted lines
> Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:
> > +static char *redact_credential(const struct strbuf *line)
> > +{
> > + struct strbuf redacted_line = STRBUF_INIT;
> > + char *at = strchr(line->buf, '@');
> > + char *colon;
> > + int redacted = 0;
> > +
> > + if (at) {
> > + strbuf_addf(&redacted_line, "%.*s",
> > + (int)(at - line->buf), line->buf);
> > + colon = strrchr(redacted_line.buf, ':');
>
> Just showing my ignorance, but ...
>
> - Is the above strrchr() that forbids a colon in the password
> intended, or should it be strchr() that only forbids a colon in
> the username instead?neither; that ":" is the separator we put in there between username and password (unless it was edited out), as any original ":" in both is already urlencoded.
strrchr() was used in purpose to back only as much as it is needed from the string to hopefully find the beginning of the password.
indeed, most of the uglyness of the code comes from the fact it tries really hard to only redact the password so the rest of the credential is still useful for context and do so without changing the original line.
Show 8 quoted lines
> - Would it hurt to redact both username and password as sensitive?
> If not, it would certainly make it simpler to unconditionally:
>
> int i;
> for (i = 0; i < redacted_line.len; i++) {
> if (redacted_line.buf[i] != ':')
> redacted_line.buf[i] = 'x';
> }that would leak the length of the password which might also be considered sensitive
IMHO if we are redacting both might as well not print anything and let the user find the offending credential by line number instead ;) after all I got a feeling most of those entries are empty lines or "comments".
for example with the current code merged in pu we get :
$ cat secretfile
://u:p@example.com http://user:some%3apass@example.com
$ git credential-store --file secretfile get protocol=http
warning: secretfile:1 ignoring invalid credential: warning: secretfile:2 ignoring invalid credential: ://u:<redacted>@example.com username=user password=some:pass
Carlo