| Message ID | pull.1443.v3.git.git.1675545372271.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] credential: new attribute password_expiry_utc | expand |
"M Hickford via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: M Hickford <mirth.hickford@gmail.com> > > Some passwords have an expiry date known at generation. This may be > years away for a personal access token or hours for an OAuth access > token. > ... > t/t0300-credentials.sh | 94 ++++++++++++++++++++++++++++++ https://github.com/git/git/actions/runs/4169057114/jobs/7217377625 Other platforms seem to be OK, but Windows test seems to be unhappy with it when this topic gets merged to 'seen'.
On Sat, 4 Feb 2023 at 23:03, M Hickford via GitGitGadget <gitgitgadget@gmail.com> wrote: > --- a/Documentation/gitcredentials.txt > +++ b/Documentation/gitcredentials.txt > @@ -167,7 +167,7 @@ helper:: > If there are multiple instances of the `credential.helper` configuration > variable, each helper will be tried in turn, and may provide a username, > password, or nothing. Once Git has acquired both a username and a > -password, no more helpers will be tried. > +unexpired password, no more helpers will be tried. s/a unexpired/an unexpired/ (or "a non-expired", perhaps) Martin
On Tue, 14 Feb 2023 at 01:59, Junio C Hamano <gitster@pobox.com> wrote: > > "M Hickford via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: M Hickford <mirth.hickford@gmail.com> > > > > Some passwords have an expiry date known at generation. This may be > > years away for a personal access token or hours for an OAuth access > > token. > > ... > > t/t0300-credentials.sh | 94 ++++++++++++++++++++++++++++++ > > https://github.com/git/git/actions/runs/4169057114/jobs/7217377625 > > Other platforms seem to be OK, but Windows test seems to be unhappy > with it when this topic gets merged to 'seen'. Curious, let me take a look. I see that the tests failed on freebsd too. I don't have a Windows machine to debug. If anyone reading has a hypothesis, please share. I shall try changing the default value for a password without expiry from TIME_MAX to 0, see if that works any better [2]. [1] https://github.com/git/git/pull/1443/checks?check_run_id=11112315291 [2] https://github.com/git/git/pull/1443/checks?check_run_id=11343677685
> static int run_credential_helper(struct credential *c, > @@ -342,6 +352,10 @@ void credential_fill(struct credential *c) > > for (i = 0; i < c->helpers.nr; i++) { > credential_do(c, c->helpers.items[i].string, "get"); > + if (c->password_expiry_utc < time(NULL)) { > + FREE_AND_NULL(c->password); > + c->password_expiry_utc = TIME_MAX; > + } > if (c->username && c->password) > return; > if (c->quit) I see you null out c->password in the expiry if block so that the following c->password check in the following if statement fails. While I think it's neat little trick, I wonder if others on list think it's better to be more explicit with how the logic should work (eg. adding the c->passowrd_expiry_utc check as an inner block inside of the c->username && c->password block).
On 2/14/23 3:36 PM, M Hickford wrote: > Curious, let me take a look. I see that the tests failed on freebsd too. > I was curious about this as well and took a look on my Windows machine. It appears that errno will be set to 'No such file or directory' unless you explicitly set it before checking it. This is odd, given that the helper script I wrote worked just fine without this requirement. However, I took a look through the rest of the codebase and noticed that `errno = 0` seems to always be declared before this type of conditional check. That did indeed fix the issue - tests all pass on Windows with this patch. I have not confirmed on freebsd, though, as a heads up. Thanks, Lessley --- diff --git a/credential.c b/credential.c index d3e1bf7a67..b9a9a1d7b1 100644 --- a/credential.c +++ b/credential.c @@ -236,6 +236,7 @@ int credential_read(struct credential *c, FILE *fp) free(c->path); c->path = xstrdup(value); } else if (!strcmp(key, "password_expiry_utc")) { + errno = 0; c->password_expiry_utc = parse_timestamp(value, NULL, 10); if (c->password_expiry_utc == 0 || errno) c->password_expiry_utc = TIME_MAX;
Lessley Dennington <lessleydennington@gmail.com> writes: > diff --git a/credential.c b/credential.c > index d3e1bf7a67..b9a9a1d7b1 100644 > --- a/credential.c > +++ b/credential.c > @@ -236,6 +236,7 @@ int credential_read(struct credential *c, FILE *fp) > free(c->path); > c->path = xstrdup(value); > } else if (!strcmp(key, "password_expiry_utc")) { > + errno = 0; > c->password_expiry_utc = parse_timestamp(value, NULL, 10); > if (c->password_expiry_utc == 0 || errno) > c->password_expiry_utc = TIME_MAX; Ah, that is quite understandable. Successful library function calls would not _clera_ errno, so if there were a failure before the control reaches this codepath, errno may have been set, and then parse_timestamp() call, which would be a strto$some_integral_type() call, may succeed and will leave errno as-is. Your fix is absolutely correct as long as we want to use "errno" after the call returns. When strtoumax() etc. wants to report overflow or underflow, the returned value must be UINTMAX_MAX/UINTMAX_MIN and errno would be ERANGE, so it would probably want to check that errno is that value, and/or what c->password_expiry_utc has these overflow/underflow values. > ... I have not confirmed on freebsd, > though, as a heads up.
On Thu, 16 Feb 2023 at 19:16, Calvin Wan <calvinwan@google.com> wrote: > > > static int run_credential_helper(struct credential *c, > > @@ -342,6 +352,10 @@ void credential_fill(struct credential *c) > > > > for (i = 0; i < c->helpers.nr; i++) { > > credential_do(c, c->helpers.items[i].string, "get"); > > + if (c->password_expiry_utc < time(NULL)) { > > + FREE_AND_NULL(c->password); > > + c->password_expiry_utc = TIME_MAX; > > + } > > if (c->username && c->password) > > return; > > if (c->quit) > > I see you null out c->password in the expiry if block so that the > following c->password check in the following if statement fails. > While I think it's neat little trick, I wonder if others on list > think it's better to be more explicit with how the logic should > work (eg. adding the c->passowrd_expiry_utc check as an inner > block inside of the c->username && c->password block). It's important to reset the expiry date as well as discard the expired password so that fill accepts a later password without expiry (see test cases). I'll add a comment in patch v4.
On Fri, 17 Feb 2023 at 21:59, Junio C Hamano <gitster@pobox.com> wrote: > > Lessley Dennington <lessleydennington@gmail.com> writes: > > > diff --git a/credential.c b/credential.c > > index d3e1bf7a67..b9a9a1d7b1 100644 > > --- a/credential.c > > +++ b/credential.c > > @@ -236,6 +236,7 @@ int credential_read(struct credential *c, FILE *fp) > > free(c->path); > > c->path = xstrdup(value); > > } else if (!strcmp(key, "password_expiry_utc")) { > > + errno = 0; > > c->password_expiry_utc = parse_timestamp(value, NULL, 10); > > if (c->password_expiry_utc == 0 || errno) > > c->password_expiry_utc = TIME_MAX; > > Ah, that is quite understandable. Successful library function calls > would not _clera_ errno, so if there were a failure before the > control reaches this codepath, errno may have been set, and then > parse_timestamp() call, which would be a strto$some_integral_type() call, > may succeed and will leave errno as-is. Your fix is absolutely correct > as long as we want to use "errno" after the call returns. > > When strtoumax() etc. wants to report overflow or underflow, the > returned value must be UINTMAX_MAX/UINTMAX_MIN and errno would be > ERANGE, so it would probably want to check that errno is that value, > and/or what c->password_expiry_utc has these overflow/underflow > values. > > > ... I have not confirmed on freebsd, > > though, as a heads up. > That's a subtle one! Thanks Lessley very much for your help debugging. I shall send a patch v4 with this and some minor changes discussed in review club.
diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt index ac2818b9f66..29d184ab824 100644 --- a/Documentation/git-credential.txt +++ b/Documentation/git-credential.txt @@ -144,6 +144,12 @@ Git understands the following attributes: The credential's password, if we are asking it to be stored. +`password_expiry_utc`:: + + Generated passwords such as an OAuth access token may have an expiry date. + When reading credentials from helpers, `git credential fill` ignores expired + passwords. Represented as Unix time UTC, seconds since 1970. + `url`:: When this special attribute is read by `git credential`, the diff --git a/Documentation/gitcredentials.txt b/Documentation/gitcredentials.txt index 4522471c337..95636b18439 100644 --- a/Documentation/gitcredentials.txt +++ b/Documentation/gitcredentials.txt @@ -167,7 +167,7 @@ helper:: If there are multiple instances of the `credential.helper` configuration variable, each helper will be tried in turn, and may provide a username, password, or nothing. Once Git has acquired both a username and a -password, no more helpers will be tried. +unexpired password, no more helpers will be tried. + If `credential.helper` is configured to the empty string, this resets the helper list to empty (so you may override a helper set by a diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c index f3c89831d4a..338058be7f9 100644 --- a/builtin/credential-cache--daemon.c +++ b/builtin/credential-cache--daemon.c @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out) if (e) { fprintf(out, "username=%s\n", e->item.username); fprintf(out, "password=%s\n", e->item.password); + if (e->item.password_expiry_utc != TIME_MAX) + fprintf(out, "password_expiry_utc=%"PRItime"\n", + e->item.password_expiry_utc); } } else if (!strcmp(action.buf, "exit")) { diff --git a/credential.c b/credential.c index f6389a50684..d3e1bf7a679 100644 --- a/credential.c +++ b/credential.c @@ -7,6 +7,7 @@ #include "prompt.h" #include "sigchain.h" #include "urlmatch.h" +#include "git-compat-util.h" void credential_init(struct credential *c) { @@ -234,6 +235,10 @@ int credential_read(struct credential *c, FILE *fp) } else if (!strcmp(key, "path")) { free(c->path); c->path = xstrdup(value); + } else if (!strcmp(key, "password_expiry_utc")) { + c->password_expiry_utc = parse_timestamp(value, NULL, 10); + if (c->password_expiry_utc == 0 || errno) + c->password_expiry_utc = TIME_MAX; } else if (!strcmp(key, "url")) { credential_from_url(c, value); } else if (!strcmp(key, "quit")) { @@ -269,6 +274,11 @@ void credential_write(const struct credential *c, FILE *fp) credential_write_item(fp, "path", c->path, 0); credential_write_item(fp, "username", c->username, 0); credential_write_item(fp, "password", c->password, 0); + if (c->password_expiry_utc != TIME_MAX) { + char *s = xstrfmt("%"PRItime, c->password_expiry_utc); + credential_write_item(fp, "password_expiry_utc", s, 0); + free(s); + } } static int run_credential_helper(struct credential *c, @@ -342,6 +352,10 @@ void credential_fill(struct credential *c) for (i = 0; i < c->helpers.nr; i++) { credential_do(c, c->helpers.items[i].string, "get"); + if (c->password_expiry_utc < time(NULL)) { + FREE_AND_NULL(c->password); + c->password_expiry_utc = TIME_MAX; + } if (c->username && c->password) return; if (c->quit) @@ -360,7 +374,7 @@ void credential_approve(struct credential *c) if (c->approved) return; - if (!c->username || !c->password) + if (!c->username || !c->password || c->password_expiry_utc < time(NULL)) return; credential_apply_config(c); @@ -381,6 +395,7 @@ void credential_reject(struct credential *c) FREE_AND_NULL(c->username); FREE_AND_NULL(c->password); + c->password_expiry_utc = TIME_MAX; c->approved = 0; } diff --git a/credential.h b/credential.h index f430e77fea4..935b28a70f1 100644 --- a/credential.h +++ b/credential.h @@ -126,10 +126,12 @@ struct credential { char *protocol; char *host; char *path; + timestamp_t password_expiry_utc; }; #define CREDENTIAL_INIT { \ .helpers = STRING_LIST_INIT_DUP, \ + .password_expiry_utc = TIME_MAX, \ } /* Initialize a credential structure, setting all fields to empty. */ diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh index 3485c0534e6..96391015af5 100755 --- a/t/t0300-credentials.sh +++ b/t/t0300-credentials.sh @@ -35,6 +35,16 @@ test_expect_success 'setup helper scripts' ' test -z "$pass" || echo password=$pass EOF + write_script git-credential-verbatim-with-expiry <<-\EOF && + user=$1; shift + pass=$1; shift + pexpiry=$1; shift + . ./dump + test -z "$user" || echo username=$user + test -z "$pass" || echo password=$pass + test -z "$pexpiry" || echo password_expiry_utc=$pexpiry + EOF + PATH="$PWD:$PATH" ' @@ -109,6 +119,43 @@ test_expect_success 'credential_fill continues through partial response' ' EOF ' +test_expect_success 'credential_fill populates password_expiry_utc' ' + check fill "verbatim-with-expiry one two 9999999999" <<-\EOF + protocol=http + host=example.com + -- + protocol=http + host=example.com + username=one + password=two + password_expiry_utc=9999999999 + -- + verbatim-with-expiry: get + verbatim-with-expiry: protocol=http + verbatim-with-expiry: host=example.com + EOF +' + +test_expect_success 'credential_fill continues through expired password' ' + check fill "verbatim-with-expiry one two 5" "verbatim three four" <<-\EOF + protocol=http + host=example.com + -- + protocol=http + host=example.com + username=three + password=four + -- + verbatim-with-expiry: get + verbatim-with-expiry: protocol=http + verbatim-with-expiry: host=example.com + verbatim: get + verbatim: protocol=http + verbatim: host=example.com + verbatim: username=one + EOF +' + test_expect_success 'credential_fill passes along metadata' ' check fill "verbatim one two" <<-\EOF protocol=ftp @@ -149,6 +196,24 @@ test_expect_success 'credential_approve calls all helpers' ' EOF ' +test_expect_success 'credential_approve stores password expiry' ' + check approve useless <<-\EOF + protocol=http + host=example.com + username=foo + password=bar + password_expiry_utc=9999999999 + -- + -- + useless: store + useless: protocol=http + useless: host=example.com + useless: username=foo + useless: password=bar + useless: password_expiry_utc=9999999999 + EOF +' + test_expect_success 'do not bother storing password-less credential' ' check approve useless <<-\EOF protocol=http @@ -159,6 +224,17 @@ test_expect_success 'do not bother storing password-less credential' ' EOF ' +test_expect_success 'credential_approve does not store expired credential' ' + check approve useless <<-\EOF + protocol=http + host=example.com + username=foo + password=bar + password_expiry_utc=5 + -- + -- + EOF +' test_expect_success 'credential_reject calls all helpers' ' check reject useless "verbatim one two" <<-\EOF @@ -181,6 +257,24 @@ test_expect_success 'credential_reject calls all helpers' ' EOF ' +test_expect_success 'credential_reject erases expired credential' ' + check reject useless <<-\EOF + protocol=http + host=example.com + username=foo + password=bar + password_expiry_utc=5 + -- + -- + useless: erase + useless: protocol=http + useless: host=example.com + useless: username=foo + useless: password=bar + useless: password_expiry_utc=5 + EOF +' + test_expect_success 'usernames can be preserved' ' check fill "verbatim \"\" three" <<-\EOF protocol=http