Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC 8693 OAuth 2.0 Token Exchange #2806

Merged
merged 3 commits into from
Jul 1, 2023

Conversation

seankhliao
Copy link
Contributor

Overview

Implement RFC 8693 which specifies exchanging third party tokens for native ones.

What this PR does / why we need it

There are times when we wish to grant machines access to a protected endpoint without using long-lived, static API keys or user/password combinations.
Many modern execution environments can expose short-lived OIDC tokens for the duration of the execution, examples include:

As these are machine users, handling a redirect is not very feasible.
In these cases, clients will have independently authenticated to the upstream IDP (sometimes implicitly by virtue of the IDP integrating with the execution environment) and obtained an ID token.
This PR allows using this prior authentication to be used in obtaining a dex issued token for use with downstream clients.

As a concrete example:
We're running ArgoCD with dex as the identity provider, connected to Okta for humans.
We want to access the ArgoCD API from our CI environments without keeping around a static key which may be compromised.

Related issues:

Special notes for your reviewer

The RFC:

  • Isn't very clear on what are the accepted subject/requested token types. I'm assuming it's ID tokens for subject, and ID/Access for requested.
  • actor_token and actor_token_type are "MUST ... if the actor token is present, also perform the appropriate validation procedures for its indicated token type", but it's unclear what to authenticate them in this context (an existing user?)
  • specifies an optional resource field which I don't see any use for
  • specifies an audience field which I'm using for connector ID, based on how GCP Workload Identity Pools use them

It was also unclear if the correct arguments are passed to newIDToken / newAccessToken

Does this PR introduce a user-facing change?

oidc connections now support the grant type "urn:ietf:params:oauth:grant-type:token-exchange"

server/handlers.go Fixed Show fixed Hide fixed
server/handlers.go Fixed Show fixed Hide fixed
server/handlers.go Fixed Show fixed Hide fixed
@seankhliao seankhliao force-pushed the dex-token-exchange branch 2 times, most recently from e2f36e3 to 051c683 Compare February 15, 2023 13:34
@seankhliao seankhliao force-pushed the dex-token-exchange branch 2 times, most recently from 5aebff1 to b25d49b Compare March 24, 2023 08:03
@seankhliao seankhliao marked this pull request as ready for review March 24, 2023 08:04
@seankhliao
Copy link
Contributor Author

I think this is good enough for a first round of reviews.
Not quite sure about what to do with the CodeQL failures on logging user input

@baz-bakerhughes
Copy link

Truly would like this merged into main so dex can be our complete Authentication proxy solution.

@sagikazarmark sagikazarmark added the release-note/new-feature Release note: Exciting New Features label Apr 5, 2023
@sagikazarmark
Copy link
Member

I can't promise it's gonna happen before KubeCon, but we will get to this.

@gnunn1
Copy link

gnunn1 commented Apr 9, 2023

Just as a data point if it helps, I tested this PR with the Openshift connector after adding a very hacky implementation of the TokenIdentity method to the connector and it worked fine.

@blairdrummond
Copy link

blairdrummond commented May 8, 2023

Are there any docs/examples of this flow for Github Actions kicking around? I would be very interested in trying it out, even off of a branch build

@nabokihms
Copy link
Member

@seankhliao, sorry for the long delay. I have no excuses (besides KubeCon 😄 ). The code looks good to me.
I want to test the feature manually and give my green light after that.

BTW could you please rebase the PR to resolve conflicts?

@seankhliao
Copy link
Contributor Author

seankhliao commented May 10, 2023

@nabokihms rebased, no need for excuses :)

@blairdrummond https://github.com/seankhliao/testrepo0191
dex config is in config.yaml
curl commands in the workflow
run: https://github.com/seankhliao/testrepo0191/actions/runs/4940627858/jobs/8832465909

@blairdrummond
Copy link

@aidansteele, just incase you're interested, I recently read aws-iam-oidc-idps-need-more-controls, and it seems like this might help resolve the things you have implemented via jwtex. I think the only thing missing is nested json claim mapping which is still a TODO.

@gnunn1
Copy link

gnunn1 commented May 11, 2023

I have the OpenShift connector updated to implement token exchange for this PR, should I wait for this to be merged before submitting a PR since it depends on this PR?

@nabokihms
Copy link
Member

@gnunn1 yes, for now it is better to hold your PR util we merge the current one.

@dhasmac-bh
Copy link

Is there any info on how we can test with oidc type connector like auth0. I want to test if the access token through client-credentials grant be exchanged for DEX approved token through token-exchange.

@dhasmac-bh
Copy link

Is there any info on how we can test with oidc type connector like auth0. I want to test if the access token through client-credentials grant be exchanged for DEX approved token through token-exchange.

It worked.

@baz-bakerhughes
Copy link

OK. We have been running this build with this PR for about 3 weeks in our test environment. I am curious if there is any concern that this will be rejected. If it eventually would be accepted, we can promote the version with the PR and then swap once its in. But if there is a concern that having dexidp support this functionality (even if it was re coded in a different way) but there is a philosophical reason against this that would change our plans. Any thoughts?

@baz-bakerhughes
Copy link

@nabokihms Is there any assistance needed to get this merged into main. We have been using the branch build in our environment for a while now and tested all the functional options on tokens we are using. That is @dhasmac-bh has and it all works great.

  • auth code flow
  • token exchange

@nabokihms
Copy link
Member

nabokihms commented Jun 6, 2023

@baz-bakerhughes, for now, no assistance is needed. DEP was accepted, so it is only a matter of testing. Because of the PTO season, we a throttling with this PR a bit, but no worries. We will merge it eventually.

server/handlers.go Outdated Show resolved Hide resolved
connector/oidc/oidc.go Outdated Show resolved Hide resolved
connector/oidc/oidc.go Show resolved Hide resolved
connector/oidc/oidc.go Outdated Show resolved Hide resolved
connector/oidc/oidc.go Show resolved Hide resolved
server/handlers.go Outdated Show resolved Hide resolved
server/handlers.go Outdated Show resolved Hide resolved
server/handlers.go Outdated Show resolved Hide resolved
server/handlers.go Outdated Show resolved Hide resolved
cmd/dex/config.go Show resolved Hide resolved
@seankhliao seankhliao force-pushed the dex-token-exchange branch 2 times, most recently from 9227b4d to a80245d Compare June 10, 2023 12:25
@seankhliao
Copy link
Contributor Author

I've reworked the TokenIdentiy part a bit so it should properly work with both id and access tokens.
I think getUserInfo should work now, except I don't have a real backend to test against, the providers I'm using don't support that endpoint at all

Co-authored-by: Maksim Nabokikh <[email protected]>
Signed-off-by: Sean Liao <[email protected]>
@nabokihms
Copy link
Member

@seankhliao I tested a dex - dex communication (configuring the second dex instance as an oidc provider for the first) 😄.

@seankhliao
Copy link
Contributor Author

Ok, I think dex - dex works too with my updated code

server/handlers.go Outdated Show resolved Hide resolved
connector/oidc/oidc.go Show resolved Hide resolved
server/handlers.go Outdated Show resolved Hide resolved
Copy link
Member

@nabokihms nabokihms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seankhliao awesome! Thank you for your work. It was a long journey!

P.S. It would be awesome if you could add a doc about the Token Exchange feature to the Dex documentation.

@nabokihms nabokihms merged commit dcf7b18 into dexidp:master Jul 1, 2023
@sagikazarmark
Copy link
Member

Thank you @seankhliao for making this happen!

@seankhliao seankhliao deleted the dex-token-exchange branch July 1, 2023 09:02
@seankhliao
Copy link
Contributor Author

I've created dexidp/website#140 for website documentation
and #3027 for adding grantTypes to the config example (linked from the website).

@dhasmac-bh
Copy link

Thanks for the merge, any ETA when this will be available as a release. I can update my helm chart accordingly.

@nabokihms
Copy link
Member

There is no release date for the upcoming releases, but it will probably be August something.

palexster pushed a commit to palexster/dex that referenced this pull request Oct 4, 2023
Signed-off-by: Sean Liao <[email protected]>
Co-authored-by: Maksim Nabokikh <[email protected]>
michaelliau pushed a commit to FlockFreight/dex that referenced this pull request Oct 4, 2023
Signed-off-by: Sean Liao <[email protected]>
Co-authored-by: Maksim Nabokikh <[email protected]>
@sl1pm4t
Copy link

sl1pm4t commented Jan 26, 2024

Thanks for this feature, we are using it already to enable ArgoCD to authenticate with managed clusters across multiple clouds.
To facilitate this, and in case it's helpful for anyone else, I wrote this Kubernetes Credential helper that exchanges a Kubernetes Service Account token with one signed by Dex:
https://github.com/sl1pm4t/tokenxchange

@nabokihms
Copy link
Member

@sl1pm4t great, thank you for sharing 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/new-feature Release note: Exciting New Features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants