* [PATCH] credential: new attribute password_expiry_utc @ 2023-01-28 14:04 M Hickford via GitGitGadget 2023-01-29 20:17 ` Junio C Hamano ` (2 more replies) 0 siblings, 3 replies; 26+ messages in thread From: M Hickford via GitGitGadget @ 2023-01-28 14:04 UTC (permalink / raw) To: git; +Cc: M Hickford, M Hickford From: M Hickford <mirth.hickford@gmail.com> If password has expired, credential fill no longer returns early, so later helpers can generate a fresh credential. This is backwards compatible -- no change in behaviour with helpers that discard the expiry attribute. The expiry logic is entirely in the git credential layer; compatible helpers simply store and return the expiry attribute verbatim. Store new attribute in cache. Signed-off-by: M Hickford <mirth.hickford@gmail.com> --- credential: new attribute password_expiry_utc Some passwords, such as a personal access token or OAuth access token, may have an expiry date (as long as years for PATs or as short as hours for an OAuth access token). Add a new credential attribute password_expiry_utc. If password has expired, credential fill no longer returns early, so later helpers have opportunity to generate a fresh credential. This is backwards compatible -- no change in behaviour with helpers that discard the expiry attribute. The expiry logic is entirely in the git credential layer. Credential-generating helpers need only output the expiry attribute. Storage helpers should store the expiry if they can. Store expiry attribute in cache. This is particularly useful when a storage helper and a credential-generating helper are configured together, eg. [credential] helper = storage # eg. cache or osxkeychain helper = generate # eg. oauth Without this patch, credential fill may return an expired credential from storage, causing authentication to fail. With this patch: a fresh credential is generated if and only if the credential is expired. Example usage in a credential-generating helper https://github.com/hickford/git-credential-oauth/pull/16/files Signed-off-by: M Hickford mirth.hickford@gmail.com Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1443%2Fhickford%2Fpassword-expiry-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1443/hickford/password-expiry-v1 Pull-Request: https://github.com/git/git/pull/1443 Documentation/git-credential.txt | 4 ++++ builtin/credential-cache--daemon.c | 3 +++ credential.c | 21 +++++++++++++++++++++ credential.h | 1 + 4 files changed, 29 insertions(+) diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt index ac2818b9f66..15ace648bdd 100644 --- a/Documentation/git-credential.txt +++ b/Documentation/git-credential.txt @@ -144,6 +144,10 @@ Git understands the following attributes: The credential's password, if we are asking it to be stored. +`password_expiry_utc`:: + + If password is a personal access token or OAuth access token, it may have an expiry date. When getting credentials from a helper, `git credential fill` ignores the password attribute if the expiry date has passed. Storage helpers should store this attribute if possible. Helpers should not implement expiry logic themselves. Represented as Unix time UTC, seconds since 1970. + `url`:: When this special attribute is read by `git credential`, the diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c index f3c89831d4a..5cb8a186b45 100644 --- a/builtin/credential-cache--daemon.c +++ b/builtin/credential-cache--daemon.c @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out) if (e) { fprintf(out, "username=%s\n", e->item.username); fprintf(out, "password=%s\n", e->item.password); + if (e->item.password_expiry_utc != 0) { + fprintf(out, "password_expiry_utc=%ld\n", e->item.password_expiry_utc); + } } } else if (!strcmp(action.buf, "exit")) { diff --git a/credential.c b/credential.c index f6389a50684..0a3a9cbf0a2 100644 --- a/credential.c +++ b/credential.c @@ -7,6 +7,7 @@ #include "prompt.h" #include "sigchain.h" #include "urlmatch.h" +#include <time.h> void credential_init(struct credential *c) { @@ -21,6 +22,7 @@ void credential_clear(struct credential *c) free(c->path); free(c->username); free(c->password); + c->password_expiry_utc = 0; string_list_clear(&c->helpers, 0); credential_init(c); @@ -234,11 +236,23 @@ int credential_read(struct credential *c, FILE *fp) } else if (!strcmp(key, "path")) { free(c->path); c->path = xstrdup(value); + } else if (!strcmp(key, "password_expiry_utc")) { + // TODO: ignore if can't parse integer + c->password_expiry_utc = atoi(value); } else if (!strcmp(key, "url")) { credential_from_url(c, value); } else if (!strcmp(key, "quit")) { c->quit = !!git_config_bool("quit", value); } + + // if expiry date has passed, ignore password and expiry fields + if (c->password_expiry_utc != 0 && time(NULL) > c->password_expiry_utc) { + trace_printf(_("Password has expired.\n")); + FREE_AND_NULL(c->username); + FREE_AND_NULL(c->password); + c->password_expiry_utc = 0; + } + /* * Ignore other lines; we don't know what they mean, but * this future-proofs us when later versions of git do @@ -269,6 +283,13 @@ void credential_write(const struct credential *c, FILE *fp) credential_write_item(fp, "path", c->path, 0); credential_write_item(fp, "username", c->username, 0); credential_write_item(fp, "password", c->password, 0); + if (c->password_expiry_utc != 0) { + int length = snprintf( NULL, 0, "%ld", c->password_expiry_utc); + char* str = malloc( length + 1 ); + snprintf( str, length + 1, "%ld", c->password_expiry_utc ); + credential_write_item(fp, "password_expiry_utc", str, 0); + free(str); + } } static int run_credential_helper(struct credential *c, diff --git a/credential.h b/credential.h index f430e77fea4..e10f7c2b313 100644 --- a/credential.h +++ b/credential.h @@ -126,6 +126,7 @@ struct credential { char *protocol; char *host; char *path; + time_t password_expiry_utc; }; #define CREDENTIAL_INIT { \ base-commit: 5cc9858f1b470844dea5c5d3e936af183fdf2c68 -- gitgitgadget ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH] credential: new attribute password_expiry_utc 2023-01-28 14:04 [PATCH] credential: new attribute password_expiry_utc M Hickford via GitGitGadget @ 2023-01-29 20:17 ` Junio C Hamano 2023-02-01 8:29 ` M Hickford 2023-01-30 0:59 ` Eric Sunshine 2023-02-01 9:39 ` [PATCH v2] " M Hickford via GitGitGadget 2 siblings, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2023-01-29 20:17 UTC (permalink / raw) To: M Hickford via GitGitGadget; +Cc: git, M Hickford "M Hickford via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: M Hickford <mirth.hickford@gmail.com> > > If password has expired, credential fill no longer returns early, > so later helpers can generate a fresh credential. This is backwards > compatible -- no change in behaviour with helpers that discard the > expiry attribute. The expiry logic is entirely in the git credential > layer; compatible helpers simply store and return the expiry > attribute verbatim. > > Store new attribute in cache. It is unclear what you are describing in the above. The current behaviour without the patch? The behaviour of the code if this patch gets applied? Write it in such a way that it is clear why the patch is a good idea, not just "this would not hurt because it is backwards compatible". The usual way to do so is to sell your change in this order: - Give background information to help readers understand what you are going to write in the following explanation. - Describe the current behaviour without any change to the code; - Present a situation where the current code results in an undesirable outcome. What exactly happens, what visible effect it has to the user, how the code could do better to help the user? - Propose an updated behaviour that would behave better in the above sample situation presented. Curiously, what you wrote below the "---" line, that will not be part of the log message, looks to be organized better than the above. The first paragraph (except for the "Add ...") prepares the readers, It is still unclear if the second paragraph "when expired" describes what happens with the current code (i.e. highlighting why a change is needed) or what you want to happen with the patch, but the paragraph should first explain the problem in the current behaviour to motivate readers to learn why the updated code would lead to a better world. And follow that with the behaviour of the updated code and its effect (e.g. "without first trying a credential that is stale and see it fail before asking to reauthenticate, such a known-to-be-stale credential gets discarded automatically"). > +`password_expiry_utc`:: > + > + If password is a personal access token or OAuth access token, it may have an expiry date. When getting credentials from a helper, `git credential fill` ignores the password attribute if the expiry date has passed. Storage helpers should store this attribute if possible. Helpers should not implement expiry logic themselves. Represented as Unix time UTC, seconds since 1970. > + A overly long line. Please follow Documentation/CodingGuidelines and Documentation/SubmittingPatches > diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c > index f3c89831d4a..5cb8a186b45 100644 > --- a/builtin/credential-cache--daemon.c > +++ b/builtin/credential-cache--daemon.c > @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out) > if (e) { > fprintf(out, "username=%s\n", e->item.username); > fprintf(out, "password=%s\n", e->item.password); > + if (e->item.password_expiry_utc != 0) { > + fprintf(out, "password_expiry_utc=%ld\n", e->item.password_expiry_utc); > + } Style (multiple issues, check CodingGuidelines): if (e->item.password_expiry_utc) fprintf(out, "... overly long format template ...", e->item.password_expiry_utc); * Using integral value or pointer value as a truth value does not require an explicit comparison with 0; * A single-statement block does not need {} around it; * Overly long line should be folded, with properly indented. > diff --git a/credential.c b/credential.c > index f6389a50684..0a3a9cbf0a2 100644 > --- a/credential.c > +++ b/credential.c > @@ -7,6 +7,7 @@ > #include "prompt.h" > #include "sigchain.h" > #include "urlmatch.h" > +#include <time.h> Don't include system headers directly; often git-compat-util.h already has it, and if not, we need to find the right place to have it in git-compat-util.h file, as there are platforms that are finicky in inclusion order of the header files and definition of feature macros. > @@ -21,6 +22,7 @@ void credential_clear(struct credential *c) > free(c->path); > free(c->username); > free(c->password); > + c->password_expiry_utc = 0; Not a huge deal, but if the rule is "an credential with expiry timestamp that is too old behaves as if it no longer exists or is valid", then a large integer, not zero, may serve as a better sentinel value for "this entry never expires". Instead of having to do if (expiry && expiry < time()) { ... expired ... } you can just do if (expiry < time()) { ... expired ... } and that would be simpler to understand for human readers, too. > @@ -234,11 +236,23 @@ int credential_read(struct credential *c, FILE *fp) > } else if (!strcmp(key, "path")) { > free(c->path); > c->path = xstrdup(value); > + } else if (!strcmp(key, "password_expiry_utc")) { > + // TODO: ignore if can't parse integer Do not use // comment. /* Our single-liner comment reads like this */ > + c->password_expiry_utc = atoi(value); Don't use atoi(); make sure value is not followed by a non-number, e.g. const char *value = "43q"; printf("%d<%s>\n", atoi(value), value); would give you 43<43q>, but you want to reject and silently ignore such an expiry timestamp. > + // if expiry date has passed, ignore password and expiry fields Ditto, but if you used a large value as sentinel for "never expires" and wrote it like this if (c->password_expiry_utc < time(NULL)) { then it is clear enough that you do not even need such a comment. The expression itself makes it clear what is going on (i.e. the current time comes later than the expiry_utc value on the number line hence it appears on the right to it, clearly showing that it has passed the threshold). > + if (c->password_expiry_utc != 0 && time(NULL) > c->password_expiry_utc) { > + trace_printf(_("Password has expired.\n")); > + FREE_AND_NULL(c->username); > + FREE_AND_NULL(c->password); > + c->password_expiry_utc = 0; > + } > + ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] credential: new attribute password_expiry_utc 2023-01-29 20:17 ` Junio C Hamano @ 2023-02-01 8:29 ` M Hickford 2023-02-01 18:50 ` Junio C Hamano 0 siblings, 1 reply; 26+ messages in thread From: M Hickford @ 2023-02-01 8:29 UTC (permalink / raw) To: Junio C Hamano, Jeff King; +Cc: M Hickford via GitGitGadget, git, M Hickford On Sun, 29 Jan 2023 at 20:17, Junio C Hamano <gitster@pobox.com> wrote: > > "M Hickford via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: M Hickford <mirth.hickford@gmail.com> > > > > If password has expired, credential fill no longer returns early, > > so later helpers can generate a fresh credential. This is backwards > > compatible -- no change in behaviour with helpers that discard the > > expiry attribute. The expiry logic is entirely in the git credential > > layer; compatible helpers simply store and return the expiry > > attribute verbatim. > > > > Store new attribute in cache. > > It is unclear what you are describing in the above. The current > behaviour without the patch? The behaviour of the code if this > patch gets applied? Write it in such a way that it is clear why > the patch is a good idea, not just "this would not hurt because it > is backwards compatible". > > The usual way to do so is to sell your change in this order: > > - Give background information to help readers understand what you > are going to write in the following explanation. > > - Describe the current behaviour without any change to the code; > > - Present a situation where the current code results in an > undesirable outcome. What exactly happens, what visible effect it > has to the user, how the code could do better to help the user? > > - Propose an updated behaviour that would behave better in the > above sample situation presented. > Thanks for the guidance. Writing a better commit message clarified my own thoughts. > Curiously, what you wrote below the "---" line, that will not be > part of the log message, looks to be organized better than the > above. The first paragraph (except for the "Add ...") prepares the > readers, It is still unclear if the second paragraph "when expired" > describes what happens with the current code (i.e. highlighting why > a change is needed) or what you want to happen with the patch, but > the paragraph should first explain the problem in the current > behaviour to motivate readers to learn why the updated code would > lead to a better world. And follow that with the behaviour of the > updated code and its effect (e.g. "without first trying a credential > that is stale and see it fail before asking to reauthenticate, such > a known-to-be-stale credential gets discarded automatically"). > > > > +`password_expiry_utc`:: > > + > > + If password is a personal access token or OAuth access token, it may have an expiry date. When getting credentials from a helper, `git credential fill` ignores the password attribute if the expiry date has passed. Storage helpers should store this attribute if possible. Helpers should not implement expiry logic themselves. Represented as Unix time UTC, seconds since 1970. > > + > > A overly long line. Please follow Documentation/CodingGuidelines > and Documentation/SubmittingPatches > > > diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c > > index f3c89831d4a..5cb8a186b45 100644 > > --- a/builtin/credential-cache--daemon.c > > +++ b/builtin/credential-cache--daemon.c > > @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out) > > if (e) { > > fprintf(out, "username=%s\n", e->item.username); > > fprintf(out, "password=%s\n", e->item.password); > > + if (e->item.password_expiry_utc != 0) { > > + fprintf(out, "password_expiry_utc=%ld\n", e->item.password_expiry_utc); > > + } > > Style (multiple issues, check CodingGuidelines): > > if (e->item.password_expiry_utc) > fprintf(out, "... overly long format template ...", > e->item.password_expiry_utc); > > * Using integral value or pointer value as a truth value does not > require an explicit comparison with 0; > > * A single-statement block does not need {} around it; > > * Overly long line should be folded, with properly indented. > > > diff --git a/credential.c b/credential.c > > index f6389a50684..0a3a9cbf0a2 100644 > > --- a/credential.c > > +++ b/credential.c > > @@ -7,6 +7,7 @@ > > #include "prompt.h" > > #include "sigchain.h" > > #include "urlmatch.h" > > +#include <time.h> > > Don't include system headers directly; often git-compat-util.h > already has it, and if not, we need to find the right place to have > it in git-compat-util.h file, as there are platforms that are > finicky in inclusion order of the header files and definition of > feature macros. > > > @@ -21,6 +22,7 @@ void credential_clear(struct credential *c) > > free(c->path); > > free(c->username); > > free(c->password); > > + c->password_expiry_utc = 0; > > Not a huge deal, but if the rule is "an credential with expiry > timestamp that is too old behaves as if it no longer exists or is > valid", then a large integer, not zero, may serve as a better > sentinel value for "this entry never expires". Instead of having to > do > > if (expiry && expiry < time()) { > ... expired ... > } > > you can just do > > if (expiry < time()) { > ... expired ... > } > > and that would be simpler to understand for human readers, too. > > > @@ -234,11 +236,23 @@ int credential_read(struct credential *c, FILE *fp) > > } else if (!strcmp(key, "path")) { > > free(c->path); > > c->path = xstrdup(value); > > + } else if (!strcmp(key, "password_expiry_utc")) { > > + // TODO: ignore if can't parse integer > > Do not use // comment. /* Our single-liner comment reads like this */ > > > + c->password_expiry_utc = atoi(value); > > Don't use atoi(); make sure value is not followed by a non-number, > e.g. > > const char *value = "43q"; > printf("%d<%s>\n", atoi(value), value); > > would give you 43<43q>, but you want to reject and silently ignore > such an expiry timestamp. > > > + // if expiry date has passed, ignore password and expiry fields > > Ditto, but if you used a large value as sentinel for "never expires" > and wrote it like this > > if (c->password_expiry_utc < time(NULL)) { > > then it is clear enough that you do not even need such a comment. > The expression itself makes it clear what is going on (i.e. the > current time comes later than the expiry_utc value on the number > line hence it appears on the right to it, clearly showing that it > has passed the threshold). > > > + if (c->password_expiry_utc != 0 && time(NULL) > c->password_expiry_utc) { > > + trace_printf(_("Password has expired.\n")); > > + FREE_AND_NULL(c->username); > > + FREE_AND_NULL(c->password); > > + c->password_expiry_utc = 0; > > + } > > + ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] credential: new attribute password_expiry_utc 2023-02-01 8:29 ` M Hickford @ 2023-02-01 18:50 ` Junio C Hamano 0 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2023-02-01 18:50 UTC (permalink / raw) To: M Hickford; +Cc: Jeff King, M Hickford via GitGitGadget, git M Hickford <mirth.hickford@gmail.com> writes: > Thanks for the guidance. Writing a better commit message clarified my > own thoughts. It is great to hear that somebody agreeing with me on this point. We try to write in a way that would make it clear to readers of "git log", but I often notice that trying to explain the changes to the code clearly does force me to realize better ways to write the changes. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] credential: new attribute password_expiry_utc 2023-01-28 14:04 [PATCH] credential: new attribute password_expiry_utc M Hickford via GitGitGadget 2023-01-29 20:17 ` Junio C Hamano @ 2023-01-30 0:59 ` Eric Sunshine 2023-02-05 6:49 ` M Hickford 2023-02-01 9:39 ` [PATCH v2] " M Hickford via GitGitGadget 2 siblings, 1 reply; 26+ messages in thread From: Eric Sunshine @ 2023-01-30 0:59 UTC (permalink / raw) To: M Hickford via GitGitGadget; +Cc: git, M Hickford On Sat, Jan 28, 2023 at 9:08 AM M Hickford via GitGitGadget <gitgitgadget@gmail.com> wrote: > If password has expired, credential fill no longer returns early, > so later helpers can generate a fresh credential. This is backwards > compatible -- no change in behaviour with helpers that discard the > expiry attribute. The expiry logic is entirely in the git credential > layer; compatible helpers simply store and return the expiry > attribute verbatim. > > Store new attribute in cache. > > Signed-off-by: M Hickford <mirth.hickford@gmail.com> Just a few comments in addition to those already provided by Junio... > diff --git a/credential.c b/credential.c > @@ -234,11 +236,23 @@ int credential_read(struct credential *c, FILE *fp) > + // if expiry date has passed, ignore password and expiry fields > + if (c->password_expiry_utc != 0 && time(NULL) > c->password_expiry_utc) { > + trace_printf(_("Password has expired.\n")); Using `_(...)` marks a string for localization, but doing so is undesirable for debugging messages which are meant for the developer, not the end user (and it creates extra work for translators). No existing[1] trace_printf() calls in the codebase use `_(...)`. [1]: Unfortunately, a couple examples exist in Documentation/MyFirstObjectWalk.txt using `_(...)` but they should be removed. > @@ -269,6 +283,13 @@ void credential_write(const struct credential *c, FILE *fp) > + if (c->password_expiry_utc != 0) { > + int length = snprintf( NULL, 0, "%ld", c->password_expiry_utc); > + char* str = malloc( length + 1 ); Style in this project is `char *str`, not `char* str`. Also, drop spaces around function arguments: char *str = malloc(length + 1); > + snprintf( str, length + 1, "%ld", c->password_expiry_utc ); Same. > + credential_write_item(fp, "password_expiry_utc", str, 0); > + free(str); > + } xstrfmt() from strbuf.h can help simplify this entire block: char *s = xstrfmt("%ld", c->password_expiry_utc); credential_write_item(fp, "password_expiry_utc", str, 0); free(s); ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] credential: new attribute password_expiry_utc 2023-01-30 0:59 ` Eric Sunshine @ 2023-02-05 6:49 ` M Hickford 0 siblings, 0 replies; 26+ messages in thread From: M Hickford @ 2023-02-05 6:49 UTC (permalink / raw) To: Eric Sunshine; +Cc: M Hickford via GitGitGadget, git, M Hickford Thanks Eric for the review On Mon, 30 Jan 2023 at 00:59, Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Sat, Jan 28, 2023 at 9:08 AM M Hickford via GitGitGadget > <gitgitgadget@gmail.com> wrote: > > If password has expired, credential fill no longer returns early, > > so later helpers can generate a fresh credential. This is backwards > > compatible -- no change in behaviour with helpers that discard the > > expiry attribute. The expiry logic is entirely in the git credential > > layer; compatible helpers simply store and return the expiry > > attribute verbatim. > > > > Store new attribute in cache. > > > > Signed-off-by: M Hickford <mirth.hickford@gmail.com> > > Just a few comments in addition to those already provided by Junio... > > > diff --git a/credential.c b/credential.c > > @@ -234,11 +236,23 @@ int credential_read(struct credential *c, FILE *fp) > > + // if expiry date has passed, ignore password and expiry fields > > + if (c->password_expiry_utc != 0 && time(NULL) > c->password_expiry_utc) { > > + trace_printf(_("Password has expired.\n")); > > Using `_(...)` marks a string for localization, but doing so is > undesirable for debugging messages which are meant for the developer, > not the end user (and it creates extra work for translators). No > existing[1] trace_printf() calls in the codebase use `_(...)`. Done in patch v3. > > [1]: Unfortunately, a couple examples exist in > Documentation/MyFirstObjectWalk.txt using `_(...)` but they should be > removed. > > > @@ -269,6 +283,13 @@ void credential_write(const struct credential *c, FILE *fp) > > + if (c->password_expiry_utc != 0) { > > + int length = snprintf( NULL, 0, "%ld", c->password_expiry_utc); > > + char* str = malloc( length + 1 ); > > Style in this project is `char *str`, not `char* str`. Also, drop > spaces around function arguments: > > char *str = malloc(length + 1); > > > + snprintf( str, length + 1, "%ld", c->password_expiry_utc ); > > Same. > > > + credential_write_item(fp, "password_expiry_utc", str, 0); > > + free(str); > > + } > > xstrfmt() from strbuf.h can help simplify this entire block: > > char *s = xstrfmt("%ld", c->password_expiry_utc); > credential_write_item(fp, "password_expiry_utc", str, 0); > free(s); Neat. Done in patch v3. ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2] credential: new attribute password_expiry_utc 2023-01-28 14:04 [PATCH] credential: new attribute password_expiry_utc M Hickford via GitGitGadget 2023-01-29 20:17 ` Junio C Hamano 2023-01-30 0:59 ` Eric Sunshine @ 2023-02-01 9:39 ` M Hickford via GitGitGadget 2023-02-01 12:10 ` Jeff King 2023-02-04 21:16 ` [PATCH v3] " M Hickford via GitGitGadget 2 siblings, 2 replies; 26+ messages in thread From: M Hickford via GitGitGadget @ 2023-02-01 9:39 UTC (permalink / raw) To: git Cc: Eric Sunshine, Jeff King, Cheetham, Dennington, M Hickford, M Hickford From: M Hickford <mirth.hickford@gmail.com> Some passwords have an expiry date known at generation. This may be years away for a personal access token or hours for an OAuth access token. Currently the credential protocol has no expiry attribute. When multiple helpers are configured, `credential fill` tries each helper in turn until it has a username and password, returning early. When a storage helper and a credential-generating helper are configured together, the credential is necessarily stored without expiry, so `credential fill` may later return an expired credential from storage. ``` [credential] helper = storage # eg. cache or osxkeychain helper = generate # eg. oauth ``` An improvement is to introduce a password expiry attribute to the credential protocol. If the expiry date has passed, `credential fill` ignores the password attribute, so subsequent helpers can generate a fresh credential. This is backwards compatible -- no change in behaviour with helpers that discard the expiry attribute. Note that the expiry logic is entirely within the credential layer. Compatible helpers store and retrieve the new attribute like any other. This keeps the helper contract simple. This patch adds support for the new attribute to cache. Example usage in a credential-generating helper https://github.com/hickford/git-credential-oauth/pull/16 Future ideas: make it possible for a storage helper to provide OAuth refresh token to subsequent helpers. https://github.com/gitgitgadget/git/pull/1394 Signed-off-by: M Hickford <mirth.hickford@gmail.com> --- credential: new attribute password_expiry_utc Some passwords have an expiry date known at generation. This may be years away for a personal access token or hours for an OAuth access token. Currently the credential protocol has no expiry attribute. When multiple helpers are configured, credential fill tries each helper in turn until it has a username and password, returning early. When a storage helper and a credential-generating helper are configured together, the credential is necessarily stored without expiry, so credential fill may later return an expired credential from storage. [credential] helper = storage # eg. cache or osxkeychain helper = generate # eg. oauth An improvement is to introduce a password expiry attribute to the credential protocol. If the password has expired, credential fill no longer returns early, so subsequent helpers can generate a fresh credential. This is backwards compatible -- no change in behaviour with helpers that discard the expiry attribute. Note that the expiry logic is entirely within the credential layer. Compatible helpers store and retrieve the new attribute like any other. This keeps the helper contract simple. This patch adds support for the new attribute to cache. Example usage in a credential-generating helper https://github.com/hickford/git-credential-oauth/pull/16 Future ideas: make it possible for a storage helper to provide OAuth refresh token to subsequent helpers. https://github.com/gitgitgadget/git/pull/1394 Questions for reviewers: * Does the behaviour implemented match the documentation? (I'm not famiiliar with C) * Any edge cases? * How to test in t0300-credentials.sh ? Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1443%2Fhickford%2Fpassword-expiry-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1443/hickford/password-expiry-v2 Pull-Request: https://github.com/git/git/pull/1443 Range-diff vs v1: 1: 184b0aa6514 ! 1: b9ee729ee4d credential: new attribute password_expiry_utc @@ Metadata ## Commit message ## credential: new attribute password_expiry_utc - If password has expired, credential fill no longer returns early, - so later helpers can generate a fresh credential. This is backwards - compatible -- no change in behaviour with helpers that discard the - expiry attribute. The expiry logic is entirely in the git credential - layer; compatible helpers simply store and return the expiry - attribute verbatim. + Some passwords have an expiry date known at generation. This may be + years away for a personal access token or hours for an OAuth access + token. - Store new attribute in cache. + Currently the credential protocol has no expiry attribute. When multiple + helpers are configured, `credential fill` tries each helper in turn + until it has a username and password, returning early. + + When a storage helper and a credential-generating helper are configured + together, the credential is necessarily stored without expiry, so + `credential fill` may later return an expired credential from storage. + + ``` + [credential] + helper = storage # eg. cache or osxkeychain + helper = generate # eg. oauth + ``` + + An improvement is to introduce a password expiry attribute to the + credential protocol. If the expiry date has passed, `credential fill` + ignores the password attribute, so subsequent helpers can generate a + fresh credential. This is backwards compatible -- no change in + behaviour with helpers that discard the expiry attribute. + + Note that the expiry logic is entirely within the credential layer. + Compatible helpers store and retrieve the new attribute like any other. + This keeps the helper contract simple. + + This patch adds support for the new attribute to cache. + + Example usage in a credential-generating helper + https://github.com/hickford/git-credential-oauth/pull/16 + + Future ideas: make it possible for a storage helper to provide OAuth + refresh token to subsequent helpers. + https://github.com/gitgitgadget/git/pull/1394 Signed-off-by: M Hickford <mirth.hickford@gmail.com> @@ Documentation/git-credential.txt: Git understands the following attributes: +`password_expiry_utc`:: + -+ If password is a personal access token or OAuth access token, it may have an expiry date. When getting credentials from a helper, `git credential fill` ignores the password attribute if the expiry date has passed. Storage helpers should store this attribute if possible. Helpers should not implement expiry logic themselves. Represented as Unix time UTC, seconds since 1970. ++ If password is a personal access token or OAuth access token, it may have an ++ expiry date. When getting credentials from a helper, `git credential fill` ++ ignores the password attribute if the expiry date has passed. Storage ++ helpers should store this attribute if possible. Helpers should not ++ implement expiry logic themselves. Represented as Unix time UTC, seconds ++ since 1970. + `url`:: @@ 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); -+ if (e->item.password_expiry_utc != 0) { -+ fprintf(out, "password_expiry_utc=%ld\n", e->item.password_expiry_utc); -+ } ++ if (e->item.password_expiry_utc != TIME_MAX) ++ fprintf(out, "password_expiry_utc=%"PRItime"\n", ++ e->item.password_expiry_utc); } } else if (!strcmp(action.buf, "exit")) { @@ credential.c #include "prompt.h" #include "sigchain.h" #include "urlmatch.h" -+#include <time.h> ++#include "git-compat-util.h" void credential_init(struct credential *c) { -@@ credential.c: void credential_clear(struct credential *c) - free(c->path); - free(c->username); - free(c->password); -+ c->password_expiry_utc = 0; - string_list_clear(&c->helpers, 0); +@@ credential.c: static void credential_getpass(struct credential *c) + if (!c->username) + c->username = credential_ask_one("Username", c, + PROMPT_ASKPASS|PROMPT_ECHO); +- if (!c->password) ++ if (!c->password || c->password_expiry_utc < time(NULL)) { ++ c->password_expiry_utc = TIME_MAX; + c->password = credential_ask_one("Password", c, + PROMPT_ASKPASS); ++ } + } + + int credential_read(struct credential *c, FILE *fp) + { + struct strbuf line = STRBUF_INIT; - credential_init(c); ++ int password_updated = 0; ++ timestamp_t this_password_expiry = TIME_MAX; ++ + while (strbuf_getline(&line, fp) != EOF) { + char *key = line.buf; + char *value = strchr(key, '='); +@@ credential.c: int credential_read(struct credential *c, FILE *fp) + } else if (!strcmp(key, "password")) { + free(c->password); + c->password = xstrdup(value); ++ password_updated = 1; + } else if (!strcmp(key, "protocol")) { + free(c->protocol); + c->protocol = xstrdup(value); @@ credential.c: int credential_read(struct credential *c, FILE *fp) } else if (!strcmp(key, "path")) { free(c->path); c->path = xstrdup(value); + } else if (!strcmp(key, "password_expiry_utc")) { -+ // TODO: ignore if can't parse integer -+ c->password_expiry_utc = atoi(value); ++ this_password_expiry = parse_timestamp(value, NULL, 10); ++ if (this_password_expiry == 0 || errno) { ++ this_password_expiry = TIME_MAX; ++ } } else if (!strcmp(key, "url")) { credential_from_url(c, value); } else if (!strcmp(key, "quit")) { - c->quit = !!git_config_bool("quit", value); - } -+ -+ // if expiry date has passed, ignore password and expiry fields -+ if (c->password_expiry_utc != 0 && time(NULL) > c->password_expiry_utc) { -+ trace_printf(_("Password has expired.\n")); -+ FREE_AND_NULL(c->username); -+ FREE_AND_NULL(c->password); -+ c->password_expiry_utc = 0; -+ } +@@ credential.c: int credential_read(struct credential *c, FILE *fp) + */ + } + ++ if (password_updated) ++ c->password_expiry_utc = this_password_expiry; + - /* - * Ignore other lines; we don't know what they mean, but - * this future-proofs us when later versions of git do + strbuf_release(&line); + return 0; + } @@ credential.c: void credential_write(const struct credential *c, FILE *fp) credential_write_item(fp, "path", c->path, 0); credential_write_item(fp, "username", c->username, 0); credential_write_item(fp, "password", c->password, 0); -+ if (c->password_expiry_utc != 0) { -+ int length = snprintf( NULL, 0, "%ld", c->password_expiry_utc); -+ char* str = malloc( length + 1 ); -+ snprintf( str, length + 1, "%ld", c->password_expiry_utc ); -+ credential_write_item(fp, "password_expiry_utc", str, 0); -+ free(str); ++ if (c->password_expiry_utc != TIME_MAX) { ++ char *s = xstrfmt("%"PRItime, c->password_expiry_utc); ++ credential_write_item(fp, "password_expiry_utc", s, 0); ++ free(s); + } } static int run_credential_helper(struct credential *c, +@@ credential.c: void credential_fill(struct credential *c) + + for (i = 0; i < c->helpers.nr; i++) { + credential_do(c, c->helpers.items[i].string, "get"); +- if (c->username && c->password) ++ if (c->username && c->password && time(NULL) < c->password_expiry_utc) + return; + if (c->quit) + die("credential helper '%s' told us to quit", ## credential.h ## @@ credential.h: struct credential { char *protocol; char *host; char *path; -+ time_t password_expiry_utc; ++ timestamp_t password_expiry_utc; }; #define CREDENTIAL_INIT { \ + .helpers = STRING_LIST_INIT_DUP, \ ++ .password_expiry_utc = TIME_MAX, \ + } + + /* Initialize a credential structure, setting all fields to empty. */ Documentation/git-credential.txt | 9 +++++++++ builtin/credential-cache--daemon.c | 3 +++ credential.c | 24 ++++++++++++++++++++++-- credential.h | 2 ++ 4 files changed, 36 insertions(+), 2 deletions(-) diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt index ac2818b9f66..667c4d80e26 100644 --- a/Documentation/git-credential.txt +++ b/Documentation/git-credential.txt @@ -144,6 +144,15 @@ Git understands the following attributes: The credential's password, if we are asking it to be stored. +`password_expiry_utc`:: + + If password is a personal access token or OAuth access token, it may have an + expiry date. When getting credentials from a helper, `git credential fill` + ignores the password attribute if the expiry date has passed. Storage + helpers should store this attribute if possible. Helpers should not + implement expiry logic themselves. Represented as Unix time UTC, seconds + since 1970. + `url`:: When this special attribute is read by `git credential`, the diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c index f3c89831d4a..338058be7f9 100644 --- a/builtin/credential-cache--daemon.c +++ b/builtin/credential-cache--daemon.c @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out) if (e) { fprintf(out, "username=%s\n", e->item.username); fprintf(out, "password=%s\n", e->item.password); + if (e->item.password_expiry_utc != TIME_MAX) + fprintf(out, "password_expiry_utc=%"PRItime"\n", + e->item.password_expiry_utc); } } else if (!strcmp(action.buf, "exit")) { diff --git a/credential.c b/credential.c index f6389a50684..354fa1652a9 100644 --- a/credential.c +++ b/credential.c @@ -7,6 +7,7 @@ #include "prompt.h" #include "sigchain.h" #include "urlmatch.h" +#include "git-compat-util.h" void credential_init(struct credential *c) { @@ -195,15 +196,20 @@ static void credential_getpass(struct credential *c) if (!c->username) c->username = credential_ask_one("Username", c, PROMPT_ASKPASS|PROMPT_ECHO); - if (!c->password) + if (!c->password || c->password_expiry_utc < time(NULL)) { + c->password_expiry_utc = TIME_MAX; c->password = credential_ask_one("Password", c, PROMPT_ASKPASS); + } } int credential_read(struct credential *c, FILE *fp) { struct strbuf line = STRBUF_INIT; + int password_updated = 0; + timestamp_t this_password_expiry = TIME_MAX; + while (strbuf_getline(&line, fp) != EOF) { char *key = line.buf; char *value = strchr(key, '='); @@ -225,6 +231,7 @@ int credential_read(struct credential *c, FILE *fp) } else if (!strcmp(key, "password")) { free(c->password); c->password = xstrdup(value); + password_updated = 1; } else if (!strcmp(key, "protocol")) { free(c->protocol); c->protocol = xstrdup(value); @@ -234,6 +241,11 @@ int credential_read(struct credential *c, FILE *fp) } else if (!strcmp(key, "path")) { free(c->path); c->path = xstrdup(value); + } else if (!strcmp(key, "password_expiry_utc")) { + this_password_expiry = parse_timestamp(value, NULL, 10); + if (this_password_expiry == 0 || errno) { + this_password_expiry = TIME_MAX; + } } else if (!strcmp(key, "url")) { credential_from_url(c, value); } else if (!strcmp(key, "quit")) { @@ -246,6 +258,9 @@ int credential_read(struct credential *c, FILE *fp) */ } + if (password_updated) + c->password_expiry_utc = this_password_expiry; + strbuf_release(&line); return 0; } @@ -269,6 +284,11 @@ void credential_write(const struct credential *c, FILE *fp) credential_write_item(fp, "path", c->path, 0); credential_write_item(fp, "username", c->username, 0); credential_write_item(fp, "password", c->password, 0); + if (c->password_expiry_utc != TIME_MAX) { + char *s = xstrfmt("%"PRItime, c->password_expiry_utc); + credential_write_item(fp, "password_expiry_utc", s, 0); + free(s); + } } static int run_credential_helper(struct credential *c, @@ -342,7 +362,7 @@ void credential_fill(struct credential *c) for (i = 0; i < c->helpers.nr; i++) { credential_do(c, c->helpers.items[i].string, "get"); - if (c->username && c->password) + if (c->username && c->password && time(NULL) < c->password_expiry_utc) return; if (c->quit) die("credential helper '%s' told us to quit", diff --git a/credential.h b/credential.h index f430e77fea4..935b28a70f1 100644 --- a/credential.h +++ b/credential.h @@ -126,10 +126,12 @@ struct credential { char *protocol; char *host; char *path; + timestamp_t password_expiry_utc; }; #define CREDENTIAL_INIT { \ .helpers = STRING_LIST_INIT_DUP, \ + .password_expiry_utc = TIME_MAX, \ } /* Initialize a credential structure, setting all fields to empty. */ base-commit: 5cc9858f1b470844dea5c5d3e936af183fdf2c68 -- gitgitgadget ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v2] credential: new attribute password_expiry_utc 2023-02-01 9:39 ` [PATCH v2] " M Hickford via GitGitGadget @ 2023-02-01 12:10 ` Jeff King 2023-02-01 17:12 ` Junio C Hamano ` (2 more replies) 2023-02-04 21:16 ` [PATCH v3] " M Hickford via GitGitGadget 1 sibling, 3 replies; 26+ messages in thread From: Jeff King @ 2023-02-01 12:10 UTC (permalink / raw) To: M Hickford via GitGitGadget Cc: git, Eric Sunshine, Cheetham, Dennington, M Hickford On Wed, Feb 01, 2023 at 09:39:51AM +0000, M Hickford via GitGitGadget wrote: > +`password_expiry_utc`:: > + > + If password is a personal access token or OAuth access token, it may have an > + expiry date. When getting credentials from a helper, `git credential fill` > + ignores the password attribute if the expiry date has passed. Storage > + helpers should store this attribute if possible. Helpers should not > + implement expiry logic themselves. Represented as Unix time UTC, seconds > + since 1970. This "should not" seems weird to me. The logic you have here throws out entries that have expired when they pass through Git. But wouldn't helpers which store things want to know about and act on the expiration, too? For example, if Git learns about a credential that expires in 60 seconds and passes it to credential-cache which is configured --timeout=300, wouldn't it want to set its internal ttl on the credential to 60, rather than 300? I think your plan here is that Git would then reject the credential if a request is made at time now+65. But the cache is holding onto it much longer than necessary. Likewise, wouldn't anything that stores credentials at least want to be able to store and regurgitate the expiration? For instance, even credential-store would want to do this. I'm OK if it doesn't, and we can consider it a quality-of-implementation issue and see if anybody cares enough to implement it. But I'd think most "real" helpers would want to do so. So it seems like helpers really do need to support this "expiration" notion. And it's actually Git itself which doesn't need to care about it, assuming the helpers are doing something sensible (though it is OK if Git _also_ throws away expired credentials to support helpers which don't). > diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c > index f3c89831d4a..338058be7f9 100644 > --- a/builtin/credential-cache--daemon.c > +++ b/builtin/credential-cache--daemon.c > @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out) > if (e) { > fprintf(out, "username=%s\n", e->item.username); > fprintf(out, "password=%s\n", e->item.password); > + if (e->item.password_expiry_utc != TIME_MAX) > + fprintf(out, "password_expiry_utc=%"PRItime"\n", > + e->item.password_expiry_utc); > } Is there a particular reason to use TIME_MAX as the sentinel value here, and not just "0"? It's not that big a deal either way, but it's more usual in our code base to use "0" if there's no reason not to (and it seems like nothing should be expiring in 1970 these days). > @@ -195,15 +196,20 @@ static void credential_getpass(struct credential *c) > if (!c->username) > c->username = credential_ask_one("Username", c, > PROMPT_ASKPASS|PROMPT_ECHO); > - if (!c->password) > + if (!c->password || c->password_expiry_utc < time(NULL)) { This is comparing a timestamp_t to a time_t, which may mix signed/unsigned. I can't offhand think of anything that would go too wrong there before 2038, so it's probably OK, but I wanted to call it out. > @@ -225,6 +231,7 @@ int credential_read(struct credential *c, FILE *fp) > } else if (!strcmp(key, "password")) { > free(c->password); > c->password = xstrdup(value); > + password_updated = 1; > } else if (!strcmp(key, "protocol")) { > free(c->protocol); > c->protocol = xstrdup(value); > @@ -234,6 +241,11 @@ int credential_read(struct credential *c, FILE *fp) > } else if (!strcmp(key, "path")) { > free(c->path); > c->path = xstrdup(value); > + } else if (!strcmp(key, "password_expiry_utc")) { > + this_password_expiry = parse_timestamp(value, NULL, 10); > + if (this_password_expiry == 0 || errno) { > + this_password_expiry = TIME_MAX; > + } > } else if (!strcmp(key, "url")) { > credential_from_url(c, value); > } else if (!strcmp(key, "quit")) { > @@ -246,6 +258,9 @@ int credential_read(struct credential *c, FILE *fp) > */ > } > > + if (password_updated) > + c->password_expiry_utc = this_password_expiry; Do we need this logic? It seems weird that a helper would output an expiration but not a password in the first place. I guess ignoring the expiration is probably a reasonable outcome, but I wonder if a helper would ever want to just add an expiration to the data coming from another helper. I.e., could we just read the value directly into c->password_expiry_utc as we do with other fields? -Peff ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] credential: new attribute password_expiry_utc 2023-02-01 12:10 ` Jeff King @ 2023-02-01 17:12 ` Junio C Hamano 2023-02-02 0:12 ` Jeff King 2023-02-01 20:02 ` Matthew John Cheetham 2023-02-05 6:34 ` M Hickford 2 siblings, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2023-02-01 17:12 UTC (permalink / raw) To: Jeff King Cc: M Hickford via GitGitGadget, git, Eric Sunshine, Cheetham, Dennington, M Hickford Jeff King <peff@peff.net> writes: >> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c >> index f3c89831d4a..338058be7f9 100644 >> --- a/builtin/credential-cache--daemon.c >> +++ b/builtin/credential-cache--daemon.c >> @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out) >> if (e) { >> fprintf(out, "username=%s\n", e->item.username); >> fprintf(out, "password=%s\n", e->item.password); >> + if (e->item.password_expiry_utc != TIME_MAX) >> + fprintf(out, "password_expiry_utc=%"PRItime"\n", >> + e->item.password_expiry_utc); >> } > > Is there a particular reason to use TIME_MAX as the sentinel value here, > and not just "0"? It's not that big a deal either way, but it's more > usual in our code base to use "0" if there's no reason not to (and it > seems like nothing should be expiring in 1970 these days). This is my fault ;-). Here, there is no difference between 0 and TIME_MAX, but elsewhere the code needed if (expiry != 0 && expiry < time(NULL)) to see if the entry has expired. If the sentinel for an entry that will never expire were TIME_MAX, you do not need the first half of the expression. I am OK either way. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] credential: new attribute password_expiry_utc 2023-02-01 17:12 ` Junio C Hamano @ 2023-02-02 0:12 ` Jeff King 0 siblings, 0 replies; 26+ messages in thread From: Jeff King @ 2023-02-02 0:12 UTC (permalink / raw) To: Junio C Hamano Cc: M Hickford via GitGitGadget, git, Eric Sunshine, Cheetham, Dennington, M Hickford On Wed, Feb 01, 2023 at 09:12:01AM -0800, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > >> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c > >> index f3c89831d4a..338058be7f9 100644 > >> --- a/builtin/credential-cache--daemon.c > >> +++ b/builtin/credential-cache--daemon.c > >> @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out) > >> if (e) { > >> fprintf(out, "username=%s\n", e->item.username); > >> fprintf(out, "password=%s\n", e->item.password); > >> + if (e->item.password_expiry_utc != TIME_MAX) > >> + fprintf(out, "password_expiry_utc=%"PRItime"\n", > >> + e->item.password_expiry_utc); > >> } > > > > Is there a particular reason to use TIME_MAX as the sentinel value here, > > and not just "0"? It's not that big a deal either way, but it's more > > usual in our code base to use "0" if there's no reason not to (and it > > seems like nothing should be expiring in 1970 these days). > > This is my fault ;-). Here, there is no difference between 0 and > TIME_MAX, but elsewhere the code needed > > if (expiry != 0 && expiry < time(NULL)) > > to see if the entry has expired. If the sentinel for an entry that > will never expire were TIME_MAX, you do not need the first half of > the expression. > > I am OK either way. Ah. That at least is a compelling reason to use TIME_MAX. I'm OK with it. -Peff ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] credential: new attribute password_expiry_utc 2023-02-01 12:10 ` Jeff King 2023-02-01 17:12 ` Junio C Hamano @ 2023-02-01 20:02 ` Matthew John Cheetham 2023-02-02 0:23 ` Jeff King 2023-02-05 6:45 ` M Hickford 2023-02-05 6:34 ` M Hickford 2 siblings, 2 replies; 26+ messages in thread From: Matthew John Cheetham @ 2023-02-01 20:02 UTC (permalink / raw) To: Jeff King, M Hickford via GitGitGadget Cc: git, Eric Sunshine, Dennington, M Hickford On 2023-02-01 04:10, Jeff King wrote: > On Wed, Feb 01, 2023 at 09:39:51AM +0000, M Hickford via GitGitGadget wrote: > >> +`password_expiry_utc`:: >> + >> + If password is a personal access token or OAuth access token, it may have an >> + expiry date. When getting credentials from a helper, `git credential fill` >> + ignores the password attribute if the expiry date has passed. Storage >> + helpers should store this attribute if possible. Helpers should not >> + implement expiry logic themselves. Represented as Unix time UTC, seconds >> + since 1970. > > This "should not" seems weird to me. The logic you have here throws out > entries that have expired when they pass through Git. But wouldn't > helpers which store things want to know about and act on the expiration, > too? > > For example, if Git learns about a credential that expires in 60 > seconds and passes it to credential-cache which is configured > --timeout=300, wouldn't it want to set its internal ttl on the > credential to 60, rather than 300? > > I think your plan here is that Git would then reject the credential if a > request is made at time now+65. But the cache is holding onto it much > longer than necessary. > > Likewise, wouldn't anything that stores credentials at least want to be > able to store and regurgitate the expiration? For instance, even > credential-store would want to do this. I'm OK if it doesn't, and we can > consider it a quality-of-implementation issue and see if anybody cares > enough to implement it. But I'd think most "real" helpers would want to > do so. > > So it seems like helpers really do need to support this "expiration" > notion. And it's actually Git itself which doesn't need to care about > it, assuming the helpers are doing something sensible (though it is OK > if Git _also_ throws away expired credentials to support helpers which > don't). I have often wondered about how, and if, Git should handle expiring credentials where the expiration is known. In my opinion I think Git should be doing *less* decision making with credentials and authentication in general, and leave that up to credential helpers. The original design of credential helpers from what I can see (and Peff can correct me here of course!) is that they were really only thought about as storage-style helpers. Helpers are consulted for a known credential, and told about bad (erase) or good (store) credentials, all without any context about the request or remote responses. If no credential helper can respond then Git itself prompts for a user/pass; so Git, or rather the user, is the 'generator'. Of course that's not to say that credential generating helpers don't exist or are wrong - Git Credential Manager being of course one example rather close to home for me! However the current model, even with generating helpers, is still that Git will try and make the request given the details included in the helper response. It doesn't make sense that a generating helper that knows about expiration would instead choose to respond with an expired credential rather than just try and generate a new credential. Now in the case of a simple storage helper without such logic, after returning an expired credential should Git not be calling 'erase' back to the same helper to inform it that it has a stale credential and should be deleted? This would also require some affinity between calls to get/erase/store. >> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c >> index f3c89831d4a..338058be7f9 100644 >> --- a/builtin/credential-cache--daemon.c >> +++ b/builtin/credential-cache--daemon.c >> @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out) >> if (e) { >> fprintf(out, "username=%s\n", e->item.username); >> fprintf(out, "password=%s\n", e->item.password); >> + if (e->item.password_expiry_utc != TIME_MAX) >> + fprintf(out, "password_expiry_utc=%"PRItime"\n", >> + e->item.password_expiry_utc); >> } > > Is there a particular reason to use TIME_MAX as the sentinel value here, > and not just "0"? It's not that big a deal either way, but it's more > usual in our code base to use "0" if there's no reason not to (and it > seems like nothing should be expiring in 1970 these days). > >> @@ -195,15 +196,20 @@ static void credential_getpass(struct credential *c) >> if (!c->username) >> c->username = credential_ask_one("Username", c, >> PROMPT_ASKPASS|PROMPT_ECHO); >> - if (!c->password) >> + if (!c->password || c->password_expiry_utc < time(NULL)) { > > This is comparing a timestamp_t to a time_t, which may mix > signed/unsigned. I can't offhand think of anything that would go too > wrong there before 2038, so it's probably OK, but I wanted to call it > out. > >> @@ -225,6 +231,7 @@ int credential_read(struct credential *c, FILE *fp) >> } else if (!strcmp(key, "password")) { >> free(c->password); >> c->password = xstrdup(value); >> + password_updated = 1; >> } else if (!strcmp(key, "protocol")) { >> free(c->protocol); >> c->protocol = xstrdup(value); >> @@ -234,6 +241,11 @@ int credential_read(struct credential *c, FILE *fp) >> } else if (!strcmp(key, "path")) { >> free(c->path); >> c->path = xstrdup(value); >> + } else if (!strcmp(key, "password_expiry_utc")) { >> + this_password_expiry = parse_timestamp(value, NULL, 10); >> + if (this_password_expiry == 0 || errno) { >> + this_password_expiry = TIME_MAX; >> + } >> } else if (!strcmp(key, "url")) { >> credential_from_url(c, value); >> } else if (!strcmp(key, "quit")) { >> @@ -246,6 +258,9 @@ int credential_read(struct credential *c, FILE *fp) >> */ >> } >> >> + if (password_updated) >> + c->password_expiry_utc = this_password_expiry; > > Do we need this logic? It seems weird that a helper would output an > expiration but not a password in the first place. I guess ignoring the > expiration is probably a reasonable outcome, but I wonder if a helper > would ever want to just add an expiration to the data coming from > another helper. > > I.e., could we just read the value directly into c->password_expiry_utc > as we do with other fields? > > -Peff ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] credential: new attribute password_expiry_utc 2023-02-01 20:02 ` Matthew John Cheetham @ 2023-02-02 0:23 ` Jeff King 2023-02-05 6:45 ` M Hickford 1 sibling, 0 replies; 26+ messages in thread From: Jeff King @ 2023-02-02 0:23 UTC (permalink / raw) To: Matthew John Cheetham Cc: M Hickford via GitGitGadget, git, Eric Sunshine, Dennington, M Hickford On Wed, Feb 01, 2023 at 12:02:16PM -0800, Matthew John Cheetham wrote: > > So it seems like helpers really do need to support this "expiration" > > notion. And it's actually Git itself which doesn't need to care about > > it, assuming the helpers are doing something sensible (though it is OK > > if Git _also_ throws away expired credentials to support helpers which > > don't). > > I have often wondered about how, and if, Git should handle expiring credentials > where the expiration is known. In my opinion I think Git should be doing > *less* decision making with credentials and authentication in general, and leave > that up to credential helpers. FWIW, that is my general philosophy, too. > The original design of credential helpers from what I can see (and Peff can > correct me here of course!) is that they were really only thought about as > storage-style helpers. Helpers are consulted for a known credential, and told > about bad (erase) or good (store) credentials, all without any context about > the request or remote responses. > > If no credential helper can respond then Git itself prompts for a user/pass; so > Git, or rather the user, is the 'generator'. They were always intended to be generators, too. In the early days we discussed having more fancy graphical prompts via helpers, though most people simply use the askpass interface for this. My personal config for the last, say, 12 years, has been to pull the password out of a read-only store, and ignore "store" and "erase" requests entirely. Which I think counts as a generator. :) But yeah, if there is more context that we can be giving the helpers to let them make a better decision, I'm all for it. I think your www-authenticate patches are a good step in that direction. I'm not sure what other context would be useful. > It doesn't make sense that a generating helper that knows about expiration would > instead choose to respond with an expired credential rather than just try and > generate a new credential. Yeah, agreed. > Now in the case of a simple storage helper without such logic, after returning > an expired credential should Git not be calling 'erase' back to the same helper > to inform it that it has a stale credential and should be deleted? > This would also require some affinity between calls to get/erase/store. That's a good point. I think in practice it would mostly happen that Git would throw away the expired credential, generate a new one (either from another helper or via prompting), and then if that works, overwrite the old one with a 'store' request. If the new credential doesn't work (or the user aborts), the expired credential is left in the helper. But in theory that doesn't matter. It will still be expired when it's served up again later. And it's not a security problem to hold onto it longer, since it's no longer valid. Still, it feels a bit clunky compared to having the helper realize it's holding garbage. -Peff ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] credential: new attribute password_expiry_utc 2023-02-01 20:02 ` Matthew John Cheetham 2023-02-02 0:23 ` Jeff King @ 2023-02-05 6:45 ` M Hickford 2023-02-06 18:59 ` Matthew John Cheetham 1 sibling, 1 reply; 26+ messages in thread From: M Hickford @ 2023-02-05 6:45 UTC (permalink / raw) To: Matthew John Cheetham Cc: Jeff King, M Hickford via GitGitGadget, git, Eric Sunshine, Dennington, M Hickford On Wed, 1 Feb 2023 at 20:02, Matthew John Cheetham <mjcheetham@outlook.com> wrote: > > On 2023-02-01 04:10, Jeff King wrote: > > > On Wed, Feb 01, 2023 at 09:39:51AM +0000, M Hickford via GitGitGadget wrote: > > > >> +`password_expiry_utc`:: > >> + > >> + If password is a personal access token or OAuth access token, it may have an > >> + expiry date. When getting credentials from a helper, `git credential fill` > >> + ignores the password attribute if the expiry date has passed. Storage > >> + helpers should store this attribute if possible. Helpers should not > >> + implement expiry logic themselves. Represented as Unix time UTC, seconds > >> + since 1970. > > > > This "should not" seems weird to me. The logic you have here throws out > > entries that have expired when they pass through Git. But wouldn't > > helpers which store things want to know about and act on the expiration, > > too? > > > > For example, if Git learns about a credential that expires in 60 > > seconds and passes it to credential-cache which is configured > > --timeout=300, wouldn't it want to set its internal ttl on the > > credential to 60, rather than 300? > > > > I think your plan here is that Git would then reject the credential if a > > request is made at time now+65. But the cache is holding onto it much > > longer than necessary. > > > > Likewise, wouldn't anything that stores credentials at least want to be > > able to store and regurgitate the expiration? For instance, even > > credential-store would want to do this. I'm OK if it doesn't, and we can > > consider it a quality-of-implementation issue and see if anybody cares > > enough to implement it. But I'd think most "real" helpers would want to > > do so. > > > > So it seems like helpers really do need to support this "expiration" > > notion. And it's actually Git itself which doesn't need to care about > > it, assuming the helpers are doing something sensible (though it is OK > > if Git _also_ throws away expired credentials to support helpers which > > don't). > > I have often wondered about how, and if, Git should handle expiring credentials > where the expiration is known. In my opinion I think Git should be doing > *less* decision making with credentials and authentication in general, and leave > that up to credential helpers. > > The original design of credential helpers from what I can see (and Peff can > correct me here of course!) is that they were really only thought about as > storage-style helpers. Helpers are consulted for a known credential, and told > about bad (erase) or good (store) credentials, all without any context about > the request or remote responses. > > If no credential helper can respond then Git itself prompts for a user/pass; so > Git, or rather the user, is the 'generator'. > > Of course that's not to say that credential generating helpers don't exist or > are wrong - Git Credential Manager being of course one example rather close to > home for me! However the current model, even with generating helpers, is still > that Git will try and make the request given the details included in the helper > response. GCM would benefit from being able to store expiry too. Whenever GCM retrieves an OAuth credential from storage, it queries the server to check whether the access token has expired [1]. This would become unnecessary. I've added more about this in patch v3 commit message. Further, it solves a problem if GCM is configured after another storage helper: ``` [credential] helper = storage # eg. osx-keychain or exotic helper = manager ``` Currently this may return an expired credential from storage. Background for others: GCM is typically configured as the *only* helper, with its own internal storage configuration [2]. These reimplement or wrap popular Git storage helpers [3][4][5]. ``` [credential] helper = manager credentialStore = keychain ``` [1] https://github.com/GitCredentialManager/git-credential-manager/blob/main/src/shared/GitLab/GitLabHostProvider.cs [2] https://github.com/GitCredentialManager/git-credential-manager/blob/main/docs/credstores.md [3] https://github.com/GitCredentialManager/git-credential-manager/blob/main/src/shared/Core/Interop/MacOS/MacOSKeychain.cs [4] https://github.com/GitCredentialManager/git-credential-manager/blob/main/src/shared/Core/Interop/Linux/SecretServiceCollection.cs [5] https://github.com/GitCredentialManager/git-credential-manager/blob/main/src/shared/Core/CredentialCacheStore.cs > > It doesn't make sense that a generating helper that knows about expiration would > instead choose to respond with an expired credential rather than just try and > generate a new credential. > > Now in the case of a simple storage helper without such logic, after returning > an expired credential should Git not be calling 'erase' back to the same helper > to inform it that it has a stale credential and should be deleted? > This would also require some affinity between calls to get/erase/store. > > > >> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c > >> index f3c89831d4a..338058be7f9 100644 > >> --- a/builtin/credential-cache--daemon.c > >> +++ b/builtin/credential-cache--daemon.c > >> @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out) > >> if (e) { > >> fprintf(out, "username=%s\n", e->item.username); > >> fprintf(out, "password=%s\n", e->item.password); > >> + if (e->item.password_expiry_utc != TIME_MAX) > >> + fprintf(out, "password_expiry_utc=%"PRItime"\n", > >> + e->item.password_expiry_utc); > >> } > > > > Is there a particular reason to use TIME_MAX as the sentinel value here, > > and not just "0"? It's not that big a deal either way, but it's more > > usual in our code base to use "0" if there's no reason not to (and it > > seems like nothing should be expiring in 1970 these days). > > > >> @@ -195,15 +196,20 @@ static void credential_getpass(struct credential *c) > >> if (!c->username) > >> c->username = credential_ask_one("Username", c, > >> PROMPT_ASKPASS|PROMPT_ECHO); > >> - if (!c->password) > >> + if (!c->password || c->password_expiry_utc < time(NULL)) { > > > > This is comparing a timestamp_t to a time_t, which may mix > > signed/unsigned. I can't offhand think of anything that would go too > > wrong there before 2038, so it's probably OK, but I wanted to call it > > out. > > > >> @@ -225,6 +231,7 @@ int credential_read(struct credential *c, FILE *fp) > >> } else if (!strcmp(key, "password")) { > >> free(c->password); > >> c->password = xstrdup(value); > >> + password_updated = 1; > >> } else if (!strcmp(key, "protocol")) { > >> free(c->protocol); > >> c->protocol = xstrdup(value); > >> @@ -234,6 +241,11 @@ int credential_read(struct credential *c, FILE *fp) > >> } else if (!strcmp(key, "path")) { > >> free(c->path); > >> c->path = xstrdup(value); > >> + } else if (!strcmp(key, "password_expiry_utc")) { > >> + this_password_expiry = parse_timestamp(value, NULL, 10); > >> + if (this_password_expiry == 0 || errno) { > >> + this_password_expiry = TIME_MAX; > >> + } > >> } else if (!strcmp(key, "url")) { > >> credential_from_url(c, value); > >> } else if (!strcmp(key, "quit")) { > >> @@ -246,6 +258,9 @@ int credential_read(struct credential *c, FILE *fp) > >> */ > >> } > >> > >> + if (password_updated) > >> + c->password_expiry_utc = this_password_expiry; > > > > Do we need this logic? It seems weird that a helper would output an > > expiration but not a password in the first place. I guess ignoring the > > expiration is probably a reasonable outcome, but I wonder if a helper > > would ever want to just add an expiration to the data coming from > > another helper. > > > > I.e., could we just read the value directly into c->password_expiry_utc > > as we do with other fields? > > > > -Peff ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] credential: new attribute password_expiry_utc 2023-02-05 6:45 ` M Hickford @ 2023-02-06 18:59 ` Matthew John Cheetham 0 siblings, 0 replies; 26+ messages in thread From: Matthew John Cheetham @ 2023-02-06 18:59 UTC (permalink / raw) To: M Hickford Cc: Jeff King, M Hickford via GitGitGadget, git, Eric Sunshine, Dennington On 2023-02-04 22:45, M Hickford wrote: > On Wed, 1 Feb 2023 at 20:02, Matthew John Cheetham > <mjcheetham@outlook.com> wrote: >> >> On 2023-02-01 04:10, Jeff King wrote: >> >>> On Wed, Feb 01, 2023 at 09:39:51AM +0000, M Hickford via GitGitGadget wrote: >>> >>>> +`password_expiry_utc`:: >>>> + >>>> + If password is a personal access token or OAuth access token, it may have an >>>> + expiry date. When getting credentials from a helper, `git credential fill` >>>> + ignores the password attribute if the expiry date has passed. Storage >>>> + helpers should store this attribute if possible. Helpers should not >>>> + implement expiry logic themselves. Represented as Unix time UTC, seconds >>>> + since 1970. >>> >>> This "should not" seems weird to me. The logic you have here throws out >>> entries that have expired when they pass through Git. But wouldn't >>> helpers which store things want to know about and act on the expiration, >>> too? >>> >>> For example, if Git learns about a credential that expires in 60 >>> seconds and passes it to credential-cache which is configured >>> --timeout=300, wouldn't it want to set its internal ttl on the >>> credential to 60, rather than 300? >>> >>> I think your plan here is that Git would then reject the credential if a >>> request is made at time now+65. But the cache is holding onto it much >>> longer than necessary. >>> >>> Likewise, wouldn't anything that stores credentials at least want to be >>> able to store and regurgitate the expiration? For instance, even >>> credential-store would want to do this. I'm OK if it doesn't, and we can >>> consider it a quality-of-implementation issue and see if anybody cares >>> enough to implement it. But I'd think most "real" helpers would want to >>> do so. >>> >>> So it seems like helpers really do need to support this "expiration" >>> notion. And it's actually Git itself which doesn't need to care about >>> it, assuming the helpers are doing something sensible (though it is OK >>> if Git _also_ throws away expired credentials to support helpers which >>> don't). >> >> I have often wondered about how, and if, Git should handle expiring credentials >> where the expiration is known. In my opinion I think Git should be doing >> *less* decision making with credentials and authentication in general, and leave >> that up to credential helpers. >> >> The original design of credential helpers from what I can see (and Peff can >> correct me here of course!) is that they were really only thought about as >> storage-style helpers. Helpers are consulted for a known credential, and told >> about bad (erase) or good (store) credentials, all without any context about >> the request or remote responses. >> >> If no credential helper can respond then Git itself prompts for a user/pass; so >> Git, or rather the user, is the 'generator'. >> >> Of course that's not to say that credential generating helpers don't exist or >> are wrong - Git Credential Manager being of course one example rather close to >> home for me! However the current model, even with generating helpers, is still >> that Git will try and make the request given the details included in the helper >> response. > > GCM would benefit from being able to store expiry too. Whenever GCM > retrieves an OAuth credential from storage, it queries the server to > check whether the access token has expired [1]. This would become > unnecessary. I've added more about this in patch v3 commit message. > > Further, it solves a problem if GCM is configured after another storage helper: > > ``` > [credential] > helper = storage # eg. osx-keychain or exotic > helper = manager > ``` > > Currently this may return an expired credential from storage. > > Background for others: GCM is typically configured as the *only* > helper, with its own internal storage configuration [2]. These > reimplement or wrap popular Git storage helpers [3][4][5]. > > ``` > [credential] > helper = manager > credentialStore = keychain > ``` One of the things that concerns me about the credential helper system today is the lack of 'affinity' across calls, and I think that we may disagree on this here. A desire I have for the future of Git auth is that helpers can negotiate to start a more long-lived or complicated converstation, with retry and detailed feedback on the auth responses. I'm increasingly feeling that the get/store/erase model is not sufficient for this. Imagine the scenario where the auth mechanism has a nonce that is updated on each successful (or failed) response. Here we'd want the helper that offered credentials to be told about the successful response for book-keeping, and not have that message 'stolen' by a simpler storage helper. Another scenario would be multiple user accounts; a helper has credentials that would potentially be valid for the request (same host/forge or IdP for example), and we're wanting to avoid unnecessary user prompts as the user has not specified explicitly the account to 'bind' to that clone or specific remote. Why could we not return credentials to Git, also indicating that we have an ability to retry on a 401/403? Increasingly, modern auth schemes have credentials that are so short lived (maybe bound to the exact specific request.. a one time use) that it really doesn't make sense to store any credentials at all. With such one-time-use credentials, and a simple storage helper ahead of a generating helper every other request would fail and the user would need to retry the command. Imagine the case that the OS/hardware security device holds the 'real' key or credential that is used to derive a one-time-use credential. Strong auth mechamisms often tie credential generation strongly to storage. > [1] https://github.com/GitCredentialManager/git-credential-manager/blob/main/src/shared/GitLab/GitLabHostProvider.cs > [2] https://github.com/GitCredentialManager/git-credential-manager/blob/main/docs/credstores.md > [3] https://github.com/GitCredentialManager/git-credential-manager/blob/main/src/shared/Core/Interop/MacOS/MacOSKeychain.cs > [4] https://github.com/GitCredentialManager/git-credential-manager/blob/main/src/shared/Core/Interop/Linux/SecretServiceCollection.cs > [5] https://github.com/GitCredentialManager/git-credential-manager/blob/main/src/shared/Core/CredentialCacheStore.cs > >> >> It doesn't make sense that a generating helper that knows about expiration would >> instead choose to respond with an expired credential rather than just try and >> generate a new credential. >> >> Now in the case of a simple storage helper without such logic, after returning >> an expired credential should Git not be calling 'erase' back to the same helper >> to inform it that it has a stale credential and should be deleted? >> This would also require some affinity between calls to get/erase/store. >> >> >>>> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c >>>> index f3c89831d4a..338058be7f9 100644 >>>> --- a/builtin/credential-cache--daemon.c >>>> +++ b/builtin/credential-cache--daemon.c >>>> @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out) >>>> if (e) { >>>> fprintf(out, "username=%s\n", e->item.username); >>>> fprintf(out, "password=%s\n", e->item.password); >>>> + if (e->item.password_expiry_utc != TIME_MAX) >>>> + fprintf(out, "password_expiry_utc=%"PRItime"\n", >>>> + e->item.password_expiry_utc); >>>> } >>> >>> Is there a particular reason to use TIME_MAX as the sentinel value here, >>> and not just "0"? It's not that big a deal either way, but it's more >>> usual in our code base to use "0" if there's no reason not to (and it >>> seems like nothing should be expiring in 1970 these days). >>> >>>> @@ -195,15 +196,20 @@ static void credential_getpass(struct credential *c) >>>> if (!c->username) >>>> c->username = credential_ask_one("Username", c, >>>> PROMPT_ASKPASS|PROMPT_ECHO); >>>> - if (!c->password) >>>> + if (!c->password || c->password_expiry_utc < time(NULL)) { >>> >>> This is comparing a timestamp_t to a time_t, which may mix >>> signed/unsigned. I can't offhand think of anything that would go too >>> wrong there before 2038, so it's probably OK, but I wanted to call it >>> out. >>> >>>> @@ -225,6 +231,7 @@ int credential_read(struct credential *c, FILE *fp) >>>> } else if (!strcmp(key, "password")) { >>>> free(c->password); >>>> c->password = xstrdup(value); >>>> + password_updated = 1; >>>> } else if (!strcmp(key, "protocol")) { >>>> free(c->protocol); >>>> c->protocol = xstrdup(value); >>>> @@ -234,6 +241,11 @@ int credential_read(struct credential *c, FILE *fp) >>>> } else if (!strcmp(key, "path")) { >>>> free(c->path); >>>> c->path = xstrdup(value); >>>> + } else if (!strcmp(key, "password_expiry_utc")) { >>>> + this_password_expiry = parse_timestamp(value, NULL, 10); >>>> + if (this_password_expiry == 0 || errno) { >>>> + this_password_expiry = TIME_MAX; >>>> + } >>>> } else if (!strcmp(key, "url")) { >>>> credential_from_url(c, value); >>>> } else if (!strcmp(key, "quit")) { >>>> @@ -246,6 +258,9 @@ int credential_read(struct credential *c, FILE *fp) >>>> */ >>>> } >>>> >>>> + if (password_updated) >>>> + c->password_expiry_utc = this_password_expiry; >>> >>> Do we need this logic? It seems weird that a helper would output an >>> expiration but not a password in the first place. I guess ignoring the >>> expiration is probably a reasonable outcome, but I wonder if a helper >>> would ever want to just add an expiration to the data coming from >>> another helper. >>> >>> I.e., could we just read the value directly into c->password_expiry_utc >>> as we do with other fields? >>> >>> -Peff ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] credential: new attribute password_expiry_utc 2023-02-01 12:10 ` Jeff King 2023-02-01 17:12 ` Junio C Hamano 2023-02-01 20:02 ` Matthew John Cheetham @ 2023-02-05 6:34 ` M Hickford 2 siblings, 0 replies; 26+ messages in thread From: M Hickford @ 2023-02-05 6:34 UTC (permalink / raw) To: Jeff King Cc: M Hickford via GitGitGadget, git, Eric Sunshine, Cheetham, Dennington, M Hickford Thanks Jeff for the review On Wed, 1 Feb 2023 at 12:10, Jeff King <peff@peff.net> wrote: > > On Wed, Feb 01, 2023 at 09:39:51AM +0000, M Hickford via GitGitGadget wrote: > > > +`password_expiry_utc`:: > > + > > + If password is a personal access token or OAuth access token, it may have an > > + expiry date. When getting credentials from a helper, `git credential fill` > > + ignores the password attribute if the expiry date has passed. Storage > > + helpers should store this attribute if possible. Helpers should not > > + implement expiry logic themselves. Represented as Unix time UTC, seconds > > + since 1970. > > This "should not" seems weird to me. The logic you have here throws out > entries that have expired when they pass through Git. But wouldn't > helpers which store things want to know about and act on the expiration, > too? I wanted to keep the helper contract as simple as possible "here is an attribute to store and retrieve like any other". It doesn't matter if a helper stores a password beyond expiry, because Git will erase or replace it when it fails authentication or another succeeds. Relax the language in the patch v3 commit message to describe "self-pruning of expired passwords is unnecessary but harmless". > > For example, if Git learns about a credential that expires in 60 > seconds and passes it to credential-cache which is configured > --timeout=300, wouldn't it want to set its internal ttl on the > credential to 60, rather than 300? > > I think your plan here is that Git would then reject the credential if a > request is made at time now+65. But the cache is holding onto it much > longer than necessary. Even after the password expires, there's value to storing other attributes such as username and (perhaps in future) oauth-refresh-token. Hence I named the attribute 'password_expiry_utc' explicitly rather than 'credential_expiry_utc'. > > Likewise, wouldn't anything that stores credentials at least want to be > able to store and regurgitate the expiration? For instance, even > credential-store would want to do this. I'm OK if it doesn't, and we can > consider it a quality-of-implementation issue and see if anybody cares > enough to implement it. But I'd think most "real" helpers would want to > do so. Absolutely. Eventually I'd like to see support in git-credential-osxkeychain, git-credential-wincred, git-credential-libsecret etc. Mentioned this in patch v3 description. > > So it seems like helpers really do need to support this "expiration" > notion. And it's actually Git itself which doesn't need to care about > it, assuming the helpers are doing something sensible (though it is OK > if Git _also_ throws away expired credentials to support helpers which > don't). > > > diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c > > index f3c89831d4a..338058be7f9 100644 > > --- a/builtin/credential-cache--daemon.c > > +++ b/builtin/credential-cache--daemon.c > > @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out) > > if (e) { > > fprintf(out, "username=%s\n", e->item.username); > > fprintf(out, "password=%s\n", e->item.password); > > + if (e->item.password_expiry_utc != TIME_MAX) > > + fprintf(out, "password_expiry_utc=%"PRItime"\n", > > + e->item.password_expiry_utc); > > } > > Is there a particular reason to use TIME_MAX as the sentinel value here, > and not just "0"? It's not that big a deal either way, but it's more > usual in our code base to use "0" if there's no reason not to (and it > seems like nothing should be expiring in 1970 these days). Junio made a persuasive argument for readability https://lore.kernel.org/git/CAPig+cQPLMrUKp0aqLCknSYCs5TAso-VSBYsQbGZ8g8wgY2Liw@mail.gmail.com/T/#mf66955cf3f53c073c68ad5ade7213617907bab63 > > > @@ -195,15 +196,20 @@ static void credential_getpass(struct credential *c) > > if (!c->username) > > c->username = credential_ask_one("Username", c, > > PROMPT_ASKPASS|PROMPT_ECHO); > > - if (!c->password) > > + if (!c->password || c->password_expiry_utc < time(NULL)) { > > This is comparing a timestamp_t to a time_t, which may mix > signed/unsigned. I can't offhand think of anything that would go too > wrong there before 2038, so it's probably OK, but I wanted to call it > out. > > > @@ -225,6 +231,7 @@ int credential_read(struct credential *c, FILE *fp) > > } else if (!strcmp(key, "password")) { > > free(c->password); > > c->password = xstrdup(value); > > + password_updated = 1; > > } else if (!strcmp(key, "protocol")) { > > free(c->protocol); > > c->protocol = xstrdup(value); > > @@ -234,6 +241,11 @@ int credential_read(struct credential *c, FILE *fp) > > } else if (!strcmp(key, "path")) { > > free(c->path); > > c->path = xstrdup(value); > > + } else if (!strcmp(key, "password_expiry_utc")) { > > + this_password_expiry = parse_timestamp(value, NULL, 10); > > + if (this_password_expiry == 0 || errno) { > > + this_password_expiry = TIME_MAX; > > + } > > } else if (!strcmp(key, "url")) { > > credential_from_url(c, value); > > } else if (!strcmp(key, "quit")) { > > @@ -246,6 +258,9 @@ int credential_read(struct credential *c, FILE *fp) > > */ > > } > > > > + if (password_updated) > > + c->password_expiry_utc = this_password_expiry; > > Do we need this logic? It seems weird that a helper would output an > expiration but not a password in the first place. I guess ignoring the > expiration is probably a reasonable outcome, but I wonder if a helper > would ever want to just add an expiration to the data coming from > another helper. > I.e., could we just read the value directly into c->password_expiry_utc > as we do with other fields? Done in patch v3. The logic that remains (moved to credential_fill) is a simple sanity check. > > -Peff ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3] credential: new attribute password_expiry_utc 2023-02-01 9:39 ` [PATCH v2] " M Hickford via GitGitGadget 2023-02-01 12:10 ` Jeff King @ 2023-02-04 21:16 ` M Hickford via GitGitGadget 2023-02-14 1:59 ` Junio C Hamano ` (3 more replies) 1 sibling, 4 replies; 26+ messages in thread From: M Hickford via GitGitGadget @ 2023-02-04 21:16 UTC (permalink / raw) To: git Cc: Eric Sunshine, Jeff King, Cheetham, Dennington, M Hickford, M Hickford From: M Hickford <mirth.hickford@gmail.com> Some passwords have an expiry date known at generation. This may be years away for a personal access token or hours for an OAuth access token. When multiple credential helpers are configured, `credential fill` tries each helper in turn until it has a username and password, returning early. If Git authentication succeeds, `credential approve` stores the successful credential in all helpers. If authentication fails, `credential reject` erases matching credentials in all helpers. Helpers implement corresponding operations: get, store, erase. The credential protocol has no expiry attribute, so helpers cannot store expiry information. (Even if a helper returned an improvised expiry attribute, git credential discards unrecognised attributes between operations and between helpers.) As a workaround, whenever monolithic helper Git Credential Manager (GCM) retrieves an OAuth credential from its storage, it makes a HTTP request to check whether the OAuth token has expired [1]. This complicates and slows the authentication happy path. Worse is the case that a storage helper and a credential-generating helper are configured together: [credential] helper = storage # eg. cache or osxkeychain helper = generate # eg. oauth or manager `credential approve` stores the generated credential in both helpers without expiry information. Later `credential fill` may return an expired credential from storage. There is no workaround, no matter how clever the second helper. Introduce a password expiry attribute. In `credential fill`, ignore expired passwords and continue to query subsequent helpers. In the example above, `credential fill` ignores the expired credential and a fresh credential is generated. If authentication succeeds, `credential approve` replaces the expired credential in storage. If authentication fails, the expired credential is erased by `credential reject`. It is unnecessary but harmless for storage helpers to self prune expired credentials. Add support for the new attribute to credential-cache. Eventually, I hope to see support in other storage helpers. Example usage in a credential-generating helper https://github.com/hickford/git-credential-oauth/pull/16 [1] https://github.com/GitCredentialManager/git-credential-manager/blob/66b94e489ad8cc1982836355493e369770b30211/src/shared/GitLab/GitLabHostProvider.cs#L217 Signed-off-by: M Hickford <mirth.hickford@gmail.com> --- credential: new attribute password_expiry_utc details in commit message Changes in patch v3: * Tests * Simplified credential_read, moved expiry logic to credential_fill Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1443%2Fhickford%2Fpassword-expiry-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1443/hickford/password-expiry-v3 Pull-Request: https://github.com/git/git/pull/1443 Range-diff vs v2: 1: b9ee729ee4d ! 1: 1846815a5c1 credential: new attribute password_expiry_utc @@ Commit message years away for a personal access token or hours for an OAuth access token. - Currently the credential protocol has no expiry attribute. When multiple - helpers are configured, `credential fill` tries each helper in turn - until it has a username and password, returning early. + When multiple credential helpers are configured, `credential fill` tries + each helper in turn until it has a username and password, returning + early. If Git authentication succeeds, `credential approve` + stores the successful credential in all helpers. If authentication + fails, `credential reject` erases matching credentials in all helpers. + Helpers implement corresponding operations: get, store, erase. - When a storage helper and a credential-generating helper are configured - together, the credential is necessarily stored without expiry, so - `credential fill` may later return an expired credential from storage. + The credential protocol has no expiry attribute, so helpers cannot + store expiry information. (Even if a helper returned an improvised + expiry attribute, git credential discards unrecognised attributes + between operations and between helpers.) - ``` - [credential] - helper = storage # eg. cache or osxkeychain - helper = generate # eg. oauth - ``` + As a workaround, whenever monolithic helper Git Credential Manager (GCM) + retrieves an OAuth credential from its storage, it makes a HTTP request + to check whether the OAuth token has expired [1]. This complicates and + slows the authentication happy path. - An improvement is to introduce a password expiry attribute to the - credential protocol. If the expiry date has passed, `credential fill` - ignores the password attribute, so subsequent helpers can generate a - fresh credential. This is backwards compatible -- no change in - behaviour with helpers that discard the expiry attribute. + Worse is the case that a storage helper and a credential-generating + helper are configured together: - Note that the expiry logic is entirely within the credential layer. - Compatible helpers store and retrieve the new attribute like any other. - This keeps the helper contract simple. + [credential] + helper = storage # eg. cache or osxkeychain + helper = generate # eg. oauth or manager - This patch adds support for the new attribute to cache. + `credential approve` stores the generated credential in both helpers + without expiry information. Later `credential fill` may return an + expired credential from storage. There is no workaround, no matter how + clever the second helper. + + Introduce a password expiry attribute. In `credential fill`, ignore + expired passwords and continue to query subsequent helpers. + + In the example above, `credential fill` ignores the expired credential + and a fresh credential is generated. If authentication succeeds, + `credential approve` replaces the expired credential in storage. + If authentication fails, the expired credential is erased by + `credential reject`. It is unnecessary but harmless for storage + helpers to self prune expired credentials. + + Add support for the new attribute to credential-cache. + Eventually, I hope to see support in other storage helpers. Example usage in a credential-generating helper https://github.com/hickford/git-credential-oauth/pull/16 - Future ideas: make it possible for a storage helper to provide OAuth - refresh token to subsequent helpers. - https://github.com/gitgitgadget/git/pull/1394 + [1] https://github.com/GitCredentialManager/git-credential-manager/blob/66b94e489ad8cc1982836355493e369770b30211/src/shared/GitLab/GitLabHostProvider.cs#L217 Signed-off-by: M Hickford <mirth.hickford@gmail.com> @@ Documentation/git-credential.txt: Git understands the following attributes: +`password_expiry_utc`:: + -+ If password is a personal access token or OAuth access token, it may have an -+ expiry date. When getting credentials from a helper, `git credential fill` -+ ignores the password attribute if the expiry date has passed. Storage -+ helpers should store this attribute if possible. Helpers should not -+ implement expiry logic themselves. Represented as Unix time UTC, seconds -+ since 1970. ++ Generated passwords such as an OAuth access token may have an expiry date. ++ When reading credentials from helpers, `git credential fill` ignores expired ++ passwords. Represented as Unix time UTC, seconds since 1970. + `url`:: When this special attribute is read by `git credential`, the + ## Documentation/gitcredentials.txt ## +@@ Documentation/gitcredentials.txt: helper:: + If there are multiple instances of the `credential.helper` configuration + variable, each helper will be tried in turn, and may provide a username, + password, or nothing. Once Git has acquired both a username and a +-password, no more helpers will be tried. ++unexpired password, no more helpers will be tried. + + + If `credential.helper` is configured to the empty string, this resets + the helper list to empty (so you may override a helper set by a + ## builtin/credential-cache--daemon.c ## @@ builtin/credential-cache--daemon.c: static void serve_one_client(FILE *in, FILE *out) if (e) { @@ credential.c void credential_init(struct credential *c) { -@@ credential.c: static void credential_getpass(struct credential *c) - if (!c->username) - c->username = credential_ask_one("Username", c, - PROMPT_ASKPASS|PROMPT_ECHO); -- if (!c->password) -+ if (!c->password || c->password_expiry_utc < time(NULL)) { -+ c->password_expiry_utc = TIME_MAX; - c->password = credential_ask_one("Password", c, - PROMPT_ASKPASS); -+ } - } - - int credential_read(struct credential *c, FILE *fp) - { - struct strbuf line = STRBUF_INIT; - -+ int password_updated = 0; -+ timestamp_t this_password_expiry = TIME_MAX; -+ - while (strbuf_getline(&line, fp) != EOF) { - char *key = line.buf; - char *value = strchr(key, '='); -@@ credential.c: int credential_read(struct credential *c, FILE *fp) - } else if (!strcmp(key, "password")) { - free(c->password); - c->password = xstrdup(value); -+ password_updated = 1; - } else if (!strcmp(key, "protocol")) { - free(c->protocol); - c->protocol = xstrdup(value); @@ credential.c: int credential_read(struct credential *c, FILE *fp) } else if (!strcmp(key, "path")) { free(c->path); c->path = xstrdup(value); + } else if (!strcmp(key, "password_expiry_utc")) { -+ this_password_expiry = parse_timestamp(value, NULL, 10); -+ if (this_password_expiry == 0 || errno) { -+ this_password_expiry = TIME_MAX; -+ } ++ c->password_expiry_utc = parse_timestamp(value, NULL, 10); ++ if (c->password_expiry_utc == 0 || errno) ++ c->password_expiry_utc = TIME_MAX; } else if (!strcmp(key, "url")) { credential_from_url(c, value); } else if (!strcmp(key, "quit")) { -@@ credential.c: int credential_read(struct credential *c, FILE *fp) - */ - } - -+ if (password_updated) -+ c->password_expiry_utc = this_password_expiry; -+ - strbuf_release(&line); - return 0; - } @@ credential.c: void credential_write(const struct credential *c, FILE *fp) credential_write_item(fp, "path", c->path, 0); credential_write_item(fp, "username", c->username, 0); @@ credential.c: void credential_fill(struct credential *c) for (i = 0; i < c->helpers.nr; i++) { credential_do(c, c->helpers.items[i].string, "get"); -- if (c->username && c->password) -+ if (c->username && c->password && time(NULL) < c->password_expiry_utc) ++ if (c->password_expiry_utc < time(NULL)) { ++ FREE_AND_NULL(c->password); ++ c->password_expiry_utc = TIME_MAX; ++ } + if (c->username && c->password) return; if (c->quit) - die("credential helper '%s' told us to quit", +@@ credential.c: void credential_approve(struct credential *c) + + if (c->approved) + return; +- if (!c->username || !c->password) ++ if (!c->username || !c->password || c->password_expiry_utc < time(NULL)) + return; + + credential_apply_config(c); +@@ credential.c: void credential_reject(struct credential *c) + + FREE_AND_NULL(c->username); + FREE_AND_NULL(c->password); ++ c->password_expiry_utc = TIME_MAX; + c->approved = 0; + } + ## credential.h ## @@ credential.h: struct credential { @@ credential.h: struct credential { } /* Initialize a credential structure, setting all fields to empty. */ + + ## t/t0300-credentials.sh ## +@@ t/t0300-credentials.sh: test_expect_success 'setup helper scripts' ' + test -z "$pass" || echo password=$pass + EOF + ++ write_script git-credential-verbatim-with-expiry <<-\EOF && ++ user=$1; shift ++ pass=$1; shift ++ pexpiry=$1; shift ++ . ./dump ++ test -z "$user" || echo username=$user ++ test -z "$pass" || echo password=$pass ++ test -z "$pexpiry" || echo password_expiry_utc=$pexpiry ++ EOF ++ + PATH="$PWD:$PATH" + ' + +@@ t/t0300-credentials.sh: test_expect_success 'credential_fill continues through partial response' ' + EOF + ' + ++test_expect_success 'credential_fill populates password_expiry_utc' ' ++ check fill "verbatim-with-expiry one two 9999999999" <<-\EOF ++ protocol=http ++ host=example.com ++ -- ++ protocol=http ++ host=example.com ++ username=one ++ password=two ++ password_expiry_utc=9999999999 ++ -- ++ verbatim-with-expiry: get ++ verbatim-with-expiry: protocol=http ++ verbatim-with-expiry: host=example.com ++ EOF ++' ++ ++test_expect_success 'credential_fill continues through expired password' ' ++ check fill "verbatim-with-expiry one two 5" "verbatim three four" <<-\EOF ++ protocol=http ++ host=example.com ++ -- ++ protocol=http ++ host=example.com ++ username=three ++ password=four ++ -- ++ verbatim-with-expiry: get ++ verbatim-with-expiry: protocol=http ++ verbatim-with-expiry: host=example.com ++ verbatim: get ++ verbatim: protocol=http ++ verbatim: host=example.com ++ verbatim: username=one ++ EOF ++' ++ + test_expect_success 'credential_fill passes along metadata' ' + check fill "verbatim one two" <<-\EOF + protocol=ftp +@@ t/t0300-credentials.sh: test_expect_success 'credential_approve calls all helpers' ' + EOF + ' + ++test_expect_success 'credential_approve stores password expiry' ' ++ check approve useless <<-\EOF ++ protocol=http ++ host=example.com ++ username=foo ++ password=bar ++ password_expiry_utc=9999999999 ++ -- ++ -- ++ useless: store ++ useless: protocol=http ++ useless: host=example.com ++ useless: username=foo ++ useless: password=bar ++ useless: password_expiry_utc=9999999999 ++ EOF ++' ++ + test_expect_success 'do not bother storing password-less credential' ' + check approve useless <<-\EOF + protocol=http +@@ t/t0300-credentials.sh: test_expect_success 'do not bother storing password-less credential' ' + EOF + ' + ++test_expect_success 'credential_approve does not store expired credential' ' ++ check approve useless <<-\EOF ++ protocol=http ++ host=example.com ++ username=foo ++ password=bar ++ password_expiry_utc=5 ++ -- ++ -- ++ EOF ++' + + test_expect_success 'credential_reject calls all helpers' ' + check reject useless "verbatim one two" <<-\EOF +@@ t/t0300-credentials.sh: test_expect_success 'credential_reject calls all helpers' ' + EOF + ' + ++test_expect_success 'credential_reject erases expired credential' ' ++ check reject useless <<-\EOF ++ protocol=http ++ host=example.com ++ username=foo ++ password=bar ++ password_expiry_utc=5 ++ -- ++ -- ++ useless: erase ++ useless: protocol=http ++ useless: host=example.com ++ useless: username=foo ++ useless: password=bar ++ useless: password_expiry_utc=5 ++ EOF ++' ++ + test_expect_success 'usernames can be preserved' ' + check fill "verbatim \"\" three" <<-\EOF + protocol=http Documentation/git-credential.txt | 6 ++ Documentation/gitcredentials.txt | 2 +- builtin/credential-cache--daemon.c | 3 + credential.c | 17 +++++- credential.h | 2 + t/t0300-credentials.sh | 94 ++++++++++++++++++++++++++++++ 6 files changed, 122 insertions(+), 2 deletions(-) diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt index ac2818b9f66..29d184ab824 100644 --- a/Documentation/git-credential.txt +++ b/Documentation/git-credential.txt @@ -144,6 +144,12 @@ Git understands the following attributes: The credential's password, if we are asking it to be stored. +`password_expiry_utc`:: + + Generated passwords such as an OAuth access token may have an expiry date. + When reading credentials from helpers, `git credential fill` ignores expired + passwords. Represented as Unix time UTC, seconds since 1970. + `url`:: When this special attribute is read by `git credential`, the diff --git a/Documentation/gitcredentials.txt b/Documentation/gitcredentials.txt index 4522471c337..95636b18439 100644 --- a/Documentation/gitcredentials.txt +++ b/Documentation/gitcredentials.txt @@ -167,7 +167,7 @@ helper:: If there are multiple instances of the `credential.helper` configuration variable, each helper will be tried in turn, and may provide a username, password, or nothing. Once Git has acquired both a username and a -password, no more helpers will be tried. +unexpired password, no more helpers will be tried. + If `credential.helper` is configured to the empty string, this resets the helper list to empty (so you may override a helper set by a diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c index f3c89831d4a..338058be7f9 100644 --- a/builtin/credential-cache--daemon.c +++ b/builtin/credential-cache--daemon.c @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out) if (e) { fprintf(out, "username=%s\n", e->item.username); fprintf(out, "password=%s\n", e->item.password); + if (e->item.password_expiry_utc != TIME_MAX) + fprintf(out, "password_expiry_utc=%"PRItime"\n", + e->item.password_expiry_utc); } } else if (!strcmp(action.buf, "exit")) { diff --git a/credential.c b/credential.c index f6389a50684..d3e1bf7a679 100644 --- a/credential.c +++ b/credential.c @@ -7,6 +7,7 @@ #include "prompt.h" #include "sigchain.h" #include "urlmatch.h" +#include "git-compat-util.h" void credential_init(struct credential *c) { @@ -234,6 +235,10 @@ int credential_read(struct credential *c, FILE *fp) } else if (!strcmp(key, "path")) { free(c->path); c->path = xstrdup(value); + } else if (!strcmp(key, "password_expiry_utc")) { + c->password_expiry_utc = parse_timestamp(value, NULL, 10); + if (c->password_expiry_utc == 0 || errno) + c->password_expiry_utc = TIME_MAX; } else if (!strcmp(key, "url")) { credential_from_url(c, value); } else if (!strcmp(key, "quit")) { @@ -269,6 +274,11 @@ void credential_write(const struct credential *c, FILE *fp) credential_write_item(fp, "path", c->path, 0); credential_write_item(fp, "username", c->username, 0); credential_write_item(fp, "password", c->password, 0); + if (c->password_expiry_utc != TIME_MAX) { + char *s = xstrfmt("%"PRItime, c->password_expiry_utc); + credential_write_item(fp, "password_expiry_utc", s, 0); + free(s); + } } static int run_credential_helper(struct credential *c, @@ -342,6 +352,10 @@ void credential_fill(struct credential *c) for (i = 0; i < c->helpers.nr; i++) { credential_do(c, c->helpers.items[i].string, "get"); + if (c->password_expiry_utc < time(NULL)) { + FREE_AND_NULL(c->password); + c->password_expiry_utc = TIME_MAX; + } if (c->username && c->password) return; if (c->quit) @@ -360,7 +374,7 @@ void credential_approve(struct credential *c) if (c->approved) return; - if (!c->username || !c->password) + if (!c->username || !c->password || c->password_expiry_utc < time(NULL)) return; credential_apply_config(c); @@ -381,6 +395,7 @@ void credential_reject(struct credential *c) FREE_AND_NULL(c->username); FREE_AND_NULL(c->password); + c->password_expiry_utc = TIME_MAX; c->approved = 0; } diff --git a/credential.h b/credential.h index f430e77fea4..935b28a70f1 100644 --- a/credential.h +++ b/credential.h @@ -126,10 +126,12 @@ struct credential { char *protocol; char *host; char *path; + timestamp_t password_expiry_utc; }; #define CREDENTIAL_INIT { \ .helpers = STRING_LIST_INIT_DUP, \ + .password_expiry_utc = TIME_MAX, \ } /* Initialize a credential structure, setting all fields to empty. */ diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh index 3485c0534e6..96391015af5 100755 --- a/t/t0300-credentials.sh +++ b/t/t0300-credentials.sh @@ -35,6 +35,16 @@ test_expect_success 'setup helper scripts' ' test -z "$pass" || echo password=$pass EOF + write_script git-credential-verbatim-with-expiry <<-\EOF && + user=$1; shift + pass=$1; shift + pexpiry=$1; shift + . ./dump + test -z "$user" || echo username=$user + test -z "$pass" || echo password=$pass + test -z "$pexpiry" || echo password_expiry_utc=$pexpiry + EOF + PATH="$PWD:$PATH" ' @@ -109,6 +119,43 @@ test_expect_success 'credential_fill continues through partial response' ' EOF ' +test_expect_success 'credential_fill populates password_expiry_utc' ' + check fill "verbatim-with-expiry one two 9999999999" <<-\EOF + protocol=http + host=example.com + -- + protocol=http + host=example.com + username=one + password=two + password_expiry_utc=9999999999 + -- + verbatim-with-expiry: get + verbatim-with-expiry: protocol=http + verbatim-with-expiry: host=example.com + EOF +' + +test_expect_success 'credential_fill continues through expired password' ' + check fill "verbatim-with-expiry one two 5" "verbatim three four" <<-\EOF + protocol=http + host=example.com + -- + protocol=http + host=example.com + username=three + password=four + -- + verbatim-with-expiry: get + verbatim-with-expiry: protocol=http + verbatim-with-expiry: host=example.com + verbatim: get + verbatim: protocol=http + verbatim: host=example.com + verbatim: username=one + EOF +' + test_expect_success 'credential_fill passes along metadata' ' check fill "verbatim one two" <<-\EOF protocol=ftp @@ -149,6 +196,24 @@ test_expect_success 'credential_approve calls all helpers' ' EOF ' +test_expect_success 'credential_approve stores password expiry' ' + check approve useless <<-\EOF + protocol=http + host=example.com + username=foo + password=bar + password_expiry_utc=9999999999 + -- + -- + useless: store + useless: protocol=http + useless: host=example.com + useless: username=foo + useless: password=bar + useless: password_expiry_utc=9999999999 + EOF +' + test_expect_success 'do not bother storing password-less credential' ' check approve useless <<-\EOF protocol=http @@ -159,6 +224,17 @@ test_expect_success 'do not bother storing password-less credential' ' EOF ' +test_expect_success 'credential_approve does not store expired credential' ' + check approve useless <<-\EOF + protocol=http + host=example.com + username=foo + password=bar + password_expiry_utc=5 + -- + -- + EOF +' test_expect_success 'credential_reject calls all helpers' ' check reject useless "verbatim one two" <<-\EOF @@ -181,6 +257,24 @@ test_expect_success 'credential_reject calls all helpers' ' EOF ' +test_expect_success 'credential_reject erases expired credential' ' + check reject useless <<-\EOF + protocol=http + host=example.com + username=foo + password=bar + password_expiry_utc=5 + -- + -- + useless: erase + useless: protocol=http + useless: host=example.com + useless: username=foo + useless: password=bar + useless: password_expiry_utc=5 + EOF +' + test_expect_success 'usernames can be preserved' ' check fill "verbatim \"\" three" <<-\EOF protocol=http base-commit: 2fc9e9ca3c7505bc60069f11e7ef09b1aeeee473 -- gitgitgadget ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v3] credential: new attribute password_expiry_utc 2023-02-04 21:16 ` [PATCH v3] " M Hickford via GitGitGadget @ 2023-02-14 1:59 ` Junio C Hamano 2023-02-14 22:36 ` M Hickford 2023-02-14 8:03 ` Martin Ågren ` (2 subsequent siblings) 3 siblings, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2023-02-14 1:59 UTC (permalink / raw) To: M Hickford via GitGitGadget Cc: git, Eric Sunshine, Jeff King, Cheetham, Dennington, M Hickford "M Hickford via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: M Hickford <mirth.hickford@gmail.com> > > Some passwords have an expiry date known at generation. This may be > years away for a personal access token or hours for an OAuth access > token. > ... > t/t0300-credentials.sh | 94 ++++++++++++++++++++++++++++++ https://github.com/git/git/actions/runs/4169057114/jobs/7217377625 Other platforms seem to be OK, but Windows test seems to be unhappy with it when this topic gets merged to 'seen'. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3] credential: new attribute password_expiry_utc 2023-02-14 1:59 ` Junio C Hamano @ 2023-02-14 22:36 ` M Hickford 2023-02-17 21:44 ` Lessley Dennington 0 siblings, 1 reply; 26+ messages in thread From: M Hickford @ 2023-02-14 22:36 UTC (permalink / raw) To: Junio C Hamano Cc: M Hickford via GitGitGadget, git, Eric Sunshine, Jeff King, Cheetham, Dennington, M Hickford On Tue, 14 Feb 2023 at 01:59, Junio C Hamano <gitster@pobox.com> wrote: > > "M Hickford via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: M Hickford <mirth.hickford@gmail.com> > > > > Some passwords have an expiry date known at generation. This may be > > years away for a personal access token or hours for an OAuth access > > token. > > ... > > t/t0300-credentials.sh | 94 ++++++++++++++++++++++++++++++ > > https://github.com/git/git/actions/runs/4169057114/jobs/7217377625 > > Other platforms seem to be OK, but Windows test seems to be unhappy > with it when this topic gets merged to 'seen'. Curious, let me take a look. I see that the tests failed on freebsd too. I don't have a Windows machine to debug. If anyone reading has a hypothesis, please share. I shall try changing the default value for a password without expiry from TIME_MAX to 0, see if that works any better [2]. [1] https://github.com/git/git/pull/1443/checks?check_run_id=11112315291 [2] https://github.com/git/git/pull/1443/checks?check_run_id=11343677685 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3] credential: new attribute password_expiry_utc 2023-02-14 22:36 ` M Hickford @ 2023-02-17 21:44 ` Lessley Dennington 2023-02-17 21:59 ` Junio C Hamano 0 siblings, 1 reply; 26+ messages in thread From: Lessley Dennington @ 2023-02-17 21:44 UTC (permalink / raw) To: M Hickford, Junio C Hamano Cc: M Hickford via GitGitGadget, git, Eric Sunshine, Jeff King, Cheetham On 2/14/23 3:36 PM, M Hickford wrote: > Curious, let me take a look. I see that the tests failed on freebsd too. > I was curious about this as well and took a look on my Windows machine. It appears that errno will be set to 'No such file or directory' unless you explicitly set it before checking it. This is odd, given that the helper script I wrote worked just fine without this requirement. However, I took a look through the rest of the codebase and noticed that `errno = 0` seems to always be declared before this type of conditional check. That did indeed fix the issue - tests all pass on Windows with this patch. I have not confirmed on freebsd, though, as a heads up. Thanks, Lessley --- diff --git a/credential.c b/credential.c index d3e1bf7a67..b9a9a1d7b1 100644 --- a/credential.c +++ b/credential.c @@ -236,6 +236,7 @@ int credential_read(struct credential *c, FILE *fp) free(c->path); c->path = xstrdup(value); } else if (!strcmp(key, "password_expiry_utc")) { + errno = 0; c->password_expiry_utc = parse_timestamp(value, NULL, 10); if (c->password_expiry_utc == 0 || errno) c->password_expiry_utc = TIME_MAX; ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v3] credential: new attribute password_expiry_utc 2023-02-17 21:44 ` Lessley Dennington @ 2023-02-17 21:59 ` Junio C Hamano 2023-02-18 8:00 ` M Hickford 0 siblings, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2023-02-17 21:59 UTC (permalink / raw) To: Lessley Dennington Cc: M Hickford, M Hickford via GitGitGadget, git, Eric Sunshine, Jeff King, Cheetham Lessley Dennington <lessleydennington@gmail.com> writes: > diff --git a/credential.c b/credential.c > index d3e1bf7a67..b9a9a1d7b1 100644 > --- a/credential.c > +++ b/credential.c > @@ -236,6 +236,7 @@ int credential_read(struct credential *c, FILE *fp) > free(c->path); > c->path = xstrdup(value); > } else if (!strcmp(key, "password_expiry_utc")) { > + errno = 0; > c->password_expiry_utc = parse_timestamp(value, NULL, 10); > if (c->password_expiry_utc == 0 || errno) > c->password_expiry_utc = TIME_MAX; Ah, that is quite understandable. Successful library function calls would not _clera_ errno, so if there were a failure before the control reaches this codepath, errno may have been set, and then parse_timestamp() call, which would be a strto$some_integral_type() call, may succeed and will leave errno as-is. Your fix is absolutely correct as long as we want to use "errno" after the call returns. When strtoumax() etc. wants to report overflow or underflow, the returned value must be UINTMAX_MAX/UINTMAX_MIN and errno would be ERANGE, so it would probably want to check that errno is that value, and/or what c->password_expiry_utc has these overflow/underflow values. > ... I have not confirmed on freebsd, > though, as a heads up. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3] credential: new attribute password_expiry_utc 2023-02-17 21:59 ` Junio C Hamano @ 2023-02-18 8:00 ` M Hickford 0 siblings, 0 replies; 26+ messages in thread From: M Hickford @ 2023-02-18 8:00 UTC (permalink / raw) To: Junio C Hamano, Lessley Dennington Cc: M Hickford, M Hickford via GitGitGadget, git, Eric Sunshine, Jeff King, Cheetham On Fri, 17 Feb 2023 at 21:59, Junio C Hamano <gitster@pobox.com> wrote: > > Lessley Dennington <lessleydennington@gmail.com> writes: > > > diff --git a/credential.c b/credential.c > > index d3e1bf7a67..b9a9a1d7b1 100644 > > --- a/credential.c > > +++ b/credential.c > > @@ -236,6 +236,7 @@ int credential_read(struct credential *c, FILE *fp) > > free(c->path); > > c->path = xstrdup(value); > > } else if (!strcmp(key, "password_expiry_utc")) { > > + errno = 0; > > c->password_expiry_utc = parse_timestamp(value, NULL, 10); > > if (c->password_expiry_utc == 0 || errno) > > c->password_expiry_utc = TIME_MAX; > > Ah, that is quite understandable. Successful library function calls > would not _clera_ errno, so if there were a failure before the > control reaches this codepath, errno may have been set, and then > parse_timestamp() call, which would be a strto$some_integral_type() call, > may succeed and will leave errno as-is. Your fix is absolutely correct > as long as we want to use "errno" after the call returns. > > When strtoumax() etc. wants to report overflow or underflow, the > returned value must be UINTMAX_MAX/UINTMAX_MIN and errno would be > ERANGE, so it would probably want to check that errno is that value, > and/or what c->password_expiry_utc has these overflow/underflow > values. > > > ... I have not confirmed on freebsd, > > though, as a heads up. > That's a subtle one! Thanks Lessley very much for your help debugging. I shall send a patch v4 with this and some minor changes discussed in review club. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3] credential: new attribute password_expiry_utc 2023-02-04 21:16 ` [PATCH v3] " M Hickford via GitGitGadget 2023-02-14 1:59 ` Junio C Hamano @ 2023-02-14 8:03 ` Martin Ågren 2023-02-16 19:16 ` Calvin Wan 2023-02-18 6:32 ` [PATCH v4] " M Hickford via GitGitGadget 3 siblings, 0 replies; 26+ messages in thread From: Martin Ågren @ 2023-02-14 8:03 UTC (permalink / raw) To: M Hickford via GitGitGadget Cc: git, Eric Sunshine, Jeff King, Cheetham, Dennington, M Hickford On Sat, 4 Feb 2023 at 23:03, M Hickford via GitGitGadget <gitgitgadget@gmail.com> wrote: > --- a/Documentation/gitcredentials.txt > +++ b/Documentation/gitcredentials.txt > @@ -167,7 +167,7 @@ helper:: > If there are multiple instances of the `credential.helper` configuration > variable, each helper will be tried in turn, and may provide a username, > password, or nothing. Once Git has acquired both a username and a > -password, no more helpers will be tried. > +unexpired password, no more helpers will be tried. s/a unexpired/an unexpired/ (or "a non-expired", perhaps) Martin ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3] credential: new attribute password_expiry_utc 2023-02-04 21:16 ` [PATCH v3] " M Hickford via GitGitGadget 2023-02-14 1:59 ` Junio C Hamano 2023-02-14 8:03 ` Martin Ågren @ 2023-02-16 19:16 ` Calvin Wan 2023-02-18 8:00 ` M Hickford 2023-02-18 6:32 ` [PATCH v4] " M Hickford via GitGitGadget 3 siblings, 1 reply; 26+ messages in thread From: Calvin Wan @ 2023-02-16 19:16 UTC (permalink / raw) To: M Hickford via GitGitGadget Cc: Calvin Wan, git, Eric Sunshine, Jeff King, Cheetham, Dennington, M Hickford > static int run_credential_helper(struct credential *c, > @@ -342,6 +352,10 @@ void credential_fill(struct credential *c) > > for (i = 0; i < c->helpers.nr; i++) { > credential_do(c, c->helpers.items[i].string, "get"); > + if (c->password_expiry_utc < time(NULL)) { > + FREE_AND_NULL(c->password); > + c->password_expiry_utc = TIME_MAX; > + } > if (c->username && c->password) > return; > if (c->quit) I see you null out c->password in the expiry if block so that the following c->password check in the following if statement fails. While I think it's neat little trick, I wonder if others on list think it's better to be more explicit with how the logic should work (eg. adding the c->passowrd_expiry_utc check as an inner block inside of the c->username && c->password block). ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3] credential: new attribute password_expiry_utc 2023-02-16 19:16 ` Calvin Wan @ 2023-02-18 8:00 ` M Hickford 0 siblings, 0 replies; 26+ messages in thread From: M Hickford @ 2023-02-18 8:00 UTC (permalink / raw) To: Calvin Wan Cc: M Hickford via GitGitGadget, git, Eric Sunshine, Jeff King, Cheetham, Dennington, M Hickford On Thu, 16 Feb 2023 at 19:16, Calvin Wan <calvinwan@google.com> wrote: > > > static int run_credential_helper(struct credential *c, > > @@ -342,6 +352,10 @@ void credential_fill(struct credential *c) > > > > for (i = 0; i < c->helpers.nr; i++) { > > credential_do(c, c->helpers.items[i].string, "get"); > > + if (c->password_expiry_utc < time(NULL)) { > > + FREE_AND_NULL(c->password); > > + c->password_expiry_utc = TIME_MAX; > > + } > > if (c->username && c->password) > > return; > > if (c->quit) > > I see you null out c->password in the expiry if block so that the > following c->password check in the following if statement fails. > While I think it's neat little trick, I wonder if others on list > think it's better to be more explicit with how the logic should > work (eg. adding the c->passowrd_expiry_utc check as an inner > block inside of the c->username && c->password block). It's important to reset the expiry date as well as discard the expired password so that fill accepts a later password without expiry (see test cases). I'll add a comment in patch v4. ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v4] credential: new attribute password_expiry_utc 2023-02-04 21:16 ` [PATCH v3] " M Hickford via GitGitGadget ` (2 preceding siblings ...) 2023-02-16 19:16 ` Calvin Wan @ 2023-02-18 6:32 ` M Hickford via GitGitGadget 2023-02-22 19:22 ` Calvin Wan 3 siblings, 1 reply; 26+ messages in thread From: M Hickford via GitGitGadget @ 2023-02-18 6:32 UTC (permalink / raw) To: git Cc: Eric Sunshine, Jeff King, Cheetham, Dennington, Martin Ågren, Calvin Wan, M Hickford, M Hickford From: M Hickford <mirth.hickford@gmail.com> Some passwords have an expiry date known at generation. This may be years away for a personal access token or hours for an OAuth access token. When multiple credential helpers are configured, `credential fill` tries each helper in turn until it has a username and password, returning early. If Git authentication succeeds, `credential approve` stores the successful credential in all helpers. If authentication fails, `credential reject` erases matching credentials in all helpers. Helpers implement corresponding operations: get, store, erase. The credential protocol has no expiry attribute, so helpers cannot store expiry information. Even if a helper returned an improvised expiry attribute, git credential discards unrecognised attributes between operations and between helpers. This is a particular issue when a storage helper and a credential-generating helper are configured together: [credential] helper = storage # eg. cache or osxkeychain helper = generate # eg. oauth `credential approve` stores the generated credential in both helpers without expiry information. Later `credential fill` may return an expired credential from storage. There is no workaround, no matter how clever the second helper. The user sees authentication fail (a retry will succeed). Introduce a password expiry attribute. In `credential fill`, ignore expired passwords and continue to query subsequent helpers. In the example above, `credential fill` ignores the expired password and a fresh credential is generated. If authentication succeeds, `credential approve` replaces the expired password in storage. If authentication fails, the expired credential is erased by `credential reject`. It is unnecessary but harmless for storage helpers to self prune expired credentials. Add support for the new attribute to credential-cache. Eventually, I hope to see support in other popular storage helpers. Example usage in a credential-generating helper https://github.com/hickford/git-credential-oauth/pull/16 Signed-off-by: M Hickford <mirth.hickford@gmail.com> --- credential: new attribute password_expiry_utc details in commit message Changes in patch v4: * Set errno = 0 to fix tests on Windows and FreeBSD (thanks Lessley Dennington) * Clarify rationale as discussed at review club Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1443%2Fhickford%2Fpassword-expiry-v4 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1443/hickford/password-expiry-v4 Pull-Request: https://github.com/git/git/pull/1443 Range-diff vs v3: 1: 1846815a5c1 ! 1: ae8bddbb30a credential: new attribute password_expiry_utc @@ Commit message Helpers implement corresponding operations: get, store, erase. The credential protocol has no expiry attribute, so helpers cannot - store expiry information. (Even if a helper returned an improvised + store expiry information. Even if a helper returned an improvised expiry attribute, git credential discards unrecognised attributes - between operations and between helpers.) + between operations and between helpers. - As a workaround, whenever monolithic helper Git Credential Manager (GCM) - retrieves an OAuth credential from its storage, it makes a HTTP request - to check whether the OAuth token has expired [1]. This complicates and - slows the authentication happy path. - - Worse is the case that a storage helper and a credential-generating - helper are configured together: + This is a particular issue when a storage helper and a + credential-generating helper are configured together: [credential] helper = storage # eg. cache or osxkeychain - helper = generate # eg. oauth or manager + helper = generate # eg. oauth `credential approve` stores the generated credential in both helpers without expiry information. Later `credential fill` may return an expired credential from storage. There is no workaround, no matter how - clever the second helper. + clever the second helper. The user sees authentication fail (a retry + will succeed). Introduce a password expiry attribute. In `credential fill`, ignore expired passwords and continue to query subsequent helpers. - In the example above, `credential fill` ignores the expired credential + In the example above, `credential fill` ignores the expired password and a fresh credential is generated. If authentication succeeds, - `credential approve` replaces the expired credential in storage. + `credential approve` replaces the expired password in storage. If authentication fails, the expired credential is erased by `credential reject`. It is unnecessary but harmless for storage helpers to self prune expired credentials. Add support for the new attribute to credential-cache. - Eventually, I hope to see support in other storage helpers. + Eventually, I hope to see support in other popular storage helpers. Example usage in a credential-generating helper https://github.com/hickford/git-credential-oauth/pull/16 - [1] https://github.com/GitCredentialManager/git-credential-manager/blob/66b94e489ad8cc1982836355493e369770b30211/src/shared/GitLab/GitLabHostProvider.cs#L217 - Signed-off-by: M Hickford <mirth.hickford@gmail.com> ## Documentation/git-credential.txt ## @@ Documentation/gitcredentials.txt: helper:: variable, each helper will be tried in turn, and may provide a username, password, or nothing. Once Git has acquired both a username and a -password, no more helpers will be tried. -+unexpired password, no more helpers will be tried. ++non-expired password, no more helpers will be tried. + If `credential.helper` is configured to the empty string, this resets the helper list to empty (so you may override a helper set by a @@ credential.c: int credential_read(struct credential *c, FILE *fp) free(c->path); c->path = xstrdup(value); + } else if (!strcmp(key, "password_expiry_utc")) { ++ errno = 0; + c->password_expiry_utc = parse_timestamp(value, NULL, 10); -+ if (c->password_expiry_utc == 0 || errno) ++ if (c->password_expiry_utc == 0 || errno == ERANGE) + c->password_expiry_utc = TIME_MAX; } else if (!strcmp(key, "url")) { credential_from_url(c, value); @@ credential.c: void credential_fill(struct credential *c) for (i = 0; i < c->helpers.nr; i++) { credential_do(c, c->helpers.items[i].string, "get"); + if (c->password_expiry_utc < time(NULL)) { ++ /* Discard expired password */ + FREE_AND_NULL(c->password); ++ /* Reset expiry to maintain consistency */ + c->password_expiry_utc = TIME_MAX; + } if (c->username && c->password) @@ t/t0300-credentials.sh: test_expect_success 'credential_fill continues through p + EOF +' + -+test_expect_success 'credential_fill continues through expired password' ' ++test_expect_success 'credential_fill ignores expired password' ' + check fill "verbatim-with-expiry one two 5" "verbatim three four" <<-\EOF + protocol=http + host=example.com @@ t/t0300-credentials.sh: test_expect_success 'do not bother storing password-less EOF ' -+test_expect_success 'credential_approve does not store expired credential' ' ++test_expect_success 'credential_approve does not store expired password' ' + check approve useless <<-\EOF + protocol=http + host=example.com @@ t/t0300-credentials.sh: test_expect_success 'credential_reject calls all helpers EOF ' -+test_expect_success 'credential_reject erases expired credential' ' ++test_expect_success 'credential_reject erases credential regardless of expiry' ' + check reject useless <<-\EOF + protocol=http + host=example.com Documentation/git-credential.txt | 6 ++ Documentation/gitcredentials.txt | 2 +- builtin/credential-cache--daemon.c | 3 + credential.c | 20 ++++++- credential.h | 2 + t/t0300-credentials.sh | 94 ++++++++++++++++++++++++++++++ 6 files changed, 125 insertions(+), 2 deletions(-) diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt index ac2818b9f66..29d184ab824 100644 --- a/Documentation/git-credential.txt +++ b/Documentation/git-credential.txt @@ -144,6 +144,12 @@ Git understands the following attributes: The credential's password, if we are asking it to be stored. +`password_expiry_utc`:: + + Generated passwords such as an OAuth access token may have an expiry date. + When reading credentials from helpers, `git credential fill` ignores expired + passwords. Represented as Unix time UTC, seconds since 1970. + `url`:: When this special attribute is read by `git credential`, the diff --git a/Documentation/gitcredentials.txt b/Documentation/gitcredentials.txt index 4522471c337..100f045bb1a 100644 --- a/Documentation/gitcredentials.txt +++ b/Documentation/gitcredentials.txt @@ -167,7 +167,7 @@ helper:: If there are multiple instances of the `credential.helper` configuration variable, each helper will be tried in turn, and may provide a username, password, or nothing. Once Git has acquired both a username and a -password, no more helpers will be tried. +non-expired password, no more helpers will be tried. + If `credential.helper` is configured to the empty string, this resets the helper list to empty (so you may override a helper set by a diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c index f3c89831d4a..338058be7f9 100644 --- a/builtin/credential-cache--daemon.c +++ b/builtin/credential-cache--daemon.c @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out) if (e) { fprintf(out, "username=%s\n", e->item.username); fprintf(out, "password=%s\n", e->item.password); + if (e->item.password_expiry_utc != TIME_MAX) + fprintf(out, "password_expiry_utc=%"PRItime"\n", + e->item.password_expiry_utc); } } else if (!strcmp(action.buf, "exit")) { diff --git a/credential.c b/credential.c index f6389a50684..f32011343f9 100644 --- a/credential.c +++ b/credential.c @@ -7,6 +7,7 @@ #include "prompt.h" #include "sigchain.h" #include "urlmatch.h" +#include "git-compat-util.h" void credential_init(struct credential *c) { @@ -234,6 +235,11 @@ int credential_read(struct credential *c, FILE *fp) } else if (!strcmp(key, "path")) { free(c->path); c->path = xstrdup(value); + } else if (!strcmp(key, "password_expiry_utc")) { + errno = 0; + c->password_expiry_utc = parse_timestamp(value, NULL, 10); + if (c->password_expiry_utc == 0 || errno == ERANGE) + c->password_expiry_utc = TIME_MAX; } else if (!strcmp(key, "url")) { credential_from_url(c, value); } else if (!strcmp(key, "quit")) { @@ -269,6 +275,11 @@ void credential_write(const struct credential *c, FILE *fp) credential_write_item(fp, "path", c->path, 0); credential_write_item(fp, "username", c->username, 0); credential_write_item(fp, "password", c->password, 0); + if (c->password_expiry_utc != TIME_MAX) { + char *s = xstrfmt("%"PRItime, c->password_expiry_utc); + credential_write_item(fp, "password_expiry_utc", s, 0); + free(s); + } } static int run_credential_helper(struct credential *c, @@ -342,6 +353,12 @@ void credential_fill(struct credential *c) for (i = 0; i < c->helpers.nr; i++) { credential_do(c, c->helpers.items[i].string, "get"); + if (c->password_expiry_utc < time(NULL)) { + /* Discard expired password */ + FREE_AND_NULL(c->password); + /* Reset expiry to maintain consistency */ + c->password_expiry_utc = TIME_MAX; + } if (c->username && c->password) return; if (c->quit) @@ -360,7 +377,7 @@ void credential_approve(struct credential *c) if (c->approved) return; - if (!c->username || !c->password) + if (!c->username || !c->password || c->password_expiry_utc < time(NULL)) return; credential_apply_config(c); @@ -381,6 +398,7 @@ void credential_reject(struct credential *c) FREE_AND_NULL(c->username); FREE_AND_NULL(c->password); + c->password_expiry_utc = TIME_MAX; c->approved = 0; } diff --git a/credential.h b/credential.h index f430e77fea4..935b28a70f1 100644 --- a/credential.h +++ b/credential.h @@ -126,10 +126,12 @@ struct credential { char *protocol; char *host; char *path; + timestamp_t password_expiry_utc; }; #define CREDENTIAL_INIT { \ .helpers = STRING_LIST_INIT_DUP, \ + .password_expiry_utc = TIME_MAX, \ } /* Initialize a credential structure, setting all fields to empty. */ diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh index 3485c0534e6..c66d91e82d8 100755 --- a/t/t0300-credentials.sh +++ b/t/t0300-credentials.sh @@ -35,6 +35,16 @@ test_expect_success 'setup helper scripts' ' test -z "$pass" || echo password=$pass EOF + write_script git-credential-verbatim-with-expiry <<-\EOF && + user=$1; shift + pass=$1; shift + pexpiry=$1; shift + . ./dump + test -z "$user" || echo username=$user + test -z "$pass" || echo password=$pass + test -z "$pexpiry" || echo password_expiry_utc=$pexpiry + EOF + PATH="$PWD:$PATH" ' @@ -109,6 +119,43 @@ test_expect_success 'credential_fill continues through partial response' ' EOF ' +test_expect_success 'credential_fill populates password_expiry_utc' ' + check fill "verbatim-with-expiry one two 9999999999" <<-\EOF + protocol=http + host=example.com + -- + protocol=http + host=example.com + username=one + password=two + password_expiry_utc=9999999999 + -- + verbatim-with-expiry: get + verbatim-with-expiry: protocol=http + verbatim-with-expiry: host=example.com + EOF +' + +test_expect_success 'credential_fill ignores expired password' ' + check fill "verbatim-with-expiry one two 5" "verbatim three four" <<-\EOF + protocol=http + host=example.com + -- + protocol=http + host=example.com + username=three + password=four + -- + verbatim-with-expiry: get + verbatim-with-expiry: protocol=http + verbatim-with-expiry: host=example.com + verbatim: get + verbatim: protocol=http + verbatim: host=example.com + verbatim: username=one + EOF +' + test_expect_success 'credential_fill passes along metadata' ' check fill "verbatim one two" <<-\EOF protocol=ftp @@ -149,6 +196,24 @@ test_expect_success 'credential_approve calls all helpers' ' EOF ' +test_expect_success 'credential_approve stores password expiry' ' + check approve useless <<-\EOF + protocol=http + host=example.com + username=foo + password=bar + password_expiry_utc=9999999999 + -- + -- + useless: store + useless: protocol=http + useless: host=example.com + useless: username=foo + useless: password=bar + useless: password_expiry_utc=9999999999 + EOF +' + test_expect_success 'do not bother storing password-less credential' ' check approve useless <<-\EOF protocol=http @@ -159,6 +224,17 @@ test_expect_success 'do not bother storing password-less credential' ' EOF ' +test_expect_success 'credential_approve does not store expired password' ' + check approve useless <<-\EOF + protocol=http + host=example.com + username=foo + password=bar + password_expiry_utc=5 + -- + -- + EOF +' test_expect_success 'credential_reject calls all helpers' ' check reject useless "verbatim one two" <<-\EOF @@ -181,6 +257,24 @@ test_expect_success 'credential_reject calls all helpers' ' EOF ' +test_expect_success 'credential_reject erases credential regardless of expiry' ' + check reject useless <<-\EOF + protocol=http + host=example.com + username=foo + password=bar + password_expiry_utc=5 + -- + -- + useless: erase + useless: protocol=http + useless: host=example.com + useless: username=foo + useless: password=bar + useless: password_expiry_utc=5 + EOF +' + test_expect_success 'usernames can be preserved' ' check fill "verbatim \"\" three" <<-\EOF protocol=http base-commit: 2fc9e9ca3c7505bc60069f11e7ef09b1aeeee473 -- gitgitgadget ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v4] credential: new attribute password_expiry_utc 2023-02-18 6:32 ` [PATCH v4] " M Hickford via GitGitGadget @ 2023-02-22 19:22 ` Calvin Wan 0 siblings, 0 replies; 26+ messages in thread From: Calvin Wan @ 2023-02-22 19:22 UTC (permalink / raw) To: M Hickford via GitGitGadget Cc: git, Eric Sunshine, Jeff King, Cheetham, Dennington, Martin Ågren, M Hickford > static int run_credential_helper(struct credential *c, > @@ -342,6 +353,12 @@ void credential_fill(struct credential *c) > > for (i = 0; i < c->helpers.nr; i++) { > credential_do(c, c->helpers.items[i].string, "get"); > + if (c->password_expiry_utc < time(NULL)) { > + /* Discard expired password */ > + FREE_AND_NULL(c->password); > + /* Reset expiry to maintain consistency */ > + c->password_expiry_utc = TIME_MAX; > + } > if (c->username && c->password) > return; > if (c->quit) Thanks for clarifying this block! Overall, this patch is additive and shouldn't cause any regressions for current users of credential/credential-helper so I'm all for adding an expiry attribute to alleviate the use case pains you described above. Reviewed-by: Calvin Wan <calvinwan@google.com> ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2023-02-22 19:22 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-01-28 14:04 [PATCH] credential: new attribute password_expiry_utc M Hickford via GitGitGadget 2023-01-29 20:17 ` Junio C Hamano 2023-02-01 8:29 ` M Hickford 2023-02-01 18:50 ` Junio C Hamano 2023-01-30 0:59 ` Eric Sunshine 2023-02-05 6:49 ` M Hickford 2023-02-01 9:39 ` [PATCH v2] " M Hickford via GitGitGadget 2023-02-01 12:10 ` Jeff King 2023-02-01 17:12 ` Junio C Hamano 2023-02-02 0:12 ` Jeff King 2023-02-01 20:02 ` Matthew John Cheetham 2023-02-02 0:23 ` Jeff King 2023-02-05 6:45 ` M Hickford 2023-02-06 18:59 ` Matthew John Cheetham 2023-02-05 6:34 ` M Hickford 2023-02-04 21:16 ` [PATCH v3] " M Hickford via GitGitGadget 2023-02-14 1:59 ` Junio C Hamano 2023-02-14 22:36 ` M Hickford 2023-02-17 21:44 ` Lessley Dennington 2023-02-17 21:59 ` Junio C Hamano 2023-02-18 8:00 ` M Hickford 2023-02-14 8:03 ` Martin Ågren 2023-02-16 19:16 ` Calvin Wan 2023-02-18 8:00 ` M Hickford 2023-02-18 6:32 ` [PATCH v4] " M Hickford via GitGitGadget 2023-02-22 19:22 ` Calvin Wan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).