[PATCH v3] credential: new attribute password_expiry_utc

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

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

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

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

As a workaround, whenever monolithic helper Git Credential Manager (GCM) retrieves an OAuth credential from its storage, it makes a HTTP request to check whether the OAuth token has expired [1]. This complicates and slows the authentication happy path.

Worse is the case that a storage helper and a credential-generating helper are configured together:

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

`credential approve` stores the generated credential in both helpers without expiry information. Later `credential fill` may return an expired credential from storage. There is no workaround, no matter how clever the second helper.

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

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

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

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

[1] https://github.com/GitCredentialManager/git-credential-manager/blob/66b94e489ad8cc1982836355493e369770b30211/src/shared/GitLab/GitLabHostProvider.cs#L217

Signed-off-by: M Hickford <mirth.hickford@gmail.com>
---
    credential: new attribute password_expiry_utc
    
    details in commit message
    
    Changes in patch v3:
    
     * Tests
     * Simplified credential_read, moved expiry logic to credential_fill

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

Range-diff vs v2:

 1:  b9ee729ee4d ! 1:  1846815a5c1 credential: new attribute password_expiry_utc
     @@ Commit message
          years away for a personal access token or hours for an OAuth access
          token.
      
     -    Currently the credential protocol has no expiry attribute. When multiple
     -    helpers are configured, `credential fill` tries each helper in turn
     -    until it has a username and password, returning early.
     +    When multiple credential helpers are configured, `credential fill` tries
     +    each helper in turn until it has a username and password, returning
     +    early. If Git authentication succeeds, `credential approve`
     +    stores the successful credential in all helpers. If authentication
     +    fails, `credential reject` erases matching credentials in all helpers.
     +    Helpers implement corresponding operations: get, store, erase.
      
     -    When a storage helper and a credential-generating helper are configured
     -    together, the credential is necessarily stored without expiry, so
     -    `credential fill` may later return an expired credential from storage.
     +    The credential protocol has no expiry attribute, so helpers cannot
     +    store expiry information. (Even if a helper returned an improvised
     +    expiry attribute, git credential discards unrecognised attributes
     +    between operations and between helpers.)
      
     -    ```
     -    [credential]
     -            helper = storage  # eg. cache or osxkeychain
     -            helper = generate  # eg. oauth
     -    ```
     +    As a workaround, whenever monolithic helper Git Credential Manager (GCM)
     +    retrieves an OAuth credential from its storage, it makes a HTTP request
     +    to check whether the OAuth token has expired [1]. This complicates and
     +    slows the authentication happy path.
      
     -    An improvement is to introduce a password expiry attribute to the
     -    credential protocol. If the expiry date has passed, `credential fill`
     -    ignores the password attribute, so subsequent helpers can generate a
     -    fresh credential. This is backwards compatible -- no change in
     -    behaviour with helpers that discard the expiry attribute.
     +    Worse is the case that a storage helper and a credential-generating
     +    helper are configured together:
      
     -    Note that the expiry logic is entirely within the credential layer.
     -    Compatible helpers store and retrieve the new attribute like any other.
     -    This keeps the helper contract simple.
     +            [credential]
     +                    helper = storage  # eg. cache or osxkeychain
     +                    helper = generate  # eg. oauth or manager
      
     -    This patch adds support for the new attribute to cache.
     +    `credential approve` stores the generated credential in both helpers
     +    without expiry information. Later `credential fill` may return an
     +    expired credential from storage. There is no workaround, no matter how
     +    clever the second helper.
     +
     +    Introduce a password expiry attribute. In `credential fill`, ignore
     +    expired passwords and continue to query subsequent helpers.
     +
     +    In the example above, `credential fill` ignores the expired credential
     +    and a fresh credential is generated. If authentication succeeds,
     +    `credential approve` replaces the expired credential in storage.
     +    If authentication fails, the expired credential is erased by
     +    `credential reject`. It is unnecessary but harmless for storage
     +    helpers to self prune expired credentials.
     +
     +    Add support for the new attribute to credential-cache.
     +    Eventually, I hope to see support in other storage helpers.
      
          Example usage in a credential-generating helper
          https://github.com/hickford/git-credential-oauth/pull/16
      
     -    Future ideas: make it possible for a storage helper to provide OAuth
     -    refresh token to subsequent helpers.
     -    https://github.com/gitgitgadget/git/pull/1394
     +    [1] https://github.com/GitCredentialManager/git-credential-manager/blob/66b94e489ad8cc1982836355493e369770b30211/src/shared/GitLab/GitLabHostProvider.cs#L217
      
          Signed-off-by: M Hickford <mirth.hickford@gmail.com>
      
     @@ Documentation/git-credential.txt: Git understands the following attributes:
       
      +`password_expiry_utc`::
      +
     -+	If password is a personal access token or OAuth access token, it may have an
     -+	expiry date. When getting credentials from a helper, `git credential fill`
     -+	ignores the password attribute if the expiry date has passed. Storage
     -+	helpers should store this attribute if possible. Helpers should not
     -+	implement expiry logic themselves. Represented as Unix time UTC, seconds
     -+	since 1970.
     ++	Generated passwords such as an OAuth access token may have an expiry date.
     ++	When reading credentials from helpers, `git credential fill` ignores expired
     ++	passwords. Represented as Unix time UTC, seconds since 1970.
      +
       `url`::
       
       	When this special attribute is read by `git credential`, the
      
     + ## Documentation/gitcredentials.txt ##
     +@@ Documentation/gitcredentials.txt: helper::
     + If there are multiple instances of the `credential.helper` configuration
     + variable, each helper will be tried in turn, and may provide a username,
     + password, or nothing. Once Git has acquired both a username and a
     +-password, no more helpers will be tried.
     ++unexpired password, no more helpers will be tried.
     + +
     + If `credential.helper` is configured to the empty string, this resets
     + the helper list to empty (so you may override a helper set by a
     +
       ## builtin/credential-cache--daemon.c ##
      @@ builtin/credential-cache--daemon.c: static void serve_one_client(FILE *in, FILE *out)
       		if (e) {
     @@ credential.c
       
       void credential_init(struct credential *c)
       {
     -@@ credential.c: static void credential_getpass(struct credential *c)
     - 	if (!c->username)
     - 		c->username = credential_ask_one("Username", c,
     - 						 PROMPT_ASKPASS|PROMPT_ECHO);
     --	if (!c->password)
     -+	if (!c->password || c->password_expiry_utc < time(NULL)) {
     -+		c->password_expiry_utc = TIME_MAX;
     - 		c->password = credential_ask_one("Password", c,
     - 						 PROMPT_ASKPASS);
     -+	}
     - }
     - 
     - int credential_read(struct credential *c, FILE *fp)
     - {
     - 	struct strbuf line = STRBUF_INIT;
     - 
     -+	int password_updated = 0;
     -+	timestamp_t this_password_expiry = TIME_MAX;
     -+
     - 	while (strbuf_getline(&line, fp) != EOF) {
     - 		char *key = line.buf;
     - 		char *value = strchr(key, '=');
     -@@ credential.c: int credential_read(struct credential *c, FILE *fp)
     - 		} else if (!strcmp(key, "password")) {
     - 			free(c->password);
     - 			c->password = xstrdup(value);
     -+			password_updated = 1;
     - 		} else if (!strcmp(key, "protocol")) {
     - 			free(c->protocol);
     - 			c->protocol = xstrdup(value);
      @@ credential.c: int credential_read(struct credential *c, FILE *fp)
       		} else if (!strcmp(key, "path")) {
       			free(c->path);
       			c->path = xstrdup(value);
      +		} else if (!strcmp(key, "password_expiry_utc")) {
     -+			this_password_expiry = parse_timestamp(value, NULL, 10);
     -+			if (this_password_expiry == 0 || errno) {
     -+				this_password_expiry = TIME_MAX;
     -+			}
     ++			c->password_expiry_utc = parse_timestamp(value, NULL, 10);
     ++			if (c->password_expiry_utc == 0 || errno)
     ++				c->password_expiry_utc = TIME_MAX;
       		} else if (!strcmp(key, "url")) {
       			credential_from_url(c, value);
       		} else if (!strcmp(key, "quit")) {
     -@@ credential.c: int credential_read(struct credential *c, FILE *fp)
     - 		 */
     - 	}
     - 
     -+	if (password_updated)
     -+		c->password_expiry_utc = this_password_expiry;
     -+
     - 	strbuf_release(&line);
     - 	return 0;
     - }
      @@ credential.c: void credential_write(const struct credential *c, FILE *fp)
       	credential_write_item(fp, "path", c->path, 0);
       	credential_write_item(fp, "username", c->username, 0);
     @@ credential.c: void credential_fill(struct credential *c)
       
       	for (i = 0; i < c->helpers.nr; i++) {
       		credential_do(c, c->helpers.items[i].string, "get");
     --		if (c->username && c->password)
     -+		if (c->username && c->password && time(NULL) < c->password_expiry_utc)
     ++		if (c->password_expiry_utc < time(NULL)) {
     ++			FREE_AND_NULL(c->password);
     ++			c->password_expiry_utc = TIME_MAX;
     ++		}
     + 		if (c->username && c->password)
       			return;
       		if (c->quit)
     - 			die("credential helper '%s' told us to quit",
     +@@ credential.c: void credential_approve(struct credential *c)
     + 
     + 	if (c->approved)
     + 		return;
     +-	if (!c->username || !c->password)
     ++	if (!c->username || !c->password || c->password_expiry_utc < time(NULL))
     + 		return;
     + 
     + 	credential_apply_config(c);
     +@@ credential.c: void credential_reject(struct credential *c)
     + 
     + 	FREE_AND_NULL(c->username);
     + 	FREE_AND_NULL(c->password);
     ++	c->password_expiry_utc = TIME_MAX;
     + 	c->approved = 0;
     + }
     + 
      
       ## credential.h ##
      @@ credential.h: struct credential {
     @@ credential.h: struct credential {
       }
       
       /* Initialize a credential structure, setting all fields to empty. */
     +
     + ## t/t0300-credentials.sh ##
     +@@ t/t0300-credentials.sh: test_expect_success 'setup helper scripts' '
     + 	test -z "$pass" || echo password=$pass
     + 	EOF
     + 
     ++	write_script git-credential-verbatim-with-expiry <<-\EOF &&
     ++	user=$1; shift
     ++	pass=$1; shift
     ++	pexpiry=$1; shift
     ++	. ./dump
     ++	test -z "$user" || echo username=$user
     ++	test -z "$pass" || echo password=$pass
     ++	test -z "$pexpiry" || echo password_expiry_utc=$pexpiry
     ++	EOF
     ++
     + 	PATH="$PWD:$PATH"
     + '
     + 
     +@@ t/t0300-credentials.sh: test_expect_success 'credential_fill continues through partial response' '
     + 	EOF
     + '
     + 
     ++test_expect_success 'credential_fill populates password_expiry_utc' '
     ++	check fill "verbatim-with-expiry one two 9999999999" <<-\EOF
     ++	protocol=http
     ++	host=example.com
     ++	--
     ++	protocol=http
     ++	host=example.com
     ++	username=one
     ++	password=two
     ++	password_expiry_utc=9999999999
     ++	--
     ++	verbatim-with-expiry: get
     ++	verbatim-with-expiry: protocol=http
     ++	verbatim-with-expiry: host=example.com
     ++	EOF
     ++'
     ++
     ++test_expect_success 'credential_fill continues through expired password' '
     ++	check fill "verbatim-with-expiry one two 5" "verbatim three four" <<-\EOF
     ++	protocol=http
     ++	host=example.com
     ++	--
     ++	protocol=http
     ++	host=example.com
     ++	username=three
     ++	password=four
     ++	--
     ++	verbatim-with-expiry: get
     ++	verbatim-with-expiry: protocol=http
     ++	verbatim-with-expiry: host=example.com
     ++	verbatim: get
     ++	verbatim: protocol=http
     ++	verbatim: host=example.com
     ++	verbatim: username=one
     ++	EOF
     ++'
     ++
     + test_expect_success 'credential_fill passes along metadata' '
     + 	check fill "verbatim one two" <<-\EOF
     + 	protocol=ftp
     +@@ t/t0300-credentials.sh: test_expect_success 'credential_approve calls all helpers' '
     + 	EOF
     + '
     + 
     ++test_expect_success 'credential_approve stores password expiry' '
     ++	check approve useless <<-\EOF
     ++	protocol=http
     ++	host=example.com
     ++	username=foo
     ++	password=bar
     ++	password_expiry_utc=9999999999
     ++	--
     ++	--
     ++	useless: store
     ++	useless: protocol=http
     ++	useless: host=example.com
     ++	useless: username=foo
     ++	useless: password=bar
     ++	useless: password_expiry_utc=9999999999
     ++	EOF
     ++'
     ++
     + test_expect_success 'do not bother storing password-less credential' '
     + 	check approve useless <<-\EOF
     + 	protocol=http
     +@@ t/t0300-credentials.sh: test_expect_success 'do not bother storing password-less credential' '
     + 	EOF
     + '
     + 
     ++test_expect_success 'credential_approve does not store expired credential' '
     ++	check approve useless <<-\EOF
     ++	protocol=http
     ++	host=example.com
     ++	username=foo
     ++	password=bar
     ++	password_expiry_utc=5
     ++	--
     ++	--
     ++	EOF
     ++'
     + 
     + test_expect_success 'credential_reject calls all helpers' '
     + 	check reject useless "verbatim one two" <<-\EOF
     +@@ t/t0300-credentials.sh: test_expect_success 'credential_reject calls all helpers' '
     + 	EOF
     + '
     + 
     ++test_expect_success 'credential_reject erases expired credential' '
     ++	check reject useless <<-\EOF
     ++	protocol=http
     ++	host=example.com
     ++	username=foo
     ++	password=bar
     ++	password_expiry_utc=5
     ++	--
     ++	--
     ++	useless: erase
     ++	useless: protocol=http
     ++	useless: host=example.com
     ++	useless: username=foo
     ++	useless: password=bar
     ++	useless: password_expiry_utc=5
     ++	EOF
     ++'
     ++
     + test_expect_success 'usernames can be preserved' '
     + 	check fill "verbatim \"\" three" <<-\EOF
     + 	protocol=http

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