| Message ID | pull.1443.v2.git.git.1675244392025.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] credential: new attribute password_expiry_utc | expand |
On Wed, Feb 01, 2023 at 09:39:51AM +0000, M Hickford via GitGitGadget wrote: > +`password_expiry_utc`:: > + > + If password is a personal access token or OAuth access token, it may have an > + expiry date. When getting credentials from a helper, `git credential fill` > + ignores the password attribute if the expiry date has passed. Storage > + helpers should store this attribute if possible. Helpers should not > + implement expiry logic themselves. Represented as Unix time UTC, seconds > + since 1970. This "should not" seems weird to me. The logic you have here throws out entries that have expired when they pass through Git. But wouldn't helpers which store things want to know about and act on the expiration, too? For example, if Git learns about a credential that expires in 60 seconds and passes it to credential-cache which is configured --timeout=300, wouldn't it want to set its internal ttl on the credential to 60, rather than 300? I think your plan here is that Git would then reject the credential if a request is made at time now+65. But the cache is holding onto it much longer than necessary. Likewise, wouldn't anything that stores credentials at least want to be able to store and regurgitate the expiration? For instance, even credential-store would want to do this. I'm OK if it doesn't, and we can consider it a quality-of-implementation issue and see if anybody cares enough to implement it. But I'd think most "real" helpers would want to do so. So it seems like helpers really do need to support this "expiration" notion. And it's actually Git itself which doesn't need to care about it, assuming the helpers are doing something sensible (though it is OK if Git _also_ throws away expired credentials to support helpers which don't). > diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c > index f3c89831d4a..338058be7f9 100644 > --- a/builtin/credential-cache--daemon.c > +++ b/builtin/credential-cache--daemon.c > @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out) > if (e) { > fprintf(out, "username=%s\n", e->item.username); > fprintf(out, "password=%s\n", e->item.password); > + if (e->item.password_expiry_utc != TIME_MAX) > + fprintf(out, "password_expiry_utc=%"PRItime"\n", > + e->item.password_expiry_utc); > } Is there a particular reason to use TIME_MAX as the sentinel value here, and not just "0"? It's not that big a deal either way, but it's more usual in our code base to use "0" if there's no reason not to (and it seems like nothing should be expiring in 1970 these days). > @@ -195,15 +196,20 @@ static void credential_getpass(struct credential *c) > if (!c->username) > c->username = credential_ask_one("Username", c, > PROMPT_ASKPASS|PROMPT_ECHO); > - if (!c->password) > + if (!c->password || c->password_expiry_utc < time(NULL)) { This is comparing a timestamp_t to a time_t, which may mix signed/unsigned. I can't offhand think of anything that would go too wrong there before 2038, so it's probably OK, but I wanted to call it out. > @@ -225,6 +231,7 @@ int credential_read(struct credential *c, FILE *fp) > } else if (!strcmp(key, "password")) { > free(c->password); > c->password = xstrdup(value); > + password_updated = 1; > } else if (!strcmp(key, "protocol")) { > free(c->protocol); > c->protocol = xstrdup(value); > @@ -234,6 +241,11 @@ int credential_read(struct credential *c, FILE *fp) > } else if (!strcmp(key, "path")) { > free(c->path); > c->path = xstrdup(value); > + } else if (!strcmp(key, "password_expiry_utc")) { > + this_password_expiry = parse_timestamp(value, NULL, 10); > + if (this_password_expiry == 0 || errno) { > + this_password_expiry = TIME_MAX; > + } > } else if (!strcmp(key, "url")) { > credential_from_url(c, value); > } else if (!strcmp(key, "quit")) { > @@ -246,6 +258,9 @@ int credential_read(struct credential *c, FILE *fp) > */ > } > > + if (password_updated) > + c->password_expiry_utc = this_password_expiry; Do we need this logic? It seems weird that a helper would output an expiration but not a password in the first place. I guess ignoring the expiration is probably a reasonable outcome, but I wonder if a helper would ever want to just add an expiration to the data coming from another helper. I.e., could we just read the value directly into c->password_expiry_utc as we do with other fields? -Peff
Jeff King <peff@peff.net> writes: >> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c >> index f3c89831d4a..338058be7f9 100644 >> --- a/builtin/credential-cache--daemon.c >> +++ b/builtin/credential-cache--daemon.c >> @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out) >> if (e) { >> fprintf(out, "username=%s\n", e->item.username); >> fprintf(out, "password=%s\n", e->item.password); >> + if (e->item.password_expiry_utc != TIME_MAX) >> + fprintf(out, "password_expiry_utc=%"PRItime"\n", >> + e->item.password_expiry_utc); >> } > > Is there a particular reason to use TIME_MAX as the sentinel value here, > and not just "0"? It's not that big a deal either way, but it's more > usual in our code base to use "0" if there's no reason not to (and it > seems like nothing should be expiring in 1970 these days). This is my fault ;-). Here, there is no difference between 0 and TIME_MAX, but elsewhere the code needed if (expiry != 0 && expiry < time(NULL)) to see if the entry has expired. If the sentinel for an entry that will never expire were TIME_MAX, you do not need the first half of the expression. I am OK either way.
On 2023-02-01 04:10, Jeff King wrote: > On Wed, Feb 01, 2023 at 09:39:51AM +0000, M Hickford via GitGitGadget wrote: > >> +`password_expiry_utc`:: >> + >> + If password is a personal access token or OAuth access token, it may have an >> + expiry date. When getting credentials from a helper, `git credential fill` >> + ignores the password attribute if the expiry date has passed. Storage >> + helpers should store this attribute if possible. Helpers should not >> + implement expiry logic themselves. Represented as Unix time UTC, seconds >> + since 1970. > > This "should not" seems weird to me. The logic you have here throws out > entries that have expired when they pass through Git. But wouldn't > helpers which store things want to know about and act on the expiration, > too? > > For example, if Git learns about a credential that expires in 60 > seconds and passes it to credential-cache which is configured > --timeout=300, wouldn't it want to set its internal ttl on the > credential to 60, rather than 300? > > I think your plan here is that Git would then reject the credential if a > request is made at time now+65. But the cache is holding onto it much > longer than necessary. > > Likewise, wouldn't anything that stores credentials at least want to be > able to store and regurgitate the expiration? For instance, even > credential-store would want to do this. I'm OK if it doesn't, and we can > consider it a quality-of-implementation issue and see if anybody cares > enough to implement it. But I'd think most "real" helpers would want to > do so. > > So it seems like helpers really do need to support this "expiration" > notion. And it's actually Git itself which doesn't need to care about > it, assuming the helpers are doing something sensible (though it is OK > if Git _also_ throws away expired credentials to support helpers which > don't). I have often wondered about how, and if, Git should handle expiring credentials where the expiration is known. In my opinion I think Git should be doing *less* decision making with credentials and authentication in general, and leave that up to credential helpers. The original design of credential helpers from what I can see (and Peff can correct me here of course!) is that they were really only thought about as storage-style helpers. Helpers are consulted for a known credential, and told about bad (erase) or good (store) credentials, all without any context about the request or remote responses. If no credential helper can respond then Git itself prompts for a user/pass; so Git, or rather the user, is the 'generator'. Of course that's not to say that credential generating helpers don't exist or are wrong - Git Credential Manager being of course one example rather close to home for me! However the current model, even with generating helpers, is still that Git will try and make the request given the details included in the helper response. It doesn't make sense that a generating helper that knows about expiration would instead choose to respond with an expired credential rather than just try and generate a new credential. Now in the case of a simple storage helper without such logic, after returning an expired credential should Git not be calling 'erase' back to the same helper to inform it that it has a stale credential and should be deleted? This would also require some affinity between calls to get/erase/store. >> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c >> index f3c89831d4a..338058be7f9 100644 >> --- a/builtin/credential-cache--daemon.c >> +++ b/builtin/credential-cache--daemon.c >> @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out) >> if (e) { >> fprintf(out, "username=%s\n", e->item.username); >> fprintf(out, "password=%s\n", e->item.password); >> + if (e->item.password_expiry_utc != TIME_MAX) >> + fprintf(out, "password_expiry_utc=%"PRItime"\n", >> + e->item.password_expiry_utc); >> } > > Is there a particular reason to use TIME_MAX as the sentinel value here, > and not just "0"? It's not that big a deal either way, but it's more > usual in our code base to use "0" if there's no reason not to (and it > seems like nothing should be expiring in 1970 these days). > >> @@ -195,15 +196,20 @@ static void credential_getpass(struct credential *c) >> if (!c->username) >> c->username = credential_ask_one("Username", c, >> PROMPT_ASKPASS|PROMPT_ECHO); >> - if (!c->password) >> + if (!c->password || c->password_expiry_utc < time(NULL)) { > > This is comparing a timestamp_t to a time_t, which may mix > signed/unsigned. I can't offhand think of anything that would go too > wrong there before 2038, so it's probably OK, but I wanted to call it > out. > >> @@ -225,6 +231,7 @@ int credential_read(struct credential *c, FILE *fp) >> } else if (!strcmp(key, "password")) { >> free(c->password); >> c->password = xstrdup(value); >> + password_updated = 1; >> } else if (!strcmp(key, "protocol")) { >> free(c->protocol); >> c->protocol = xstrdup(value); >> @@ -234,6 +241,11 @@ int credential_read(struct credential *c, FILE *fp) >> } else if (!strcmp(key, "path")) { >> free(c->path); >> c->path = xstrdup(value); >> + } else if (!strcmp(key, "password_expiry_utc")) { >> + this_password_expiry = parse_timestamp(value, NULL, 10); >> + if (this_password_expiry == 0 || errno) { >> + this_password_expiry = TIME_MAX; >> + } >> } else if (!strcmp(key, "url")) { >> credential_from_url(c, value); >> } else if (!strcmp(key, "quit")) { >> @@ -246,6 +258,9 @@ int credential_read(struct credential *c, FILE *fp) >> */ >> } >> >> + if (password_updated) >> + c->password_expiry_utc = this_password_expiry; > > Do we need this logic? It seems weird that a helper would output an > expiration but not a password in the first place. I guess ignoring the > expiration is probably a reasonable outcome, but I wonder if a helper > would ever want to just add an expiration to the data coming from > another helper. > > I.e., could we just read the value directly into c->password_expiry_utc > as we do with other fields? > > -Peff
On Wed, Feb 01, 2023 at 09:12:01AM -0800, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > >> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c > >> index f3c89831d4a..338058be7f9 100644 > >> --- a/builtin/credential-cache--daemon.c > >> +++ b/builtin/credential-cache--daemon.c > >> @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out) > >> if (e) { > >> fprintf(out, "username=%s\n", e->item.username); > >> fprintf(out, "password=%s\n", e->item.password); > >> + if (e->item.password_expiry_utc != TIME_MAX) > >> + fprintf(out, "password_expiry_utc=%"PRItime"\n", > >> + e->item.password_expiry_utc); > >> } > > > > Is there a particular reason to use TIME_MAX as the sentinel value here, > > and not just "0"? It's not that big a deal either way, but it's more > > usual in our code base to use "0" if there's no reason not to (and it > > seems like nothing should be expiring in 1970 these days). > > This is my fault ;-). Here, there is no difference between 0 and > TIME_MAX, but elsewhere the code needed > > if (expiry != 0 && expiry < time(NULL)) > > to see if the entry has expired. If the sentinel for an entry that > will never expire were TIME_MAX, you do not need the first half of > the expression. > > I am OK either way. Ah. That at least is a compelling reason to use TIME_MAX. I'm OK with it. -Peff
On Wed, Feb 01, 2023 at 12:02:16PM -0800, Matthew John Cheetham wrote: > > So it seems like helpers really do need to support this "expiration" > > notion. And it's actually Git itself which doesn't need to care about > > it, assuming the helpers are doing something sensible (though it is OK > > if Git _also_ throws away expired credentials to support helpers which > > don't). > > I have often wondered about how, and if, Git should handle expiring credentials > where the expiration is known. In my opinion I think Git should be doing > *less* decision making with credentials and authentication in general, and leave > that up to credential helpers. FWIW, that is my general philosophy, too. > The original design of credential helpers from what I can see (and Peff can > correct me here of course!) is that they were really only thought about as > storage-style helpers. Helpers are consulted for a known credential, and told > about bad (erase) or good (store) credentials, all without any context about > the request or remote responses. > > If no credential helper can respond then Git itself prompts for a user/pass; so > Git, or rather the user, is the 'generator'. They were always intended to be generators, too. In the early days we discussed having more fancy graphical prompts via helpers, though most people simply use the askpass interface for this. My personal config for the last, say, 12 years, has been to pull the password out of a read-only store, and ignore "store" and "erase" requests entirely. Which I think counts as a generator. :) But yeah, if there is more context that we can be giving the helpers to let them make a better decision, I'm all for it. I think your www-authenticate patches are a good step in that direction. I'm not sure what other context would be useful. > It doesn't make sense that a generating helper that knows about expiration would > instead choose to respond with an expired credential rather than just try and > generate a new credential. Yeah, agreed. > Now in the case of a simple storage helper without such logic, after returning > an expired credential should Git not be calling 'erase' back to the same helper > to inform it that it has a stale credential and should be deleted? > This would also require some affinity between calls to get/erase/store. That's a good point. I think in practice it would mostly happen that Git would throw away the expired credential, generate a new one (either from another helper or via prompting), and then if that works, overwrite the old one with a 'store' request. If the new credential doesn't work (or the user aborts), the expired credential is left in the helper. But in theory that doesn't matter. It will still be expired when it's served up again later. And it's not a security problem to hold onto it longer, since it's no longer valid. Still, it feels a bit clunky compared to having the helper realize it's holding garbage. -Peff
Thanks Jeff for the review On Wed, 1 Feb 2023 at 12:10, Jeff King <peff@peff.net> wrote: > > On Wed, Feb 01, 2023 at 09:39:51AM +0000, M Hickford via GitGitGadget wrote: > > > +`password_expiry_utc`:: > > + > > + If password is a personal access token or OAuth access token, it may have an > > + expiry date. When getting credentials from a helper, `git credential fill` > > + ignores the password attribute if the expiry date has passed. Storage > > + helpers should store this attribute if possible. Helpers should not > > + implement expiry logic themselves. Represented as Unix time UTC, seconds > > + since 1970. > > This "should not" seems weird to me. The logic you have here throws out > entries that have expired when they pass through Git. But wouldn't > helpers which store things want to know about and act on the expiration, > too? I wanted to keep the helper contract as simple as possible "here is an attribute to store and retrieve like any other". It doesn't matter if a helper stores a password beyond expiry, because Git will erase or replace it when it fails authentication or another succeeds. Relax the language in the patch v3 commit message to describe "self-pruning of expired passwords is unnecessary but harmless". > > For example, if Git learns about a credential that expires in 60 > seconds and passes it to credential-cache which is configured > --timeout=300, wouldn't it want to set its internal ttl on the > credential to 60, rather than 300? > > I think your plan here is that Git would then reject the credential if a > request is made at time now+65. But the cache is holding onto it much > longer than necessary. Even after the password expires, there's value to storing other attributes such as username and (perhaps in future) oauth-refresh-token. Hence I named the attribute 'password_expiry_utc' explicitly rather than 'credential_expiry_utc'. > > Likewise, wouldn't anything that stores credentials at least want to be > able to store and regurgitate the expiration? For instance, even > credential-store would want to do this. I'm OK if it doesn't, and we can > consider it a quality-of-implementation issue and see if anybody cares > enough to implement it. But I'd think most "real" helpers would want to > do so. Absolutely. Eventually I'd like to see support in git-credential-osxkeychain, git-credential-wincred, git-credential-libsecret etc. Mentioned this in patch v3 description. > > So it seems like helpers really do need to support this "expiration" > notion. And it's actually Git itself which doesn't need to care about > it, assuming the helpers are doing something sensible (though it is OK > if Git _also_ throws away expired credentials to support helpers which > don't). > > > diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c > > index f3c89831d4a..338058be7f9 100644 > > --- a/builtin/credential-cache--daemon.c > > +++ b/builtin/credential-cache--daemon.c > > @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out) > > if (e) { > > fprintf(out, "username=%s\n", e->item.username); > > fprintf(out, "password=%s\n", e->item.password); > > + if (e->item.password_expiry_utc != TIME_MAX) > > + fprintf(out, "password_expiry_utc=%"PRItime"\n", > > + e->item.password_expiry_utc); > > } > > Is there a particular reason to use TIME_MAX as the sentinel value here, > and not just "0"? It's not that big a deal either way, but it's more > usual in our code base to use "0" if there's no reason not to (and it > seems like nothing should be expiring in 1970 these days). Junio made a persuasive argument for readability https://lore.kernel.org/git/CAPig+cQPLMrUKp0aqLCknSYCs5TAso-VSBYsQbGZ8g8wgY2Liw@mail.gmail.com/T/#mf66955cf3f53c073c68ad5ade7213617907bab63 > > > @@ -195,15 +196,20 @@ static void credential_getpass(struct credential *c) > > if (!c->username) > > c->username = credential_ask_one("Username", c, > > PROMPT_ASKPASS|PROMPT_ECHO); > > - if (!c->password) > > + if (!c->password || c->password_expiry_utc < time(NULL)) { > > This is comparing a timestamp_t to a time_t, which may mix > signed/unsigned. I can't offhand think of anything that would go too > wrong there before 2038, so it's probably OK, but I wanted to call it > out. > > > @@ -225,6 +231,7 @@ int credential_read(struct credential *c, FILE *fp) > > } else if (!strcmp(key, "password")) { > > free(c->password); > > c->password = xstrdup(value); > > + password_updated = 1; > > } else if (!strcmp(key, "protocol")) { > > free(c->protocol); > > c->protocol = xstrdup(value); > > @@ -234,6 +241,11 @@ int credential_read(struct credential *c, FILE *fp) > > } else if (!strcmp(key, "path")) { > > free(c->path); > > c->path = xstrdup(value); > > + } else if (!strcmp(key, "password_expiry_utc")) { > > + this_password_expiry = parse_timestamp(value, NULL, 10); > > + if (this_password_expiry == 0 || errno) { > > + this_password_expiry = TIME_MAX; > > + } > > } else if (!strcmp(key, "url")) { > > credential_from_url(c, value); > > } else if (!strcmp(key, "quit")) { > > @@ -246,6 +258,9 @@ int credential_read(struct credential *c, FILE *fp) > > */ > > } > > > > + if (password_updated) > > + c->password_expiry_utc = this_password_expiry; > > Do we need this logic? It seems weird that a helper would output an > expiration but not a password in the first place. I guess ignoring the > expiration is probably a reasonable outcome, but I wonder if a helper > would ever want to just add an expiration to the data coming from > another helper. > I.e., could we just read the value directly into c->password_expiry_utc > as we do with other fields? Done in patch v3. The logic that remains (moved to credential_fill) is a simple sanity check. > > -Peff
On Wed, 1 Feb 2023 at 20:02, Matthew John Cheetham <mjcheetham@outlook.com> wrote: > > On 2023-02-01 04:10, Jeff King wrote: > > > On Wed, Feb 01, 2023 at 09:39:51AM +0000, M Hickford via GitGitGadget wrote: > > > >> +`password_expiry_utc`:: > >> + > >> + If password is a personal access token or OAuth access token, it may have an > >> + expiry date. When getting credentials from a helper, `git credential fill` > >> + ignores the password attribute if the expiry date has passed. Storage > >> + helpers should store this attribute if possible. Helpers should not > >> + implement expiry logic themselves. Represented as Unix time UTC, seconds > >> + since 1970. > > > > This "should not" seems weird to me. The logic you have here throws out > > entries that have expired when they pass through Git. But wouldn't > > helpers which store things want to know about and act on the expiration, > > too? > > > > For example, if Git learns about a credential that expires in 60 > > seconds and passes it to credential-cache which is configured > > --timeout=300, wouldn't it want to set its internal ttl on the > > credential to 60, rather than 300? > > > > I think your plan here is that Git would then reject the credential if a > > request is made at time now+65. But the cache is holding onto it much > > longer than necessary. > > > > Likewise, wouldn't anything that stores credentials at least want to be > > able to store and regurgitate the expiration? For instance, even > > credential-store would want to do this. I'm OK if it doesn't, and we can > > consider it a quality-of-implementation issue and see if anybody cares > > enough to implement it. But I'd think most "real" helpers would want to > > do so. > > > > So it seems like helpers really do need to support this "expiration" > > notion. And it's actually Git itself which doesn't need to care about > > it, assuming the helpers are doing something sensible (though it is OK > > if Git _also_ throws away expired credentials to support helpers which > > don't). > > I have often wondered about how, and if, Git should handle expiring credentials > where the expiration is known. In my opinion I think Git should be doing > *less* decision making with credentials and authentication in general, and leave > that up to credential helpers. > > The original design of credential helpers from what I can see (and Peff can > correct me here of course!) is that they were really only thought about as > storage-style helpers. Helpers are consulted for a known credential, and told > about bad (erase) or good (store) credentials, all without any context about > the request or remote responses. > > If no credential helper can respond then Git itself prompts for a user/pass; so > Git, or rather the user, is the 'generator'. > > Of course that's not to say that credential generating helpers don't exist or > are wrong - Git Credential Manager being of course one example rather close to > home for me! However the current model, even with generating helpers, is still > that Git will try and make the request given the details included in the helper > response. GCM would benefit from being able to store expiry too. Whenever GCM retrieves an OAuth credential from storage, it queries the server to check whether the access token has expired [1]. This would become unnecessary. I've added more about this in patch v3 commit message. Further, it solves a problem if GCM is configured after another storage helper: ``` [credential] helper = storage # eg. osx-keychain or exotic helper = manager ``` Currently this may return an expired credential from storage. Background for others: GCM is typically configured as the *only* helper, with its own internal storage configuration [2]. These reimplement or wrap popular Git storage helpers [3][4][5]. ``` [credential] helper = manager credentialStore = keychain ``` [1] https://github.com/GitCredentialManager/git-credential-manager/blob/main/src/shared/GitLab/GitLabHostProvider.cs [2] https://github.com/GitCredentialManager/git-credential-manager/blob/main/docs/credstores.md [3] https://github.com/GitCredentialManager/git-credential-manager/blob/main/src/shared/Core/Interop/MacOS/MacOSKeychain.cs [4] https://github.com/GitCredentialManager/git-credential-manager/blob/main/src/shared/Core/Interop/Linux/SecretServiceCollection.cs [5] https://github.com/GitCredentialManager/git-credential-manager/blob/main/src/shared/Core/CredentialCacheStore.cs > > It doesn't make sense that a generating helper that knows about expiration would > instead choose to respond with an expired credential rather than just try and > generate a new credential. > > Now in the case of a simple storage helper without such logic, after returning > an expired credential should Git not be calling 'erase' back to the same helper > to inform it that it has a stale credential and should be deleted? > This would also require some affinity between calls to get/erase/store. > > > >> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c > >> index f3c89831d4a..338058be7f9 100644 > >> --- a/builtin/credential-cache--daemon.c > >> +++ b/builtin/credential-cache--daemon.c > >> @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out) > >> if (e) { > >> fprintf(out, "username=%s\n", e->item.username); > >> fprintf(out, "password=%s\n", e->item.password); > >> + if (e->item.password_expiry_utc != TIME_MAX) > >> + fprintf(out, "password_expiry_utc=%"PRItime"\n", > >> + e->item.password_expiry_utc); > >> } > > > > Is there a particular reason to use TIME_MAX as the sentinel value here, > > and not just "0"? It's not that big a deal either way, but it's more > > usual in our code base to use "0" if there's no reason not to (and it > > seems like nothing should be expiring in 1970 these days). > > > >> @@ -195,15 +196,20 @@ static void credential_getpass(struct credential *c) > >> if (!c->username) > >> c->username = credential_ask_one("Username", c, > >> PROMPT_ASKPASS|PROMPT_ECHO); > >> - if (!c->password) > >> + if (!c->password || c->password_expiry_utc < time(NULL)) { > > > > This is comparing a timestamp_t to a time_t, which may mix > > signed/unsigned. I can't offhand think of anything that would go too > > wrong there before 2038, so it's probably OK, but I wanted to call it > > out. > > > >> @@ -225,6 +231,7 @@ int credential_read(struct credential *c, FILE *fp) > >> } else if (!strcmp(key, "password")) { > >> free(c->password); > >> c->password = xstrdup(value); > >> + password_updated = 1; > >> } else if (!strcmp(key, "protocol")) { > >> free(c->protocol); > >> c->protocol = xstrdup(value); > >> @@ -234,6 +241,11 @@ int credential_read(struct credential *c, FILE *fp) > >> } else if (!strcmp(key, "path")) { > >> free(c->path); > >> c->path = xstrdup(value); > >> + } else if (!strcmp(key, "password_expiry_utc")) { > >> + this_password_expiry = parse_timestamp(value, NULL, 10); > >> + if (this_password_expiry == 0 || errno) { > >> + this_password_expiry = TIME_MAX; > >> + } > >> } else if (!strcmp(key, "url")) { > >> credential_from_url(c, value); > >> } else if (!strcmp(key, "quit")) { > >> @@ -246,6 +258,9 @@ int credential_read(struct credential *c, FILE *fp) > >> */ > >> } > >> > >> + if (password_updated) > >> + c->password_expiry_utc = this_password_expiry; > > > > Do we need this logic? It seems weird that a helper would output an > > expiration but not a password in the first place. I guess ignoring the > > expiration is probably a reasonable outcome, but I wonder if a helper > > would ever want to just add an expiration to the data coming from > > another helper. > > > > I.e., could we just read the value directly into c->password_expiry_utc > > as we do with other fields? > > > > -Peff
On 2023-02-04 22:45, M Hickford wrote: > On Wed, 1 Feb 2023 at 20:02, Matthew John Cheetham > <mjcheetham@outlook.com> wrote: >> >> On 2023-02-01 04:10, Jeff King wrote: >> >>> On Wed, Feb 01, 2023 at 09:39:51AM +0000, M Hickford via GitGitGadget wrote: >>> >>>> +`password_expiry_utc`:: >>>> + >>>> + If password is a personal access token or OAuth access token, it may have an >>>> + expiry date. When getting credentials from a helper, `git credential fill` >>>> + ignores the password attribute if the expiry date has passed. Storage >>>> + helpers should store this attribute if possible. Helpers should not >>>> + implement expiry logic themselves. Represented as Unix time UTC, seconds >>>> + since 1970. >>> >>> This "should not" seems weird to me. The logic you have here throws out >>> entries that have expired when they pass through Git. But wouldn't >>> helpers which store things want to know about and act on the expiration, >>> too? >>> >>> For example, if Git learns about a credential that expires in 60 >>> seconds and passes it to credential-cache which is configured >>> --timeout=300, wouldn't it want to set its internal ttl on the >>> credential to 60, rather than 300? >>> >>> I think your plan here is that Git would then reject the credential if a >>> request is made at time now+65. But the cache is holding onto it much >>> longer than necessary. >>> >>> Likewise, wouldn't anything that stores credentials at least want to be >>> able to store and regurgitate the expiration? For instance, even >>> credential-store would want to do this. I'm OK if it doesn't, and we can >>> consider it a quality-of-implementation issue and see if anybody cares >>> enough to implement it. But I'd think most "real" helpers would want to >>> do so. >>> >>> So it seems like helpers really do need to support this "expiration" >>> notion. And it's actually Git itself which doesn't need to care about >>> it, assuming the helpers are doing something sensible (though it is OK >>> if Git _also_ throws away expired credentials to support helpers which >>> don't). >> >> I have often wondered about how, and if, Git should handle expiring credentials >> where the expiration is known. In my opinion I think Git should be doing >> *less* decision making with credentials and authentication in general, and leave >> that up to credential helpers. >> >> The original design of credential helpers from what I can see (and Peff can >> correct me here of course!) is that they were really only thought about as >> storage-style helpers. Helpers are consulted for a known credential, and told >> about bad (erase) or good (store) credentials, all without any context about >> the request or remote responses. >> >> If no credential helper can respond then Git itself prompts for a user/pass; so >> Git, or rather the user, is the 'generator'. >> >> Of course that's not to say that credential generating helpers don't exist or >> are wrong - Git Credential Manager being of course one example rather close to >> home for me! However the current model, even with generating helpers, is still >> that Git will try and make the request given the details included in the helper >> response. > > GCM would benefit from being able to store expiry too. Whenever GCM > retrieves an OAuth credential from storage, it queries the server to > check whether the access token has expired [1]. This would become > unnecessary. I've added more about this in patch v3 commit message. > > Further, it solves a problem if GCM is configured after another storage helper: > > ``` > [credential] > helper = storage # eg. osx-keychain or exotic > helper = manager > ``` > > Currently this may return an expired credential from storage. > > Background for others: GCM is typically configured as the *only* > helper, with its own internal storage configuration [2]. These > reimplement or wrap popular Git storage helpers [3][4][5]. > > ``` > [credential] > helper = manager > credentialStore = keychain > ``` One of the things that concerns me about the credential helper system today is the lack of 'affinity' across calls, and I think that we may disagree on this here. A desire I have for the future of Git auth is that helpers can negotiate to start a more long-lived or complicated converstation, with retry and detailed feedback on the auth responses. I'm increasingly feeling that the get/store/erase model is not sufficient for this. Imagine the scenario where the auth mechanism has a nonce that is updated on each successful (or failed) response. Here we'd want the helper that offered credentials to be told about the successful response for book-keeping, and not have that message 'stolen' by a simpler storage helper. Another scenario would be multiple user accounts; a helper has credentials that would potentially be valid for the request (same host/forge or IdP for example), and we're wanting to avoid unnecessary user prompts as the user has not specified explicitly the account to 'bind' to that clone or specific remote. Why could we not return credentials to Git, also indicating that we have an ability to retry on a 401/403? Increasingly, modern auth schemes have credentials that are so short lived (maybe bound to the exact specific request.. a one time use) that it really doesn't make sense to store any credentials at all. With such one-time-use credentials, and a simple storage helper ahead of a generating helper every other request would fail and the user would need to retry the command. Imagine the case that the OS/hardware security device holds the 'real' key or credential that is used to derive a one-time-use credential. Strong auth mechamisms often tie credential generation strongly to storage. > [1] https://github.com/GitCredentialManager/git-credential-manager/blob/main/src/shared/GitLab/GitLabHostProvider.cs > [2] https://github.com/GitCredentialManager/git-credential-manager/blob/main/docs/credstores.md > [3] https://github.com/GitCredentialManager/git-credential-manager/blob/main/src/shared/Core/Interop/MacOS/MacOSKeychain.cs > [4] https://github.com/GitCredentialManager/git-credential-manager/blob/main/src/shared/Core/Interop/Linux/SecretServiceCollection.cs > [5] https://github.com/GitCredentialManager/git-credential-manager/blob/main/src/shared/Core/CredentialCacheStore.cs > >> >> It doesn't make sense that a generating helper that knows about expiration would >> instead choose to respond with an expired credential rather than just try and >> generate a new credential. >> >> Now in the case of a simple storage helper without such logic, after returning >> an expired credential should Git not be calling 'erase' back to the same helper >> to inform it that it has a stale credential and should be deleted? >> This would also require some affinity between calls to get/erase/store. >> >> >>>> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c >>>> index f3c89831d4a..338058be7f9 100644 >>>> --- a/builtin/credential-cache--daemon.c >>>> +++ b/builtin/credential-cache--daemon.c >>>> @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out) >>>> if (e) { >>>> fprintf(out, "username=%s\n", e->item.username); >>>> fprintf(out, "password=%s\n", e->item.password); >>>> + if (e->item.password_expiry_utc != TIME_MAX) >>>> + fprintf(out, "password_expiry_utc=%"PRItime"\n", >>>> + e->item.password_expiry_utc); >>>> } >>> >>> Is there a particular reason to use TIME_MAX as the sentinel value here, >>> and not just "0"? It's not that big a deal either way, but it's more >>> usual in our code base to use "0" if there's no reason not to (and it >>> seems like nothing should be expiring in 1970 these days). >>> >>>> @@ -195,15 +196,20 @@ static void credential_getpass(struct credential *c) >>>> if (!c->username) >>>> c->username = credential_ask_one("Username", c, >>>> PROMPT_ASKPASS|PROMPT_ECHO); >>>> - if (!c->password) >>>> + if (!c->password || c->password_expiry_utc < time(NULL)) { >>> >>> This is comparing a timestamp_t to a time_t, which may mix >>> signed/unsigned. I can't offhand think of anything that would go too >>> wrong there before 2038, so it's probably OK, but I wanted to call it >>> out. >>> >>>> @@ -225,6 +231,7 @@ int credential_read(struct credential *c, FILE *fp) >>>> } else if (!strcmp(key, "password")) { >>>> free(c->password); >>>> c->password = xstrdup(value); >>>> + password_updated = 1; >>>> } else if (!strcmp(key, "protocol")) { >>>> free(c->protocol); >>>> c->protocol = xstrdup(value); >>>> @@ -234,6 +241,11 @@ int credential_read(struct credential *c, FILE *fp) >>>> } else if (!strcmp(key, "path")) { >>>> free(c->path); >>>> c->path = xstrdup(value); >>>> + } else if (!strcmp(key, "password_expiry_utc")) { >>>> + this_password_expiry = parse_timestamp(value, NULL, 10); >>>> + if (this_password_expiry == 0 || errno) { >>>> + this_password_expiry = TIME_MAX; >>>> + } >>>> } else if (!strcmp(key, "url")) { >>>> credential_from_url(c, value); >>>> } else if (!strcmp(key, "quit")) { >>>> @@ -246,6 +258,9 @@ int credential_read(struct credential *c, FILE *fp) >>>> */ >>>> } >>>> >>>> + if (password_updated) >>>> + c->password_expiry_utc = this_password_expiry; >>> >>> Do we need this logic? It seems weird that a helper would output an >>> expiration but not a password in the first place. I guess ignoring the >>> expiration is probably a reasonable outcome, but I wonder if a helper >>> would ever want to just add an expiration to the data coming from >>> another helper. >>> >>> I.e., could we just read the value directly into c->password_expiry_utc >>> as we do with other fields? >>> >>> -Peff
diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt index ac2818b9f66..667c4d80e26 100644 --- a/Documentation/git-credential.txt +++ b/Documentation/git-credential.txt @@ -144,6 +144,15 @@ Git understands the following attributes: The credential's password, if we are asking it to be stored. +`password_expiry_utc`:: + + If password is a personal access token or OAuth access token, it may have an + expiry date. When getting credentials from a helper, `git credential fill` + ignores the password attribute if the expiry date has passed. Storage + helpers should store this attribute if possible. Helpers should not + implement expiry logic themselves. Represented as Unix time UTC, seconds + since 1970. + `url`:: When this special attribute is read by `git credential`, the diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c index f3c89831d4a..338058be7f9 100644 --- a/builtin/credential-cache--daemon.c +++ b/builtin/credential-cache--daemon.c @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out) if (e) { fprintf(out, "username=%s\n", e->item.username); fprintf(out, "password=%s\n", e->item.password); + if (e->item.password_expiry_utc != TIME_MAX) + fprintf(out, "password_expiry_utc=%"PRItime"\n", + e->item.password_expiry_utc); } } else if (!strcmp(action.buf, "exit")) { diff --git a/credential.c b/credential.c index f6389a50684..354fa1652a9 100644 --- a/credential.c +++ b/credential.c @@ -7,6 +7,7 @@ #include "prompt.h" #include "sigchain.h" #include "urlmatch.h" +#include "git-compat-util.h" void credential_init(struct credential *c) { @@ -195,15 +196,20 @@ static void credential_getpass(struct credential *c) if (!c->username) c->username = credential_ask_one("Username", c, PROMPT_ASKPASS|PROMPT_ECHO); - if (!c->password) + if (!c->password || c->password_expiry_utc < time(NULL)) { + c->password_expiry_utc = TIME_MAX; c->password = credential_ask_one("Password", c, PROMPT_ASKPASS); + } } int credential_read(struct credential *c, FILE *fp) { struct strbuf line = STRBUF_INIT; + int password_updated = 0; + timestamp_t this_password_expiry = TIME_MAX; + while (strbuf_getline(&line, fp) != EOF) { char *key = line.buf; char *value = strchr(key, '='); @@ -225,6 +231,7 @@ int credential_read(struct credential *c, FILE *fp) } else if (!strcmp(key, "password")) { free(c->password); c->password = xstrdup(value); + password_updated = 1; } else if (!strcmp(key, "protocol")) { free(c->protocol); c->protocol = xstrdup(value); @@ -234,6 +241,11 @@ int credential_read(struct credential *c, FILE *fp) } else if (!strcmp(key, "path")) { free(c->path); c->path = xstrdup(value); + } else if (!strcmp(key, "password_expiry_utc")) { + this_password_expiry = parse_timestamp(value, NULL, 10); + if (this_password_expiry == 0 || errno) { + this_password_expiry = TIME_MAX; + } } else if (!strcmp(key, "url")) { credential_from_url(c, value); } else if (!strcmp(key, "quit")) { @@ -246,6 +258,9 @@ int credential_read(struct credential *c, FILE *fp) */ } + if (password_updated) + c->password_expiry_utc = this_password_expiry; + strbuf_release(&line); return 0; } @@ -269,6 +284,11 @@ void credential_write(const struct credential *c, FILE *fp) credential_write_item(fp, "path", c->path, 0); credential_write_item(fp, "username", c->username, 0); credential_write_item(fp, "password", c->password, 0); + if (c->password_expiry_utc != TIME_MAX) { + char *s = xstrfmt("%"PRItime, c->password_expiry_utc); + credential_write_item(fp, "password_expiry_utc", s, 0); + free(s); + } } static int run_credential_helper(struct credential *c, @@ -342,7 +362,7 @@ void credential_fill(struct credential *c) for (i = 0; i < c->helpers.nr; i++) { credential_do(c, c->helpers.items[i].string, "get"); - if (c->username && c->password) + if (c->username && c->password && time(NULL) < c->password_expiry_utc) return; if (c->quit) die("credential helper '%s' told us to quit", diff --git a/credential.h b/credential.h index f430e77fea4..935b28a70f1 100644 --- a/credential.h +++ b/credential.h @@ -126,10 +126,12 @@ struct credential { char *protocol; char *host; char *path; + timestamp_t password_expiry_utc; }; #define CREDENTIAL_INIT { \ .helpers = STRING_LIST_INIT_DUP, \ + .password_expiry_utc = TIME_MAX, \ } /* Initialize a credential structure, setting all fields to empty. */