credential: avoid erasing distinct password

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@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
index 756c5f02aef56ef4f6601729154c16dddc358810..f64dd21d335b3f2c9c6f3f0fc0c1488bb5c733a6 100644 (file)
--- 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 30c6ccf56c05df3ae6e14497a5c6b719f933fecb..0937230bced2cbeaaafc92e93f5847a64d0e7db6 100644 (file)
--- 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;
@@ -30,7 +31,7 @@ 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)) {
+                   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 023b59d5711cf2fede0aa0408b47a1a6f64df059..8825c6f132018c8527563c4ca0f1a1ea747b92e0 100644 (file)
--- 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(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 b8e2936d1dcb978d67fb6660c6ee288d9a26dfc5..acc41adf5481a21148a6b4797f9a8267feb5d8b6 100644 (file)
--- 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 f1ab92ba35c432052e1f903750795443122869ad..77baec53b6a58c2af23f5804ef0f1a6f4cac32a3 100644 (file)
--- 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 user-distinct-pass
+       reject $1 https example.com user-overwrite
        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=user-overwrite
+               password=pass1
+               EOF
+               check approve $HELPER <<-\EOF &&
+               protocol=https
+               host=example.com
+               username=user-overwrite
+               password=pass2
+               EOF
+               check fill $HELPER <<-\EOF &&
+               protocol=https
+               host=example.com
+               username=user-overwrite
+               --
+               protocol=https
+               host=example.com
+               username=user-overwrite
+               password=pass2
+               EOF
+               check reject $HELPER <<-\EOF &&
+               protocol=https
+               host=example.com
+               username=user-overwrite
+               password=pass2
+               EOF
+               check fill $HELPER <<-\EOF
+               protocol=https
+               host=example.com
+               username=user-overwrite
+               --
+               protocol=https
+               host=example.com
+               username=user-overwrite
+               password=askpass-password
+               --
+               askpass: Password for '\''https://user-overwrite@example.com'\'':
+               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=user-distinct-pass
+               password=pass1
+               EOF
+               check reject $HELPER <<-\EOF &&
+               protocol=https
+               host=example.com
+               username=user-distinct-pass
+               password=pass2
+               EOF
+               check fill $HELPER <<-\EOF
+               protocol=https
+               host=example.com
+               username=user-distinct-pass
+               --
+               protocol=https
+               host=example.com
+               username=user-distinct-pass
+               password=pass1
+               EOF
+       '
+
        test_expect_success "helper ($HELPER) can forget user" '
                check reject $HELPER <<-\EOF &&
                protocol=https