Re: [PATCH v9] credential-store: warn instead of fatal for bogus lines from store

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 30, 2020, 20:21 UTC
Message-ID: <xmqq1ro4sp1p.fsf@gitster.c.googlers.com>
In-Reply-To: <20200430160642.90096-1-carenas@gmail.com>

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

Show 21 quoted lines

> With the added checks for invalid URLs in credentials, any locally
> modified store files which might have empty lines or even comments
> were reported[1] failing to parse as valid credentials.
>
> Instead of doing a hard check for credentials, do a soft one and
> therefore avoid the reported fatal error.
>
> Warn the user indicating the filename and line number so any invalid
> entries could be corrected but continue parsing until a match is
> found or all valid credentials are processed.
>
> Make sure that the credential that we will use to match is complete by
> confirming it has all fields set as expected by the updated rules.
>
> [1] https://stackoverflow.com/a/61420852/5005936
>
> Reported-by: Dirk <dirk@ed4u.de>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Based-on-patch-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---

Looks good.

Show 18 quoted lines

> diff --git a/credential-store.c b/credential-store.c
> index c010497cb2..b1f5b2dea6 100644
> --- a/credential-store.c
> +++ b/credential-store.c
> @@ -4,10 +4,20 @@
>  #include "string-list.h"
>  #include "parse-options.h"
>  
> +#define PARSE_VERBOSE 0x01
> +
>  static struct lock_file credential_lock;
>  
> +static int valid_credential(struct credential *entry)
> +{
> +	return (entry->username && entry->password &&
> +		entry->protocol && *entry->protocol &&
> +		((entry->host && *entry->host) || entry->path));
> +}

OK.

Show 23 quoted lines

>  static int parse_credential_file(const char *fn,
>  				  struct credential *c,
> +				  int flags,
>  				  void (*match_cb)(struct credential *),
>  				  void (*other_cb)(struct strbuf *))
>  {
> @@ -15,6 +25,7 @@ static int parse_credential_file(const char *fn,
>  	struct strbuf line = STRBUF_INIT;
>  	struct credential entry = CREDENTIAL_INIT;
>  	int found_credential = 0;
> +	int lineno = 0;
>  
>  	fh = fopen(fn, "r");
>  	if (!fh) {
> @@ -23,17 +34,24 @@ static int parse_credential_file(const char *fn,
>  		return found_credential;
>  	}
>  
> -	while (strbuf_getline_lf(&line, fh) != EOF) {
> -		credential_from_url(&entry, line.buf);
> -		if (entry.username && entry.password &&
> -		    credential_match(c, &entry)) {
> +	while (strbuf_getline(&line, fh) != EOF) {

Hmph, I probably have missed some discussion that happened in the last few days, but from the use of _lf() in the original, I sense that it is very much deliberate that the file format is designed to be LF delimited records, *not* a text file in which each line is terminated with some end-of-line marker that is platform dependent. IOW, an explicit use of _lf() shouts, at least to me, that we want a sequence of LF terminated records even on Windows and Macs.

So, I am not sure why this change is desirable.

> +		lineno++;
> +
> +		if (credential_from_url_gently(&entry, line.buf, 1) ||
> +			!valid_credential(&entry)) {

OK, so we read a line, fed it to the parser, and if we had trouble parsing or the line did not have enough credential material, we discard and warn (when told to).

Show 10 quoted lines

> +			if (flags & PARSE_VERBOSE)
> +				warning(_("%s:%d: ignoring invalid credential"),
> +					fn, lineno);
> +		} else if (credential_match(c, &entry)) {
>  			found_credential = 1;
>  			if (match_cb) {
>  				match_cb(&entry);
>  				break;
>  			}
> +			continue;

And if the credential material on a good line matches, we remember we saw a match. If there is match callback, we call it and leave the loop to return from the function. Otherwise we go to the next line.

>  		}
> +
> +		if (other_cb)
>  			other_cb(&line);

A malformed/underspecified line and an unmatched line is fed to the other callback.

Show 8 quoted lines

>  	}
>  
> @@ -62,7 +80,7 @@ static void rewrite_credential_file(const char *fn, struct credential *c,
>  		die_errno("unable to get credential storage lock");
>  	if (extra)
>  		print_line(extra);
> -	parse_credential_file(fn, c, NULL, print_line);
> +	parse_credential_file(fn, c, 0, NULL, print_line);

This helper function is called from two places.

 - In store_credential_file, we write the credential we were asked
   to store to a strbuf, and then call this function.  The function
   first writes that new credential and then calls the parse helper
   without PARSE_VERBOSE.  We do not give any match callback, and
   other callback is to just print the line read from the credential
   file.  So, the final output would be the new credential, followed
   by the current contents of the credential file except for the
   records that match the one we are storing.  We do this without
   warning about malformed entries at all.

 - In remove_credential, we go over multiple files and copy the
   lines in each of them, except the ones that match the credential
   we are given.  We also do this without warning about malformed
   entries at all.

Show 12 quoted lines

>  	if (commit_lock_file(&credential_lock) < 0)
>  		die_errno("unable to write credential store");
>  }
> @@ -139,7 +157,8 @@ static void lookup_credential(const struct string_list *fns, struct credential *
>  	struct string_list_item *fn;
>  
>  	for_each_string_list_item(fn, fns)
> -		if (parse_credential_file(fn->string, c, print_entry, NULL))
> +		if (parse_credential_file(fn->string, c, PARSE_VERBOSE,
> +						 print_entry, NULL))
>  			return; /* Found credential */
>  }

This is triggered by the "get" operation. We read until we hit the entry we are looking for, at that point the match-callback will print out "username=... / password=..." lines and we ignore the remainder of the file. While we look for the first matching credential, we do warn about malformed or underspecified lines that we are ignoring, but because we leave the loop once a matching line is found, bad lines that come after it won't be even looked at to be warned.

Validating and warning about bad entries is *not* the main purpose of the "credential-store" program, so I fully agree with the design of the "get" part. I am not so sure about the other two operations (i.e. "store" and "erase") that do scan all the entries and has chance to warn about bad ones, though (note: I am not saying that we should parse verbosely---it is just that I do not know why you chose not to and I am not convinced that it is a good idea not to warn).

The tests looked good.

Thanks.