credential: new attribute oauth_refresh_token

Patch v2 adds an additional test

 Documentation/git-credential.txt   |  6 ++++++
 builtin/credential-cache--daemon.c |  3 +++
 credential.c                       |  6 ++++++
 credential.h                       |  1 +
 t/lib-credential.sh                | 30 ++++++++++++++++++++++++++++++
 t/t0300-credentials.sh             | 18 ++++++++++++++++++
 t/t0301-credential-cache.sh        |  1 +
 7 files changed, 65 insertions(+)

Quoted 1 line · M Hickford via GitGitGadget

Patch v2 adds an additional test

What we have below shows some differences outside the test, but the range-diff I see locally does show that the change since v1 is purely to the tests.

Quoted 9 lines

 Documentation/git-credential.txt   |  6 ++++++
 builtin/credential-cache--daemon.c |  3 +++
 credential.c                       |  6 ++++++
 credential.h                       |  1 +
 t/lib-credential.sh                | 30 ++++++++++++++++++++++++++++++
 t/t0300-credentials.sh             | 18 ++++++++++++++++++
 t/t0301-credential-cache.sh        |  1 +
 7 files changed, 65 insertions(+)

Will replace what I kept in 'seen'.

The original unfortunately did not see anybody interested enough in the topic to review and comment. Let's hope this time is different ;-)

THanks.

Git authentication with OAuth access token is supported by every popular Git host including GitHub, GitLab and BitBucket [1][2][3]. Credential helpers Git Credential Manager (GCM) and git-credential-oauth generate OAuth credentials [4][5]. Following RFC 6749, the application prints a link for the user to authorize access in browser. A loopback redirect communicates the response including access token to the application.

For security, RFC 6749 recommends that OAuth response also includes expiry date and refresh token [6]. After expiry, applications can use the refresh token to generate a new access token without user reauthorization in browser. GitLab and BitBucket set the expiry at two hours [2][3]. (GitHub doesn't populate expiry or refresh token.)

However the Git credential protocol has no attribute to store the OAuth refresh token (unrecognised attributes are silently discarded). This means that the user has to regularly reauthorize the helper in browser. On a browserless system, this is particularly intrusive, requiring a second device.

Introduce a new attribute oauth_refresh_token. This is especially useful when a storage helper and a read-only OAuth helper are configured together. Recall that credential fill calls each helper until it has a non-expired password.

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

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

Alternatives considered: ask helpers to store all unrecognised attributes. This seems excessively complex for no obvious gain. Helpers would also need extra information to distinguish between confidential and non-confidential attributes.

builtin/credential-cache--daemon.c +0 -0

index 62c09a271d6..9db5f00184d 100644

builtin/credential-cache--daemon.c +18 -0

+helper_test_oauth_refresh_token() {
+	HELPER=$1
+
+	test_expect_success "helper ($HELPER) stores oauth_refresh_token" '
+		check approve $HELPER <<-\EOF
+		protocol=https
+		host=example.com
+		username=user4
+		password=pass
+		oauth_refresh_token=xyzzy
+		EOF
+	'
+
+	test_expect_success "helper ($HELPER) gets oauth_refresh_token" '
+		check fill $HELPER <<-\EOF
+		protocol=https
+		host=example.com
+		username=user4

t/t0300-credentials.sh +7 -0

index c66d91e82d8..b49fc14a2bd 100755
@@ -214,6 +214,24 @@ test_expect_success 'credential_approve stores password expiry' '
 	EOF
 '
 
+test_expect_success 'credential_approve stores oauth refresh token' '
+	check approve useless <<-\EOF
+	protocol=http
+	host=example.com
+	username=foo
+	password=bar
+	oauth_refresh_token=xyzzy

Quoted 24 lines · M Hickford via GitGitGadget

Git authentication with OAuth access token is supported by every popular Git host including GitHub, GitLab and BitBucket [1][2][3]. Credential helpers Git Credential Manager (GCM) and git-credential-oauth generate OAuth credentials [4][5]. Following RFC 6749, the application prints a link for the user to authorize access in browser. A loopback redirect communicates the response including access token to the application.

For security, RFC 6749 recommends that OAuth response also includes expiry date and refresh token [6]. After expiry, applications can use the refresh token to generate a new access token without user reauthorization in browser. GitLab and BitBucket set the expiry at two hours [2][3]. (GitHub doesn't populate expiry or refresh token.)

However the Git credential protocol has no attribute to store the OAuth refresh token (unrecognised attributes are silently discarded). This means that the user has to regularly reauthorize the helper in browser. On a browserless system, this is particularly intrusive, requiring a second device.

Introduce a new attribute oauth_refresh_token. This is especially useful when a storage helper and a read-only OAuth helper are configured together. Recall that credential fill calls each helper until it has a non-expired password.

Quoted 5 lines

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

OK. I don't have much knowledge of OAuth, but taking the notion of "this is a useful thing for oauth clients to store" as a given, the implementation seems reasonable.

It does feel a bit funny, in that nothing _except_ credential-cache is going to understand or store this thing. In general, I'd expect most helpers doing anything as complex as oauth to just implement their own storage layer. But I get that your goal here is trying to keep things as composable as possible. I just wonder how successful it will be if there is only one helper that you can actually use for storage.

Quoted 2 lines

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

At least your optimism is documented. :)

Quoted 5 lines

Alternatives considered: ask helpers to store all unrecognised attributes. This seems excessively complex for no obvious gain. Helpers would also need extra information to distinguish between confidential and non-confidential attributes.

I've written the "store arbitrary attributes" patch before, and it actually is quite simple (if you neglect the "confidential" concept, which is largely what the current protocol and helpers do). But I don't know that it buys that much anyway, if helpers need to decide whether and how to store these items anyway. We get support in credential-cache for "free" because it happens to store the same in-memory struct that Git itself does, but most other helpers would need special patches. And because they're often interfacing with system storage, they can't necessarily just store arbitrary items.

Quoted 5 lines

builtin/credential-cache--daemon.c +0 -0

index 62c09a271d6..9db5f00184d 100644

builtin/credential-cache--daemon.c static void serve_one_client(FILE *in, FILE *out) · line 141

Quoted 9 lines

builtin/credential-cache--daemon.c +3 -0

@@ -133,6 +133,9 @@ static void serve_one_client(FILE *in, FILE *out)
                      if (e->item.password_expiry_utc != TIME_MAX)
                              fprintf(out, "password_expiry_utc=%"PRItime"\n",
                                      e->item.password_expiry_utc);
+                     if (e->item.oauth_refresh_token)
+                             fprintf(out, "oauth_refresh_token=%s\n",
+                                     e->item.oauth_refresh_token);
              }
      }
      else if (!strcmp(action.buf, "exit")) {

This hunk makes me wonder if the cache daemon should just be using credential_write(), which would then support this automatically (along with any other fields, like the expiry, or the new wwwauth stuff).

It would mean that it spits back parts of the query (like host, etc) for a "get", rather than just username/password. That's not wrong, but is maybe a little weird.

I can live with it as-is, certainly.

Quoted 18 lines

builtin/credential-cache--daemon.c +18 -0

+helper_test_oauth_refresh_token() {
+     HELPER=$1
+
+     test_expect_success "helper ($HELPER) stores oauth_refresh_token" '
+             check approve $HELPER <<-\EOF
+             protocol=https
+             host=example.com
+             username=user4
+             password=pass
+             oauth_refresh_token=xyzzy
+             EOF
+     '
+
+     test_expect_success "helper ($HELPER) gets oauth_refresh_token" '
+             check fill $HELPER <<-\EOF
+             protocol=https
+             host=example.com
+             username=user4

builtin/credential-cache--daemon.c

Quoted 10 lines

builtin/credential-cache--daemon.c +10 -0

+             --
+             protocol=https
+             host=example.com
+             username=user4
+             password=pass
+             oauth_refresh_token=xyzzy
+             --
+             EOF
+     '
+}

This is in a separate function, so normal t0303 tests won't run it (and confuse users when they fail because they don't support this field). Good.

Possibly t0303 should learn to optionally trigger this the way it does with helper_test_timeout. But I'm also content to leave it until somebody has a helper they want to test with it.

Quoted 15 lines

t/t0300-credentials.sh +7 -0

index c66d91e82d8..b49fc14a2bd 100755
@@ -214,6 +214,24 @@ test_expect_success 'credential_approve stores password expiry' '
      EOF
 '
 
+test_expect_success 'credential_approve stores oauth refresh token' '
+     check approve useless <<-\EOF
+     protocol=http
+     host=example.com
+     username=foo
+     password=bar
+     oauth_refresh_token=xyzzy

t/t0300-credentials.sh test_expect_success 'credential_approve stores password expiry' ' · line 233

Quoted 10 lines

t/t0300-credentials.sh +10 -0

+     --
+     --
+     useless: store
+     useless: protocol=http
+     useless: host=example.com
+     useless: username=foo
+     useless: password=bar
+     useless: oauth_refresh_token=xyzzy
+     EOF
+'

Makes sense. This file is just checking how Git passes the data around between helpers.

t/t0301-credential-cache.sh test_atexit 'git credential-cache exit' · line 35

Quoted 12 lines

t/t0301-credential-cache.sh +1 -0

index 698b7159f03..c02a3b5969c 100755
@@ -29,6 +29,7 @@ test_atexit 'git credential-cache exit'
 
 # test that the daemon works with no special setup
 helper_test cache
+helper_test_oauth_refresh_token cache
 
 test_expect_success 'socket defaults to ~/.cache/git/credential/socket' '
      test_when_finished "

And this one is just testing credential-cache, which we know supports the feature. Good.

So the patch looks like it correctly implements the intent. I don't feel strongly about what the patch is trying to do, but it's not a problem area I know a lot about or use myself. So if it's doing something useful for you, that sounds reasonable to me. :)

-Peff

Quoted 4 lines

Quoted 2 lines

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

At least your optimism is documented. :)

I have a draft patch for credential-libsecret
https://github.com/gitgitgadget/git/pull/1524 which I could add to this patch series if you like. Helpers credential-wincred and credential-osxkeychain would be easy to update following the same approach.

Quoted 24 lines · M Hickford via GitGitGadget

Git authentication with OAuth access token is supported by every popular Git host including GitHub, GitLab and BitBucket [1][2][3]. Credential helpers Git Credential Manager (GCM) and git-credential-oauth generate OAuth credentials [4][5]. Following RFC 6749, the application prints a link for the user to authorize access in browser. A loopback redirect communicates the response including access token to the application.

For security, RFC 6749 recommends that OAuth response also includes expiry date and refresh token [6]. After expiry, applications can use the refresh token to generate a new access token without user reauthorization in browser. GitLab and BitBucket set the expiry at two hours [2][3]. (GitHub doesn't populate expiry or refresh token.)

However the Git credential protocol has no attribute to store the OAuth refresh token (unrecognised attributes are silently discarded). This means that the user has to regularly reauthorize the helper in browser. On a browserless system, this is particularly intrusive, requiring a second device.

Introduce a new attribute oauth_refresh_token. This is especially useful when a storage helper and a read-only OAuth helper are configured together. Recall that credential fill calls each helper until it has a non-expired password.

Quoted 5 lines

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

OK. I don't have much knowledge of OAuth, but taking the notion of "this is a useful thing for oauth clients to store" as a given, the implementation seems reasonable.