[PATCH v4] credential: new attribute password_expiry_utc

["M Hickford via GitGitGadget" <gitgitgadget@gmail.com>]

From: M Hickford <mirth.hickford@gmail.com>

Some passwords have an expiry date known at generation. This may be
years away for a personal access token or hours for an OAuth access
token.

When multiple credential helpers are configured, `credential fill` tries
each helper in turn until it has a username and password, returning
early. If Git authentication succeeds, `credential approve`
stores the successful credential in all helpers. If authentication
fails, `credential reject` erases matching credentials in all helpers.
Helpers implement corresponding operations: get, store, erase.

The credential protocol has no expiry attribute, so helpers cannot
store expiry information. Even if a helper returned an improvised
expiry attribute, git credential discards unrecognised attributes
between operations and between helpers.

This is a particular issue when a storage helper and a
credential-generating helper are configured together:

	[credential]
		helper = storage  # eg. cache or osxkeychain
		helper = generate  # eg. oauth

`credential approve` stores the generated credential in both helpers
without expiry information. Later `credential fill` may return an
expired credential from storage. There is no workaround, no matter how
clever the second helper. The user sees authentication fail (a retry
will succeed).

Introduce a password expiry attribute. In `credential fill`, ignore
expired passwords and continue to query subsequent helpers.

In the example above, `credential fill` ignores the expired password
and a fresh credential is generated. If authentication succeeds,
`credential approve` replaces the expired password in storage.
If authentication fails, the expired credential is erased by
`credential reject`. It is unnecessary but harmless for storage
helpers to self prune expired credentials.

Add support for the new attribute to credential-cache.
Eventually, I hope to see support in other popular storage helpers.

Example usage in a credential-generating helper
https://github.com/hickford/git-credential-oauth/pull/16

Signed-off-by: M Hickford <mirth.hickford@gmail.com>
---
    credential: new attribute password_expiry_utc
    
    details in commit message
    
    Changes in patch v4:
    
     * Set errno = 0 to fix tests on Windows and FreeBSD (thanks Lessley
       Dennington)
     * Clarify rationale as discussed at review club

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1443%2Fhickford%2Fpassword-expiry-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1443/hickford/password-expiry-v4
Pull-Request: https://github.com/git/git/pull/1443

Range-diff vs v3:

 1:  1846815a5c1 ! 1:  ae8bddbb30a credential: new attribute password_expiry_utc
     @@ Commit message
          Helpers implement corresponding operations: get, store, erase.
      
          The credential protocol has no expiry attribute, so helpers cannot
     -    store expiry information. (Even if a helper returned an improvised
     +    store expiry information. Even if a helper returned an improvised
          expiry attribute, git credential discards unrecognised attributes
     -    between operations and between helpers.)
     +    between operations and between helpers.
      
     -    As a workaround, whenever monolithic helper Git Credential Manager (GCM)
     -    retrieves an OAuth credential from its storage, it makes a HTTP request
     -    to check whether the OAuth token has expired [1]. This complicates and
     -    slows the authentication happy path.
     -
     -    Worse is the case that a storage helper and a credential-generating
     -    helper are configured together:
     +    This is a particular issue when a storage helper and a
     +    credential-generating helper are configured together:
      
                  [credential]
                          helper = storage  # eg. cache or osxkeychain
     -                    helper = generate  # eg. oauth or manager
     +                    helper = generate  # eg. oauth
      
          `credential approve` stores the generated credential in both helpers
          without expiry information. Later `credential fill` may return an
          expired credential from storage. There is no workaround, no matter how
     -    clever the second helper.
     +    clever the second helper. The user sees authentication fail (a retry
     +    will succeed).
      
          Introduce a password expiry attribute. In `credential fill`, ignore
          expired passwords and continue to query subsequent helpers.
      
     -    In the example above, `credential fill` ignores the expired credential
     +    In the example above, `credential fill` ignores the expired password
          and a fresh credential is generated. If authentication succeeds,
     -    `credential approve` replaces the expired credential in storage.
     +    `credential approve` replaces the expired password in storage.
          If authentication fails, the expired credential is erased by
          `credential reject`. It is unnecessary but harmless for storage
          helpers to self prune expired credentials.
      
          Add support for the new attribute to credential-cache.
     -    Eventually, I hope to see support in other storage helpers.
     +    Eventually, I hope to see support in other popular storage helpers.
      
          Example usage in a credential-generating helper
          https://github.com/hickford/git-credential-oauth/pull/16
      
     -    [1] https://github.com/GitCredentialManager/git-credential-manager/blob/66b94e489ad8cc1982836355493e369770b30211/src/shared/GitLab/GitLabHostProvider.cs#L217
     -
          Signed-off-by: M Hickford <mirth.hickford@gmail.com>
      
       ## Documentation/git-credential.txt ##
     @@ Documentation/gitcredentials.txt: helper::
       variable, each helper will be tried in turn, and may provide a username,
       password, or nothing. Once Git has acquired both a username and a
      -password, no more helpers will be tried.
     -+unexpired password, no more helpers will be tried.
     ++non-expired password, no more helpers will be tried.
       +
       If `credential.helper` is configured to the empty string, this resets
       the helper list to empty (so you may override a helper set by a
     @@ credential.c: int credential_read(struct credential *c, FILE *fp)
       			free(c->path);
       			c->path = xstrdup(value);
      +		} else if (!strcmp(key, "password_expiry_utc")) {
     ++			errno = 0;
      +			c->password_expiry_utc = parse_timestamp(value, NULL, 10);
     -+			if (c->password_expiry_utc == 0 || errno)
     ++			if (c->password_expiry_utc == 0 || errno == ERANGE)
      +				c->password_expiry_utc = TIME_MAX;
       		} else if (!strcmp(key, "url")) {
       			credential_from_url(c, value);
     @@ credential.c: void credential_fill(struct credential *c)
       	for (i = 0; i < c->helpers.nr; i++) {
       		credential_do(c, c->helpers.items[i].string, "get");
      +		if (c->password_expiry_utc < time(NULL)) {
     ++			/* Discard expired password */
      +			FREE_AND_NULL(c->password);
     ++			/* Reset expiry to maintain consistency */
      +			c->password_expiry_utc = TIME_MAX;
      +		}
       		if (c->username && c->password)
     @@ t/t0300-credentials.sh: test_expect_success 'credential_fill continues through p
      +	EOF
      +'
      +
     -+test_expect_success 'credential_fill continues through expired password' '
     ++test_expect_success 'credential_fill ignores expired password' '
      +	check fill "verbatim-with-expiry one two 5" "verbatim three four" <<-\EOF
      +	protocol=http
      +	host=example.com
     @@ t/t0300-credentials.sh: test_expect_success 'do not bother storing password-less
       	EOF
       '
       
     -+test_expect_success 'credential_approve does not store expired credential' '
     ++test_expect_success 'credential_approve does not store expired password' '
      +	check approve useless <<-\EOF
      +	protocol=http
      +	host=example.com
     @@ t/t0300-credentials.sh: test_expect_success 'credential_reject calls all helpers
       	EOF
       '
       
     -+test_expect_success 'credential_reject erases expired credential' '
     ++test_expect_success 'credential_reject erases credential regardless of expiry' '
      +	check reject useless <<-\EOF
      +	protocol=http
      +	host=example.com


 Documentation/git-credential.txt   |  6 ++
 Documentation/gitcredentials.txt   |  2 +-
 builtin/credential-cache--daemon.c |  3 +
 credential.c                       | 20 ++++++-
 credential.h                       |  2 +
 t/t0300-credentials.sh             | 94 ++++++++++++++++++++++++++++++
 6 files changed, 125 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt
index ac2818b9f66..29d184ab824 100644
--- a/Documentation/git-credential.txt
+++ b/Documentation/git-credential.txt
@@ -144,6 +144,12 @@ Git understands the following attributes:
 
 	The credential's password, if we are asking it to be stored.
 
+`password_expiry_utc`::
+
+	Generated passwords such as an OAuth access token may have an expiry date.
+	When reading credentials from helpers, `git credential fill` ignores expired
+	passwords. Represented as Unix time UTC, seconds since 1970.
+
 `url`::
 
 	When this special attribute is read by `git credential`, the
diff --git a/Documentation/gitcredentials.txt b/Documentation/gitcredentials.txt
index 4522471c337..100f045bb1a 100644
--- a/Documentation/gitcredentials.txt
+++ b/Documentation/gitcredentials.txt
@@ -167,7 +167,7 @@ helper::
 If there are multiple instances of the `credential.helper` configuration
 variable, each helper will be tried in turn, and may provide a username,
 password, or nothing. Once Git has acquired both a username and a
-password, no more helpers will be tried.
+non-expired password, no more helpers will be tried.
 +
 If `credential.helper` is configured to the empty string, this resets
 the helper list to empty (so you may override a helper set by a
diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
index f3c89831d4a..338058be7f9 100644
--- a/builtin/credential-cache--daemon.c
+++ b/builtin/credential-cache--daemon.c
@@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out)
 		if (e) {
 			fprintf(out, "username=%s\n", e->item.username);
 			fprintf(out, "password=%s\n", e->item.password);
+			if (e->item.password_expiry_utc != TIME_MAX)
+				fprintf(out, "password_expiry_utc=%"PRItime"\n",
+					e->item.password_expiry_utc);
 		}
 	}
 	else if (!strcmp(action.buf, "exit")) {
diff --git a/credential.c b/credential.c
index f6389a50684..f32011343f9 100644
--- a/credential.c
+++ b/credential.c
@@ -7,6 +7,7 @@
 #include "prompt.h"
 #include "sigchain.h"
 #include "urlmatch.h"
+#include "git-compat-util.h"
 
 void credential_init(struct credential *c)
 {
@@ -234,6 +235,11 @@ int credential_read(struct credential *c, FILE *fp)
 		} else if (!strcmp(key, "path")) {
 			free(c->path);
 			c->path = xstrdup(value);
+		} else if (!strcmp(key, "password_expiry_utc")) {
+			errno = 0;
+			c->password_expiry_utc = parse_timestamp(value, NULL, 10);
+			if (c->password_expiry_utc == 0 || errno == ERANGE)
+				c->password_expiry_utc = TIME_MAX;
 		} else if (!strcmp(key, "url")) {
 			credential_from_url(c, value);
 		} else if (!strcmp(key, "quit")) {
@@ -269,6 +275,11 @@ void credential_write(const struct credential *c, FILE *fp)
 	credential_write_item(fp, "path", c->path, 0);
 	credential_write_item(fp, "username", c->username, 0);
 	credential_write_item(fp, "password", c->password, 0);
+	if (c->password_expiry_utc != TIME_MAX) {
+		char *s = xstrfmt("%"PRItime, c->password_expiry_utc);
+		credential_write_item(fp, "password_expiry_utc", s, 0);
+		free(s);
+	}
 }
 
 static int run_credential_helper(struct credential *c,
@@ -342,6 +353,12 @@ void credential_fill(struct credential *c)
 
 	for (i = 0; i < c->helpers.nr; i++) {
 		credential_do(c, c->helpers.items[i].string, "get");
+		if (c->password_expiry_utc < time(NULL)) {
+			/* Discard expired password */
+			FREE_AND_NULL(c->password);
+			/* Reset expiry to maintain consistency */
+			c->password_expiry_utc = TIME_MAX;
+		}
 		if (c->username && c->password)
 			return;
 		if (c->quit)
@@ -360,7 +377,7 @@ void credential_approve(struct credential *c)
 
 	if (c->approved)
 		return;
-	if (!c->username || !c->password)
+	if (!c->username || !c->password || c->password_expiry_utc < time(NULL))
 		return;
 
 	credential_apply_config(c);
@@ -381,6 +398,7 @@ void credential_reject(struct credential *c)
 
 	FREE_AND_NULL(c->username);
 	FREE_AND_NULL(c->password);
+	c->password_expiry_utc = TIME_MAX;
 	c->approved = 0;
 }
 
diff --git a/credential.h b/credential.h
index f430e77fea4..935b28a70f1 100644
--- a/credential.h
+++ b/credential.h
@@ -126,10 +126,12 @@ struct credential {
 	char *protocol;
 	char *host;
 	char *path;
+	timestamp_t password_expiry_utc;
 };
 
 #define CREDENTIAL_INIT { \
 	.helpers = STRING_LIST_INIT_DUP, \
+	.password_expiry_utc = TIME_MAX, \
 }
 
 /* Initialize a credential structure, setting all fields to empty. */
diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index 3485c0534e6..c66d91e82d8 100755
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -35,6 +35,16 @@ test_expect_success 'setup helper scripts' '
 	test -z "$pass" || echo password=$pass
 	EOF
 
+	write_script git-credential-verbatim-with-expiry <<-\EOF &&
+	user=$1; shift
+	pass=$1; shift
+	pexpiry=$1; shift
+	. ./dump
+	test -z "$user" || echo username=$user
+	test -z "$pass" || echo password=$pass
+	test -z "$pexpiry" || echo password_expiry_utc=$pexpiry
+	EOF
+
 	PATH="$PWD:$PATH"
 '
 
@@ -109,6 +119,43 @@ test_expect_success 'credential_fill continues through partial response' '
 	EOF
 '
 
+test_expect_success 'credential_fill populates password_expiry_utc' '
+	check fill "verbatim-with-expiry one two 9999999999" <<-\EOF
+	protocol=http
+	host=example.com
+	--
+	protocol=http
+	host=example.com
+	username=one
+	password=two
+	password_expiry_utc=9999999999
+	--
+	verbatim-with-expiry: get
+	verbatim-with-expiry: protocol=http
+	verbatim-with-expiry: host=example.com
+	EOF
+'
+
+test_expect_success 'credential_fill ignores expired password' '
+	check fill "verbatim-with-expiry one two 5" "verbatim three four" <<-\EOF
+	protocol=http
+	host=example.com
+	--
+	protocol=http
+	host=example.com
+	username=three
+	password=four
+	--
+	verbatim-with-expiry: get
+	verbatim-with-expiry: protocol=http
+	verbatim-with-expiry: host=example.com
+	verbatim: get
+	verbatim: protocol=http
+	verbatim: host=example.com
+	verbatim: username=one
+	EOF
+'
+
 test_expect_success 'credential_fill passes along metadata' '
 	check fill "verbatim one two" <<-\EOF
 	protocol=ftp
@@ -149,6 +196,24 @@ test_expect_success 'credential_approve calls all helpers' '
 	EOF
 '
 
+test_expect_success 'credential_approve stores password expiry' '
+	check approve useless <<-\EOF
+	protocol=http
+	host=example.com
+	username=foo
+	password=bar
+	password_expiry_utc=9999999999
+	--
+	--
+	useless: store
+	useless: protocol=http
+	useless: host=example.com
+	useless: username=foo
+	useless: password=bar
+	useless: password_expiry_utc=9999999999
+	EOF
+'
+
 test_expect_success 'do not bother storing password-less credential' '
 	check approve useless <<-\EOF
 	protocol=http
@@ -159,6 +224,17 @@ test_expect_success 'do not bother storing password-less credential' '
 	EOF
 '
 
+test_expect_success 'credential_approve does not store expired password' '
+	check approve useless <<-\EOF
+	protocol=http
+	host=example.com
+	username=foo
+	password=bar
+	password_expiry_utc=5
+	--
+	--
+	EOF
+'
 
 test_expect_success 'credential_reject calls all helpers' '
 	check reject useless "verbatim one two" <<-\EOF
@@ -181,6 +257,24 @@ test_expect_success 'credential_reject calls all helpers' '
 	EOF
 '
 
+test_expect_success 'credential_reject erases credential regardless of expiry' '
+	check reject useless <<-\EOF
+	protocol=http
+	host=example.com
+	username=foo
+	password=bar
+	password_expiry_utc=5
+	--
+	--
+	useless: erase
+	useless: protocol=http
+	useless: host=example.com
+	useless: username=foo
+	useless: password=bar
+	useless: password_expiry_utc=5
+	EOF
+'
+
 test_expect_success 'usernames can be preserved' '
 	check fill "verbatim \"\" three" <<-\EOF
 	protocol=http

base-commit: 2fc9e9ca3c7505bc60069f11e7ef09b1aeeee473
-- 
gitgitgadget