Feature #16669

Accept OpenID Connect access token

Added by Peter Amstutz 3 months ago. Updated 1 day ago.

Status:
In Progress
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
09/24/2020
Due date:
% Done:

50%

Estimated time:
(Total: 0.00 h)
Story points:
-

Description

When getting an unrecognized token, add an option to validate the token against an OpenID Connect provider.

  1. Determine if the token is valid & when it expires using the OAuth2 token Introspection endpoint https://tools.ietf.org/html/rfc7662
  2. If valid and not expired, make a call to the UserInfo endpoint of the provider, this will return similar claims as the existing log in process, or an error. https://openid.net/specs/openid-connect-core-1_0.html#UserInfo
  3. Cache the token in the Arvados database along with the expiration time.

If a LoginCluster is configured, the token is checked with the upstream LoginCluster (only change is that this happens for JWT tokens and not just v2 tokens).

The endpoint URLs to the Introspection and UserInfo endpoints can be discovered by looking at the "provider configuration" endpoint.

https://openid.net/specs/openid-connect-discovery-1_0.html

https://docs.pingidentity.com/bundle/pingfederate-101/page/bwm1564003025542.html

Additional notes:

Accepting OpenID access tokens


Subtasks

Task #16757: Review 16669-oidc-access-tokenResolvedTom Clegg

Task #17023: Manual testing on dev clustersNewPeter Amstutz


Related issues

Related to Arvados - Feature #17037: [controller] Improve use of given_name/family_name fields for generic OpenID Connect providersNew

Related to Arvados - Feature #17038: [controller] Option to request additional scopes, and verify additional claims, during OpenID Connect authNew

Associated revisions

Revision dfeee281
Added by Tom Clegg 1 day ago

Merge branch '16669-oidc-access-token'

refs #16669

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <>

History

#1 Updated by Peter Amstutz 3 months ago

  • Description updated (diff)

#2 Updated by Peter Amstutz 3 months ago

  • Description updated (diff)

#3 Updated by Peter Amstutz 3 months ago

  • Description updated (diff)

#4 Updated by Peter Amstutz about 2 months ago

  • Assigned To set to Tom Clegg

#5 Updated by Tom Clegg about 2 months ago

  • Status changed from New to In Progress

#6 Updated by Tom Clegg about 2 months ago

  • Description updated (diff)

#7 Updated by Tom Clegg about 1 month ago

  • Target version changed from 2020-09-09 Sprint to 2020-09-23 Sprint

#8 Updated by Tom Clegg about 1 month ago

16669-oidc-access-token @ 61d493d84cff707e291132847ac3e9792ae94aee -- https://ci.arvados.org/view/Developer/job/developer-run-tests/2096/
  • We don't get an expiry time from the access token. It's not clear (to me) whether the introspection API is likely to be supported/available, and the Go library doesn't provide support for it, so for now this code always treats the OIDC access token expiry as "unknown" and re-verifies after 5m.

#9 Updated by Peter Amstutz about 1 month ago

Tom Clegg wrote:

16669-oidc-access-token @ 61d493d84cff707e291132847ac3e9792ae94aee -- https://ci.arvados.org/view/Developer/job/developer-run-tests/2096/
  • We don't get an expiry time from the access token. It's not clear (to me) whether the introspection API is likely to be supported/available, and the Go library doesn't provide support for it, so for now this code always treats the OIDC access token expiry as "unknown" and re-verifies after 5m.

PingFederate supports the introspection API, so ideally we should support it if available.

#11 Updated by Tom Clegg 29 days ago

  • Target version changed from 2020-09-23 Sprint to 2020-10-07 Sprint

#12 Updated by Tom Clegg 29 days ago

16669-oidc-access-token @ 43a3273c3aaed1d28c21a83890810eb62783cf32 -- https://ci.arvados.org/view/Developer/job/developer-run-tests/2117/
  • If LoginCluster is in use, when RailsAPI receives a non-v2 token that isn't [unexpired] in the local database, it asks LoginCluster's controller to verify it.
  • If OIDC login is in use, when controller receives a non-v2 token that isn't [unexpired] in the local database, it asks the OIDC provider to verify it.
    • When controller accepts an OIDC access token, it is cached using hmac(systemroottoken,oidcaccesstoken) as the secret part (api_token column) in the local database.
  • When controller's federation delegate/fan-out code needs to send a request to a remote cluster and it's working with a non-v2 token, it first checks whether the token belongs to a user account on the target cluster, and if so, it passes it through instead of making a salted token.

#13 Updated by Peter Amstutz 27 days ago

Question: should we only support a login flow that exchanges the OIDC access token for an Arvados token, instead of accepting the access token for every API call?

This is based on:

https://www.ory.sh/hydra/docs/concepts/before-oauth2

which among other things states

A common misconception is that access and refresh tokens represent a user session.
...
That is because both applications are built by different people and different companies, and the user behavior on one site does not reflect user intent on another. It would be crazy if some app could just log you out of GitHub, or if signing out of Google would invalidate your sessions on all the websites where you used "Sign in with Google".

However, after studying this some more, there's an important distinction: the OAuth2 case described on that page is for "peer" applications where one is granting limited access for one application to interact with another.

But for us, we are supporting the Single-Sign-On case. In that case, it does behave more like a session. When the user logs off from SSO, they should be logged off everywhere.

So I think the objections about using generic OAuth2 for sessions don't apply.

#14 Updated by Peter Amstutz 27 days ago

The result of OIDC login with Google is an access token that can access Google APIs (only the ones with granted scopes, though). So I think that answers the question "are access tokens supposed to be used as API tokens".

#15 Updated by Peter Amstutz 16 days ago

  • Target version changed from 2020-10-07 Sprint to 2020-10-21 Sprint

#16 Updated by Peter Amstutz 2 days ago

I rebased this branch on master since it was a month old. I force-pushed the rebase.

16669-oidc-access-token @ 64028c0cae469d3da33878a30bd40c5409e64641

Let me see if I understand how this works.

  1. We receive a bare token
  2. Check local controller cache, which can be a miss, possibly valid (check expiration), or probably invalid (recheck after expiration).
  3. Hash the token and check if the hash exists in the database. If it present and still valid, return success.
  4. Check that the token is valid from the OIDC provider
  5. Create or update an arvados token record based on the information from the OIDC provider, and update the local cache.

Is this comment accurate? I don't think we're swapping anything, we're just ensuring that the Rails API server will accept the token?

// Check whether the token is a valid OIDC access token. If
// so, swap it out for an Arvados token (creating/updating an
// api_client_authorizations row if needed) which downstream
// server components will accept.

We should have configuration options GivenNameClaim (default "given_name") and FamilyNameClaim (default "family_name") for setting first_name / last_name. Right now we only get those from the Google People API, so the name will be blank for when using another provider.

Standard claims are here:

https://openid.net/specs/openid-connect-core-1_0.html#StandardClaims

I wonder if we should add an option to check/enforce that an OIDC token has been issued with "arvados" scope (or whatever).

For testing: I ran the unit tests and they passed. I'd like to do some manual integration tests, but that is kind of a hassle to set up. Maybe once the other comments above are addressed, we go ahead and merge and then I can follow with some testing on the dev cluster.

#17 Updated by Tom Clegg 1 day ago

Is this comment accurate? I don't think we're swapping anything, we're just ensuring that the Rails API server will accept the token?

Yes, oidcTokenAuthorizer ensures that if the token is (or was very recently) a valid OIDC access token, then RailsAPI will recognize it (by HMAC) as an Arvados token.

We should have configuration options GivenNameClaim ...

We could -- does someone need it? That feature wouldn't affect this branch though anyway, right?

I wonder if we should add an option to check/enforce that an OIDC token has been issued with "arvados" scope (or whatever).

That feature wouldn't affect this branch though, right?

I'd like to do some manual integration tests

Yes, I've also been looking forward to having this deployed on dev clusters. ;)

16669-oidc-access-token @ b8de8845c856f7fe1232e5f048824211d1207ee7 -- https://ci.arvados.org/view/Developer/job/developer-run-tests/2139/

#18 Updated by Peter Amstutz 1 day ago

Tom Clegg wrote:

Is this comment accurate? I don't think we're swapping anything, we're just ensuring that the Rails API server will accept the token?

Yes, oidcTokenAuthorizer ensures that if the token is (or was very recently) a valid OIDC access token, then RailsAPI will recognize it (by HMAC) as an Arvados token.

We should have configuration options GivenNameClaim ...

We could -- does someone need it? That feature wouldn't affect this branch though anyway, right?

Sure, it doesn't have to be a blocker for an initial merge.

I don't know if GivenName needs to be configurable with a GivenNameClaim or if given_name/family_name are actually used consistently across implementations. But we definitely need it because someone using generic OIDC for login wouldn't have correct display names otherwise.

I wonder if we should add an option to check/enforce that an OIDC token has been issued with "arvados" scope (or whatever).

That feature wouldn't affect this branch though, right?

Sure.

I'd like to do some manual integration tests

Yes, I've also been looking forward to having this deployed on dev clusters. ;)

16669-oidc-access-token @ b8de8845c856f7fe1232e5f048824211d1207ee7 -- https://ci.arvados.org/view/Developer/job/developer-run-tests/2139/

Ok, please merge but let's keep the ticket open for follow-up.

#19 Updated by Peter Amstutz 1 day ago

  • Target version changed from 2020-10-21 Sprint to 2020-11-04 Sprint

#20 Updated by Tom Clegg 1 day ago

  • Related to Feature #17037: [controller] Improve use of given_name/family_name fields for generic OpenID Connect providers added

#21 Updated by Tom Clegg 1 day ago

Peter Amstutz wrote:

I don't know if GivenName needs to be configurable with a GivenNameClaim or if given_name/family_name are actually used consistently across implementations. But we definitely need it because someone using generic OIDC for login wouldn't have correct display names otherwise.

Added #17037

#22 Updated by Tom Clegg 1 day ago

  • Related to Feature #17038: [controller] Option to request additional scopes, and verify additional claims, during OpenID Connect auth added

Also available in: Atom PDF