[v5] credential-store: warn instead of fatal for bogus lines from store
diff mbox series

Message ID 20200429003303.93583-1-carenas@gmail.com
State New
Headers show
Series
  • [v5] credential-store: warn instead of fatal for bogus lines from store
Related show

Commit Message

Carlo Marcelo Arenas Belón April 29, 2020, 12:33 a.m. UTC
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
warn the user so any invalid entries could be corrected.

[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>
---
v5:
* q_to_tab this round, with a single echo to make sure empty line
  is covered, as that seems to be a popular issue
* rebase on top of jc/credential-store-file-format-doc
* implement a redacter for credentials to use on errora to avoid
  leaking passwordss
v4:
* use credential_from_url_gently instead as shown by Jonathan
* add documentation to clarify "comments" is not a supported feature
v3:
* avoid using q_to_cr as suggested by Peff
* a more verbose commit message and slightly more complete documentation
v2:
* use a here-doc for clarity as suggested by Eric
* improve commit message and include documentation

 Documentation/git-credential-store.txt | 12 +++++-
 credential-store.c                     | 51 ++++++++++++++++++++++----
 t/t0302-credential-store.sh            | 43 ++++++++++++++++++++++
 3 files changed, 96 insertions(+), 10 deletions(-)


base-commit: 272281efcc18fcedd248597b8859f343cae1c5a0

Comments

Junio C Hamano April 29, 2020, 4:36 a.m. UTC | #1
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?

 - 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';
                }
Carlo Marcelo Arenas Belón April 29, 2020, 7:31 a.m. UTC | #2
On Tue, Apr 28, 2020 at 09:36:00PM -0700, Junio C Hamano wrote:
> 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.

>  - 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
Junio C Hamano April 29, 2020, 4:46 p.m. UTC | #3
Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:

> 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".

Very sensible.

Patch
diff mbox series

diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
index 76b0798856..30d498fe54 100644
--- a/Documentation/git-credential-store.txt
+++ b/Documentation/git-credential-store.txt
@@ -95,8 +95,16 @@  https://user:pass@example.com
 ------------------------------
 
 No other kinds of lines (e.g. empty lines or comment lines) are
-allowed in the file, even though some may be silently ignored. Do
-not view or edit the file with editors.
+allowed in the file, even though historically the parser was very
+lenient and some might had been silently ignored.
+
+Do not edit the file with editors as it could compromise the validity
+of your credentials by sometimes subtle formatting issues (like spaces)
+
+In cases where those formatting issues are detected during parsing a
+warning indicating the line found will be printed to stderr so it can
+be corrected at your earliest convenience, but the remaining valid
+credentials will be used to try to find a match as described below.
 
 When Git needs authentication for a particular URL context,
 credential-store will consider that context a pattern to match against
diff --git a/credential-store.c b/credential-store.c
index c010497cb2..5324c56ce1 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -6,6 +6,32 @@ 
 
 static struct lock_file credential_lock;
 
+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, ':');
+		if (colon && *(colon + 1) != '/') {
+			redacted_line.len = colon - redacted_line.buf + 1;
+			strbuf_addstr(&redacted_line, "<redacted>");
+			strbuf_addstr(&redacted_line, at);
+			redacted = 1;
+		}
+		else
+			strbuf_reset(&redacted_line);
+	}
+	if (!redacted)
+		strbuf_addbuf(&redacted_line, line);
+
+	return redacted_line.buf;
+}
+
 static int parse_credential_file(const char *fn,
 				  struct credential *c,
 				  void (*match_cb)(struct credential *),
@@ -15,6 +41,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) {
@@ -24,16 +51,24 @@  static int parse_credential_file(const char *fn,
 	}
 
 	while (strbuf_getline_lf(&line, fh) != EOF) {
-		credential_from_url(&entry, line.buf);
-		if (entry.username && entry.password &&
-		    credential_match(c, &entry)) {
-			found_credential = 1;
-			if (match_cb) {
-				match_cb(&entry);
-				break;
+		lineno++;
+		if (!credential_from_url_gently(&entry, line.buf, 1)) {
+			if (entry.username && entry.password &&
+				credential_match(c, &entry)) {
+				found_credential = 1;
+				if (match_cb) {
+					match_cb(&entry);
+					break;
+				}
 			}
 		}
-		else if (other_cb)
+		else {
+			char *redacted = redact_credential(&line);
+			warning("%s:%d %s: %s", fn, lineno,
+				 _("ignoring invalid credential"), redacted);
+			free(redacted);
+		}
+		if (!found_credential && other_cb)
 			other_cb(&line);
 	}
 
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index d6b54e8c65..68b59e6f98 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -120,4 +120,47 @@  test_expect_success 'erase: erase matching credentials from both xdg and home fi
 	test_must_be_empty "$HOME/.config/git/credentials"
 '
 
+test_expect_success 'get: credentials without scheme are invalid' '
+	echo "://user:secret@example.com" >"$HOME/.git-credentials" &&
+	cat >expect-stdout <<-\STDOUT &&
+	protocol=https
+	host=example.com
+	username=askpass-username
+	password=askpass-password
+	STDOUT
+	test_config credential.helper store &&
+	git credential fill <<-\EOF >stdout 2>stderr &&
+	protocol=https
+	host=example.com
+	EOF
+	test_cmp expect-stdout stdout &&
+	grep "askpass: Username for '\''https://example.com'\'':" stderr &&
+	grep "askpass: Password for '\''https://askpass-username@example.com'\'':" stderr &&
+	test_i18ngrep "ignoring invalid credential" stderr &&
+	! grep secret stderr
+'
+
+test_expect_success 'get: store file can contain empty/bogus lines' '
+	echo "" > "$HOME/.git-credentials" &&
+	q_to_tab <<-\CONFIG >>"$HOME/.git-credentials" &&
+	#comment
+	Q
+	https://user:pass@example.com
+	CONFIG
+	cat >expect-stdout <<-\STDOUT &&
+	protocol=https
+	host=example.com
+	username=user
+	password=pass
+	STDOUT
+	test_config credential.helper store &&
+	git credential fill <<-\EOF >stdout 2>stderr &&
+	protocol=https
+	host=example.com
+	EOF
+	test_cmp expect-stdout stdout &&
+	test_i18ngrep "ignoring invalid credential" stderr &&
+	test_line_count = 3 stderr
+'
+
 test_done