From: M Hickford <mirth.hickford@xxxxxxxxx>
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 <mirth.hickford@xxxxxxxxx>
---
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(-)
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://user8@xxxxxxxxxxx'\'':
+ 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
--
gitgitgadget
