-
Notifications
You must be signed in to change notification settings - Fork 69
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
documentation for token exchanges #140
Conversation
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.
From my point of view, everything looks good.
@nate-double-u could you also review the PR from your perspective?
staticClients: | ||
# dex issued tokens are bound to clients. | ||
# For the token exchange flow, the client id and secret pair must be submitted as the username:password | ||
# via Basic Authentication. | ||
- name: My App | ||
id: my-app | ||
secret: my-secret | ||
# We set public to indicate we don't intend to keep the client secret actually secret. | ||
# https://dexidp.io/docs/custom-scopes-claims-clients/#public-clients | ||
public: true |
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'm confused by the presence of the staticClients
block -- when I first read this, my mental model was that it would be necessary to declare clients (either in a staticClients
block or via the gRPC API) in order to conduct token exchange for them. However, in the example below that shows how token exchange can be used in Github Actions (which happens to be something I am working on right now, so this doc update is quite timely for me!) there are no staticClients
configured in the Dex config, nor does there appear to be any interaction with Dex aside from the token exchange POST to the /token
endpoint. Could you clarify whether or not a client would need to be configured (through staticClients
or otherwise) in order to support token exchange for a given client?
It seems that because the client ID and secret must be provided in the basic auth for the token exchange post, that perhaps Dex has all the information it needs without the client actually having been configured, but in that case I do not understand why the client is configured here, since this config seems to only allow token exchange. I understand it would be necessary to include the client when we want to implement both token exchange and user-mediated auth flows, and perhaps the purpose of including this block in the example is so you can include the comment regarding setting public: true
because the existence of the token exchange flow indicates that the client secret is not really a secret, but at this point I feel I am guessing quite a lot and it would be very helpful for the documentation to clarify this.
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.
Sorry, i left out part of the config during the copy/paste, a static client is required.
Line 68 is correct.
Within dex, there are 2 separate entities: connectors (upstreams that authenticate users), and clients (downstreams that trigger authentication, and use the dex issued token). The tokens that dex issued are always bound to both: it's bound by which connector authenticated the user (exposed when the reequested scope includes federated:id
, and which client is expected to consume the token (always exposed as audience / aud
).
In a token exchange, dex has no prior information about how to bind the tokens (in a web flow, the app that triggers auth will identify itself as the client), so the user has to provide both: it identifies the connector with connector_id
and it identifies the client with client id + client secret in basic auth.
In a dual user/machine scenario, you'll likely want to make use of dex's cross client trust:
your consuming application is configured with a real pair of client id/secret, and trusts a public client for use with token exchange.
staticClients:
- name: My App - Humans
# configure you app with these
id: my-app
secret: keep-this-secret-an-actual-secret
trustedPeers:
- my-app-ci
- name: My App - CI
# perform token exchange with these, and request scope `audience:server:client_id:my-app`
id: my-app-ci
secret: not-really-a-secret
public: true
Given the above information, what would make the docs clearer (though I feel explaining connectors/clients is out of scope for this page).
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 agree that explaining connectors/clients should be out of scope for this page. I think my main confusion came from the missing config block, as I originally had the right idea that a (public) client needs to be configured.
I do think that since the use of token exchange necessitates treating the client as public as well as the use-cases involving token exchange being likely to be apps that also have real (i.e. human) users, it would be helpful to include the example (essentially from your comment) of how one might want to employ cross-client trust to trust the public client and then request the scope of the main application client. Or at a minimum, these docs could mention that with a reference to the main cross client trust docs without giving an example (though I would note that those docs do not seem to specifically address the use-case of trusting a public client, though perhaps the section on public clients would be a more appropriate place to include that as an option, and this doc could then just reference that).
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've added a paragraph explaining why a public client is required, plus a link to cross client trust.
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.
Thanks, that looks great.
Co-authored-by: Maksim Nabokikh <[email protected]> Signed-off-by: Sean Liao <[email protected]>
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.
Thanks for this @seankhliao, here is a copy-edit pass, feel free to take or leave suggestions you agree with or don't 🙂
Co-authored-by: Nate W <[email protected]> Signed-off-by: Sean Liao <[email protected]>
Thanks for the review, I think they all make sense and applied them. |
Awesome, LGTM from me then 🙂 |
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.
@seankhliao thank you! The time has come.
No description provided.