-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Client Credentials Grant #1629
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
base: master
Are you sure you want to change the base?
Client Credentials Grant #1629
Conversation
|
@xtremerui I've been evaluating Dex as SP front ending Okta IdP. One of the use cases, I was looking at is support for client_credentials grant type. In a way, I want client application to use static client id and secret, configured at the time of Dex connector configuration, to retrieve id_token, refresh_token, access_token, code from Dex. This PR is going to support that? The connector type 'am exploring is OIDC. |
|
@karunchennuri yes we have similar use case thus we implemented this in our fork https://github.com/concourse/dex. Not sure how it is gonna be in upstream. |
|
@xtremerui I tested this PR on the master branch of dex. Worked perfectly fine. |
|
@karunchennuri glad it works. The test could be ran using the Lines 44 to 45 in 2ca992e
make test |
|
@xtremerui Would you be open to update this PR since it has conflicts? |
|
@mvdkleijn absolutely. Most of my PRs got conflicts I will update all of them. Thank you! |
d06e60b to
37225ee
Compare
579a1de to
133d9b6
Compare
47d4519 to
b648030
Compare
b648030 to
00c37b6
Compare
5b265dd to
12421b2
Compare
12421b2 to
378e4c2
Compare
4530e4b to
1eeff38
Compare
1eeff38 to
93b8d35
Compare
e84cf0e to
dbb4f3e
Compare
|
@sagikazarmark could I get some feedback on this PR please? |
a031678 to
73f1657
Compare
1905f8f to
4e3fe5b
Compare
4e3fe5b to
895f3b2
Compare
|
Hey folks, is there anything I can do to help this PR move forward? |
895f3b2 to
7a07880
Compare
7a07880 to
3f0f531
Compare
|
Hi there, any intention still to merge this feature? |
|
I've tested the fork this change is based on and found that in the case of a public client, Dex will issue client credentials for the public client if I provide the client ID and leave out the secret value (or set it to null). This is an issue for our use; simply knowing the client ID (which is sent in plaintext by the UI when getting a token) is enough to get an access token. Not sure if that has been resolved in this PR though. We've ended up using the Device Code Flow with a staticClient and staticPassword as an alternative, but having full support for the Client Credentials Grant would be better. |
|
With the allowedGrants, I think now we can add the support for client credentials grant and make it not enabled by default. @sagikazarmark what do you think? |
|
Hello, Thank you very much for your work. Thanks in advance :-) |
3f0f531 to
f72d7eb
Compare
f72d7eb to
08844c6
Compare
08844c6 to
ce3617b
Compare
Signed-off-by: Rui Yang <ruiya@vmware.com>
This fixes two issues in the existing client credentials change: - client_credentials was not listed as a supported grant type - access tokens are not the storage ID Signed-off-by: Michael Kelly <mkelly@arista.com>
Signed-off-by: Rui Yang <ruiya@vmware.com>
ce3617b to
3925296
Compare
|
@sagikazarmark and @nabokihms wanted to re-raise what you mentioned about including this and leaving it disabled by default. What would you think? Reason I ask is that I know folks are using this for robot / machine accounts. It might also be acceptable to add the ability to have the hidden connectors that robot / machine accounts utilize without having that prompt users. |
|
@sagikazarmark We have exactly the same issue as @xtremerui and @kellyma2 with Flyte. Flyte deploys in a Kubernetes env and has a concept of a worker which needs to authenticate to the Flyte control plane. We can only setup a single OIDC/OAuth2 server, and we use Dex for it (Okta connector). Flyte uses client credentials grant for the worker. As per @nabokihms @cardoe suggestions, would it be fine to disable it by default. Thanks |
|
I'm inclined to accept this given the popular demand. Unfortunately I have limited time, so if someone could run some tests, it would be immensely helpful. The code change itself is relatively small, so once we have confirmation it works as expected, we should be able to get this merged relatively quickly. (I'm constantly flooded by GitHub emails, so keep pinging me if I don't respond. Thanks! 🙏 ) |
|
To further this along, I've pulled this PR into build that I made at https://github.com/cardoe/dex/releases/tag/v2.44.90 You can grab the container to test with at ghcr.io/cardoe/dex:v2.44.90 and report back on this PR. |
|
I pulled this PR as well on my side and have tested it against Flyte (our use case). Works without issues. I have reviewed the code which looks alright (although I'm new to the codebase). My 2 cents |
sagikazarmark
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really hate to be the gatekeeper (especially since @xtremerui has been rebasing this PR for years now), but I had some comments, one of them is potentially a blocker.
| w.Write(claims) | ||
| } | ||
|
|
||
| func (s *Server) handleClientCredentialsGrant(w http.ResponseWriter, r *http.Request, client storage.Client) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct me if I'm wrong, but client credentials grant isn't supposed to work with public clients, is it?
Shouldn't there be some sort of check here?
Technically, withClientFromStorage checks the secret, but public clients have empty secret, so it will always match.
If I'm right, this is a rather serious vulnerability.
@cardoe any chance you could give this a try in your build?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it would probably be a good idea to cover at least the happy path with tests.
|
|
||
| claims := storage.Claims{UserID: client.ID} | ||
|
|
||
| accessToken, _, err := s.newAccessToken(r.Context(), client.ID, claims, scopes, nonce, "client") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is generating an access token always necessary? Shouldn't it be returned based on the requested scopes?
| accessToken, _, err := s.newAccessToken(r.Context(), client.ID, claims, scopes, nonce, "client") | ||
| if err != nil { | ||
| s.logger.ErrorContext(r.Context(), "failed to create new access token", "err", err) | ||
| s.tokenErrHelper(w, errServerError, err.Error(), http.StatusInternalServerError) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've just merged #4457 removing internal error messages from responses. I think a generic error message should be returned here.
See the linked PR.
|
|
||
| idToken, expiry, err := s.newIDToken(r.Context(), client.ID, claims, scopes, nonce, accessToken, "", "client") | ||
| if err != nil { | ||
| s.tokenErrHelper(w, errServerError, fmt.Sprintf("failed to create ID token: %v", err), http.StatusInternalServerError) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
so client can get access to Dex resources e.g. forwarding auth request and performing token exchange while handling callback from Dex.