[v3,0/2] credential: improvements to erase in helpers

Message

Derrick Stolee via GitGitGadget June 15, 2023, 6:03 a.m. UTC

M Hickford (2):
  credential: avoid erasing distinct password
  credential: erase all matching credentials

 Documentation/git-credential.txt   |   2 +-
 Documentation/gitcredentials.txt   |   2 +-
 builtin/credential-cache--daemon.c |  17 +++--
 builtin/credential-store.c         |  18 ++---
 credential.c                       |  11 +--
 credential.h                       |   2 +-
 t/lib-credential.sh                | 103 +++++++++++++++++++++++++++++
 7 files changed, 131 insertions(+), 24 deletions(-)


base-commit: d7d8841f67f29e6ecbad85a11805c907d0f00d5d
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1525%2Fhickford%2Ferase-test-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1525/hickford/erase-test-v3
Pull-Request: https://github.com/git/git/pull/1525

Range-diff vs v2:

 1:  35ee1795bcd ! 1:  df3c8a15bf8 credential: avoid erasing distinct password
     @@ builtin/credential-cache--daemon.c: static void cache_credential(struct credenti
       			return &entries[i];
       	}
       	return NULL;
     -@@ builtin/credential-cache--daemon.c: static void remove_credential(const struct credential *c)
     + }
     + 
     +-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, c->password != NULL);
     ++	e = lookup_credential(c, match_password);
       	if (e)
       		e->expiration = 0;
       }
     @@ builtin/credential-cache--daemon.c: static void serve_one_client(FILE *in, FILE
       		if (e) {
       			fprintf(out, "username=%s\n", e->item.username);
       			fprintf(out, "password=%s\n", e->item.password);
     +@@ builtin/credential-cache--daemon.c: 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);
     + 		}
     + 	}
      
       ## builtin/credential-store.c ##
      @@ builtin/credential-store.c: static struct lock_file credential_lock;
     @@ builtin/credential-store.c: static struct lock_file credential_lock;
       				  void (*match_cb)(struct credential *),
      -				  void (*other_cb)(struct strbuf *))
      +				  void (*other_cb)(struct strbuf *),
     -+				  const char* op)
     ++				  int match_password)
       {
       	FILE *fh;
       	struct strbuf line = STRBUF_INIT;
     - 	struct credential entry = CREDENTIAL_INIT;
     - 	int found_credential = 0;
     -+	const int match_password = !strcmp(op, "erase") && c->password != NULL;
     - 
     - 	fh = fopen(fn, "r");
     - 	if (!fh) {
      @@ builtin/credential-store.c: static int parse_credential_file(const char *fn,
       
       	while (strbuf_getline_lf(&line, fh) != EOF) {
     @@ builtin/credential-store.c: static void print_line(struct strbuf *buf)
       
       static void rewrite_credential_file(const char *fn, struct credential *c,
      -				    struct strbuf *extra)
     -+				    struct strbuf *extra, const char *op)
     ++				    struct strbuf *extra, int match_password)
       {
       	int timeout_ms = 1000;
       
     @@ builtin/credential-store.c: static void rewrite_credential_file(const char *fn,
       	if (extra)
       		print_line(extra);
      -	parse_credential_file(fn, c, NULL, print_line);
     -+	parse_credential_file(fn, c, NULL, print_line, op);
     ++	parse_credential_file(fn, c, NULL, print_line, match_password);
       	if (commit_lock_file(&credential_lock) < 0)
       		die_errno("unable to write credential store");
       }
     @@ builtin/credential-store.c: static void store_credential_file(const char *fn, st
       	}
       
      -	rewrite_credential_file(fn, c, &buf);
     -+	rewrite_credential_file(fn, c, &buf, "store");
     ++	rewrite_credential_file(fn, c, &buf, 0);
       	strbuf_release(&buf);
       }
       
     @@ builtin/credential-store.c: static void remove_credential(const struct string_li
       	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, "erase");
     ++			rewrite_credential_file(fn->string, c, NULL, 1);
       }
       
       static void lookup_credential(const struct string_list *fns, struct credential *c)
     @@ builtin/credential-store.c: static void lookup_credential(const struct string_li
       
       	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, "get"))
     ++		if (parse_credential_file(fn->string, c, print_entry, NULL, 0))
       			return; /* Found credential */
       }
       
     @@ t/lib-credential.sh: helper_test_clean() {
       	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
     @@ t/lib-credential.sh: 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
     +@@ t/lib-credential.sh: helper_test() {
     + 		EOF
     + 	'
     + 
      +	test_expect_success "helper ($HELPER) does not erase a password distinct from input" '
      +		check approve $HELPER <<-\EOF &&
      +		protocol=https
 2:  9b12f17dc7e ! 2:  e06d80e99a0 credential: erase all matching credentials
     @@ Commit message
          Signed-off-by: M Hickford <[e-mail]>
      
       ## Documentation/git-credential.txt ##
     -@@ Documentation/git-credential.txt: to any configured credential helpers, which may store the credential
     - for later use.
     +@@ Documentation/git-credential.txt: for later use.
       
       If the action is `reject`, git-credential will send the description to
     --any configured credential helpers, which may erase any stored
     + any configured credential helpers, which may erase any stored
      -credential matching the description.
     -+any configured credential helpers, which may erase stored credentials
     -+matching the description.
     ++credentials matching the description.
       
       If the action is `approve` or `reject`, no output should be emitted.
       
     @@ builtin/credential-cache--daemon.c: static void cache_credential(struct credenti
       			return &entries[i];
       	}
       	return NULL;
     -@@ builtin/credential-cache--daemon.c: static void remove_credential(const struct credential *c)
     +@@ builtin/credential-cache--daemon.c: static void remove_credential(const struct credential *c, int match_password)
       {
       	struct credential_cache_entry *e;
       
     --	e = lookup_credential(c, c->password != NULL);
     +-	e = lookup_credential(c, match_password);
      -	if (e)
      -		e->expiration = 0;
      +	int i;
      +	for (i = 0; i < entries_nr; i++) {
      +		e = &entries[i];
     -+		if (credential_match(c, &e->item, c->password != NULL))
     ++		if (credential_match(c, &e->item, match_password))
      +			e->expiration = 0;
      +	}
       }
     @@ builtin/credential-store.c: static int parse_credential_file(const char *fn,
       			if (match_cb) {
       				match_cb(&entry);
      -				break;
     -+				if (strcmp(op, "erase"))
     -+					break;
       			}
       		}
       		else if (other_cb)
     @@ t/lib-credential.sh: helper_test_clean() {
       	reject $1 https example.com user5
      +	reject $1 https example.com user6
      +	reject $1 https example.com user7
     + 	reject $1 https example.com user8
       	reject $1 http path.tld user
       	reject $1 https timeout.tld user
     - 	reject $1 https sso.tld
      @@ t/lib-credential.sh: helper_test() {
       		EOF
       	'