[v3,1/2] credential: avoid erasing distinct password

Commit Message

M Hickford June 15, 2023, 6:03 a.m. UTC

From: M Hickford <[e-mail]>

Test that credential helpers do not erase a password distinct from the
input. Such calls can happen when multiple credential helpers are
configured.

Fixes for credential-cache and credential-store.

Signed-off-by: M Hickford <[e-mail]>
---
 builtin/credential-cache--daemon.c | 14 +++---
 builtin/credential-store.c         | 17 ++++----
 credential.c                       | 11 ++---
 credential.h                       |  2 +-
 t/lib-credential.sh                | 70 ++++++++++++++++++++++++++++++
 5 files changed, 93 insertions(+), 21 deletions(-)

Comments

Jeff King June 15, 2023, 7:08 a.m. UTC | #1

On Thu, Jun 15, 2023 at 06:03:23AM +0000, M Hickford via GitGitGadget wrote:

> @@ -151,14 +151,14 @@ static void serve_one_client(FILE *in, FILE *out)
>  		exit(0);
>  	}
>  	else if (!strcmp(action.buf, "erase"))
> -		remove_credential(&c);
> +		remove_credential(&c, 1);
>  	else if (!strcmp(action.buf, "store")) {
>  		if (timeout < 0)
>  			warning("cache client didn't specify a timeout");
>  		else if (!c.username || !c.password)
>  			warning("cache client gave us a partial credential");
>  		else {
> -			remove_credential(&c);
> +			remove_credential(&c, 0);
>  			cache_credential(&c, timeout);
>  		}
>  	}

Great, this hunk fixes the overwrite problem from the previous round.
All of the credential-cache bits look good to me.

> @@ -29,8 +30,8 @@ static int parse_credential_file(const char *fn,
>  
>  	while (strbuf_getline_lf(&line, fh) != EOF) {
>  		if (!credential_from_url_gently(&entry, line.buf, 1) &&
> -		    entry.username && entry.password &&
> -		    credential_match(c, &entry)) {
> +			entry.username && entry.password &&
> +			credential_match(c, &entry, match_password)) {
>  			found_credential = 1;
>  			if (match_cb) {
>  				match_cb(&entry);

This hunk is from credential-store. I think the code change is doing the
right thing, but the modified indentation is wrong (we'd usually do a
4-space indentation to line up the conditions; doing a full tab confuses
the conditions with the actual body).

> diff --git a/credential.c b/credential.c
> index 023b59d5711..9157ff0865e 100644
> --- a/credential.c
> +++ b/credential.c
> @@ -33,13 +33,14 @@ void credential_clear(struct credential *c)
>  }
>  
>  int credential_match(const struct credential *want,
> -		     const struct credential *have)
> +		     const struct credential *have, int match_password)
>  {
>  #define CHECK(x) (!want->x || (have->x && !strcmp(want->x, have->x)))
>  	return CHECK(protocol) &&
> -	       CHECK(host) &&
> -	       CHECK(path) &&
> -	       CHECK(username);
> +		CHECK(host) &&
> +		CHECK(path) &&
> +		CHECK(username) &&
> +		(!match_password || CHECK(password));
>  #undef CHECK
>  }

The indentation here seemed funny to me, too, though I think it's a
matter of taste (indent a tab versus line up with the seven chars of
"return ". I'd have a slight preference to leave it as-is, just because
it makes the diff noisier, but I'm OK with it either way.

> diff --git a/t/lib-credential.sh b/t/lib-credential.sh
> index f1ab92ba35c..7b4fdd011d7 100644
> --- a/t/lib-credential.sh
> +++ b/t/lib-credential.sh
> @@ -44,6 +44,8 @@ helper_test_clean() {
>  	reject $1 https example.com user1
>  	reject $1 https example.com user2
>  	reject $1 https example.com user4
> +	reject $1 https example.com user5
> +	reject $1 https example.com user8
>  	reject $1 http path.tld user
>  	reject $1 https timeout.tld user
>  	reject $1 https sso.tld

This is a funny gap to have (I realize it is because you use 6-7 in the
next patch, but usually we'd try to do things in order and rebase
downstream patches as appropriate). Doubly funny here is that the test
for "8" comes before "5". That's not wrong from a code/testing
standpoint, but it may make the script harder to read or modify later
on.

Maybe it would be easier to read if we gave these users non-numeric
names that match how we will use them. E.g., can we call user8
user-overwrite or something? That makes the intention obvious, and then
we do not have to worry about maintaining ordering constraints.

> @@ -167,6 +169,49 @@ helper_test() {
>  		EOF
>  	'
>  
> +	test_expect_success "helper ($HELPER) overwrites on store" '
> [...]

Great, thank you for adding a test for this case. The test itself looks
good to me.

> @@ -221,6 +266,31 @@ helper_test() {
>  		EOF
>  	'
>  
> +	test_expect_success "helper ($HELPER) does not erase a password distinct from input" '
> [...]

And this one continues to look good. I think the username could be
"user-distinct-pass" or something if we wanted to go with
human-understandable ones.

-Peff

Patch

diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
index 756c5f02aef..f64dd21d335 100644
--- a/builtin/credential-cache--daemon.c
+++ b/builtin/credential-cache--daemon.c
@@ -33,22 +33,22 @@  static void cache_credential(struct credential *c, int timeout)
 	e->expiration = time(NULL) + timeout;
 }
 
-static struct credential_cache_entry *lookup_credential(const struct credential *c)
+static struct credential_cache_entry *lookup_credential(const struct credential *c, int match_password)
 {
 	int i;
 	for (i = 0; i < entries_nr; i++) {
 		struct credential *e = &entries[i].item;
-		if (credential_match(c, e))
+		if (credential_match(c, e, match_password))
 			return &entries[i];
 	}
 	return NULL;
 }
 
-static void remove_credential(const struct credential *c)
+static void remove_credential(const struct credential *c, int match_password)
 {
 	struct credential_cache_entry *e;
 
-	e = lookup_credential(c);
+	e = lookup_credential(c, match_password);
 	if (e)
 		e->expiration = 0;
 }
@@ -127,7 +127,7 @@  static void serve_one_client(FILE *in, FILE *out)
 	if (read_request(in, &c, &action, &timeout) < 0)
 		/* ignore error */ ;
 	else if (!strcmp(action.buf, "get")) {
-		struct credential_cache_entry *e = lookup_credential(&c);
+		struct credential_cache_entry *e = lookup_credential(&c, 0);
 		if (e) {
 			fprintf(out, "username=%s\n", e->item.username);
 			fprintf(out, "password=%s\n", e->item.password);
@@ -151,14 +151,14 @@  static void serve_one_client(FILE *in, FILE *out)
 		exit(0);
 	}
 	else if (!strcmp(action.buf, "erase"))
-		remove_credential(&c);
+		remove_credential(&c, 1);
 	else if (!strcmp(action.buf, "store")) {
 		if (timeout < 0)
 			warning("cache client didn't specify a timeout");
 		else if (!c.username || !c.password)
 			warning("cache client gave us a partial credential");
 		else {
-			remove_credential(&c);
+			remove_credential(&c, 0);
 			cache_credential(&c, timeout);
 		}
 	}
diff --git a/builtin/credential-store.c b/builtin/credential-store.c
index 30c6ccf56c0..f80ff59f18a 100644
--- a/builtin/credential-store.c
+++ b/builtin/credential-store.c
@@ -13,7 +13,8 @@  static struct lock_file credential_lock;
 static int parse_credential_file(const char *fn,
 				  struct credential *c,
 				  void (*match_cb)(struct credential *),
-				  void (*other_cb)(struct strbuf *))
+				  void (*other_cb)(struct strbuf *),
+				  int match_password)
 {
 	FILE *fh;
 	struct strbuf line = STRBUF_INIT;
@@ -29,8 +30,8 @@  static int parse_credential_file(const char *fn,
 
 	while (strbuf_getline_lf(&line, fh) != EOF) {
 		if (!credential_from_url_gently(&entry, line.buf, 1) &&
-		    entry.username && entry.password &&
-		    credential_match(c, &entry)) {
+			entry.username && entry.password &&
+			credential_match(c, &entry, match_password)) {
 			found_credential = 1;
 			if (match_cb) {
 				match_cb(&entry);
@@ -60,7 +61,7 @@  static void print_line(struct strbuf *buf)
 }
 
 static void rewrite_credential_file(const char *fn, struct credential *c,
-				    struct strbuf *extra)
+				    struct strbuf *extra, int match_password)
 {
 	int timeout_ms = 1000;
 
@@ -69,7 +70,7 @@  static void rewrite_credential_file(const char *fn, struct credential *c,
 		die_errno(_("unable to get credential storage lock in %d ms"), timeout_ms);
 	if (extra)
 		print_line(extra);
-	parse_credential_file(fn, c, NULL, print_line);
+	parse_credential_file(fn, c, NULL, print_line, match_password);
 	if (commit_lock_file(&credential_lock) < 0)
 		die_errno("unable to write credential store");
 }
@@ -91,7 +92,7 @@  static void store_credential_file(const char *fn, struct credential *c)
 					is_rfc3986_reserved_or_unreserved);
 	}
 
-	rewrite_credential_file(fn, c, &buf);
+	rewrite_credential_file(fn, c, &buf, 0);
 	strbuf_release(&buf);
 }
 
@@ -138,7 +139,7 @@  static void remove_credential(const struct string_list *fns, struct credential *
 		return;
 	for_each_string_list_item(fn, fns)
 		if (!access(fn->string, F_OK))
-			rewrite_credential_file(fn->string, c, NULL);
+			rewrite_credential_file(fn->string, c, NULL, 1);
 }
 
 static void lookup_credential(const struct string_list *fns, struct credential *c)
@@ -146,7 +147,7 @@  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, print_entry, NULL, 0))
 			return; /* Found credential */
 }
 
diff --git a/credential.c b/credential.c
index 023b59d5711..9157ff0865e 100644
--- a/credential.c
+++ b/credential.c
@@ -33,13 +33,14 @@  void credential_clear(struct credential *c)
 }
 
 int credential_match(const struct credential *want,
-		     const struct credential *have)
+		     const struct credential *have, int match_password)
 {
 #define CHECK(x) (!want->x || (have->x && !strcmp(want->x, have->x)))
 	return CHECK(protocol) &&
-	       CHECK(host) &&
-	       CHECK(path) &&
-	       CHECK(username);
+		CHECK(host) &&
+		CHECK(path) &&
+		CHECK(username) &&
+		(!match_password || CHECK(password));
 #undef CHECK
 }
 
@@ -102,7 +103,7 @@  static int match_partial_url(const char *url, void *cb)
 		warning(_("skipping credential lookup for key: credential.%s"),
 			url);
 	else
-		matches = credential_match(&want, c);
+		matches = credential_match(&want, c, 0);
 	credential_clear(&want);
 
 	return matches;
diff --git a/credential.h b/credential.h
index b8e2936d1dc..acc41adf548 100644
--- a/credential.h
+++ b/credential.h
@@ -211,6 +211,6 @@  void credential_from_url(struct credential *, const char *url);
 int credential_from_url_gently(struct credential *, const char *url, int quiet);
 
 int credential_match(const struct credential *want,
-		     const struct credential *have);
+		     const struct credential *have, int match_password);
 
 #endif /* CREDENTIAL_H */
diff --git a/t/lib-credential.sh b/t/lib-credential.sh
index f1ab92ba35c..7b4fdd011d7 100644
--- a/t/lib-credential.sh
+++ b/t/lib-credential.sh
@@ -44,6 +44,8 @@  helper_test_clean() {
 	reject $1 https example.com user1
 	reject $1 https example.com user2
 	reject $1 https example.com user4
+	reject $1 https example.com user5
+	reject $1 https example.com user8
 	reject $1 http path.tld user
 	reject $1 https timeout.tld user
 	reject $1 https sso.tld
@@ -167,6 +169,49 @@  helper_test() {
 		EOF
 	'
 
+	test_expect_success "helper ($HELPER) overwrites on store" '
+		check approve $HELPER <<-\EOF &&
+		protocol=https
+		host=example.com
+		username=user8
+		password=pass1
+		EOF
+		check approve $HELPER <<-\EOF &&
+		protocol=https
+		host=example.com
+		username=user8
+		password=pass2
+		EOF
+		check fill $HELPER <<-\EOF &&
+		protocol=https
+		host=example.com
+		username=user8
+		--
+		protocol=https
+		host=example.com
+		username=user8
+		password=pass2
+		EOF
+		check reject $HELPER <<-\EOF &&
+		protocol=https
+		host=example.com
+		username=user8
+		password=pass2
+		EOF
+		check fill $HELPER <<-\EOF
+		protocol=https
+		host=example.com
+		username=user8
+		--
+		protocol=https
+		host=example.com
+		username=user8
+		password=askpass-password
+		--
+		askpass: Password for '\''https://[e-mail]'\'':
+		EOF
+	'
+
 	test_expect_success "helper ($HELPER) can forget host" '
 		check reject $HELPER <<-\EOF &&
 		protocol=https
@@ -221,6 +266,31 @@  helper_test() {
 		EOF
 	'
 
+	test_expect_success "helper ($HELPER) does not erase a password distinct from input" '
+		check approve $HELPER <<-\EOF &&
+		protocol=https
+		host=example.com
+		username=user5
+		password=pass1
+		EOF
+		check reject $HELPER <<-\EOF &&
+		protocol=https
+		host=example.com
+		username=user5
+		password=pass2
+		EOF
+		check fill $HELPER <<-\EOF
+		protocol=https
+		host=example.com
+		username=user5
+		--
+		protocol=https
+		host=example.com
+		username=user5
+		password=pass1
+		EOF
+	'
+
 	test_expect_success "helper ($HELPER) can forget user" '
 		check reject $HELPER <<-\EOF &&
 		protocol=https