{"thread":{"id":"59159","subject":"[PATCH] credential: new attribute password_expiry_utc","startedAt":"2023-01-28T14:04:17Z","lastAt":"2023-02-22T19:22:18Z","messageCount":26,"participants":["M Hickford via GitGitGadget","Junio C Hamano","Eric Sunshine","M Hickford","Jeff King","Matthew John Cheetham","Martin Ågren","Calvin Wan","Lessley Dennington"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"471115","messageId":"pull.1443.git.git.1674914650588.gitgitgadget@gmail.com","threadId":"59159","inReplyTo":null,"subject":"[PATCH] credential: new attribute password_expiry_utc","fromName":"M Hickford via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-28T14:04:10Z","receivedAt":"2023-01-28T14:04:17Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"From: M Hickford \n\nIf password has expired, credential fill no longer returns early,\nso later helpers can generate a fresh credential. This is backwards\ncompatible -- no change in behaviour with helpers that discard the\nexpiry attribute. The expiry logic is entirely in the git credential\nlayer; compatible helpers simply store and return the expiry\nattribute verbatim.\n\nStore new attribute in cache.\n\nSigned-off-by: M Hickford \n---\n credential: new attribute password_expiry_utc\n \n Some passwords, such as a personal access token or OAuth access token,\n may have an expiry date (as long as years for PATs or as short as hours\n for an OAuth access token). Add a new credential attribute\n password_expiry_utc.\n \n If password has expired, credential fill no longer returns early, so\n later helpers have opportunity to generate a fresh credential. This is\n backwards compatible -- no change in behaviour with helpers that discard\n the expiry attribute. The expiry logic is entirely in the git credential\n layer. Credential-generating helpers need only output the expiry\n attribute. Storage helpers should store the expiry if they can.\n \n Store expiry attribute in cache.\n \n This is particularly useful when a storage helper and a\n credential-generating helper are configured together, eg.\n \n [credential]\n helper = storage # eg. cache or osxkeychain\n helper = generate # eg. oauth\n \n \n Without this patch, credential fill may return an expired credential\n from storage, causing authentication to fail. With this patch: a fresh\n credential is generated if and only if the credential is expired.\n \n Example usage in a credential-generating helper\n https://github.com/hickford/git-credential-oauth/pull/16/files\n \n Signed-off-by: M Hickford mirth.hickford@gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1443%2Fhickford%2Fpassword-expiry-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1443/hickford/password-expiry-v1\nPull-Request: https://github.com/git/git/pull/1443\n\n Documentation/git-credential.txt | 4 ++++\n builtin/credential-cache--daemon.c | 3 +++\n credential.c | 21 +++++++++++++++++++++\n credential.h | 1 +\n 4 files changed, 29 insertions(+)\n\ndiff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt\nindex ac2818b9f66..15ace648bdd 100644\n--- a/Documentation/git-credential.txt\n+++ b/Documentation/git-credential.txt\n@@ -144,6 +144,10 @@ Git understands the following attributes:\n \n \tThe credential's password, if we are asking it to be stored.\n \n+`password_expiry_utc`::\n+\n+\tIf 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.\n+\n `url`::\n \n \tWhen this special attribute is read by `git credential`, the\ndiff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\nindex f3c89831d4a..5cb8a186b45 100644\n--- a/builtin/credential-cache--daemon.c\n+++ b/builtin/credential-cache--daemon.c\n@@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out)\n \t\tif (e) {\n \t\t\tfprintf(out, \"username=%s\\n\", e->item.username);\n \t\t\tfprintf(out, \"password=%s\\n\", e->item.password);\n+\t\t\tif (e->item.password_expiry_utc != 0) {\n+\t\t\t\tfprintf(out, \"password_expiry_utc=%ld\\n\", e->item.password_expiry_utc);\n+\t\t\t}\n \t\t}\n \t}\n \telse if (!strcmp(action.buf, \"exit\")) {\ndiff --git a/credential.c b/credential.c\nindex f6389a50684..0a3a9cbf0a2 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -7,6 +7,7 @@\n #include \"prompt.h\"\n #include \"sigchain.h\"\n #include \"urlmatch.h\"\n+#include \n \n void credential_init(struct credential *c)\n {\n@@ -21,6 +22,7 @@ void credential_clear(struct credential *c)\n \tfree(c->path);\n \tfree(c->username);\n \tfree(c->password);\n+\tc->password_expiry_utc = 0;\n \tstring_list_clear(&c->helpers, 0);\n \n \tcredential_init(c);\n@@ -234,11 +236,23 @@ int credential_read(struct credential *c, FILE *fp)\n \t\t} else if (!strcmp(key, \"path\")) {\n \t\t\tfree(c->path);\n \t\t\tc->path = xstrdup(value);\n+\t\t} else if (!strcmp(key, \"password_expiry_utc\")) {\n+\t\t\t// TODO: ignore if can't parse integer\n+\t\t\tc->password_expiry_utc = atoi(value);\n \t\t} else if (!strcmp(key, \"url\")) {\n \t\t\tcredential_from_url(c, value);\n \t\t} else if (!strcmp(key, \"quit\")) {\n \t\t\tc->quit = !!git_config_bool(\"quit\", value);\n \t\t}\n+\n+\t\t// if expiry date has passed, ignore password and expiry fields\n+\t\tif (c->password_expiry_utc != 0 && time(NULL) > c->password_expiry_utc) {\n+\t\t\ttrace_printf(_(\"Password has expired.\\n\"));\n+\t\t\tFREE_AND_NULL(c->username);\n+\t\t\tFREE_AND_NULL(c->password);\n+\t\t\tc->password_expiry_utc = 0;\n+\t\t}\n+\n \t\t/*\n \t\t * Ignore other lines; we don't know what they mean, but\n \t\t * this future-proofs us when later versions of git do\n@@ -269,6 +283,13 @@ void credential_write(const struct credential *c, FILE *fp)\n \tcredential_write_item(fp, \"path\", c->path, 0);\n \tcredential_write_item(fp, \"username\", c->username, 0);\n \tcredential_write_item(fp, \"password\", c->password, 0);\n+\tif (c->password_expiry_utc != 0) {\n+\t\tint length = snprintf( NULL, 0, \"%ld\", c->password_expiry_utc);\n+\t\tchar* str = malloc( length + 1 );\n+\t\tsnprintf( str, length + 1, \"%ld\", c->password_expiry_utc );\n+\t\tcredential_write_item(fp, \"password_expiry_utc\", str, 0);\n+\t\tfree(str);\n+\t}\n }\n \n static int run_credential_helper(struct credential *c,\ndiff --git a/credential.h b/credential.h\nindex f430e77fea4..e10f7c2b313 100644\n--- a/credential.h\n+++ b/credential.h\n@@ -126,6 +126,7 @@ struct credential {\n \tchar *protocol;\n \tchar *host;\n \tchar *path;\n+\ttime_t password_expiry_utc;\n };\n \n #define CREDENTIAL_INIT { \\\n\nbase-commit: 5cc9858f1b470844dea5c5d3e936af183fdf2c68\n-- \ngitgitgadget\n"},{"id":"471129","messageId":"xmqqpmax5c4v.fsf@gitster.g","threadId":"59159","inReplyTo":"pull.1443.git.git.1674914650588.gitgitgadget@gmail.com","subject":"Re: [PATCH] credential: new attribute password_expiry_utc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-29T20:17:20Z","receivedAt":"2023-01-29T20:17:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"M Hickford via GitGitGadget\" writes:\n\n> From: M Hickford \n>\n> If password has expired, credential fill no longer returns early,\n> so later helpers can generate a fresh credential. This is backwards\n> compatible -- no change in behaviour with helpers that discard the\n> expiry attribute. The expiry logic is entirely in the git credential\n> layer; compatible helpers simply store and return the expiry\n> attribute verbatim.\n>\n> Store new attribute in cache.\n\nIt is unclear what you are describing in the above. The current\nbehaviour without the patch? The behaviour of the code if this\npatch gets applied? Write it in such a way that it is clear why\nthe patch is a good idea, not just \"this would not hurt because it\nis backwards compatible\".\n\nThe usual way to do so is to sell your change in this order:\n\n - Give background information to help readers understand what you\n are going to write in the following explanation.\n\n - Describe the current behaviour without any change to the code;\n\n - Present a situation where the current code results in an\n undesirable outcome. What exactly happens, what visible effect it\n has to the user, how the code could do better to help the user?\n\n - Propose an updated behaviour that would behave better in the\n above sample situation presented.\n\nCuriously, what you wrote below the \"---\" line, that will not be\npart of the log message, looks to be organized better than the\nabove. The first paragraph (except for the \"Add ...\") prepares the\nreaders, It is still unclear if the second paragraph \"when expired\"\ndescribes what happens with the current code (i.e. highlighting why\na change is needed) or what you want to happen with the patch, but\nthe paragraph should first explain the problem in the current\nbehaviour to motivate readers to learn why the updated code would\nlead to a better world. And follow that with the behaviour of the\nupdated code and its effect (e.g. \"without first trying a credential\nthat is stale and see it fail before asking to reauthenticate, such\na known-to-be-stale credential gets discarded automatically\").\n\n\n> +`password_expiry_utc`::\n> +\n> +\tIf 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.\n> +\n\nA overly long line. Please follow Documentation/CodingGuidelines\nand Documentation/SubmittingPatches\n\n> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\n> index f3c89831d4a..5cb8a186b45 100644\n> --- a/builtin/credential-cache--daemon.c\n> +++ b/builtin/credential-cache--daemon.c\n> @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out)\n> \t\tif (e) {\n> \t\t\tfprintf(out, \"username=%s\\n\", e->item.username);\n> \t\t\tfprintf(out, \"password=%s\\n\", e->item.password);\n> +\t\t\tif (e->item.password_expiry_utc != 0) {\n> +\t\t\t\tfprintf(out, \"password_expiry_utc=%ld\\n\", e->item.password_expiry_utc);\n> +\t\t\t}\n\nStyle (multiple issues, check CodingGuidelines):\n\n\t\tif (e->item.password_expiry_utc)\n\t\t\tfprintf(out, \"... overly long format template ...\",\n\t\t\t\te->item.password_expiry_utc);\n\n * Using integral value or pointer value as a truth value does not\n require an explicit comparison with 0;\n\n * A single-statement block does not need {} around it;\n\n * Overly long line should be folded, with properly indented.\n\n> diff --git a/credential.c b/credential.c\n> index f6389a50684..0a3a9cbf0a2 100644\n> --- a/credential.c\n> +++ b/credential.c\n> @@ -7,6 +7,7 @@\n> #include \"prompt.h\"\n> #include \"sigchain.h\"\n> #include \"urlmatch.h\"\n> +#include \n\nDon't include system headers directly; often git-compat-util.h\nalready has it, and if not, we need to find the right place to have\nit in git-compat-util.h file, as there are platforms that are\nfinicky in inclusion order of the header files and definition of\nfeature macros.\n\n> @@ -21,6 +22,7 @@ void credential_clear(struct credential *c)\n> \tfree(c->path);\n> \tfree(c->username);\n> \tfree(c->password);\n> +\tc->password_expiry_utc = 0;\n\nNot a huge deal, but if the rule is \"an credential with expiry\ntimestamp that is too old behaves as if it no longer exists or is\nvalid\", then a large integer, not zero, may serve as a better\nsentinel value for \"this entry never expires\". Instead of having to\ndo\n\n\tif (expiry && expiry < time()) {\n\t\t... expired ...\n\t}\n\nyou can just do\n\n\tif (expiry < time()) {\n\t\t... expired ...\n\t}\n\nand that would be simpler to understand for human readers, too.\n\n> @@ -234,11 +236,23 @@ int credential_read(struct credential *c, FILE *fp)\n> \t\t} else if (!strcmp(key, \"path\")) {\n> \t\t\tfree(c->path);\n> \t\t\tc->path = xstrdup(value);\n> +\t\t} else if (!strcmp(key, \"password_expiry_utc\")) {\n> +\t\t\t// TODO: ignore if can't parse integer\n\nDo not use // comment. /* Our single-liner comment reads like this */\n\n> +\t\t\tc->password_expiry_utc = atoi(value);\n\nDon't use atoi(); make sure value is not followed by a non-number,\ne.g.\n\n\tconst char *value = \"43q\";\n\tprintf(\"%d<%s>\\n\", atoi(value), value);\n\nwould give you 43<43q>, but you want to reject and silently ignore\nsuch an expiry timestamp.\n\n> +\t\t// if expiry date has passed, ignore password and expiry fields\n\nDitto, but if you used a large value as sentinel for \"never expires\"\nand wrote it like this\n\n\t\tif (c->password_expiry_utc < time(NULL)) {\n\nthen it is clear enough that you do not even need such a comment.\nThe expression itself makes it clear what is going on (i.e. the\ncurrent time comes later than the expiry_utc value on the number\nline hence it appears on the right to it, clearly showing that it\nhas passed the threshold).\n\n> +\t\tif (c->password_expiry_utc != 0 && time(NULL) > c->password_expiry_utc) {\n> +\t\t\ttrace_printf(_(\"Password has expired.\\n\"));\n> +\t\t\tFREE_AND_NULL(c->username);\n> +\t\t\tFREE_AND_NULL(c->password);\n> +\t\t\tc->password_expiry_utc = 0;\n> +\t\t}\n> +\n"},{"id":"471130","messageId":"CAPig+cQPLMrUKp0aqLCknSYCs5TAso-VSBYsQbGZ8g8wgY2Liw@mail.gmail.com","threadId":"59159","inReplyTo":"pull.1443.git.git.1674914650588.gitgitgadget@gmail.com","subject":"Re: [PATCH] credential: new attribute password_expiry_utc","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-01-30T00:59:04Z","receivedAt":"2023-01-30T01:00:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Jan 28, 2023 at 9:08 AM M Hickford via GitGitGadget\n wrote:\n> If password has expired, credential fill no longer returns early,\n> so later helpers can generate a fresh credential. This is backwards\n> compatible -- no change in behaviour with helpers that discard the\n> expiry attribute. The expiry logic is entirely in the git credential\n> layer; compatible helpers simply store and return the expiry\n> attribute verbatim.\n>\n> Store new attribute in cache.\n>\n> Signed-off-by: M Hickford \n\nJust a few comments in addition to those already provided by Junio...\n\n> diff --git a/credential.c b/credential.c\n> @@ -234,11 +236,23 @@ int credential_read(struct credential *c, FILE *fp)\n> + // if expiry date has passed, ignore password and expiry fields\n> + if (c->password_expiry_utc != 0 && time(NULL) > c->password_expiry_utc) {\n> + trace_printf(_(\"Password has expired.\\n\"));\n\nUsing `_(...)` marks a string for localization, but doing so is\nundesirable for debugging messages which are meant for the developer,\nnot the end user (and it creates extra work for translators). No\nexisting[1] trace_printf() calls in the codebase use `_(...)`.\n\n[1]: Unfortunately, a couple examples exist in\nDocumentation/MyFirstObjectWalk.txt using `_(...)` but they should be\nremoved.\n\n> @@ -269,6 +283,13 @@ void credential_write(const struct credential *c, FILE *fp)\n> + if (c->password_expiry_utc != 0) {\n> + int length = snprintf( NULL, 0, \"%ld\", c->password_expiry_utc);\n> + char* str = malloc( length + 1 );\n\nStyle in this project is `char *str`, not `char* str`. Also, drop\nspaces around function arguments:\n\n char *str = malloc(length + 1);\n\n> + snprintf( str, length + 1, \"%ld\", c->password_expiry_utc );\n\nSame.\n\n> + credential_write_item(fp, \"password_expiry_utc\", str, 0);\n> + free(str);\n> + }\n\nxstrfmt() from strbuf.h can help simplify this entire block:\n\n char *s = xstrfmt(\"%ld\", c->password_expiry_utc);\n credential_write_item(fp, \"password_expiry_utc\", str, 0);\n free(s);\n"},{"id":"471218","messageId":"CAGJzqskO0sGNtuuSkKWxknh1qv523TfA3U17X_9higDdYdg+PA@mail.gmail.com","threadId":"59159","inReplyTo":"xmqqpmax5c4v.fsf@gitster.g","subject":"Re: [PATCH] credential: new attribute password_expiry_utc","fromName":"M Hickford","fromEmail":"mirth.hickford@gmail.com","sentAt":"2023-02-01T08:29:56Z","receivedAt":"2023-02-01T08:30:41Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"On Sun, 29 Jan 2023 at 20:17, Junio C Hamano wrote:\n>\n> \"M Hickford via GitGitGadget\" writes:\n>\n> > From: M Hickford \n> >\n> > If password has expired, credential fill no longer returns early,\n> > so later helpers can generate a fresh credential. This is backwards\n> > compatible -- no change in behaviour with helpers that discard the\n> > expiry attribute. The expiry logic is entirely in the git credential\n> > layer; compatible helpers simply store and return the expiry\n> > attribute verbatim.\n> >\n> > Store new attribute in cache.\n>\n> It is unclear what you are describing in the above. The current\n> behaviour without the patch? The behaviour of the code if this\n> patch gets applied? Write it in such a way that it is clear why\n> the patch is a good idea, not just \"this would not hurt because it\n> is backwards compatible\".\n>\n> The usual way to do so is to sell your change in this order:\n>\n> - Give background information to help readers understand what you\n> are going to write in the following explanation.\n>\n> - Describe the current behaviour without any change to the code;\n>\n> - Present a situation where the current code results in an\n> undesirable outcome. What exactly happens, what visible effect it\n> has to the user, how the code could do better to help the user?\n>\n> - Propose an updated behaviour that would behave better in the\n> above sample situation presented.\n>\n\nThanks for the guidance. Writing a better commit message clarified my\nown thoughts.\n\n> Curiously, what you wrote below the \"---\" line, that will not be\n> part of the log message, looks to be organized better than the\n> above. The first paragraph (except for the \"Add ...\") prepares the\n> readers, It is still unclear if the second paragraph \"when expired\"\n> describes what happens with the current code (i.e. highlighting why\n> a change is needed) or what you want to happen with the patch, but\n> the paragraph should first explain the problem in the current\n> behaviour to motivate readers to learn why the updated code would\n> lead to a better world. And follow that with the behaviour of the\n> updated code and its effect (e.g. \"without first trying a credential\n> that is stale and see it fail before asking to reauthenticate, such\n> a known-to-be-stale credential gets discarded automatically\").\n>\n>\n> > +`password_expiry_utc`::\n> > +\n> > + 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.\n> > +\n>\n> A overly long line. Please follow Documentation/CodingGuidelines\n> and Documentation/SubmittingPatches\n>\n> > diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\n> > index f3c89831d4a..5cb8a186b45 100644\n> > --- a/builtin/credential-cache--daemon.c\n> > +++ b/builtin/credential-cache--daemon.c\n> > @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out)\n> > if (e) {\n> > fprintf(out, \"username=%s\\n\", e->item.username);\n> > fprintf(out, \"password=%s\\n\", e->item.password);\n> > + if (e->item.password_expiry_utc != 0) {\n> > + fprintf(out, \"password_expiry_utc=%ld\\n\", e->item.password_expiry_utc);\n> > + }\n>\n> Style (multiple issues, check CodingGuidelines):\n>\n> if (e->item.password_expiry_utc)\n> fprintf(out, \"... overly long format template ...\",\n> e->item.password_expiry_utc);\n>\n> * Using integral value or pointer value as a truth value does not\n> require an explicit comparison with 0;\n>\n> * A single-statement block does not need {} around it;\n>\n> * Overly long line should be folded, with properly indented.\n>\n> > diff --git a/credential.c b/credential.c\n> > index f6389a50684..0a3a9cbf0a2 100644\n> > --- a/credential.c\n> > +++ b/credential.c\n> > @@ -7,6 +7,7 @@\n> > #include \"prompt.h\"\n> > #include \"sigchain.h\"\n> > #include \"urlmatch.h\"\n> > +#include \n>\n> Don't include system headers directly; often git-compat-util.h\n> already has it, and if not, we need to find the right place to have\n> it in git-compat-util.h file, as there are platforms that are\n> finicky in inclusion order of the header files and definition of\n> feature macros.\n>\n> > @@ -21,6 +22,7 @@ void credential_clear(struct credential *c)\n> > free(c->path);\n> > free(c->username);\n> > free(c->password);\n> > + c->password_expiry_utc = 0;\n>\n> Not a huge deal, but if the rule is \"an credential with expiry\n> timestamp that is too old behaves as if it no longer exists or is\n> valid\", then a large integer, not zero, may serve as a better\n> sentinel value for \"this entry never expires\". Instead of having to\n> do\n>\n> if (expiry && expiry < time()) {\n> ... expired ...\n> }\n>\n> you can just do\n>\n> if (expiry < time()) {\n> ... expired ...\n> }\n>\n> and that would be simpler to understand for human readers, too.\n>\n> > @@ -234,11 +236,23 @@ int credential_read(struct credential *c, FILE *fp)\n> > } else if (!strcmp(key, \"path\")) {\n> > free(c->path);\n> > c->path = xstrdup(value);\n> > + } else if (!strcmp(key, \"password_expiry_utc\")) {\n> > + // TODO: ignore if can't parse integer\n>\n> Do not use // comment. /* Our single-liner comment reads like this */\n>\n> > + c->password_expiry_utc = atoi(value);\n>\n> Don't use atoi(); make sure value is not followed by a non-number,\n> e.g.\n>\n> const char *value = \"43q\";\n> printf(\"%d<%s>\\n\", atoi(value), value);\n>\n> would give you 43<43q>, but you want to reject and silently ignore\n> such an expiry timestamp.\n>\n> > + // if expiry date has passed, ignore password and expiry fields\n>\n> Ditto, but if you used a large value as sentinel for \"never expires\"\n> and wrote it like this\n>\n> if (c->password_expiry_utc < time(NULL)) {\n>\n> then it is clear enough that you do not even need such a comment.\n> The expression itself makes it clear what is going on (i.e. the\n> current time comes later than the expiry_utc value on the number\n> line hence it appears on the right to it, clearly showing that it\n> has passed the threshold).\n>\n> > + if (c->password_expiry_utc != 0 && time(NULL) > c->password_expiry_utc) {\n> > + trace_printf(_(\"Password has expired.\\n\"));\n> > + FREE_AND_NULL(c->username);\n> > + FREE_AND_NULL(c->password);\n> > + c->password_expiry_utc = 0;\n> > + }\n> > +\n"},{"id":"471220","messageId":"pull.1443.v2.git.git.1675244392025.gitgitgadget@gmail.com","threadId":"59159","inReplyTo":"pull.1443.git.git.1674914650588.gitgitgadget@gmail.com","subject":"[PATCH v2] credential: new attribute password_expiry_utc","fromName":"M Hickford via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-02-01T09:39:51Z","receivedAt":"2023-02-01T09:39:59Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"From: M Hickford \n\nSome passwords have an expiry date known at generation. This may be\nyears away for a personal access token or hours for an OAuth access\ntoken.\n\nCurrently the credential protocol has no expiry attribute. When multiple\nhelpers are configured, `credential fill` tries each helper in turn\nuntil it has a username and password, returning early.\n\nWhen a storage helper and a credential-generating helper are configured\ntogether, the credential is necessarily stored without expiry, so\n`credential fill` may later return an expired credential from storage.\n\n```\n[credential]\n\thelper = storage # eg. cache or osxkeychain\n\thelper = generate # eg. oauth\n```\n\nAn improvement is to introduce a password expiry attribute to the\ncredential protocol. If the expiry date has passed, `credential fill`\nignores the password attribute, so subsequent helpers can generate a\nfresh credential. This is backwards compatible -- no change in\nbehaviour with helpers that discard the expiry attribute.\n\nNote that the expiry logic is entirely within the credential layer.\nCompatible helpers store and retrieve the new attribute like any other.\nThis keeps the helper contract simple.\n\nThis patch adds support for the new attribute to cache.\n\nExample usage in a credential-generating helper\nhttps://github.com/hickford/git-credential-oauth/pull/16\n\nFuture ideas: make it possible for a storage helper to provide OAuth\nrefresh token to subsequent helpers.\nhttps://github.com/gitgitgadget/git/pull/1394\n\nSigned-off-by: M Hickford \n---\n credential: new attribute password_expiry_utc\n \n Some passwords have an expiry date known at generation. This may be\n years away for a personal access token or hours for an OAuth access\n token.\n \n Currently the credential protocol has no expiry attribute. When multiple\n helpers are configured, credential fill tries each helper in turn until\n it has a username and password, returning early.\n \n When a storage helper and a credential-generating helper are configured\n together, the credential is necessarily stored without expiry, so\n credential fill may later return an expired credential from storage.\n \n [credential]\n helper = storage # eg. cache or osxkeychain\n helper = generate # eg. oauth\n \n \n An improvement is to introduce a password expiry attribute to the\n credential protocol. If the password has expired, credential fill no\n longer returns early, so subsequent helpers can generate a fresh\n credential. This is backwards compatible -- no change in behaviour with\n helpers that discard the expiry attribute.\n \n Note that the expiry logic is entirely within the credential layer.\n Compatible helpers store and retrieve the new attribute like any other.\n This keeps the helper contract simple.\n \n This patch adds support for the new attribute to cache.\n \n Example usage in a credential-generating helper\n https://github.com/hickford/git-credential-oauth/pull/16\n \n Future ideas: make it possible for a storage helper to provide OAuth\n refresh token to subsequent helpers.\n https://github.com/gitgitgadget/git/pull/1394\n \n Questions for reviewers:\n \n * Does the behaviour implemented match the documentation? (I'm not\n famiiliar with C)\n * Any edge cases?\n * How to test in t0300-credentials.sh ?\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1443%2Fhickford%2Fpassword-expiry-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1443/hickford/password-expiry-v2\nPull-Request: https://github.com/git/git/pull/1443\n\nRange-diff vs v1:\n\n 1: 184b0aa6514 ! 1: b9ee729ee4d credential: new attribute password_expiry_utc\n @@ Metadata\n ## Commit message ##\n credential: new attribute password_expiry_utc\n \n - If password has expired, credential fill no longer returns early,\n - so later helpers can generate a fresh credential. This is backwards\n - compatible -- no change in behaviour with helpers that discard the\n - expiry attribute. The expiry logic is entirely in the git credential\n - layer; compatible helpers simply store and return the expiry\n - attribute verbatim.\n + Some passwords have an expiry date known at generation. This may be\n + years away for a personal access token or hours for an OAuth access\n + token.\n \n - Store new attribute in cache.\n + Currently the credential protocol has no expiry attribute. When multiple\n + helpers are configured, `credential fill` tries each helper in turn\n + until it has a username and password, returning early.\n +\n + When a storage helper and a credential-generating helper are configured\n + together, the credential is necessarily stored without expiry, so\n + `credential fill` may later return an expired credential from storage.\n +\n + ```\n + [credential]\n + helper = storage # eg. cache or osxkeychain\n + helper = generate # eg. oauth\n + ```\n +\n + An improvement is to introduce a password expiry attribute to the\n + credential protocol. If the expiry date has passed, `credential fill`\n + ignores the password attribute, so subsequent helpers can generate a\n + fresh credential. This is backwards compatible -- no change in\n + behaviour with helpers that discard the expiry attribute.\n +\n + Note that the expiry logic is entirely within the credential layer.\n + Compatible helpers store and retrieve the new attribute like any other.\n + This keeps the helper contract simple.\n +\n + This patch adds support for the new attribute to cache.\n +\n + Example usage in a credential-generating helper\n + https://github.com/hickford/git-credential-oauth/pull/16\n +\n + Future ideas: make it possible for a storage helper to provide OAuth\n + refresh token to subsequent helpers.\n + https://github.com/gitgitgadget/git/pull/1394\n \n Signed-off-by: M Hickford \n \n @@ Documentation/git-credential.txt: Git understands the following attributes:\n \n +`password_expiry_utc`::\n +\n -+\tIf 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.\n ++\tIf password is a personal access token or OAuth access token, it may have an\n ++\texpiry date. When getting credentials from a helper, `git credential fill`\n ++\tignores the password attribute if the expiry date has passed. Storage\n ++\thelpers should store this attribute if possible. Helpers should not\n ++\timplement expiry logic themselves. Represented as Unix time UTC, seconds\n ++\tsince 1970.\n +\n `url`::\n \n @@ builtin/credential-cache--daemon.c: static void serve_one_client(FILE *in, FILE\n \t\tif (e) {\n \t\t\tfprintf(out, \"username=%s\\n\", e->item.username);\n \t\t\tfprintf(out, \"password=%s\\n\", e->item.password);\n -+\t\t\tif (e->item.password_expiry_utc != 0) {\n -+\t\t\t\tfprintf(out, \"password_expiry_utc=%ld\\n\", e->item.password_expiry_utc);\n -+\t\t\t}\n ++\t\t\tif (e->item.password_expiry_utc != TIME_MAX)\n ++\t\t\t\tfprintf(out, \"password_expiry_utc=%\"PRItime\"\\n\",\n ++\t\t\t\t\te->item.password_expiry_utc);\n \t\t}\n \t}\n \telse if (!strcmp(action.buf, \"exit\")) {\n @@ credential.c\n #include \"prompt.h\"\n #include \"sigchain.h\"\n #include \"urlmatch.h\"\n -+#include \n ++#include \"git-compat-util.h\"\n \n void credential_init(struct credential *c)\n {\n -@@ credential.c: void credential_clear(struct credential *c)\n - \tfree(c->path);\n - \tfree(c->username);\n - \tfree(c->password);\n -+\tc->password_expiry_utc = 0;\n - \tstring_list_clear(&c->helpers, 0);\n +@@ credential.c: static void credential_getpass(struct credential *c)\n + \tif (!c->username)\n + \t\tc->username = credential_ask_one(\"Username\", c,\n + \t\t\t\t\t\t PROMPT_ASKPASS|PROMPT_ECHO);\n +-\tif (!c->password)\n ++\tif (!c->password || c->password_expiry_utc < time(NULL)) {\n ++\t\tc->password_expiry_utc = TIME_MAX;\n + \t\tc->password = credential_ask_one(\"Password\", c,\n + \t\t\t\t\t\t PROMPT_ASKPASS);\n ++\t}\n + }\n + \n + int credential_read(struct credential *c, FILE *fp)\n + {\n + \tstruct strbuf line = STRBUF_INIT;\n \n - \tcredential_init(c);\n ++\tint password_updated = 0;\n ++\ttimestamp_t this_password_expiry = TIME_MAX;\n ++\n + \twhile (strbuf_getline(&line, fp) != EOF) {\n + \t\tchar *key = line.buf;\n + \t\tchar *value = strchr(key, '=');\n +@@ credential.c: int credential_read(struct credential *c, FILE *fp)\n + \t\t} else if (!strcmp(key, \"password\")) {\n + \t\t\tfree(c->password);\n + \t\t\tc->password = xstrdup(value);\n ++\t\t\tpassword_updated = 1;\n + \t\t} else if (!strcmp(key, \"protocol\")) {\n + \t\t\tfree(c->protocol);\n + \t\t\tc->protocol = xstrdup(value);\n @@ credential.c: int credential_read(struct credential *c, FILE *fp)\n \t\t} else if (!strcmp(key, \"path\")) {\n \t\t\tfree(c->path);\n \t\t\tc->path = xstrdup(value);\n +\t\t} else if (!strcmp(key, \"password_expiry_utc\")) {\n -+\t\t\t// TODO: ignore if can't parse integer\n -+\t\t\tc->password_expiry_utc = atoi(value);\n ++\t\t\tthis_password_expiry = parse_timestamp(value, NULL, 10);\n ++\t\t\tif (this_password_expiry == 0 || errno) {\n ++\t\t\t\tthis_password_expiry = TIME_MAX;\n ++\t\t\t}\n \t\t} else if (!strcmp(key, \"url\")) {\n \t\t\tcredential_from_url(c, value);\n \t\t} else if (!strcmp(key, \"quit\")) {\n - \t\t\tc->quit = !!git_config_bool(\"quit\", value);\n - \t\t}\n -+\n -+\t\t// if expiry date has passed, ignore password and expiry fields\n -+\t\tif (c->password_expiry_utc != 0 && time(NULL) > c->password_expiry_utc) {\n -+\t\t\ttrace_printf(_(\"Password has expired.\\n\"));\n -+\t\t\tFREE_AND_NULL(c->username);\n -+\t\t\tFREE_AND_NULL(c->password);\n -+\t\t\tc->password_expiry_utc = 0;\n -+\t\t}\n +@@ credential.c: int credential_read(struct credential *c, FILE *fp)\n + \t\t */\n + \t}\n + \n ++\tif (password_updated)\n ++\t\tc->password_expiry_utc = this_password_expiry;\n +\n - \t\t/*\n - \t\t * Ignore other lines; we don't know what they mean, but\n - \t\t * this future-proofs us when later versions of git do\n + \tstrbuf_release(&line);\n + \treturn 0;\n + }\n @@ credential.c: void credential_write(const struct credential *c, FILE *fp)\n \tcredential_write_item(fp, \"path\", c->path, 0);\n \tcredential_write_item(fp, \"username\", c->username, 0);\n \tcredential_write_item(fp, \"password\", c->password, 0);\n -+\tif (c->password_expiry_utc != 0) {\n -+\t\tint length = snprintf( NULL, 0, \"%ld\", c->password_expiry_utc);\n -+\t\tchar* str = malloc( length + 1 );\n -+\t\tsnprintf( str, length + 1, \"%ld\", c->password_expiry_utc );\n -+\t\tcredential_write_item(fp, \"password_expiry_utc\", str, 0);\n -+\t\tfree(str);\n ++\tif (c->password_expiry_utc != TIME_MAX) {\n ++\t\tchar *s = xstrfmt(\"%\"PRItime, c->password_expiry_utc);\n ++\t\tcredential_write_item(fp, \"password_expiry_utc\", s, 0);\n ++\t\tfree(s);\n +\t}\n }\n \n static int run_credential_helper(struct credential *c,\n +@@ credential.c: void credential_fill(struct credential *c)\n + \n + \tfor (i = 0; i < c->helpers.nr; i++) {\n + \t\tcredential_do(c, c->helpers.items[i].string, \"get\");\n +-\t\tif (c->username && c->password)\n ++\t\tif (c->username && c->password && time(NULL) < c->password_expiry_utc)\n + \t\t\treturn;\n + \t\tif (c->quit)\n + \t\t\tdie(\"credential helper '%s' told us to quit\",\n \n ## credential.h ##\n @@ credential.h: struct credential {\n \tchar *protocol;\n \tchar *host;\n \tchar *path;\n -+\ttime_t password_expiry_utc;\n ++\ttimestamp_t password_expiry_utc;\n };\n \n #define CREDENTIAL_INIT { \\\n + \t.helpers = STRING_LIST_INIT_DUP, \\\n ++\t.password_expiry_utc = TIME_MAX, \\\n + }\n + \n + /* Initialize a credential structure, setting all fields to empty. */\n\n\n Documentation/git-credential.txt | 9 +++++++++\n builtin/credential-cache--daemon.c | 3 +++\n credential.c | 24 ++++++++++++++++++++++--\n credential.h | 2 ++\n 4 files changed, 36 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt\nindex ac2818b9f66..667c4d80e26 100644\n--- a/Documentation/git-credential.txt\n+++ b/Documentation/git-credential.txt\n@@ -144,6 +144,15 @@ Git understands the following attributes:\n \n \tThe credential's password, if we are asking it to be stored.\n \n+`password_expiry_utc`::\n+\n+\tIf password is a personal access token or OAuth access token, it may have an\n+\texpiry date. When getting credentials from a helper, `git credential fill`\n+\tignores the password attribute if the expiry date has passed. Storage\n+\thelpers should store this attribute if possible. Helpers should not\n+\timplement expiry logic themselves. Represented as Unix time UTC, seconds\n+\tsince 1970.\n+\n `url`::\n \n \tWhen this special attribute is read by `git credential`, the\ndiff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\nindex f3c89831d4a..338058be7f9 100644\n--- a/builtin/credential-cache--daemon.c\n+++ b/builtin/credential-cache--daemon.c\n@@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out)\n \t\tif (e) {\n \t\t\tfprintf(out, \"username=%s\\n\", e->item.username);\n \t\t\tfprintf(out, \"password=%s\\n\", e->item.password);\n+\t\t\tif (e->item.password_expiry_utc != TIME_MAX)\n+\t\t\t\tfprintf(out, \"password_expiry_utc=%\"PRItime\"\\n\",\n+\t\t\t\t\te->item.password_expiry_utc);\n \t\t}\n \t}\n \telse if (!strcmp(action.buf, \"exit\")) {\ndiff --git a/credential.c b/credential.c\nindex f6389a50684..354fa1652a9 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -7,6 +7,7 @@\n #include \"prompt.h\"\n #include \"sigchain.h\"\n #include \"urlmatch.h\"\n+#include \"git-compat-util.h\"\n \n void credential_init(struct credential *c)\n {\n@@ -195,15 +196,20 @@ static void credential_getpass(struct credential *c)\n \tif (!c->username)\n \t\tc->username = credential_ask_one(\"Username\", c,\n \t\t\t\t\t\t PROMPT_ASKPASS|PROMPT_ECHO);\n-\tif (!c->password)\n+\tif (!c->password || c->password_expiry_utc < time(NULL)) {\n+\t\tc->password_expiry_utc = TIME_MAX;\n \t\tc->password = credential_ask_one(\"Password\", c,\n \t\t\t\t\t\t PROMPT_ASKPASS);\n+\t}\n }\n \n int credential_read(struct credential *c, FILE *fp)\n {\n \tstruct strbuf line = STRBUF_INIT;\n \n+\tint password_updated = 0;\n+\ttimestamp_t this_password_expiry = TIME_MAX;\n+\n \twhile (strbuf_getline(&line, fp) != EOF) {\n \t\tchar *key = line.buf;\n \t\tchar *value = strchr(key, '=');\n@@ -225,6 +231,7 @@ int credential_read(struct credential *c, FILE *fp)\n \t\t} else if (!strcmp(key, \"password\")) {\n \t\t\tfree(c->password);\n \t\t\tc->password = xstrdup(value);\n+\t\t\tpassword_updated = 1;\n \t\t} else if (!strcmp(key, \"protocol\")) {\n \t\t\tfree(c->protocol);\n \t\t\tc->protocol = xstrdup(value);\n@@ -234,6 +241,11 @@ int credential_read(struct credential *c, FILE *fp)\n \t\t} else if (!strcmp(key, \"path\")) {\n \t\t\tfree(c->path);\n \t\t\tc->path = xstrdup(value);\n+\t\t} else if (!strcmp(key, \"password_expiry_utc\")) {\n+\t\t\tthis_password_expiry = parse_timestamp(value, NULL, 10);\n+\t\t\tif (this_password_expiry == 0 || errno) {\n+\t\t\t\tthis_password_expiry = TIME_MAX;\n+\t\t\t}\n \t\t} else if (!strcmp(key, \"url\")) {\n \t\t\tcredential_from_url(c, value);\n \t\t} else if (!strcmp(key, \"quit\")) {\n@@ -246,6 +258,9 @@ int credential_read(struct credential *c, FILE *fp)\n \t\t */\n \t}\n \n+\tif (password_updated)\n+\t\tc->password_expiry_utc = this_password_expiry;\n+\n \tstrbuf_release(&line);\n \treturn 0;\n }\n@@ -269,6 +284,11 @@ void credential_write(const struct credential *c, FILE *fp)\n \tcredential_write_item(fp, \"path\", c->path, 0);\n \tcredential_write_item(fp, \"username\", c->username, 0);\n \tcredential_write_item(fp, \"password\", c->password, 0);\n+\tif (c->password_expiry_utc != TIME_MAX) {\n+\t\tchar *s = xstrfmt(\"%\"PRItime, c->password_expiry_utc);\n+\t\tcredential_write_item(fp, \"password_expiry_utc\", s, 0);\n+\t\tfree(s);\n+\t}\n }\n \n static int run_credential_helper(struct credential *c,\n@@ -342,7 +362,7 @@ void credential_fill(struct credential *c)\n \n \tfor (i = 0; i < c->helpers.nr; i++) {\n \t\tcredential_do(c, c->helpers.items[i].string, \"get\");\n-\t\tif (c->username && c->password)\n+\t\tif (c->username && c->password && time(NULL) < c->password_expiry_utc)\n \t\t\treturn;\n \t\tif (c->quit)\n \t\t\tdie(\"credential helper '%s' told us to quit\",\ndiff --git a/credential.h b/credential.h\nindex f430e77fea4..935b28a70f1 100644\n--- a/credential.h\n+++ b/credential.h\n@@ -126,10 +126,12 @@ struct credential {\n \tchar *protocol;\n \tchar *host;\n \tchar *path;\n+\ttimestamp_t password_expiry_utc;\n };\n \n #define CREDENTIAL_INIT { \\\n \t.helpers = STRING_LIST_INIT_DUP, \\\n+\t.password_expiry_utc = TIME_MAX, \\\n }\n \n /* Initialize a credential structure, setting all fields to empty. */\n\nbase-commit: 5cc9858f1b470844dea5c5d3e936af183fdf2c68\n-- \ngitgitgadget\n"},{"id":"471233","messageId":"Y9pWxHfgPtgCKO+B@coredump.intra.peff.net","threadId":"59159","inReplyTo":"pull.1443.v2.git.git.1675244392025.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] credential: new attribute password_expiry_utc","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-01T12:10:44Z","receivedAt":"2023-02-01T12:10:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 01, 2023 at 09:39:51AM +0000, M Hickford via GitGitGadget wrote:\n\n> +`password_expiry_utc`::\n> +\n> +\tIf password is a personal access token or OAuth access token, it may have an\n> +\texpiry date. When getting credentials from a helper, `git credential fill`\n> +\tignores the password attribute if the expiry date has passed. Storage\n> +\thelpers should store this attribute if possible. Helpers should not\n> +\timplement expiry logic themselves. Represented as Unix time UTC, seconds\n> +\tsince 1970.\n\nThis \"should not\" seems weird to me. The logic you have here throws out\nentries that have expired when they pass through Git. But wouldn't\nhelpers which store things want to know about and act on the expiration,\ntoo?\n\nFor example, if Git learns about a credential that expires in 60\nseconds and passes it to credential-cache which is configured\n--timeout=300, wouldn't it want to set its internal ttl on the\ncredential to 60, rather than 300?\n\nI think your plan here is that Git would then reject the credential if a\nrequest is made at time now+65. But the cache is holding onto it much\nlonger than necessary.\n\nLikewise, wouldn't anything that stores credentials at least want to be\nable to store and regurgitate the expiration? For instance, even\ncredential-store would want to do this. I'm OK if it doesn't, and we can\nconsider it a quality-of-implementation issue and see if anybody cares\nenough to implement it. But I'd think most \"real\" helpers would want to\ndo so.\n\nSo it seems like helpers really do need to support this \"expiration\"\nnotion. And it's actually Git itself which doesn't need to care about\nit, assuming the helpers are doing something sensible (though it is OK\nif Git _also_ throws away expired credentials to support helpers which\ndon't).\n\n> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\n> index f3c89831d4a..338058be7f9 100644\n> --- a/builtin/credential-cache--daemon.c\n> +++ b/builtin/credential-cache--daemon.c\n> @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out)\n> \t\tif (e) {\n> \t\t\tfprintf(out, \"username=%s\\n\", e->item.username);\n> \t\t\tfprintf(out, \"password=%s\\n\", e->item.password);\n> +\t\t\tif (e->item.password_expiry_utc != TIME_MAX)\n> +\t\t\t\tfprintf(out, \"password_expiry_utc=%\"PRItime\"\\n\",\n> +\t\t\t\t\te->item.password_expiry_utc);\n> \t\t}\n\nIs there a particular reason to use TIME_MAX as the sentinel value here,\nand not just \"0\"? It's not that big a deal either way, but it's more\nusual in our code base to use \"0\" if there's no reason not to (and it\nseems like nothing should be expiring in 1970 these days).\n\n> @@ -195,15 +196,20 @@ static void credential_getpass(struct credential *c)\n> \tif (!c->username)\n> \t\tc->username = credential_ask_one(\"Username\", c,\n> \t\t\t\t\t\t PROMPT_ASKPASS|PROMPT_ECHO);\n> -\tif (!c->password)\n> +\tif (!c->password || c->password_expiry_utc < time(NULL)) {\n\nThis is comparing a timestamp_t to a time_t, which may mix\nsigned/unsigned. I can't offhand think of anything that would go too\nwrong there before 2038, so it's probably OK, but I wanted to call it\nout.\n\n> @@ -225,6 +231,7 @@ int credential_read(struct credential *c, FILE *fp)\n> \t\t} else if (!strcmp(key, \"password\")) {\n> \t\t\tfree(c->password);\n> \t\t\tc->password = xstrdup(value);\n> +\t\t\tpassword_updated = 1;\n> \t\t} else if (!strcmp(key, \"protocol\")) {\n> \t\t\tfree(c->protocol);\n> \t\t\tc->protocol = xstrdup(value);\n> @@ -234,6 +241,11 @@ int credential_read(struct credential *c, FILE *fp)\n> \t\t} else if (!strcmp(key, \"path\")) {\n> \t\t\tfree(c->path);\n> \t\t\tc->path = xstrdup(value);\n> +\t\t} else if (!strcmp(key, \"password_expiry_utc\")) {\n> +\t\t\tthis_password_expiry = parse_timestamp(value, NULL, 10);\n> +\t\t\tif (this_password_expiry == 0 || errno) {\n> +\t\t\t\tthis_password_expiry = TIME_MAX;\n> +\t\t\t}\n> \t\t} else if (!strcmp(key, \"url\")) {\n> \t\t\tcredential_from_url(c, value);\n> \t\t} else if (!strcmp(key, \"quit\")) {\n> @@ -246,6 +258,9 @@ int credential_read(struct credential *c, FILE *fp)\n> \t\t */\n> \t}\n> \n> +\tif (password_updated)\n> +\t\tc->password_expiry_utc = this_password_expiry;\n\nDo we need this logic? It seems weird that a helper would output an\nexpiration but not a password in the first place. I guess ignoring the\nexpiration is probably a reasonable outcome, but I wonder if a helper\nwould ever want to just add an expiration to the data coming from\nanother helper.\n\nI.e., could we just read the value directly into c->password_expiry_utc\nas we do with other fields?\n\n-Peff\n"},{"id":"471253","messageId":"xmqq8rhh2tum.fsf@gitster.g","threadId":"59159","inReplyTo":"Y9pWxHfgPtgCKO+B@coredump.intra.peff.net","subject":"Re: [PATCH v2] credential: new attribute password_expiry_utc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-01T17:12:01Z","receivedAt":"2023-02-01T17:12:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\n>> index f3c89831d4a..338058be7f9 100644\n>> --- a/builtin/credential-cache--daemon.c\n>> +++ b/builtin/credential-cache--daemon.c\n>> @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out)\n>> \t\tif (e) {\n>> \t\t\tfprintf(out, \"username=%s\\n\", e->item.username);\n>> \t\t\tfprintf(out, \"password=%s\\n\", e->item.password);\n>> +\t\t\tif (e->item.password_expiry_utc != TIME_MAX)\n>> +\t\t\t\tfprintf(out, \"password_expiry_utc=%\"PRItime\"\\n\",\n>> +\t\t\t\t\te->item.password_expiry_utc);\n>> \t\t}\n>\n> Is there a particular reason to use TIME_MAX as the sentinel value here,\n> and not just \"0\"? It's not that big a deal either way, but it's more\n> usual in our code base to use \"0\" if there's no reason not to (and it\n> seems like nothing should be expiring in 1970 these days).\n\nThis is my fault ;-). Here, there is no difference between 0 and\nTIME_MAX, but elsewhere the code needed\n\n\tif (expiry != 0 && expiry < time(NULL))\n\nto see if the entry has expired. If the sentinel for an entry that\nwill never expire were TIME_MAX, you do not need the first half of\nthe expression.\n\nI am OK either way.\n"},{"id":"471259","messageId":"xmqqwn51z0df.fsf@gitster.g","threadId":"59159","inReplyTo":"CAGJzqskO0sGNtuuSkKWxknh1qv523TfA3U17X_9higDdYdg+PA@mail.gmail.com","subject":"Re: [PATCH] credential: new attribute password_expiry_utc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-01T18:50:04Z","receivedAt":"2023-02-01T18:50:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"M Hickford writes:\n\n> Thanks for the guidance. Writing a better commit message clarified my\n> own thoughts.\n\nIt is great to hear that somebody agreeing with me on this point.\n\nWe try to write in a way that would make it clear to readers of \"git\nlog\", but I often notice that trying to explain the changes to the\ncode clearly does force me to realize better ways to write the\nchanges.\n"},{"id":"471263","messageId":"AS2PR03MB9815DDCB7B107E7FD37EC972C0D19@AS2PR03MB9815.eurprd03.prod.outlook.com","threadId":"59159","inReplyTo":"Y9pWxHfgPtgCKO+B@coredump.intra.peff.net","subject":"Re: [PATCH v2] credential: new attribute password_expiry_utc","fromName":"Matthew John Cheetham","fromEmail":"mjcheetham@outlook.com","sentAt":"2023-02-01T20:02:16Z","receivedAt":"2023-02-01T20:02:29Z","isPatch":true,"sender":{"key":"mjcheetham@outlook.com","avatar":"https://avatars.githubusercontent.com/u/5658207?v=4"},"body":"On 2023-02-01 04:10, Jeff King wrote:\n\n> On Wed, Feb 01, 2023 at 09:39:51AM +0000, M Hickford via GitGitGadget wrote:\n> \n>> +`password_expiry_utc`::\n>> +\n>> +\tIf password is a personal access token or OAuth access token, it may have an\n>> +\texpiry date. When getting credentials from a helper, `git credential fill`\n>> +\tignores the password attribute if the expiry date has passed. Storage\n>> +\thelpers should store this attribute if possible. Helpers should not\n>> +\timplement expiry logic themselves. Represented as Unix time UTC, seconds\n>> +\tsince 1970.\n> \n> This \"should not\" seems weird to me. The logic you have here throws out\n> entries that have expired when they pass through Git. But wouldn't\n> helpers which store things want to know about and act on the expiration,\n> too?\n> \n> For example, if Git learns about a credential that expires in 60\n> seconds and passes it to credential-cache which is configured\n> --timeout=300, wouldn't it want to set its internal ttl on the\n> credential to 60, rather than 300?\n> \n> I think your plan here is that Git would then reject the credential if a\n> request is made at time now+65. But the cache is holding onto it much\n> longer than necessary.\n> \n> Likewise, wouldn't anything that stores credentials at least want to be\n> able to store and regurgitate the expiration? For instance, even\n> credential-store would want to do this. I'm OK if it doesn't, and we can\n> consider it a quality-of-implementation issue and see if anybody cares\n> enough to implement it. But I'd think most \"real\" helpers would want to\n> do so.\n> \n> So it seems like helpers really do need to support this \"expiration\"\n> notion. And it's actually Git itself which doesn't need to care about\n> it, assuming the helpers are doing something sensible (though it is OK\n> if Git _also_ throws away expired credentials to support helpers which\n> don't).\n\nI have often wondered about how, and if, Git should handle expiring credentials\nwhere the expiration is known. In my opinion I think Git should be doing\n*less* decision making with credentials and authentication in general, and leave\nthat up to credential helpers.\n\nThe original design of credential helpers from what I can see (and Peff can\ncorrect me here of course!) is that they were really only thought about as\nstorage-style helpers. Helpers are consulted for a known credential, and told\nabout bad (erase) or good (store) credentials, all without any context about\nthe request or remote responses.\n\nIf no credential helper can respond then Git itself prompts for a user/pass; so\nGit, or rather the user, is the 'generator'.\n\nOf course that's not to say that credential generating helpers don't exist or\nare wrong - Git Credential Manager being of course one example rather close to\nhome for me! However the current model, even with generating helpers, is still\nthat Git will try and make the request given the details included in the helper\nresponse.\n\nIt doesn't make sense that a generating helper that knows about expiration would\ninstead choose to respond with an expired credential rather than just try and\ngenerate a new credential.\n\nNow in the case of a simple storage helper without such logic, after returning\nan expired credential should Git not be calling 'erase' back to the same helper\nto inform it that it has a stale credential and should be deleted?\nThis would also require some affinity between calls to get/erase/store.\n\n\n>> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\n>> index f3c89831d4a..338058be7f9 100644\n>> --- a/builtin/credential-cache--daemon.c\n>> +++ b/builtin/credential-cache--daemon.c\n>> @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out)\n>> \t\tif (e) {\n>> \t\t\tfprintf(out, \"username=%s\\n\", e->item.username);\n>> \t\t\tfprintf(out, \"password=%s\\n\", e->item.password);\n>> +\t\t\tif (e->item.password_expiry_utc != TIME_MAX)\n>> +\t\t\t\tfprintf(out, \"password_expiry_utc=%\"PRItime\"\\n\",\n>> +\t\t\t\t\te->item.password_expiry_utc);\n>> \t\t}\n> \n> Is there a particular reason to use TIME_MAX as the sentinel value here,\n> and not just \"0\"? It's not that big a deal either way, but it's more\n> usual in our code base to use \"0\" if there's no reason not to (and it\n> seems like nothing should be expiring in 1970 these days).\n> \n>> @@ -195,15 +196,20 @@ static void credential_getpass(struct credential *c)\n>> \tif (!c->username)\n>> \t\tc->username = credential_ask_one(\"Username\", c,\n>> \t\t\t\t\t\t PROMPT_ASKPASS|PROMPT_ECHO);\n>> -\tif (!c->password)\n>> +\tif (!c->password || c->password_expiry_utc < time(NULL)) {\n> \n> This is comparing a timestamp_t to a time_t, which may mix\n> signed/unsigned. I can't offhand think of anything that would go too\n> wrong there before 2038, so it's probably OK, but I wanted to call it\n> out.\n> \n>> @@ -225,6 +231,7 @@ int credential_read(struct credential *c, FILE *fp)\n>> \t\t} else if (!strcmp(key, \"password\")) {\n>> \t\t\tfree(c->password);\n>> \t\t\tc->password = xstrdup(value);\n>> +\t\t\tpassword_updated = 1;\n>> \t\t} else if (!strcmp(key, \"protocol\")) {\n>> \t\t\tfree(c->protocol);\n>> \t\t\tc->protocol = xstrdup(value);\n>> @@ -234,6 +241,11 @@ int credential_read(struct credential *c, FILE *fp)\n>> \t\t} else if (!strcmp(key, \"path\")) {\n>> \t\t\tfree(c->path);\n>> \t\t\tc->path = xstrdup(value);\n>> +\t\t} else if (!strcmp(key, \"password_expiry_utc\")) {\n>> +\t\t\tthis_password_expiry = parse_timestamp(value, NULL, 10);\n>> +\t\t\tif (this_password_expiry == 0 || errno) {\n>> +\t\t\t\tthis_password_expiry = TIME_MAX;\n>> +\t\t\t}\n>> \t\t} else if (!strcmp(key, \"url\")) {\n>> \t\t\tcredential_from_url(c, value);\n>> \t\t} else if (!strcmp(key, \"quit\")) {\n>> @@ -246,6 +258,9 @@ int credential_read(struct credential *c, FILE *fp)\n>> \t\t */\n>> \t}\n>> \n>> +\tif (password_updated)\n>> +\t\tc->password_expiry_utc = this_password_expiry;\n> \n> Do we need this logic? It seems weird that a helper would output an\n> expiration but not a password in the first place. I guess ignoring the\n> expiration is probably a reasonable outcome, but I wonder if a helper\n> would ever want to just add an expiration to the data coming from\n> another helper.\n> \n> I.e., could we just read the value directly into c->password_expiry_utc\n> as we do with other fields?\n> \n> -Peff\n"},{"id":"471294","messageId":"Y9r/59pKKF07pBo4@coredump.intra.peff.net","threadId":"59159","inReplyTo":"xmqq8rhh2tum.fsf@gitster.g","subject":"Re: [PATCH v2] credential: new attribute password_expiry_utc","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-02T00:12:23Z","receivedAt":"2023-02-02T00:12:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 01, 2023 at 09:12:01AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> >> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\n> >> index f3c89831d4a..338058be7f9 100644\n> >> --- a/builtin/credential-cache--daemon.c\n> >> +++ b/builtin/credential-cache--daemon.c\n> >> @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out)\n> >> \t\tif (e) {\n> >> \t\t\tfprintf(out, \"username=%s\\n\", e->item.username);\n> >> \t\t\tfprintf(out, \"password=%s\\n\", e->item.password);\n> >> +\t\t\tif (e->item.password_expiry_utc != TIME_MAX)\n> >> +\t\t\t\tfprintf(out, \"password_expiry_utc=%\"PRItime\"\\n\",\n> >> +\t\t\t\t\te->item.password_expiry_utc);\n> >> \t\t}\n> >\n> > Is there a particular reason to use TIME_MAX as the sentinel value here,\n> > and not just \"0\"? It's not that big a deal either way, but it's more\n> > usual in our code base to use \"0\" if there's no reason not to (and it\n> > seems like nothing should be expiring in 1970 these days).\n> \n> This is my fault ;-). Here, there is no difference between 0 and\n> TIME_MAX, but elsewhere the code needed\n> \n> \tif (expiry != 0 && expiry < time(NULL))\n> \n> to see if the entry has expired. If the sentinel for an entry that\n> will never expire were TIME_MAX, you do not need the first half of\n> the expression.\n> \n> I am OK either way.\n\nAh. That at least is a compelling reason to use TIME_MAX. I'm OK with\nit.\n\n-Peff\n"},{"id":"471296","messageId":"Y9sCbzq0/YuacVz+@coredump.intra.peff.net","threadId":"59159","inReplyTo":"AS2PR03MB9815DDCB7B107E7FD37EC972C0D19@AS2PR03MB9815.eurprd03.prod.outlook.com","subject":"Re: [PATCH v2] credential: new attribute password_expiry_utc","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-02T00:23:11Z","receivedAt":"2023-02-02T00:23:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 01, 2023 at 12:02:16PM -0800, Matthew John Cheetham wrote:\n\n> > So it seems like helpers really do need to support this \"expiration\"\n> > notion. And it's actually Git itself which doesn't need to care about\n> > it, assuming the helpers are doing something sensible (though it is OK\n> > if Git _also_ throws away expired credentials to support helpers which\n> > don't).\n> \n> I have often wondered about how, and if, Git should handle expiring credentials\n> where the expiration is known. In my opinion I think Git should be doing\n> *less* decision making with credentials and authentication in general, and leave\n> that up to credential helpers.\n\nFWIW, that is my general philosophy, too.\n\n> The original design of credential helpers from what I can see (and Peff can\n> correct me here of course!) is that they were really only thought about as\n> storage-style helpers. Helpers are consulted for a known credential, and told\n> about bad (erase) or good (store) credentials, all without any context about\n> the request or remote responses.\n> \n> If no credential helper can respond then Git itself prompts for a user/pass; so\n> Git, or rather the user, is the 'generator'.\n\nThey were always intended to be generators, too. In the early days we\ndiscussed having more fancy graphical prompts via helpers, though most\npeople simply use the askpass interface for this.\n\nMy personal config for the last, say, 12 years, has been to pull the\npassword out of a read-only store, and ignore \"store\" and \"erase\"\nrequests entirely. Which I think counts as a generator. :)\n\nBut yeah, if there is more context that we can be giving the helpers to\nlet them make a better decision, I'm all for it. I think your\nwww-authenticate patches are a good step in that direction. I'm not sure\nwhat other context would be useful.\n\n> It doesn't make sense that a generating helper that knows about expiration would\n> instead choose to respond with an expired credential rather than just try and\n> generate a new credential.\n\nYeah, agreed.\n\n> Now in the case of a simple storage helper without such logic, after returning\n> an expired credential should Git not be calling 'erase' back to the same helper\n> to inform it that it has a stale credential and should be deleted?\n> This would also require some affinity between calls to get/erase/store.\n\nThat's a good point. I think in practice it would mostly happen that Git\nwould throw away the expired credential, generate a new one (either from\nanother helper or via prompting), and then if that works, overwrite the\nold one with a 'store' request.\n\nIf the new credential doesn't work (or the user aborts), the expired\ncredential is left in the helper. But in theory that doesn't matter. It\nwill still be expired when it's served up again later. And it's not a\nsecurity problem to hold onto it longer, since it's no longer valid.\nStill, it feels a bit clunky compared to having the helper realize it's\nholding garbage.\n\n-Peff\n"},{"id":"471502","messageId":"pull.1443.v3.git.git.1675545372271.gitgitgadget@gmail.com","threadId":"59159","inReplyTo":"pull.1443.v2.git.git.1675244392025.gitgitgadget@gmail.com","subject":"[PATCH v3] credential: new attribute password_expiry_utc","fromName":"M Hickford via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-02-04T21:16:12Z","receivedAt":"2023-02-04T21:16:34Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"From: M Hickford \n\nSome passwords have an expiry date known at generation. This may be\nyears away for a personal access token or hours for an OAuth access\ntoken.\n\nWhen multiple credential helpers are configured, `credential fill` tries\neach helper in turn until it has a username and password, returning\nearly. If Git authentication succeeds, `credential approve`\nstores the successful credential in all helpers. If authentication\nfails, `credential reject` erases matching credentials in all helpers.\nHelpers implement corresponding operations: get, store, erase.\n\nThe credential protocol has no expiry attribute, so helpers cannot\nstore expiry information. (Even if a helper returned an improvised\nexpiry attribute, git credential discards unrecognised attributes\nbetween operations and between helpers.)\n\nAs a workaround, whenever monolithic helper Git Credential Manager (GCM)\nretrieves an OAuth credential from its storage, it makes a HTTP request\nto check whether the OAuth token has expired [1]. This complicates and\nslows the authentication happy path.\n\nWorse is the case that a storage helper and a credential-generating\nhelper are configured together:\n\n\t[credential]\n\t\thelper = storage # eg. cache or osxkeychain\n\t\thelper = generate # eg. oauth or manager\n\n`credential approve` stores the generated credential in both helpers\nwithout expiry information. Later `credential fill` may return an\nexpired credential from storage. There is no workaround, no matter how\nclever the second helper.\n\nIntroduce a password expiry attribute. In `credential fill`, ignore\nexpired passwords and continue to query subsequent helpers.\n\nIn the example above, `credential fill` ignores the expired credential\nand a fresh credential is generated. If authentication succeeds,\n`credential approve` replaces the expired credential in storage.\nIf authentication fails, the expired credential is erased by\n`credential reject`. It is unnecessary but harmless for storage\nhelpers to self prune expired credentials.\n\nAdd support for the new attribute to credential-cache.\nEventually, I hope to see support in other storage helpers.\n\nExample usage in a credential-generating helper\nhttps://github.com/hickford/git-credential-oauth/pull/16\n\n[1] https://github.com/GitCredentialManager/git-credential-manager/blob/66b94e489ad8cc1982836355493e369770b30211/src/shared/GitLab/GitLabHostProvider.cs#L217\n\nSigned-off-by: M Hickford \n---\n credential: new attribute password_expiry_utc\n \n details in commit message\n \n Changes in patch v3:\n \n * Tests\n * Simplified credential_read, moved expiry logic to credential_fill\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1443%2Fhickford%2Fpassword-expiry-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1443/hickford/password-expiry-v3\nPull-Request: https://github.com/git/git/pull/1443\n\nRange-diff vs v2:\n\n 1: b9ee729ee4d ! 1: 1846815a5c1 credential: new attribute password_expiry_utc\n @@ Commit message\n years away for a personal access token or hours for an OAuth access\n token.\n \n - Currently the credential protocol has no expiry attribute. When multiple\n - helpers are configured, `credential fill` tries each helper in turn\n - until it has a username and password, returning early.\n + When multiple credential helpers are configured, `credential fill` tries\n + each helper in turn until it has a username and password, returning\n + early. If Git authentication succeeds, `credential approve`\n + stores the successful credential in all helpers. If authentication\n + fails, `credential reject` erases matching credentials in all helpers.\n + Helpers implement corresponding operations: get, store, erase.\n \n - When a storage helper and a credential-generating helper are configured\n - together, the credential is necessarily stored without expiry, so\n - `credential fill` may later return an expired credential from storage.\n + The credential protocol has no expiry attribute, so helpers cannot\n + store expiry information. (Even if a helper returned an improvised\n + expiry attribute, git credential discards unrecognised attributes\n + between operations and between helpers.)\n \n - ```\n - [credential]\n - helper = storage # eg. cache or osxkeychain\n - helper = generate # eg. oauth\n - ```\n + As a workaround, whenever monolithic helper Git Credential Manager (GCM)\n + retrieves an OAuth credential from its storage, it makes a HTTP request\n + to check whether the OAuth token has expired [1]. This complicates and\n + slows the authentication happy path.\n \n - An improvement is to introduce a password expiry attribute to the\n - credential protocol. If the expiry date has passed, `credential fill`\n - ignores the password attribute, so subsequent helpers can generate a\n - fresh credential. This is backwards compatible -- no change in\n - behaviour with helpers that discard the expiry attribute.\n + Worse is the case that a storage helper and a credential-generating\n + helper are configured together:\n \n - Note that the expiry logic is entirely within the credential layer.\n - Compatible helpers store and retrieve the new attribute like any other.\n - This keeps the helper contract simple.\n + [credential]\n + helper = storage # eg. cache or osxkeychain\n + helper = generate # eg. oauth or manager\n \n - This patch adds support for the new attribute to cache.\n + `credential approve` stores the generated credential in both helpers\n + without expiry information. Later `credential fill` may return an\n + expired credential from storage. There is no workaround, no matter how\n + clever the second helper.\n +\n + Introduce a password expiry attribute. In `credential fill`, ignore\n + expired passwords and continue to query subsequent helpers.\n +\n + In the example above, `credential fill` ignores the expired credential\n + and a fresh credential is generated. If authentication succeeds,\n + `credential approve` replaces the expired credential in storage.\n + If authentication fails, the expired credential is erased by\n + `credential reject`. It is unnecessary but harmless for storage\n + helpers to self prune expired credentials.\n +\n + Add support for the new attribute to credential-cache.\n + Eventually, I hope to see support in other storage helpers.\n \n Example usage in a credential-generating helper\n https://github.com/hickford/git-credential-oauth/pull/16\n \n - Future ideas: make it possible for a storage helper to provide OAuth\n - refresh token to subsequent helpers.\n - https://github.com/gitgitgadget/git/pull/1394\n + [1] https://github.com/GitCredentialManager/git-credential-manager/blob/66b94e489ad8cc1982836355493e369770b30211/src/shared/GitLab/GitLabHostProvider.cs#L217\n \n Signed-off-by: M Hickford \n \n @@ Documentation/git-credential.txt: Git understands the following attributes:\n \n +`password_expiry_utc`::\n +\n -+\tIf password is a personal access token or OAuth access token, it may have an\n -+\texpiry date. When getting credentials from a helper, `git credential fill`\n -+\tignores the password attribute if the expiry date has passed. Storage\n -+\thelpers should store this attribute if possible. Helpers should not\n -+\timplement expiry logic themselves. Represented as Unix time UTC, seconds\n -+\tsince 1970.\n ++\tGenerated passwords such as an OAuth access token may have an expiry date.\n ++\tWhen reading credentials from helpers, `git credential fill` ignores expired\n ++\tpasswords. Represented as Unix time UTC, seconds since 1970.\n +\n `url`::\n \n \tWhen this special attribute is read by `git credential`, the\n \n + ## Documentation/gitcredentials.txt ##\n +@@ Documentation/gitcredentials.txt: helper::\n + If there are multiple instances of the `credential.helper` configuration\n + variable, each helper will be tried in turn, and may provide a username,\n + password, or nothing. Once Git has acquired both a username and a\n +-password, no more helpers will be tried.\n ++unexpired password, no more helpers will be tried.\n + +\n + If `credential.helper` is configured to the empty string, this resets\n + the helper list to empty (so you may override a helper set by a\n +\n ## builtin/credential-cache--daemon.c ##\n @@ builtin/credential-cache--daemon.c: static void serve_one_client(FILE *in, FILE *out)\n \t\tif (e) {\n @@ credential.c\n \n void credential_init(struct credential *c)\n {\n -@@ credential.c: static void credential_getpass(struct credential *c)\n - \tif (!c->username)\n - \t\tc->username = credential_ask_one(\"Username\", c,\n - \t\t\t\t\t\t PROMPT_ASKPASS|PROMPT_ECHO);\n --\tif (!c->password)\n -+\tif (!c->password || c->password_expiry_utc < time(NULL)) {\n -+\t\tc->password_expiry_utc = TIME_MAX;\n - \t\tc->password = credential_ask_one(\"Password\", c,\n - \t\t\t\t\t\t PROMPT_ASKPASS);\n -+\t}\n - }\n - \n - int credential_read(struct credential *c, FILE *fp)\n - {\n - \tstruct strbuf line = STRBUF_INIT;\n - \n -+\tint password_updated = 0;\n -+\ttimestamp_t this_password_expiry = TIME_MAX;\n -+\n - \twhile (strbuf_getline(&line, fp) != EOF) {\n - \t\tchar *key = line.buf;\n - \t\tchar *value = strchr(key, '=');\n -@@ credential.c: int credential_read(struct credential *c, FILE *fp)\n - \t\t} else if (!strcmp(key, \"password\")) {\n - \t\t\tfree(c->password);\n - \t\t\tc->password = xstrdup(value);\n -+\t\t\tpassword_updated = 1;\n - \t\t} else if (!strcmp(key, \"protocol\")) {\n - \t\t\tfree(c->protocol);\n - \t\t\tc->protocol = xstrdup(value);\n @@ credential.c: int credential_read(struct credential *c, FILE *fp)\n \t\t} else if (!strcmp(key, \"path\")) {\n \t\t\tfree(c->path);\n \t\t\tc->path = xstrdup(value);\n +\t\t} else if (!strcmp(key, \"password_expiry_utc\")) {\n -+\t\t\tthis_password_expiry = parse_timestamp(value, NULL, 10);\n -+\t\t\tif (this_password_expiry == 0 || errno) {\n -+\t\t\t\tthis_password_expiry = TIME_MAX;\n -+\t\t\t}\n ++\t\t\tc->password_expiry_utc = parse_timestamp(value, NULL, 10);\n ++\t\t\tif (c->password_expiry_utc == 0 || errno)\n ++\t\t\t\tc->password_expiry_utc = TIME_MAX;\n \t\t} else if (!strcmp(key, \"url\")) {\n \t\t\tcredential_from_url(c, value);\n \t\t} else if (!strcmp(key, \"quit\")) {\n -@@ credential.c: int credential_read(struct credential *c, FILE *fp)\n - \t\t */\n - \t}\n - \n -+\tif (password_updated)\n -+\t\tc->password_expiry_utc = this_password_expiry;\n -+\n - \tstrbuf_release(&line);\n - \treturn 0;\n - }\n @@ credential.c: void credential_write(const struct credential *c, FILE *fp)\n \tcredential_write_item(fp, \"path\", c->path, 0);\n \tcredential_write_item(fp, \"username\", c->username, 0);\n @@ credential.c: void credential_fill(struct credential *c)\n \n \tfor (i = 0; i < c->helpers.nr; i++) {\n \t\tcredential_do(c, c->helpers.items[i].string, \"get\");\n --\t\tif (c->username && c->password)\n -+\t\tif (c->username && c->password && time(NULL) < c->password_expiry_utc)\n ++\t\tif (c->password_expiry_utc < time(NULL)) {\n ++\t\t\tFREE_AND_NULL(c->password);\n ++\t\t\tc->password_expiry_utc = TIME_MAX;\n ++\t\t}\n + \t\tif (c->username && c->password)\n \t\t\treturn;\n \t\tif (c->quit)\n - \t\t\tdie(\"credential helper '%s' told us to quit\",\n +@@ credential.c: void credential_approve(struct credential *c)\n + \n + \tif (c->approved)\n + \t\treturn;\n +-\tif (!c->username || !c->password)\n ++\tif (!c->username || !c->password || c->password_expiry_utc < time(NULL))\n + \t\treturn;\n + \n + \tcredential_apply_config(c);\n +@@ credential.c: void credential_reject(struct credential *c)\n + \n + \tFREE_AND_NULL(c->username);\n + \tFREE_AND_NULL(c->password);\n ++\tc->password_expiry_utc = TIME_MAX;\n + \tc->approved = 0;\n + }\n + \n \n ## credential.h ##\n @@ credential.h: struct credential {\n @@ credential.h: struct credential {\n }\n \n /* Initialize a credential structure, setting all fields to empty. */\n +\n + ## t/t0300-credentials.sh ##\n +@@ t/t0300-credentials.sh: test_expect_success 'setup helper scripts' '\n + \ttest -z \"$pass\" || echo password=$pass\n + \tEOF\n + \n ++\twrite_script git-credential-verbatim-with-expiry <<-\\EOF &&\n ++\tuser=$1; shift\n ++\tpass=$1; shift\n ++\tpexpiry=$1; shift\n ++\t. ./dump\n ++\ttest -z \"$user\" || echo username=$user\n ++\ttest -z \"$pass\" || echo password=$pass\n ++\ttest -z \"$pexpiry\" || echo password_expiry_utc=$pexpiry\n ++\tEOF\n ++\n + \tPATH=\"$PWD:$PATH\"\n + '\n + \n +@@ t/t0300-credentials.sh: test_expect_success 'credential_fill continues through partial response' '\n + \tEOF\n + '\n + \n ++test_expect_success 'credential_fill populates password_expiry_utc' '\n ++\tcheck fill \"verbatim-with-expiry one two 9999999999\" <<-\\EOF\n ++\tprotocol=http\n ++\thost=example.com\n ++\t--\n ++\tprotocol=http\n ++\thost=example.com\n ++\tusername=one\n ++\tpassword=two\n ++\tpassword_expiry_utc=9999999999\n ++\t--\n ++\tverbatim-with-expiry: get\n ++\tverbatim-with-expiry: protocol=http\n ++\tverbatim-with-expiry: host=example.com\n ++\tEOF\n ++'\n ++\n ++test_expect_success 'credential_fill continues through expired password' '\n ++\tcheck fill \"verbatim-with-expiry one two 5\" \"verbatim three four\" <<-\\EOF\n ++\tprotocol=http\n ++\thost=example.com\n ++\t--\n ++\tprotocol=http\n ++\thost=example.com\n ++\tusername=three\n ++\tpassword=four\n ++\t--\n ++\tverbatim-with-expiry: get\n ++\tverbatim-with-expiry: protocol=http\n ++\tverbatim-with-expiry: host=example.com\n ++\tverbatim: get\n ++\tverbatim: protocol=http\n ++\tverbatim: host=example.com\n ++\tverbatim: username=one\n ++\tEOF\n ++'\n ++\n + test_expect_success 'credential_fill passes along metadata' '\n + \tcheck fill \"verbatim one two\" <<-\\EOF\n + \tprotocol=ftp\n +@@ t/t0300-credentials.sh: test_expect_success 'credential_approve calls all helpers' '\n + \tEOF\n + '\n + \n ++test_expect_success 'credential_approve stores password expiry' '\n ++\tcheck approve useless <<-\\EOF\n ++\tprotocol=http\n ++\thost=example.com\n ++\tusername=foo\n ++\tpassword=bar\n ++\tpassword_expiry_utc=9999999999\n ++\t--\n ++\t--\n ++\tuseless: store\n ++\tuseless: protocol=http\n ++\tuseless: host=example.com\n ++\tuseless: username=foo\n ++\tuseless: password=bar\n ++\tuseless: password_expiry_utc=9999999999\n ++\tEOF\n ++'\n ++\n + test_expect_success 'do not bother storing password-less credential' '\n + \tcheck approve useless <<-\\EOF\n + \tprotocol=http\n +@@ t/t0300-credentials.sh: test_expect_success 'do not bother storing password-less credential' '\n + \tEOF\n + '\n + \n ++test_expect_success 'credential_approve does not store expired credential' '\n ++\tcheck approve useless <<-\\EOF\n ++\tprotocol=http\n ++\thost=example.com\n ++\tusername=foo\n ++\tpassword=bar\n ++\tpassword_expiry_utc=5\n ++\t--\n ++\t--\n ++\tEOF\n ++'\n + \n + test_expect_success 'credential_reject calls all helpers' '\n + \tcheck reject useless \"verbatim one two\" <<-\\EOF\n +@@ t/t0300-credentials.sh: test_expect_success 'credential_reject calls all helpers' '\n + \tEOF\n + '\n + \n ++test_expect_success 'credential_reject erases expired credential' '\n ++\tcheck reject useless <<-\\EOF\n ++\tprotocol=http\n ++\thost=example.com\n ++\tusername=foo\n ++\tpassword=bar\n ++\tpassword_expiry_utc=5\n ++\t--\n ++\t--\n ++\tuseless: erase\n ++\tuseless: protocol=http\n ++\tuseless: host=example.com\n ++\tuseless: username=foo\n ++\tuseless: password=bar\n ++\tuseless: password_expiry_utc=5\n ++\tEOF\n ++'\n ++\n + test_expect_success 'usernames can be preserved' '\n + \tcheck fill \"verbatim \\\"\\\" three\" <<-\\EOF\n + \tprotocol=http\n\n\n Documentation/git-credential.txt | 6 ++\n Documentation/gitcredentials.txt | 2 +-\n builtin/credential-cache--daemon.c | 3 +\n credential.c | 17 +++++-\n credential.h | 2 +\n t/t0300-credentials.sh | 94 ++++++++++++++++++++++++++++++\n 6 files changed, 122 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt\nindex ac2818b9f66..29d184ab824 100644\n--- a/Documentation/git-credential.txt\n+++ b/Documentation/git-credential.txt\n@@ -144,6 +144,12 @@ Git understands the following attributes:\n \n \tThe credential's password, if we are asking it to be stored.\n \n+`password_expiry_utc`::\n+\n+\tGenerated passwords such as an OAuth access token may have an expiry date.\n+\tWhen reading credentials from helpers, `git credential fill` ignores expired\n+\tpasswords. Represented as Unix time UTC, seconds since 1970.\n+\n `url`::\n \n \tWhen this special attribute is read by `git credential`, the\ndiff --git a/Documentation/gitcredentials.txt b/Documentation/gitcredentials.txt\nindex 4522471c337..95636b18439 100644\n--- a/Documentation/gitcredentials.txt\n+++ b/Documentation/gitcredentials.txt\n@@ -167,7 +167,7 @@ helper::\n If there are multiple instances of the `credential.helper` configuration\n variable, each helper will be tried in turn, and may provide a username,\n password, or nothing. Once Git has acquired both a username and a\n-password, no more helpers will be tried.\n+unexpired password, no more helpers will be tried.\n +\n If `credential.helper` is configured to the empty string, this resets\n the helper list to empty (so you may override a helper set by a\ndiff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\nindex f3c89831d4a..338058be7f9 100644\n--- a/builtin/credential-cache--daemon.c\n+++ b/builtin/credential-cache--daemon.c\n@@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out)\n \t\tif (e) {\n \t\t\tfprintf(out, \"username=%s\\n\", e->item.username);\n \t\t\tfprintf(out, \"password=%s\\n\", e->item.password);\n+\t\t\tif (e->item.password_expiry_utc != TIME_MAX)\n+\t\t\t\tfprintf(out, \"password_expiry_utc=%\"PRItime\"\\n\",\n+\t\t\t\t\te->item.password_expiry_utc);\n \t\t}\n \t}\n \telse if (!strcmp(action.buf, \"exit\")) {\ndiff --git a/credential.c b/credential.c\nindex f6389a50684..d3e1bf7a679 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -7,6 +7,7 @@\n #include \"prompt.h\"\n #include \"sigchain.h\"\n #include \"urlmatch.h\"\n+#include \"git-compat-util.h\"\n \n void credential_init(struct credential *c)\n {\n@@ -234,6 +235,10 @@ int credential_read(struct credential *c, FILE *fp)\n \t\t} else if (!strcmp(key, \"path\")) {\n \t\t\tfree(c->path);\n \t\t\tc->path = xstrdup(value);\n+\t\t} else if (!strcmp(key, \"password_expiry_utc\")) {\n+\t\t\tc->password_expiry_utc = parse_timestamp(value, NULL, 10);\n+\t\t\tif (c->password_expiry_utc == 0 || errno)\n+\t\t\t\tc->password_expiry_utc = TIME_MAX;\n \t\t} else if (!strcmp(key, \"url\")) {\n \t\t\tcredential_from_url(c, value);\n \t\t} else if (!strcmp(key, \"quit\")) {\n@@ -269,6 +274,11 @@ void credential_write(const struct credential *c, FILE *fp)\n \tcredential_write_item(fp, \"path\", c->path, 0);\n \tcredential_write_item(fp, \"username\", c->username, 0);\n \tcredential_write_item(fp, \"password\", c->password, 0);\n+\tif (c->password_expiry_utc != TIME_MAX) {\n+\t\tchar *s = xstrfmt(\"%\"PRItime, c->password_expiry_utc);\n+\t\tcredential_write_item(fp, \"password_expiry_utc\", s, 0);\n+\t\tfree(s);\n+\t}\n }\n \n static int run_credential_helper(struct credential *c,\n@@ -342,6 +352,10 @@ void credential_fill(struct credential *c)\n \n \tfor (i = 0; i < c->helpers.nr; i++) {\n \t\tcredential_do(c, c->helpers.items[i].string, \"get\");\n+\t\tif (c->password_expiry_utc < time(NULL)) {\n+\t\t\tFREE_AND_NULL(c->password);\n+\t\t\tc->password_expiry_utc = TIME_MAX;\n+\t\t}\n \t\tif (c->username && c->password)\n \t\t\treturn;\n \t\tif (c->quit)\n@@ -360,7 +374,7 @@ void credential_approve(struct credential *c)\n \n \tif (c->approved)\n \t\treturn;\n-\tif (!c->username || !c->password)\n+\tif (!c->username || !c->password || c->password_expiry_utc < time(NULL))\n \t\treturn;\n \n \tcredential_apply_config(c);\n@@ -381,6 +395,7 @@ void credential_reject(struct credential *c)\n \n \tFREE_AND_NULL(c->username);\n \tFREE_AND_NULL(c->password);\n+\tc->password_expiry_utc = TIME_MAX;\n \tc->approved = 0;\n }\n \ndiff --git a/credential.h b/credential.h\nindex f430e77fea4..935b28a70f1 100644\n--- a/credential.h\n+++ b/credential.h\n@@ -126,10 +126,12 @@ struct credential {\n \tchar *protocol;\n \tchar *host;\n \tchar *path;\n+\ttimestamp_t password_expiry_utc;\n };\n \n #define CREDENTIAL_INIT { \\\n \t.helpers = STRING_LIST_INIT_DUP, \\\n+\t.password_expiry_utc = TIME_MAX, \\\n }\n \n /* Initialize a credential structure, setting all fields to empty. */\ndiff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\nindex 3485c0534e6..96391015af5 100755\n--- a/t/t0300-credentials.sh\n+++ b/t/t0300-credentials.sh\n@@ -35,6 +35,16 @@ test_expect_success 'setup helper scripts' '\n \ttest -z \"$pass\" || echo password=$pass\n \tEOF\n \n+\twrite_script git-credential-verbatim-with-expiry <<-\\EOF &&\n+\tuser=$1; shift\n+\tpass=$1; shift\n+\tpexpiry=$1; shift\n+\t. ./dump\n+\ttest -z \"$user\" || echo username=$user\n+\ttest -z \"$pass\" || echo password=$pass\n+\ttest -z \"$pexpiry\" || echo password_expiry_utc=$pexpiry\n+\tEOF\n+\n \tPATH=\"$PWD:$PATH\"\n '\n \n@@ -109,6 +119,43 @@ test_expect_success 'credential_fill continues through partial response' '\n \tEOF\n '\n \n+test_expect_success 'credential_fill populates password_expiry_utc' '\n+\tcheck fill \"verbatim-with-expiry one two 9999999999\" <<-\\EOF\n+\tprotocol=http\n+\thost=example.com\n+\t--\n+\tprotocol=http\n+\thost=example.com\n+\tusername=one\n+\tpassword=two\n+\tpassword_expiry_utc=9999999999\n+\t--\n+\tverbatim-with-expiry: get\n+\tverbatim-with-expiry: protocol=http\n+\tverbatim-with-expiry: host=example.com\n+\tEOF\n+'\n+\n+test_expect_success 'credential_fill continues through expired password' '\n+\tcheck fill \"verbatim-with-expiry one two 5\" \"verbatim three four\" <<-\\EOF\n+\tprotocol=http\n+\thost=example.com\n+\t--\n+\tprotocol=http\n+\thost=example.com\n+\tusername=three\n+\tpassword=four\n+\t--\n+\tverbatim-with-expiry: get\n+\tverbatim-with-expiry: protocol=http\n+\tverbatim-with-expiry: host=example.com\n+\tverbatim: get\n+\tverbatim: protocol=http\n+\tverbatim: host=example.com\n+\tverbatim: username=one\n+\tEOF\n+'\n+\n test_expect_success 'credential_fill passes along metadata' '\n \tcheck fill \"verbatim one two\" <<-\\EOF\n \tprotocol=ftp\n@@ -149,6 +196,24 @@ test_expect_success 'credential_approve calls all helpers' '\n \tEOF\n '\n \n+test_expect_success 'credential_approve stores password expiry' '\n+\tcheck approve useless <<-\\EOF\n+\tprotocol=http\n+\thost=example.com\n+\tusername=foo\n+\tpassword=bar\n+\tpassword_expiry_utc=9999999999\n+\t--\n+\t--\n+\tuseless: store\n+\tuseless: protocol=http\n+\tuseless: host=example.com\n+\tuseless: username=foo\n+\tuseless: password=bar\n+\tuseless: password_expiry_utc=9999999999\n+\tEOF\n+'\n+\n test_expect_success 'do not bother storing password-less credential' '\n \tcheck approve useless <<-\\EOF\n \tprotocol=http\n@@ -159,6 +224,17 @@ test_expect_success 'do not bother storing password-less credential' '\n \tEOF\n '\n \n+test_expect_success 'credential_approve does not store expired credential' '\n+\tcheck approve useless <<-\\EOF\n+\tprotocol=http\n+\thost=example.com\n+\tusername=foo\n+\tpassword=bar\n+\tpassword_expiry_utc=5\n+\t--\n+\t--\n+\tEOF\n+'\n \n test_expect_success 'credential_reject calls all helpers' '\n \tcheck reject useless \"verbatim one two\" <<-\\EOF\n@@ -181,6 +257,24 @@ test_expect_success 'credential_reject calls all helpers' '\n \tEOF\n '\n \n+test_expect_success 'credential_reject erases expired credential' '\n+\tcheck reject useless <<-\\EOF\n+\tprotocol=http\n+\thost=example.com\n+\tusername=foo\n+\tpassword=bar\n+\tpassword_expiry_utc=5\n+\t--\n+\t--\n+\tuseless: erase\n+\tuseless: protocol=http\n+\tuseless: host=example.com\n+\tuseless: username=foo\n+\tuseless: password=bar\n+\tuseless: password_expiry_utc=5\n+\tEOF\n+'\n+\n test_expect_success 'usernames can be preserved' '\n \tcheck fill \"verbatim \\\"\\\" three\" <<-\\EOF\n \tprotocol=http\n\nbase-commit: 2fc9e9ca3c7505bc60069f11e7ef09b1aeeee473\n-- \ngitgitgadget\n"},{"id":"471514","messageId":"CAGJzqsmN4=SQv+6wPP8Tb+jmiyw=wrDKz49=s1+as+VkHgF1MQ@mail.gmail.com","threadId":"59159","inReplyTo":"Y9pWxHfgPtgCKO+B@coredump.intra.peff.net","subject":"Re: [PATCH v2] credential: new attribute password_expiry_utc","fromName":"M Hickford","fromEmail":"mirth.hickford@gmail.com","sentAt":"2023-02-05T06:34:00Z","receivedAt":"2023-02-05T06:34:44Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"Thanks Jeff for the review\n\nOn Wed, 1 Feb 2023 at 12:10, Jeff King wrote:\n>\n> On Wed, Feb 01, 2023 at 09:39:51AM +0000, M Hickford via GitGitGadget wrote:\n>\n> > +`password_expiry_utc`::\n> > +\n> > + If password is a personal access token or OAuth access token, it may have an\n> > + expiry date. When getting credentials from a helper, `git credential fill`\n> > + ignores the password attribute if the expiry date has passed. Storage\n> > + helpers should store this attribute if possible. Helpers should not\n> > + implement expiry logic themselves. Represented as Unix time UTC, seconds\n> > + since 1970.\n>\n> This \"should not\" seems weird to me. The logic you have here throws out\n> entries that have expired when they pass through Git. But wouldn't\n> helpers which store things want to know about and act on the expiration,\n> too?\n\nI wanted to keep the helper contract as simple as possible \"here is an\nattribute to store and retrieve like any other\". It doesn't matter if\na helper stores a password beyond expiry, because Git will erase or\nreplace it when it fails authentication or another succeeds. Relax the\nlanguage in the patch v3 commit message to describe \"self-pruning of\nexpired passwords is unnecessary but harmless\".\n\n>\n> For example, if Git learns about a credential that expires in 60\n> seconds and passes it to credential-cache which is configured\n> --timeout=300, wouldn't it want to set its internal ttl on the\n> credential to 60, rather than 300?\n>\n> I think your plan here is that Git would then reject the credential if a\n> request is made at time now+65. But the cache is holding onto it much\n> longer than necessary.\n\nEven after the password expires, there's value to storing other\nattributes such as username and (perhaps in future)\noauth-refresh-token. Hence I named the attribute 'password_expiry_utc'\nexplicitly rather than 'credential_expiry_utc'.\n\n>\n> Likewise, wouldn't anything that stores credentials at least want to be\n> able to store and regurgitate the expiration? For instance, even\n> credential-store would want to do this. I'm OK if it doesn't, and we can\n> consider it a quality-of-implementation issue and see if anybody cares\n> enough to implement it. But I'd think most \"real\" helpers would want to\n> do so.\n\nAbsolutely. Eventually I'd like to see support in\ngit-credential-osxkeychain, git-credential-wincred,\ngit-credential-libsecret etc. Mentioned this in patch v3 description.\n\n>\n> So it seems like helpers really do need to support this \"expiration\"\n> notion. And it's actually Git itself which doesn't need to care about\n> it, assuming the helpers are doing something sensible (though it is OK\n> if Git _also_ throws away expired credentials to support helpers which\n> don't).\n>\n> > diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\n> > index f3c89831d4a..338058be7f9 100644\n> > --- a/builtin/credential-cache--daemon.c\n> > +++ b/builtin/credential-cache--daemon.c\n> > @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out)\n> > if (e) {\n> > fprintf(out, \"username=%s\\n\", e->item.username);\n> > fprintf(out, \"password=%s\\n\", e->item.password);\n> > + if (e->item.password_expiry_utc != TIME_MAX)\n> > + fprintf(out, \"password_expiry_utc=%\"PRItime\"\\n\",\n> > + e->item.password_expiry_utc);\n> > }\n>\n> Is there a particular reason to use TIME_MAX as the sentinel value here,\n> and not just \"0\"? It's not that big a deal either way, but it's more\n> usual in our code base to use \"0\" if there's no reason not to (and it\n> seems like nothing should be expiring in 1970 these days).\n\nJunio made a persuasive argument for readability\nhttps://lore.kernel.org/git/CAPig+cQPLMrUKp0aqLCknSYCs5TAso-VSBYsQbGZ8g8wgY2Liw@mail.gmail.com/T/#mf66955cf3f53c073c68ad5ade7213617907bab63\n\n>\n> > @@ -195,15 +196,20 @@ static void credential_getpass(struct credential *c)\n> > if (!c->username)\n> > c->username = credential_ask_one(\"Username\", c,\n> > PROMPT_ASKPASS|PROMPT_ECHO);\n> > - if (!c->password)\n> > + if (!c->password || c->password_expiry_utc < time(NULL)) {\n>\n> This is comparing a timestamp_t to a time_t, which may mix\n> signed/unsigned. I can't offhand think of anything that would go too\n> wrong there before 2038, so it's probably OK, but I wanted to call it\n> out.\n>\n> > @@ -225,6 +231,7 @@ int credential_read(struct credential *c, FILE *fp)\n> > } else if (!strcmp(key, \"password\")) {\n> > free(c->password);\n> > c->password = xstrdup(value);\n> > + password_updated = 1;\n> > } else if (!strcmp(key, \"protocol\")) {\n> > free(c->protocol);\n> > c->protocol = xstrdup(value);\n> > @@ -234,6 +241,11 @@ int credential_read(struct credential *c, FILE *fp)\n> > } else if (!strcmp(key, \"path\")) {\n> > free(c->path);\n> > c->path = xstrdup(value);\n> > + } else if (!strcmp(key, \"password_expiry_utc\")) {\n> > + this_password_expiry = parse_timestamp(value, NULL, 10);\n> > + if (this_password_expiry == 0 || errno) {\n> > + this_password_expiry = TIME_MAX;\n> > + }\n> > } else if (!strcmp(key, \"url\")) {\n> > credential_from_url(c, value);\n> > } else if (!strcmp(key, \"quit\")) {\n> > @@ -246,6 +258,9 @@ int credential_read(struct credential *c, FILE *fp)\n> > */\n> > }\n> >\n> > + if (password_updated)\n> > + c->password_expiry_utc = this_password_expiry;\n>\n> Do we need this logic? It seems weird that a helper would output an\n> expiration but not a password in the first place. I guess ignoring the\n> expiration is probably a reasonable outcome, but I wonder if a helper\n> would ever want to just add an expiration to the data coming from\n> another helper.\n> I.e., could we just read the value directly into c->password_expiry_utc\n> as we do with other fields?\n\nDone in patch v3. The logic that remains (moved to credential_fill) is\na simple sanity check.\n\n>\n> -Peff\n"},{"id":"471515","messageId":"CAGJzqsnKBHPwHf-RMCxSDB6ZB5UPLH+XUbY8YiJOBxOicaG4bA@mail.gmail.com","threadId":"59159","inReplyTo":"AS2PR03MB9815DDCB7B107E7FD37EC972C0D19@AS2PR03MB9815.eurprd03.prod.outlook.com","subject":"Re: [PATCH v2] credential: new attribute password_expiry_utc","fromName":"M Hickford","fromEmail":"mirth.hickford@gmail.com","sentAt":"2023-02-05T06:45:57Z","receivedAt":"2023-02-05T06:46:41Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"On Wed, 1 Feb 2023 at 20:02, Matthew John Cheetham\n wrote:\n>\n> On 2023-02-01 04:10, Jeff King wrote:\n>\n> > On Wed, Feb 01, 2023 at 09:39:51AM +0000, M Hickford via GitGitGadget wrote:\n> >\n> >> +`password_expiry_utc`::\n> >> +\n> >> + If password is a personal access token or OAuth access token, it may have an\n> >> + expiry date. When getting credentials from a helper, `git credential fill`\n> >> + ignores the password attribute if the expiry date has passed. Storage\n> >> + helpers should store this attribute if possible. Helpers should not\n> >> + implement expiry logic themselves. Represented as Unix time UTC, seconds\n> >> + since 1970.\n> >\n> > This \"should not\" seems weird to me. The logic you have here throws out\n> > entries that have expired when they pass through Git. But wouldn't\n> > helpers which store things want to know about and act on the expiration,\n> > too?\n> >\n> > For example, if Git learns about a credential that expires in 60\n> > seconds and passes it to credential-cache which is configured\n> > --timeout=300, wouldn't it want to set its internal ttl on the\n> > credential to 60, rather than 300?\n> >\n> > I think your plan here is that Git would then reject the credential if a\n> > request is made at time now+65. But the cache is holding onto it much\n> > longer than necessary.\n> >\n> > Likewise, wouldn't anything that stores credentials at least want to be\n> > able to store and regurgitate the expiration? For instance, even\n> > credential-store would want to do this. I'm OK if it doesn't, and we can\n> > consider it a quality-of-implementation issue and see if anybody cares\n> > enough to implement it. But I'd think most \"real\" helpers would want to\n> > do so.\n> >\n> > So it seems like helpers really do need to support this \"expiration\"\n> > notion. And it's actually Git itself which doesn't need to care about\n> > it, assuming the helpers are doing something sensible (though it is OK\n> > if Git _also_ throws away expired credentials to support helpers which\n> > don't).\n>\n> I have often wondered about how, and if, Git should handle expiring credentials\n> where the expiration is known. In my opinion I think Git should be doing\n> *less* decision making with credentials and authentication in general, and leave\n> that up to credential helpers.\n>\n> The original design of credential helpers from what I can see (and Peff can\n> correct me here of course!) is that they were really only thought about as\n> storage-style helpers. Helpers are consulted for a known credential, and told\n> about bad (erase) or good (store) credentials, all without any context about\n> the request or remote responses.\n>\n> If no credential helper can respond then Git itself prompts for a user/pass; so\n> Git, or rather the user, is the 'generator'.\n>\n> Of course that's not to say that credential generating helpers don't exist or\n> are wrong - Git Credential Manager being of course one example rather close to\n> home for me! However the current model, even with generating helpers, is still\n> that Git will try and make the request given the details included in the helper\n> response.\n\nGCM would benefit from being able to store expiry too. Whenever GCM\nretrieves an OAuth credential from storage, it queries the server to\ncheck whether the access token has expired [1]. This would become\nunnecessary. I've added more about this in patch v3 commit message.\n\nFurther, it solves a problem if GCM is configured after another storage helper:\n\n```\n[credential]\n helper = storage # eg. osx-keychain or exotic\n helper = manager\n```\n\nCurrently this may return an expired credential from storage.\n\nBackground for others: GCM is typically configured as the *only*\nhelper, with its own internal storage configuration [2]. These\nreimplement or wrap popular Git storage helpers [3][4][5].\n\n```\n[credential]\n helper = manager\n credentialStore = keychain\n```\n\n[1] https://github.com/GitCredentialManager/git-credential-manager/blob/main/src/shared/GitLab/GitLabHostProvider.cs\n[2] https://github.com/GitCredentialManager/git-credential-manager/blob/main/docs/credstores.md\n[3] https://github.com/GitCredentialManager/git-credential-manager/blob/main/src/shared/Core/Interop/MacOS/MacOSKeychain.cs\n[4] https://github.com/GitCredentialManager/git-credential-manager/blob/main/src/shared/Core/Interop/Linux/SecretServiceCollection.cs\n[5] https://github.com/GitCredentialManager/git-credential-manager/blob/main/src/shared/Core/CredentialCacheStore.cs\n\n>\n> It doesn't make sense that a generating helper that knows about expiration would\n> instead choose to respond with an expired credential rather than just try and\n> generate a new credential.\n>\n> Now in the case of a simple storage helper without such logic, after returning\n> an expired credential should Git not be calling 'erase' back to the same helper\n> to inform it that it has a stale credential and should be deleted?\n> This would also require some affinity between calls to get/erase/store.\n>\n>\n> >> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\n> >> index f3c89831d4a..338058be7f9 100644\n> >> --- a/builtin/credential-cache--daemon.c\n> >> +++ b/builtin/credential-cache--daemon.c\n> >> @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out)\n> >> if (e) {\n> >> fprintf(out, \"username=%s\\n\", e->item.username);\n> >> fprintf(out, \"password=%s\\n\", e->item.password);\n> >> + if (e->item.password_expiry_utc != TIME_MAX)\n> >> + fprintf(out, \"password_expiry_utc=%\"PRItime\"\\n\",\n> >> + e->item.password_expiry_utc);\n> >> }\n> >\n> > Is there a particular reason to use TIME_MAX as the sentinel value here,\n> > and not just \"0\"? It's not that big a deal either way, but it's more\n> > usual in our code base to use \"0\" if there's no reason not to (and it\n> > seems like nothing should be expiring in 1970 these days).\n> >\n> >> @@ -195,15 +196,20 @@ static void credential_getpass(struct credential *c)\n> >> if (!c->username)\n> >> c->username = credential_ask_one(\"Username\", c,\n> >> PROMPT_ASKPASS|PROMPT_ECHO);\n> >> - if (!c->password)\n> >> + if (!c->password || c->password_expiry_utc < time(NULL)) {\n> >\n> > This is comparing a timestamp_t to a time_t, which may mix\n> > signed/unsigned. I can't offhand think of anything that would go too\n> > wrong there before 2038, so it's probably OK, but I wanted to call it\n> > out.\n> >\n> >> @@ -225,6 +231,7 @@ int credential_read(struct credential *c, FILE *fp)\n> >> } else if (!strcmp(key, \"password\")) {\n> >> free(c->password);\n> >> c->password = xstrdup(value);\n> >> + password_updated = 1;\n> >> } else if (!strcmp(key, \"protocol\")) {\n> >> free(c->protocol);\n> >> c->protocol = xstrdup(value);\n> >> @@ -234,6 +241,11 @@ int credential_read(struct credential *c, FILE *fp)\n> >> } else if (!strcmp(key, \"path\")) {\n> >> free(c->path);\n> >> c->path = xstrdup(value);\n> >> + } else if (!strcmp(key, \"password_expiry_utc\")) {\n> >> + this_password_expiry = parse_timestamp(value, NULL, 10);\n> >> + if (this_password_expiry == 0 || errno) {\n> >> + this_password_expiry = TIME_MAX;\n> >> + }\n> >> } else if (!strcmp(key, \"url\")) {\n> >> credential_from_url(c, value);\n> >> } else if (!strcmp(key, \"quit\")) {\n> >> @@ -246,6 +258,9 @@ int credential_read(struct credential *c, FILE *fp)\n> >> */\n> >> }\n> >>\n> >> + if (password_updated)\n> >> + c->password_expiry_utc = this_password_expiry;\n> >\n> > Do we need this logic? It seems weird that a helper would output an\n> > expiration but not a password in the first place. I guess ignoring the\n> > expiration is probably a reasonable outcome, but I wonder if a helper\n> > would ever want to just add an expiration to the data coming from\n> > another helper.\n> >\n> > I.e., could we just read the value directly into c->password_expiry_utc\n> > as we do with other fields?\n> >\n> > -Peff\n"},{"id":"471516","messageId":"CAGJzqsnoTo=B3hiD7LtPRUG22TtkOOsZW2XMiptHa+6Ax9PEBg@mail.gmail.com","threadId":"59159","inReplyTo":"CAPig+cQPLMrUKp0aqLCknSYCs5TAso-VSBYsQbGZ8g8wgY2Liw@mail.gmail.com","subject":"Re: [PATCH] credential: new attribute password_expiry_utc","fromName":"M Hickford","fromEmail":"mirth.hickford@gmail.com","sentAt":"2023-02-05T06:49:29Z","receivedAt":"2023-02-05T06:50:12Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"Thanks Eric for the review\n\nOn Mon, 30 Jan 2023 at 00:59, Eric Sunshine wrote:\n>\n> On Sat, Jan 28, 2023 at 9:08 AM M Hickford via GitGitGadget\n> wrote:\n> > If password has expired, credential fill no longer returns early,\n> > so later helpers can generate a fresh credential. This is backwards\n> > compatible -- no change in behaviour with helpers that discard the\n> > expiry attribute. The expiry logic is entirely in the git credential\n> > layer; compatible helpers simply store and return the expiry\n> > attribute verbatim.\n> >\n> > Store new attribute in cache.\n> >\n> > Signed-off-by: M Hickford \n>\n> Just a few comments in addition to those already provided by Junio...\n>\n> > diff --git a/credential.c b/credential.c\n> > @@ -234,11 +236,23 @@ int credential_read(struct credential *c, FILE *fp)\n> > + // if expiry date has passed, ignore password and expiry fields\n> > + if (c->password_expiry_utc != 0 && time(NULL) > c->password_expiry_utc) {\n> > + trace_printf(_(\"Password has expired.\\n\"));\n>\n> Using `_(...)` marks a string for localization, but doing so is\n> undesirable for debugging messages which are meant for the developer,\n> not the end user (and it creates extra work for translators). No\n> existing[1] trace_printf() calls in the codebase use `_(...)`.\n\nDone in patch v3.\n\n>\n> [1]: Unfortunately, a couple examples exist in\n> Documentation/MyFirstObjectWalk.txt using `_(...)` but they should be\n> removed.\n>\n> > @@ -269,6 +283,13 @@ void credential_write(const struct credential *c, FILE *fp)\n> > + if (c->password_expiry_utc != 0) {\n> > + int length = snprintf( NULL, 0, \"%ld\", c->password_expiry_utc);\n> > + char* str = malloc( length + 1 );\n>\n> Style in this project is `char *str`, not `char* str`. Also, drop\n> spaces around function arguments:\n>\n> char *str = malloc(length + 1);\n>\n> > + snprintf( str, length + 1, \"%ld\", c->password_expiry_utc );\n>\n> Same.\n>\n> > + credential_write_item(fp, \"password_expiry_utc\", str, 0);\n> > + free(str);\n> > + }\n>\n> xstrfmt() from strbuf.h can help simplify this entire block:\n>\n> char *s = xstrfmt(\"%ld\", c->password_expiry_utc);\n> credential_write_item(fp, \"password_expiry_utc\", str, 0);\n> free(s);\n\nNeat. Done in patch v3.\n"},{"id":"471576","messageId":"DB9PR03MB9831BD6602F8B904E9D1CBE2C0DA9@DB9PR03MB9831.eurprd03.prod.outlook.com","threadId":"59159","inReplyTo":"CAGJzqsnKBHPwHf-RMCxSDB6ZB5UPLH+XUbY8YiJOBxOicaG4bA@mail.gmail.com","subject":"Re: [PATCH v2] credential: new attribute password_expiry_utc","fromName":"Matthew John Cheetham","fromEmail":"mjcheetham@outlook.com","sentAt":"2023-02-06T18:59:03Z","receivedAt":"2023-02-06T18:59:20Z","isPatch":true,"sender":{"key":"mjcheetham@outlook.com","avatar":"https://avatars.githubusercontent.com/u/5658207?v=4"},"body":"On 2023-02-04 22:45, M Hickford wrote:\n\n> On Wed, 1 Feb 2023 at 20:02, Matthew John Cheetham\n> wrote:\n>>\n>> On 2023-02-01 04:10, Jeff King wrote:\n>>\n>>> On Wed, Feb 01, 2023 at 09:39:51AM +0000, M Hickford via GitGitGadget wrote:\n>>>\n>>>> +`password_expiry_utc`::\n>>>> +\n>>>> + If password is a personal access token or OAuth access token, it may have an\n>>>> + expiry date. When getting credentials from a helper, `git credential fill`\n>>>> + ignores the password attribute if the expiry date has passed. Storage\n>>>> + helpers should store this attribute if possible. Helpers should not\n>>>> + implement expiry logic themselves. Represented as Unix time UTC, seconds\n>>>> + since 1970.\n>>>\n>>> This \"should not\" seems weird to me. The logic you have here throws out\n>>> entries that have expired when they pass through Git. But wouldn't\n>>> helpers which store things want to know about and act on the expiration,\n>>> too?\n>>>\n>>> For example, if Git learns about a credential that expires in 60\n>>> seconds and passes it to credential-cache which is configured\n>>> --timeout=300, wouldn't it want to set its internal ttl on the\n>>> credential to 60, rather than 300?\n>>>\n>>> I think your plan here is that Git would then reject the credential if a\n>>> request is made at time now+65. But the cache is holding onto it much\n>>> longer than necessary.\n>>>\n>>> Likewise, wouldn't anything that stores credentials at least want to be\n>>> able to store and regurgitate the expiration? For instance, even\n>>> credential-store would want to do this. I'm OK if it doesn't, and we can\n>>> consider it a quality-of-implementation issue and see if anybody cares\n>>> enough to implement it. But I'd think most \"real\" helpers would want to\n>>> do so.\n>>>\n>>> So it seems like helpers really do need to support this \"expiration\"\n>>> notion. And it's actually Git itself which doesn't need to care about\n>>> it, assuming the helpers are doing something sensible (though it is OK\n>>> if Git _also_ throws away expired credentials to support helpers which\n>>> don't).\n>>\n>> I have often wondered about how, and if, Git should handle expiring credentials\n>> where the expiration is known. In my opinion I think Git should be doing\n>> *less* decision making with credentials and authentication in general, and leave\n>> that up to credential helpers.\n>>\n>> The original design of credential helpers from what I can see (and Peff can\n>> correct me here of course!) is that they were really only thought about as\n>> storage-style helpers. Helpers are consulted for a known credential, and told\n>> about bad (erase) or good (store) credentials, all without any context about\n>> the request or remote responses.\n>>\n>> If no credential helper can respond then Git itself prompts for a user/pass; so\n>> Git, or rather the user, is the 'generator'.\n>>\n>> Of course that's not to say that credential generating helpers don't exist or\n>> are wrong - Git Credential Manager being of course one example rather close to\n>> home for me! However the current model, even with generating helpers, is still\n>> that Git will try and make the request given the details included in the helper\n>> response.\n> \n> GCM would benefit from being able to store expiry too. Whenever GCM\n> retrieves an OAuth credential from storage, it queries the server to\n> check whether the access token has expired [1]. This would become\n> unnecessary. I've added more about this in patch v3 commit message.\n> \n> Further, it solves a problem if GCM is configured after another storage helper:\n> \n> ```\n> [credential]\n> helper = storage # eg. osx-keychain or exotic\n> helper = manager\n> ```\n> \n> Currently this may return an expired credential from storage.\n> \n> Background for others: GCM is typically configured as the *only*\n> helper, with its own internal storage configuration [2]. These\n> reimplement or wrap popular Git storage helpers [3][4][5].\n> \n> ```\n> [credential]\n> helper = manager\n> credentialStore = keychain\n> ```\n\n\nOne of the things that concerns me about the credential helper system\ntoday is the lack of 'affinity' across calls, and I think that we may\ndisagree on this here.\n\nA desire I have for the future of Git auth is that helpers can negotiate\nto start a more long-lived or complicated converstation, with retry and\ndetailed feedback on the auth responses. I'm increasingly feeling that\nthe get/store/erase model is not sufficient for this.\n\nImagine the scenario where the auth mechanism has a nonce that is\nupdated on each successful (or failed) response. Here we'd want the\nhelper that offered credentials to be told about the successful response\nfor book-keeping, and not have that message 'stolen' by a simpler storage\nhelper.\n\nAnother scenario would be multiple user accounts; a helper has credentials\nthat would potentially be valid for the request (same host/forge or IdP\nfor example), and we're wanting to avoid unnecessary user prompts as the\nuser has not specified explicitly the account to 'bind' to that clone or\nspecific remote. Why could we not return credentials to Git, also indicating\nthat we have an ability to retry on a 401/403?\n\nIncreasingly, modern auth schemes have credentials that are so short lived\n(maybe bound to the exact specific request.. a one time use) that it really\ndoesn't make sense to store any credentials at all. With such one-time-use\ncredentials, and a simple storage helper ahead of a generating helper every\nother request would fail and the user would need to retry the command.\nImagine the case that the OS/hardware security device holds the 'real'\nkey or credential that is used to derive a one-time-use credential.\n\nStrong auth mechamisms often tie credential generation strongly to storage.\n\n> [1] https://github.com/GitCredentialManager/git-credential-manager/blob/main/src/shared/GitLab/GitLabHostProvider.cs\n> [2] https://github.com/GitCredentialManager/git-credential-manager/blob/main/docs/credstores.md\n> [3] https://github.com/GitCredentialManager/git-credential-manager/blob/main/src/shared/Core/Interop/MacOS/MacOSKeychain.cs\n> [4] https://github.com/GitCredentialManager/git-credential-manager/blob/main/src/shared/Core/Interop/Linux/SecretServiceCollection.cs\n> [5] https://github.com/GitCredentialManager/git-credential-manager/blob/main/src/shared/Core/CredentialCacheStore.cs\n> \n>>\n>> It doesn't make sense that a generating helper that knows about expiration would\n>> instead choose to respond with an expired credential rather than just try and\n>> generate a new credential.\n>>\n>> Now in the case of a simple storage helper without such logic, after returning\n>> an expired credential should Git not be calling 'erase' back to the same helper\n>> to inform it that it has a stale credential and should be deleted?\n>> This would also require some affinity between calls to get/erase/store.\n>>\n>>\n>>>> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\n>>>> index f3c89831d4a..338058be7f9 100644\n>>>> --- a/builtin/credential-cache--daemon.c\n>>>> +++ b/builtin/credential-cache--daemon.c\n>>>> @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out)\n>>>> if (e) {\n>>>> fprintf(out, \"username=%s\\n\", e->item.username);\n>>>> fprintf(out, \"password=%s\\n\", e->item.password);\n>>>> + if (e->item.password_expiry_utc != TIME_MAX)\n>>>> + fprintf(out, \"password_expiry_utc=%\"PRItime\"\\n\",\n>>>> + e->item.password_expiry_utc);\n>>>> }\n>>>\n>>> Is there a particular reason to use TIME_MAX as the sentinel value here,\n>>> and not just \"0\"? It's not that big a deal either way, but it's more\n>>> usual in our code base to use \"0\" if there's no reason not to (and it\n>>> seems like nothing should be expiring in 1970 these days).\n>>>\n>>>> @@ -195,15 +196,20 @@ static void credential_getpass(struct credential *c)\n>>>> if (!c->username)\n>>>> c->username = credential_ask_one(\"Username\", c,\n>>>> PROMPT_ASKPASS|PROMPT_ECHO);\n>>>> - if (!c->password)\n>>>> + if (!c->password || c->password_expiry_utc < time(NULL)) {\n>>>\n>>> This is comparing a timestamp_t to a time_t, which may mix\n>>> signed/unsigned. I can't offhand think of anything that would go too\n>>> wrong there before 2038, so it's probably OK, but I wanted to call it\n>>> out.\n>>>\n>>>> @@ -225,6 +231,7 @@ int credential_read(struct credential *c, FILE *fp)\n>>>> } else if (!strcmp(key, \"password\")) {\n>>>> free(c->password);\n>>>> c->password = xstrdup(value);\n>>>> + password_updated = 1;\n>>>> } else if (!strcmp(key, \"protocol\")) {\n>>>> free(c->protocol);\n>>>> c->protocol = xstrdup(value);\n>>>> @@ -234,6 +241,11 @@ int credential_read(struct credential *c, FILE *fp)\n>>>> } else if (!strcmp(key, \"path\")) {\n>>>> free(c->path);\n>>>> c->path = xstrdup(value);\n>>>> + } else if (!strcmp(key, \"password_expiry_utc\")) {\n>>>> + this_password_expiry = parse_timestamp(value, NULL, 10);\n>>>> + if (this_password_expiry == 0 || errno) {\n>>>> + this_password_expiry = TIME_MAX;\n>>>> + }\n>>>> } else if (!strcmp(key, \"url\")) {\n>>>> credential_from_url(c, value);\n>>>> } else if (!strcmp(key, \"quit\")) {\n>>>> @@ -246,6 +258,9 @@ int credential_read(struct credential *c, FILE *fp)\n>>>> */\n>>>> }\n>>>>\n>>>> + if (password_updated)\n>>>> + c->password_expiry_utc = this_password_expiry;\n>>>\n>>> Do we need this logic? It seems weird that a helper would output an\n>>> expiration but not a password in the first place. I guess ignoring the\n>>> expiration is probably a reasonable outcome, but I wonder if a helper\n>>> would ever want to just add an expiration to the data coming from\n>>> another helper.\n>>>\n>>> I.e., could we just read the value directly into c->password_expiry_utc\n>>> as we do with other fields?\n>>>\n>>> -Peff\n"},{"id":"472040","messageId":"xmqqmt5h80s8.fsf@gitster.g","threadId":"59159","inReplyTo":"pull.1443.v3.git.git.1675545372271.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] credential: new attribute password_expiry_utc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-14T01:59:35Z","receivedAt":"2023-02-14T01:59:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"M Hickford via GitGitGadget\" writes:\n\n> From: M Hickford \n>\n> Some passwords have an expiry date known at generation. This may be\n> years away for a personal access token or hours for an OAuth access\n> token.\n> ...\n> t/t0300-credentials.sh | 94 ++++++++++++++++++++++++++++++\n\nhttps://github.com/git/git/actions/runs/4169057114/jobs/7217377625\n\nOther platforms seem to be OK, but Windows test seems to be unhappy\nwith it when this topic gets merged to 'seen'.\n"},{"id":"472053","messageId":"CAN0heSq8OSOyX=FpRd2SPvyb6EBbju-5gbTQGxHre0wkwB=tkQ@mail.gmail.com","threadId":"59159","inReplyTo":"pull.1443.v3.git.git.1675545372271.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] credential: new attribute password_expiry_utc","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2023-02-14T08:03:56Z","receivedAt":"2023-02-14T08:04:42Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Sat, 4 Feb 2023 at 23:03, M Hickford via GitGitGadget\n wrote:\n\n> --- a/Documentation/gitcredentials.txt\n> +++ b/Documentation/gitcredentials.txt\n> @@ -167,7 +167,7 @@ helper::\n> If there are multiple instances of the `credential.helper` configuration\n> variable, each helper will be tried in turn, and may provide a username,\n> password, or nothing. Once Git has acquired both a username and a\n> -password, no more helpers will be tried.\n> +unexpired password, no more helpers will be tried.\n\ns/a unexpired/an unexpired/ (or \"a non-expired\", perhaps)\n\nMartin\n"},{"id":"472094","messageId":"CAGJzqs=t7k2zRKKq9xN-Avbo2uXgqsg7i0Utfv-ee6yZ2CWNDA@mail.gmail.com","threadId":"59159","inReplyTo":"xmqqmt5h80s8.fsf@gitster.g","subject":"Re: [PATCH v3] credential: new attribute password_expiry_utc","fromName":"M Hickford","fromEmail":"mirth.hickford@gmail.com","sentAt":"2023-02-14T22:36:01Z","receivedAt":"2023-02-14T22:36:43Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"On Tue, 14 Feb 2023 at 01:59, Junio C Hamano wrote:\n>\n> \"M Hickford via GitGitGadget\" writes:\n>\n> > From: M Hickford \n> >\n> > Some passwords have an expiry date known at generation. This may be\n> > years away for a personal access token or hours for an OAuth access\n> > token.\n> > ...\n> > t/t0300-credentials.sh | 94 ++++++++++++++++++++++++++++++\n>\n> https://github.com/git/git/actions/runs/4169057114/jobs/7217377625\n>\n> Other platforms seem to be OK, but Windows test seems to be unhappy\n> with it when this topic gets merged to 'seen'.\n\nCurious, let me take a look. I see that the tests failed on freebsd too.\n\nI don't have a Windows machine to debug. If anyone reading has a\nhypothesis, please share.\n\nI shall try changing the default value for a password without expiry\nfrom TIME_MAX to 0, see if that works any better [2].\n\n[1] https://github.com/git/git/pull/1443/checks?check_run_id=11112315291\n[2] https://github.com/git/git/pull/1443/checks?check_run_id=11343677685\n"},{"id":"472219","messageId":"20230216191644.315615-1-calvinwan@google.com","threadId":"59159","inReplyTo":"pull.1443.v3.git.git.1675545372271.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] credential: new attribute password_expiry_utc","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2023-02-16T19:16:43Z","receivedAt":"2023-02-16T19:16:51Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"> static int run_credential_helper(struct credential *c,\n> @@ -342,6 +352,10 @@ void credential_fill(struct credential *c)\n> \n> \tfor (i = 0; i < c->helpers.nr; i++) {\n> \t\tcredential_do(c, c->helpers.items[i].string, \"get\");\n> +\t\tif (c->password_expiry_utc < time(NULL)) {\n> +\t\t\tFREE_AND_NULL(c->password);\n> +\t\t\tc->password_expiry_utc = TIME_MAX;\n> +\t\t}\n> \t\tif (c->username && c->password)\n> \t\t\treturn;\n> \t\tif (c->quit)\n\nI see you null out c->password in the expiry if block so that the\nfollowing c->password check in the following if statement fails.\nWhile I think it's neat little trick, I wonder if others on list\nthink it's better to be more explicit with how the logic should\nwork (eg. adding the c->passowrd_expiry_utc check as an inner\nblock inside of the c->username && c->password block).\n"},{"id":"472273","messageId":"85ab572a-cd00-f62a-97ab-f344e2b6f68e@gmail.com","threadId":"59159","inReplyTo":"CAGJzqs=t7k2zRKKq9xN-Avbo2uXgqsg7i0Utfv-ee6yZ2CWNDA@mail.gmail.com","subject":"Re: [PATCH v3] credential: new attribute password_expiry_utc","fromName":"Lessley Dennington","fromEmail":"lessleydennington@gmail.com","sentAt":"2023-02-17T21:44:52Z","receivedAt":"2023-02-17T21:44:59Z","isPatch":true,"sender":{"key":"lessleydennington@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11321782?v=4"},"body":"On 2/14/23 3:36 PM, M Hickford wrote:\n> Curious, let me take a look. I see that the tests failed on freebsd too.\n> \nI was curious about this as well and took a look on my Windows machine. It\nappears that errno will be set to 'No such file or directory' unless you\nexplicitly set it before checking it. This is odd, given that the helper script\nI wrote worked just fine without this requirement. However, I took a look\nthrough the rest of the codebase and noticed that `errno = 0` seems to always be\ndeclared before this type of conditional check. That did indeed fix the issue -\ntests all pass on Windows with this patch. I have not confirmed on freebsd,\nthough, as a heads up.\n\nThanks,\nLessley\n\n---\n\ndiff --git a/credential.c b/credential.c\nindex d3e1bf7a67..b9a9a1d7b1 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -236,6 +236,7 @@ int credential_read(struct credential *c, FILE *fp)\n free(c->path);\n c->path = xstrdup(value);\n } else if (!strcmp(key, \"password_expiry_utc\")) {\n+ errno = 0;\n c->password_expiry_utc = parse_timestamp(value, NULL, 10);\n if (c->password_expiry_utc == 0 || errno)\n c->password_expiry_utc = TIME_MAX;\n\n\n"},{"id":"472275","messageId":"xmqqa61ckl6j.fsf@gitster.g","threadId":"59159","inReplyTo":"85ab572a-cd00-f62a-97ab-f344e2b6f68e@gmail.com","subject":"Re: [PATCH v3] credential: new attribute password_expiry_utc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-17T21:59:32Z","receivedAt":"2023-02-17T21:59:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lessley Dennington writes:\n\n> diff --git a/credential.c b/credential.c\n> index d3e1bf7a67..b9a9a1d7b1 100644\n> --- a/credential.c\n> +++ b/credential.c\n> @@ -236,6 +236,7 @@ int credential_read(struct credential *c, FILE *fp)\n> free(c->path);\n> c->path = xstrdup(value);\n> } else if (!strcmp(key, \"password_expiry_utc\")) {\n> + errno = 0;\n> c->password_expiry_utc = parse_timestamp(value, NULL, 10);\n> if (c->password_expiry_utc == 0 || errno)\n> c->password_expiry_utc = TIME_MAX;\n\nAh, that is quite understandable. Successful library function calls\nwould not _clera_ errno, so if there were a failure before the\ncontrol reaches this codepath, errno may have been set, and then\nparse_timestamp() call, which would be a strto$some_integral_type() call,\nmay succeed and will leave errno as-is. Your fix is absolutely correct\nas long as we want to use \"errno\" after the call returns.\n\nWhen strtoumax() etc. wants to report overflow or underflow, the\nreturned value must be UINTMAX_MAX/UINTMAX_MIN and errno would be\nERANGE, so it would probably want to check that errno is that value,\nand/or what c->password_expiry_utc has these overflow/underflow\nvalues.\n\n> ... I have not confirmed on freebsd,\n> though, as a heads up.\n\n"},{"id":"472295","messageId":"pull.1443.v4.git.git.1676701977347.gitgitgadget@gmail.com","threadId":"59159","inReplyTo":"pull.1443.v3.git.git.1675545372271.gitgitgadget@gmail.com","subject":"[PATCH v4] credential: new attribute password_expiry_utc","fromName":"M Hickford via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-02-18T06:32:57Z","receivedAt":"2023-02-18T06:33:05Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"From: M Hickford \n\nSome passwords have an expiry date known at generation. This may be\nyears away for a personal access token or hours for an OAuth access\ntoken.\n\nWhen multiple credential helpers are configured, `credential fill` tries\neach helper in turn until it has a username and password, returning\nearly. If Git authentication succeeds, `credential approve`\nstores the successful credential in all helpers. If authentication\nfails, `credential reject` erases matching credentials in all helpers.\nHelpers implement corresponding operations: get, store, erase.\n\nThe credential protocol has no expiry attribute, so helpers cannot\nstore expiry information. Even if a helper returned an improvised\nexpiry attribute, git credential discards unrecognised attributes\nbetween operations and between helpers.\n\nThis is a particular issue when a storage helper and a\ncredential-generating helper are configured together:\n\n\t[credential]\n\t\thelper = storage # eg. cache or osxkeychain\n\t\thelper = generate # eg. oauth\n\n`credential approve` stores the generated credential in both helpers\nwithout expiry information. Later `credential fill` may return an\nexpired credential from storage. There is no workaround, no matter how\nclever the second helper. The user sees authentication fail (a retry\nwill succeed).\n\nIntroduce a password expiry attribute. In `credential fill`, ignore\nexpired passwords and continue to query subsequent helpers.\n\nIn the example above, `credential fill` ignores the expired password\nand a fresh credential is generated. If authentication succeeds,\n`credential approve` replaces the expired password in storage.\nIf authentication fails, the expired credential is erased by\n`credential reject`. It is unnecessary but harmless for storage\nhelpers to self prune expired credentials.\n\nAdd support for the new attribute to credential-cache.\nEventually, I hope to see support in other popular storage helpers.\n\nExample usage in a credential-generating helper\nhttps://github.com/hickford/git-credential-oauth/pull/16\n\nSigned-off-by: M Hickford \n---\n credential: new attribute password_expiry_utc\n \n details in commit message\n \n Changes in patch v4:\n \n * Set errno = 0 to fix tests on Windows and FreeBSD (thanks Lessley\n Dennington)\n * Clarify rationale as discussed at review club\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1443%2Fhickford%2Fpassword-expiry-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1443/hickford/password-expiry-v4\nPull-Request: https://github.com/git/git/pull/1443\n\nRange-diff vs v3:\n\n 1: 1846815a5c1 ! 1: ae8bddbb30a credential: new attribute password_expiry_utc\n @@ Commit message\n Helpers implement corresponding operations: get, store, erase.\n \n The credential protocol has no expiry attribute, so helpers cannot\n - store expiry information. (Even if a helper returned an improvised\n + store expiry information. Even if a helper returned an improvised\n expiry attribute, git credential discards unrecognised attributes\n - between operations and between helpers.)\n + between operations and between helpers.\n \n - As a workaround, whenever monolithic helper Git Credential Manager (GCM)\n - retrieves an OAuth credential from its storage, it makes a HTTP request\n - to check whether the OAuth token has expired [1]. This complicates and\n - slows the authentication happy path.\n -\n - Worse is the case that a storage helper and a credential-generating\n - helper are configured together:\n + This is a particular issue when a storage helper and a\n + credential-generating helper are configured together:\n \n [credential]\n helper = storage # eg. cache or osxkeychain\n - helper = generate # eg. oauth or manager\n + helper = generate # eg. oauth\n \n `credential approve` stores the generated credential in both helpers\n without expiry information. Later `credential fill` may return an\n expired credential from storage. There is no workaround, no matter how\n - clever the second helper.\n + clever the second helper. The user sees authentication fail (a retry\n + will succeed).\n \n Introduce a password expiry attribute. In `credential fill`, ignore\n expired passwords and continue to query subsequent helpers.\n \n - In the example above, `credential fill` ignores the expired credential\n + In the example above, `credential fill` ignores the expired password\n and a fresh credential is generated. If authentication succeeds,\n - `credential approve` replaces the expired credential in storage.\n + `credential approve` replaces the expired password in storage.\n If authentication fails, the expired credential is erased by\n `credential reject`. It is unnecessary but harmless for storage\n helpers to self prune expired credentials.\n \n Add support for the new attribute to credential-cache.\n - Eventually, I hope to see support in other storage helpers.\n + Eventually, I hope to see support in other popular storage helpers.\n \n Example usage in a credential-generating helper\n https://github.com/hickford/git-credential-oauth/pull/16\n \n - [1] https://github.com/GitCredentialManager/git-credential-manager/blob/66b94e489ad8cc1982836355493e369770b30211/src/shared/GitLab/GitLabHostProvider.cs#L217\n -\n Signed-off-by: M Hickford \n \n ## Documentation/git-credential.txt ##\n @@ Documentation/gitcredentials.txt: helper::\n variable, each helper will be tried in turn, and may provide a username,\n password, or nothing. Once Git has acquired both a username and a\n -password, no more helpers will be tried.\n -+unexpired password, no more helpers will be tried.\n ++non-expired password, no more helpers will be tried.\n +\n If `credential.helper` is configured to the empty string, this resets\n the helper list to empty (so you may override a helper set by a\n @@ credential.c: int credential_read(struct credential *c, FILE *fp)\n \t\t\tfree(c->path);\n \t\t\tc->path = xstrdup(value);\n +\t\t} else if (!strcmp(key, \"password_expiry_utc\")) {\n ++\t\t\terrno = 0;\n +\t\t\tc->password_expiry_utc = parse_timestamp(value, NULL, 10);\n -+\t\t\tif (c->password_expiry_utc == 0 || errno)\n ++\t\t\tif (c->password_expiry_utc == 0 || errno == ERANGE)\n +\t\t\t\tc->password_expiry_utc = TIME_MAX;\n \t\t} else if (!strcmp(key, \"url\")) {\n \t\t\tcredential_from_url(c, value);\n @@ credential.c: void credential_fill(struct credential *c)\n \tfor (i = 0; i < c->helpers.nr; i++) {\n \t\tcredential_do(c, c->helpers.items[i].string, \"get\");\n +\t\tif (c->password_expiry_utc < time(NULL)) {\n ++\t\t\t/* Discard expired password */\n +\t\t\tFREE_AND_NULL(c->password);\n ++\t\t\t/* Reset expiry to maintain consistency */\n +\t\t\tc->password_expiry_utc = TIME_MAX;\n +\t\t}\n \t\tif (c->username && c->password)\n @@ t/t0300-credentials.sh: test_expect_success 'credential_fill continues through p\n +\tEOF\n +'\n +\n -+test_expect_success 'credential_fill continues through expired password' '\n ++test_expect_success 'credential_fill ignores expired password' '\n +\tcheck fill \"verbatim-with-expiry one two 5\" \"verbatim three four\" <<-\\EOF\n +\tprotocol=http\n +\thost=example.com\n @@ t/t0300-credentials.sh: test_expect_success 'do not bother storing password-less\n \tEOF\n '\n \n -+test_expect_success 'credential_approve does not store expired credential' '\n ++test_expect_success 'credential_approve does not store expired password' '\n +\tcheck approve useless <<-\\EOF\n +\tprotocol=http\n +\thost=example.com\n @@ t/t0300-credentials.sh: test_expect_success 'credential_reject calls all helpers\n \tEOF\n '\n \n -+test_expect_success 'credential_reject erases expired credential' '\n ++test_expect_success 'credential_reject erases credential regardless of expiry' '\n +\tcheck reject useless <<-\\EOF\n +\tprotocol=http\n +\thost=example.com\n\n\n Documentation/git-credential.txt | 6 ++\n Documentation/gitcredentials.txt | 2 +-\n builtin/credential-cache--daemon.c | 3 +\n credential.c | 20 ++++++-\n credential.h | 2 +\n t/t0300-credentials.sh | 94 ++++++++++++++++++++++++++++++\n 6 files changed, 125 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt\nindex ac2818b9f66..29d184ab824 100644\n--- a/Documentation/git-credential.txt\n+++ b/Documentation/git-credential.txt\n@@ -144,6 +144,12 @@ Git understands the following attributes:\n \n \tThe credential's password, if we are asking it to be stored.\n \n+`password_expiry_utc`::\n+\n+\tGenerated passwords such as an OAuth access token may have an expiry date.\n+\tWhen reading credentials from helpers, `git credential fill` ignores expired\n+\tpasswords. Represented as Unix time UTC, seconds since 1970.\n+\n `url`::\n \n \tWhen this special attribute is read by `git credential`, the\ndiff --git a/Documentation/gitcredentials.txt b/Documentation/gitcredentials.txt\nindex 4522471c337..100f045bb1a 100644\n--- a/Documentation/gitcredentials.txt\n+++ b/Documentation/gitcredentials.txt\n@@ -167,7 +167,7 @@ helper::\n If there are multiple instances of the `credential.helper` configuration\n variable, each helper will be tried in turn, and may provide a username,\n password, or nothing. Once Git has acquired both a username and a\n-password, no more helpers will be tried.\n+non-expired password, no more helpers will be tried.\n +\n If `credential.helper` is configured to the empty string, this resets\n the helper list to empty (so you may override a helper set by a\ndiff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\nindex f3c89831d4a..338058be7f9 100644\n--- a/builtin/credential-cache--daemon.c\n+++ b/builtin/credential-cache--daemon.c\n@@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out)\n \t\tif (e) {\n \t\t\tfprintf(out, \"username=%s\\n\", e->item.username);\n \t\t\tfprintf(out, \"password=%s\\n\", e->item.password);\n+\t\t\tif (e->item.password_expiry_utc != TIME_MAX)\n+\t\t\t\tfprintf(out, \"password_expiry_utc=%\"PRItime\"\\n\",\n+\t\t\t\t\te->item.password_expiry_utc);\n \t\t}\n \t}\n \telse if (!strcmp(action.buf, \"exit\")) {\ndiff --git a/credential.c b/credential.c\nindex f6389a50684..f32011343f9 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -7,6 +7,7 @@\n #include \"prompt.h\"\n #include \"sigchain.h\"\n #include \"urlmatch.h\"\n+#include \"git-compat-util.h\"\n \n void credential_init(struct credential *c)\n {\n@@ -234,6 +235,11 @@ int credential_read(struct credential *c, FILE *fp)\n \t\t} else if (!strcmp(key, \"path\")) {\n \t\t\tfree(c->path);\n \t\t\tc->path = xstrdup(value);\n+\t\t} else if (!strcmp(key, \"password_expiry_utc\")) {\n+\t\t\terrno = 0;\n+\t\t\tc->password_expiry_utc = parse_timestamp(value, NULL, 10);\n+\t\t\tif (c->password_expiry_utc == 0 || errno == ERANGE)\n+\t\t\t\tc->password_expiry_utc = TIME_MAX;\n \t\t} else if (!strcmp(key, \"url\")) {\n \t\t\tcredential_from_url(c, value);\n \t\t} else if (!strcmp(key, \"quit\")) {\n@@ -269,6 +275,11 @@ void credential_write(const struct credential *c, FILE *fp)\n \tcredential_write_item(fp, \"path\", c->path, 0);\n \tcredential_write_item(fp, \"username\", c->username, 0);\n \tcredential_write_item(fp, \"password\", c->password, 0);\n+\tif (c->password_expiry_utc != TIME_MAX) {\n+\t\tchar *s = xstrfmt(\"%\"PRItime, c->password_expiry_utc);\n+\t\tcredential_write_item(fp, \"password_expiry_utc\", s, 0);\n+\t\tfree(s);\n+\t}\n }\n \n static int run_credential_helper(struct credential *c,\n@@ -342,6 +353,12 @@ void credential_fill(struct credential *c)\n \n \tfor (i = 0; i < c->helpers.nr; i++) {\n \t\tcredential_do(c, c->helpers.items[i].string, \"get\");\n+\t\tif (c->password_expiry_utc < time(NULL)) {\n+\t\t\t/* Discard expired password */\n+\t\t\tFREE_AND_NULL(c->password);\n+\t\t\t/* Reset expiry to maintain consistency */\n+\t\t\tc->password_expiry_utc = TIME_MAX;\n+\t\t}\n \t\tif (c->username && c->password)\n \t\t\treturn;\n \t\tif (c->quit)\n@@ -360,7 +377,7 @@ void credential_approve(struct credential *c)\n \n \tif (c->approved)\n \t\treturn;\n-\tif (!c->username || !c->password)\n+\tif (!c->username || !c->password || c->password_expiry_utc < time(NULL))\n \t\treturn;\n \n \tcredential_apply_config(c);\n@@ -381,6 +398,7 @@ void credential_reject(struct credential *c)\n \n \tFREE_AND_NULL(c->username);\n \tFREE_AND_NULL(c->password);\n+\tc->password_expiry_utc = TIME_MAX;\n \tc->approved = 0;\n }\n \ndiff --git a/credential.h b/credential.h\nindex f430e77fea4..935b28a70f1 100644\n--- a/credential.h\n+++ b/credential.h\n@@ -126,10 +126,12 @@ struct credential {\n \tchar *protocol;\n \tchar *host;\n \tchar *path;\n+\ttimestamp_t password_expiry_utc;\n };\n \n #define CREDENTIAL_INIT { \\\n \t.helpers = STRING_LIST_INIT_DUP, \\\n+\t.password_expiry_utc = TIME_MAX, \\\n }\n \n /* Initialize a credential structure, setting all fields to empty. */\ndiff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\nindex 3485c0534e6..c66d91e82d8 100755\n--- a/t/t0300-credentials.sh\n+++ b/t/t0300-credentials.sh\n@@ -35,6 +35,16 @@ test_expect_success 'setup helper scripts' '\n \ttest -z \"$pass\" || echo password=$pass\n \tEOF\n \n+\twrite_script git-credential-verbatim-with-expiry <<-\\EOF &&\n+\tuser=$1; shift\n+\tpass=$1; shift\n+\tpexpiry=$1; shift\n+\t. ./dump\n+\ttest -z \"$user\" || echo username=$user\n+\ttest -z \"$pass\" || echo password=$pass\n+\ttest -z \"$pexpiry\" || echo password_expiry_utc=$pexpiry\n+\tEOF\n+\n \tPATH=\"$PWD:$PATH\"\n '\n \n@@ -109,6 +119,43 @@ test_expect_success 'credential_fill continues through partial response' '\n \tEOF\n '\n \n+test_expect_success 'credential_fill populates password_expiry_utc' '\n+\tcheck fill \"verbatim-with-expiry one two 9999999999\" <<-\\EOF\n+\tprotocol=http\n+\thost=example.com\n+\t--\n+\tprotocol=http\n+\thost=example.com\n+\tusername=one\n+\tpassword=two\n+\tpassword_expiry_utc=9999999999\n+\t--\n+\tverbatim-with-expiry: get\n+\tverbatim-with-expiry: protocol=http\n+\tverbatim-with-expiry: host=example.com\n+\tEOF\n+'\n+\n+test_expect_success 'credential_fill ignores expired password' '\n+\tcheck fill \"verbatim-with-expiry one two 5\" \"verbatim three four\" <<-\\EOF\n+\tprotocol=http\n+\thost=example.com\n+\t--\n+\tprotocol=http\n+\thost=example.com\n+\tusername=three\n+\tpassword=four\n+\t--\n+\tverbatim-with-expiry: get\n+\tverbatim-with-expiry: protocol=http\n+\tverbatim-with-expiry: host=example.com\n+\tverbatim: get\n+\tverbatim: protocol=http\n+\tverbatim: host=example.com\n+\tverbatim: username=one\n+\tEOF\n+'\n+\n test_expect_success 'credential_fill passes along metadata' '\n \tcheck fill \"verbatim one two\" <<-\\EOF\n \tprotocol=ftp\n@@ -149,6 +196,24 @@ test_expect_success 'credential_approve calls all helpers' '\n \tEOF\n '\n \n+test_expect_success 'credential_approve stores password expiry' '\n+\tcheck approve useless <<-\\EOF\n+\tprotocol=http\n+\thost=example.com\n+\tusername=foo\n+\tpassword=bar\n+\tpassword_expiry_utc=9999999999\n+\t--\n+\t--\n+\tuseless: store\n+\tuseless: protocol=http\n+\tuseless: host=example.com\n+\tuseless: username=foo\n+\tuseless: password=bar\n+\tuseless: password_expiry_utc=9999999999\n+\tEOF\n+'\n+\n test_expect_success 'do not bother storing password-less credential' '\n \tcheck approve useless <<-\\EOF\n \tprotocol=http\n@@ -159,6 +224,17 @@ test_expect_success 'do not bother storing password-less credential' '\n \tEOF\n '\n \n+test_expect_success 'credential_approve does not store expired password' '\n+\tcheck approve useless <<-\\EOF\n+\tprotocol=http\n+\thost=example.com\n+\tusername=foo\n+\tpassword=bar\n+\tpassword_expiry_utc=5\n+\t--\n+\t--\n+\tEOF\n+'\n \n test_expect_success 'credential_reject calls all helpers' '\n \tcheck reject useless \"verbatim one two\" <<-\\EOF\n@@ -181,6 +257,24 @@ test_expect_success 'credential_reject calls all helpers' '\n \tEOF\n '\n \n+test_expect_success 'credential_reject erases credential regardless of expiry' '\n+\tcheck reject useless <<-\\EOF\n+\tprotocol=http\n+\thost=example.com\n+\tusername=foo\n+\tpassword=bar\n+\tpassword_expiry_utc=5\n+\t--\n+\t--\n+\tuseless: erase\n+\tuseless: protocol=http\n+\tuseless: host=example.com\n+\tuseless: username=foo\n+\tuseless: password=bar\n+\tuseless: password_expiry_utc=5\n+\tEOF\n+'\n+\n test_expect_success 'usernames can be preserved' '\n \tcheck fill \"verbatim \\\"\\\" three\" <<-\\EOF\n \tprotocol=http\n\nbase-commit: 2fc9e9ca3c7505bc60069f11e7ef09b1aeeee473\n-- \ngitgitgadget\n"},{"id":"472296","messageId":"CAGJzqskx8+vkYKL6w8Pq98ZJQ3mTv12pZYBkmf=Q_2nB=A8_Sg@mail.gmail.com","threadId":"59159","inReplyTo":"20230216191644.315615-1-calvinwan@google.com","subject":"Re: [PATCH v3] credential: new attribute password_expiry_utc","fromName":"M Hickford","fromEmail":"mirth.hickford@gmail.com","sentAt":"2023-02-18T08:00:00Z","receivedAt":"2023-02-18T08:00:45Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"On Thu, 16 Feb 2023 at 19:16, Calvin Wan wrote:\n>\n> > static int run_credential_helper(struct credential *c,\n> > @@ -342,6 +352,10 @@ void credential_fill(struct credential *c)\n> >\n> > for (i = 0; i < c->helpers.nr; i++) {\n> > credential_do(c, c->helpers.items[i].string, \"get\");\n> > + if (c->password_expiry_utc < time(NULL)) {\n> > + FREE_AND_NULL(c->password);\n> > + c->password_expiry_utc = TIME_MAX;\n> > + }\n> > if (c->username && c->password)\n> > return;\n> > if (c->quit)\n>\n> I see you null out c->password in the expiry if block so that the\n> following c->password check in the following if statement fails.\n> While I think it's neat little trick, I wonder if others on list\n> think it's better to be more explicit with how the logic should\n> work (eg. adding the c->passowrd_expiry_utc check as an inner\n> block inside of the c->username && c->password block).\n\nIt's important to reset the expiry date as well as discard the expired\npassword so that fill accepts a later password without expiry (see\ntest cases). I'll add a comment in patch v4.\n"},{"id":"472297","messageId":"CAGJzqskEnFmH-df5w+9eB8f65cTe3foJv712+q4qCxjPLrh3gw@mail.gmail.com","threadId":"59159","inReplyTo":"xmqqa61ckl6j.fsf@gitster.g","subject":"Re: [PATCH v3] credential: new attribute password_expiry_utc","fromName":"M Hickford","fromEmail":"mirth.hickford@gmail.com","sentAt":"2023-02-18T08:00:00Z","receivedAt":"2023-02-18T08:01:09Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"On Fri, 17 Feb 2023 at 21:59, Junio C Hamano wrote:\n>\n> Lessley Dennington writes:\n>\n> > diff --git a/credential.c b/credential.c\n> > index d3e1bf7a67..b9a9a1d7b1 100644\n> > --- a/credential.c\n> > +++ b/credential.c\n> > @@ -236,6 +236,7 @@ int credential_read(struct credential *c, FILE *fp)\n> > free(c->path);\n> > c->path = xstrdup(value);\n> > } else if (!strcmp(key, \"password_expiry_utc\")) {\n> > + errno = 0;\n> > c->password_expiry_utc = parse_timestamp(value, NULL, 10);\n> > if (c->password_expiry_utc == 0 || errno)\n> > c->password_expiry_utc = TIME_MAX;\n>\n> Ah, that is quite understandable. Successful library function calls\n> would not _clera_ errno, so if there were a failure before the\n> control reaches this codepath, errno may have been set, and then\n> parse_timestamp() call, which would be a strto$some_integral_type() call,\n> may succeed and will leave errno as-is. Your fix is absolutely correct\n> as long as we want to use \"errno\" after the call returns.\n>\n> When strtoumax() etc. wants to report overflow or underflow, the\n> returned value must be UINTMAX_MAX/UINTMAX_MIN and errno would be\n> ERANGE, so it would probably want to check that errno is that value,\n> and/or what c->password_expiry_utc has these overflow/underflow\n> values.\n>\n> > ... I have not confirmed on freebsd,\n> > though, as a heads up.\n>\n\nThat's a subtle one! Thanks Lessley very much for your help debugging.\nI shall send a patch v4 with this and some minor changes discussed in\nreview club.\n"},{"id":"472451","messageId":"CAFySSZA-f+Qgs2bT_Vkj79PvXqGarBLtqeyEN3vWCj44no6Eig@mail.gmail.com","threadId":"59159","inReplyTo":"pull.1443.v4.git.git.1676701977347.gitgitgadget@gmail.com","subject":"Re: [PATCH v4] credential: new attribute password_expiry_utc","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2023-02-22T19:22:02Z","receivedAt":"2023-02-22T19:22:18Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"> static int run_credential_helper(struct credential *c,\n> @@ -342,6 +353,12 @@ void credential_fill(struct credential *c)\n>\n> for (i = 0; i < c->helpers.nr; i++) {\n> credential_do(c, c->helpers.items[i].string, \"get\");\n> + if (c->password_expiry_utc < time(NULL)) {\n> + /* Discard expired password */\n> + FREE_AND_NULL(c->password);\n> + /* Reset expiry to maintain consistency */\n> + c->password_expiry_utc = TIME_MAX;\n> + }\n> if (c->username && c->password)\n> return;\n> if (c->quit)\n\nThanks for clarifying this block!\n\nOverall, this patch is additive and shouldn't cause any\nregressions for current users of credential/credential-helper\nso I'm all for adding an expiry attribute to alleviate the\nuse case pains you described above.\n\nReviewed-by: Calvin Wan \n"}]}