| Message ID | 35ee1795bcdb974cdb9ca0f55ddf8f8a5bc562ae.1686778838.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | credential: improvements to erase in helpers | expand |
On Wed, Jun 14, 2023 at 09:40:37PM +0000, M Hickford via GitGitGadget wrote: > From: M Hickford <mirth.hickford@gmail.com> > > 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. I think this goal makes sense. I missed the "multiple helpers" part of your message at first, and wondered how you would even get such duplicate entries stored in a helper. But the point is that you might have two helpers with two separate passwords. Which is...weird, I'd think, because we will only ever use one of them. But from past discussions, I guess you're in a situation where you could use some kind of renewable token _or_ possibly a more stable password, via two different helpers. And when the token expires, you want to be able to remove it from a caching helper without asking for the stable password to be dropped. I don't think it's strictly necessary to spell out the intended use in the commit message. The behavior you're proposing seems like the least-surprising thing to me in general (and it is only because the situation is rare that nobody complained about the existing behavior in the last decade). But it might not hurt to outline the case that we'd expect this to help. The other thing I'd watch out for is how this behavior would affect non-erase modes. Let's see... > diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c > index 756c5f02aef..82f376d3351 100644 > --- a/builtin/credential-cache--daemon.c > +++ b/builtin/credential-cache--daemon.c > @@ -33,12 +33,12 @@ 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; > [...] > @@ -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); OK, for a cache "get" we'll continue not to match the password. This shouldn't trigger in practice, as Git will stop asking helpers once it has a username and password, but it is good to retain the existing behavior here. > @@ -48,7 +48,7 @@ static void remove_credential(const struct credential *c) > { > struct credential_cache_entry *e; > > - e = lookup_credential(c); > + e = lookup_credential(c, c->password != NULL); > if (e) > e->expiration = 0; > } And this is erase, where we do want to respect it. I actually think passing an unconditional "1" here instead of the NULL check would be OK, as the CHECK() macro in credential_match() treats NULL as "always match". But I am also OK with making that logic explicit here. This made me wonder how "store" works, since it's not touched here. It's implemented as remove_credential(), plus an unconditional allocation via cache_credential(). It seems like overwriting would be broken, then, wouldn't it? If I store: https://user:pass1@example.com and then try to store: https://user:pass2@example.com then the call to remove_credential() will see a "struct credential" with the password set to "pass2". And because it always passes match_password, it will fail to remove it, and we'll end up with two. We should be able to test that pretty easily, like this: echo url=https://user:pass1@example.com | git credential-cache store echo url=https://user:pass2@example.com | git credential-cache store echo url=https://example.com | git credential-cache get Before your patch that will return "pass1". After, it returns "pass2", because the old credential is left in place. I think you'd want something like: diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c index 82f376d335..f64dd21d33 100644 --- a/builtin/credential-cache--daemon.c +++ b/builtin/credential-cache--daemon.c @@ -44,11 +44,11 @@ static struct credential_cache_entry *lookup_credential(const struct credential 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, c->password != NULL); + e = lookup_credential(c, match_password); if (e) e->expiration = 0; } @@ -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); } } Unless your goal really is to store two entries that differ only in their passwords within a single cache. That seems weird, though. Again, it might help if your use case was fleshed out a bit more. > diff --git a/builtin/credential-store.c b/builtin/credential-store.c > index 30c6ccf56c0..e0ae028b1c3 100644 > --- a/builtin/credential-store.c > +++ b/builtin/credential-store.c > @@ -13,12 +13,14 @@ 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 *), > + const char* op) > { > 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) { > @@ -29,8 +31,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); OK, so for credential-store we only kick in the new behavior for "erase". So it would not have the same quirk that I mentioned above. I do think your logic could likewise here be: const int match_password = !strcmp(op, "erase"); because credential_match() handles the NULL case already. (It's also unusual for us to declare non-parameter variables as "const"; it's not wrong to do so, so it's really just a style thing, but there's not much benefit). > @@ -60,7 +62,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, const char *op) > { > int timeout_ms = 1000; > > @@ -69,7 +71,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, op); > if (commit_lock_file(&credential_lock) < 0) > die_errno("unable to write credential store"); > } If we have to plumb through this "op" string, I wonder if it would be more flexible and robust to just pass along a match_password flag. That's generally a bit more robust than string matching (where a misspelling in one string would not be caught by the compiler, unlike a boolean variable). > @@ -221,6 +222,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 > + ' The test makes sense. We are not using multiple helpers, but the behavior can be checked using a single helper, since the point is that it should be independent of what any other helper is doing. -Peff
On Wed, 14 Jun 2023 at 23:44, Jeff King <peff@peff.net> wrote: > > On Wed, Jun 14, 2023 at 09:40:37PM +0000, M Hickford via GitGitGadget wrote: > > > From: M Hickford <mirth.hickford@gmail.com> > > > > 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. > > I think this goal makes sense. I missed the "multiple helpers" part of > your message at first, and wondered how you would even get such > duplicate entries stored in a helper. But the point is that you might > have two helpers with two separate passwords. > > Which is...weird, I'd think, because we will only ever use one of them. > But from past discussions, I guess you're in a situation where you could > use some kind of renewable token _or_ possibly a more stable password, > via two different helpers. And when the token expires, you want to be > able to remove it from a caching helper without asking for the stable > password to be dropped. > > I don't think it's strictly necessary to spell out the intended use in > the commit message. The behavior you're proposing seems like the > least-surprising thing to me in general (and it is only because the > situation is rare that nobody complained about the existing behavior in > the last decade). But it might not hurt to outline the case that we'd > expect this to help. > > The other thing I'd watch out for is how this behavior would affect > non-erase modes. Let's see... > > > diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c > > index 756c5f02aef..82f376d3351 100644 > > --- a/builtin/credential-cache--daemon.c > > +++ b/builtin/credential-cache--daemon.c > > @@ -33,12 +33,12 @@ 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; > > [...] > > @@ -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); > > OK, for a cache "get" we'll continue not to match the password. This > shouldn't trigger in practice, as Git will stop asking helpers once it > has a username and password, but it is good to retain the existing > behavior here. > > > @@ -48,7 +48,7 @@ static void remove_credential(const struct credential *c) > > { > > struct credential_cache_entry *e; > > > > - e = lookup_credential(c); > > + e = lookup_credential(c, c->password != NULL); > > if (e) > > e->expiration = 0; > > } > > And this is erase, where we do want to respect it. I actually think > passing an unconditional "1" here instead of the NULL check would be OK, > as the CHECK() macro in credential_match() treats NULL as "always > match". But I am also OK with making that logic explicit here. > > This made me wonder how "store" works, since it's not touched here. It's > implemented as remove_credential(), plus an unconditional allocation via > cache_credential(). > > It seems like overwriting would be broken, then, wouldn't it? If I Thanks Jeff for spotting and for the fix. I'll apply and add a test in patch v3. > store: > > https://user:pass1@example.com > > and then try to store: > > https://user:pass2@example.com > > then the call to remove_credential() will see a "struct credential" with > the password set to "pass2". And because it always passes > match_password, it will fail to remove it, and we'll end up with two. > > We should be able to test that pretty easily, like this: > > echo url=https://user:pass1@example.com | git credential-cache store > echo url=https://user:pass2@example.com | git credential-cache store > echo url=https://example.com | git credential-cache get > > Before your patch that will return "pass1". After, it returns "pass2", > because the old credential is left in place. I think you'd want > something like: > > diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c > index 82f376d335..f64dd21d33 100644 > --- a/builtin/credential-cache--daemon.c > +++ b/builtin/credential-cache--daemon.c > @@ -44,11 +44,11 @@ static struct credential_cache_entry *lookup_credential(const struct credential > 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, c->password != NULL); > + e = lookup_credential(c, match_password); > if (e) > e->expiration = 0; > } > @@ -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); > } > } > > Unless your goal really is to store two entries that differ only in > their passwords within a single cache. That seems weird, though. Again, > it might help if your use case was fleshed out a bit more. > > > diff --git a/builtin/credential-store.c b/builtin/credential-store.c > > index 30c6ccf56c0..e0ae028b1c3 100644 > > --- a/builtin/credential-store.c > > +++ b/builtin/credential-store.c > > @@ -13,12 +13,14 @@ 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 *), > > + const char* op) > > { > > 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) { > > @@ -29,8 +31,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); > > OK, so for credential-store we only kick in the new behavior for > "erase". So it would not have the same quirk that I mentioned above. > > I do think your logic could likewise here be: > > const int match_password = !strcmp(op, "erase"); > > because credential_match() handles the NULL case already. > > (It's also unusual for us to declare non-parameter variables as "const"; > it's not wrong to do so, so it's really just a style thing, but there's > not much benefit). > > > @@ -60,7 +62,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, const char *op) > > { > > int timeout_ms = 1000; > > > > @@ -69,7 +71,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, op); > > if (commit_lock_file(&credential_lock) < 0) > > die_errno("unable to write credential store"); > > } > > If we have to plumb through this "op" string, I wonder if it would be > more flexible and robust to just pass along a match_password flag. > That's generally a bit more robust than string matching (where a > misspelling in one string would not be caught by the compiler, unlike a > boolean variable). Good idea. Will do in patch v3. > > > @@ -221,6 +222,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 > > + ' > > The test makes sense. We are not using multiple helpers, but the > behavior can be checked using a single helper, since the point is that > it should be independent of what any other helper is doing. > > -Peff
diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c index 756c5f02aef..82f376d3351 100644 --- a/builtin/credential-cache--daemon.c +++ b/builtin/credential-cache--daemon.c @@ -33,12 +33,12 @@ 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; @@ -48,7 +48,7 @@ static void remove_credential(const struct credential *c) { struct credential_cache_entry *e; - e = lookup_credential(c); + e = lookup_credential(c, c->password != NULL); 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); diff --git a/builtin/credential-store.c b/builtin/credential-store.c index 30c6ccf56c0..e0ae028b1c3 100644 --- a/builtin/credential-store.c +++ b/builtin/credential-store.c @@ -13,12 +13,14 @@ 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 *), + const char* op) { 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) { @@ -29,8 +31,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 +62,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, const char *op) { int timeout_ms = 1000; @@ -69,7 +71,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, op); if (commit_lock_file(&credential_lock) < 0) die_errno("unable to write credential store"); } @@ -91,7 +93,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, "store"); strbuf_release(&buf); } @@ -138,7 +140,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, "erase"); } static void lookup_credential(const struct string_list *fns, struct credential *c) @@ -146,7 +148,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, "get")) 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..f7e4e29c5e1 100644 --- a/t/lib-credential.sh +++ b/t/lib-credential.sh @@ -44,6 +44,7 @@ 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 http path.tld user reject $1 https timeout.tld user reject $1 https sso.tld @@ -221,6 +222,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