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

Support the implicit grant flow (w/ response_mode=post_form) #1256

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

colemickens
Copy link

@colemickens colemickens commented Jun 26, 2018

This stacks on top of @JoelSpeed's #1180 (rebased on coreos/master as of today). (The relevant commit is 445da8d)

This implements the implicit grant flow. This enables the OIDC connector to work with Portier. Portier only supports the implicit grant flow and only returns an id_token.

This flow requires a different method for Dex to receive the id_token. Given that Dex is a server-side component, it didn't seem prudent to implement the response_mode=fragment, so I implemented response_mode=post_form which was very straightforward.

Details:

  1. bump up go-oidc. I know it removed nonce validation recently, and I'd already implemented it here
  2. Expose a response_type so the user can request id_token only.
  3. When we detect this, specify when calling the OIDC endpoint response_mode=form_post, create a nonce, persist it with the in-flight auth_request.
  4. Update sql/kubernetes storage to include/convert the ConnectorData in the AuthRequest
  5. Modify the handlers so that it would allow POST to a non-SAML connector (to implement form_post).

Oddities:

  1. We store a dummy value in offline_sessions.connector_data rather than writing a migration to handle a null field (the implicit grant_type doesn't return a refresh token). I'm going to make a note in Implement refreshing with Google #1180 that we might want to allow null for the connector data.

  2. I've not tested with a Kubernetes data store yet.

@soggiest
Copy link

Hey @colemickens i was looking to test this branch out in a k8s cluster, do you mind rebasing to the new repo? The vendoring is off since moving to dexidp

@colemickens colemickens force-pushed the refresh-tokens-rebased branch from 445da8d to 0ebe5f5 Compare December 18, 2018 05:46
@hickey
Copy link

hickey commented Jan 23, 2019

@colemickens Just wanted to call to your attention that need to resolve the conflict in server/handlers.go so that this branch can be tested. I just did a rebase against the v2.14.0 branch and it was a simple case or changing "GET" to http.MethodGet. It would be nice to get this moving forward.

Thanks.

@colemickens colemickens force-pushed the refresh-tokens-rebased branch from 0ebe5f5 to 4daf2e1 Compare January 23, 2019 03:43
@colemickens
Copy link
Author

@hickey I rebased, and fixed tests so that they pass (locally, at least). Hope this helps.

@hickey
Copy link

hickey commented Jan 31, 2019

@colemickens I hope I am not producing smoke here... I am still learning a lot about OIDC and Dex. Feel free to push this back to me if there are configuration issues on my side.

I have built a new Dex (2.13.0) with your branch and I am connecting to Okta. I have tried with both a responseType of 'id_token' and 'code' and in both cases I receive the following error from Dex:

time="2019-01-31T00:45:49Z" level=error msg="Failed to authenticate: failed to parse connector data: unexpected end of JSON input"

Another 2 things to note is it looks like to me that Dex reports back that it can also accept a response type of token. I get the following back from Dex:

{
  "issuer": "https://dex.staging....com:5556/kubeauth",
  "authorization_endpoint": "https://dex.staging....com:5556/kubeauth/auth",
  "token_endpoint": "https://dex.staging....com:5556/kubeauth/token",
  "jwks_uri": "https://dex.staging....com:5556/kubeauth/keys",
  "response_types_supported": [
    "code",
    "id_token",
    "token"
  ],
  "subject_types_supported": [
    "public"
  ],
....
}

Yet if responseType: token is added to the configuration, Dex errors out with

failed to initialize server: server: Failed to open connector okta-test: failed to open connector: failed to create connector okta-test: failed to create okta-test provider, unsupported response_type 'token'

Finally, in dex/connector/oidc/oidc.go the responseType indicates that it defaults to "code". Yet if responseType is not specified in the connector configuration the Dex fails with the following error:

failed to initialize server: server: Failed to open connector okta: failed to open connector: failed to create connector okta: failed to create okta provider, unsupported response_type ''

I think the last two items are just fixing the code with the expected use cases (assuming that they are valid and I think that they are). As far as the unexpected JSON input that I am getting, I will leave that to those that know more than I do to determine if that is a configuration problem on my side or if this is a real issue.

@colemickens colemickens force-pushed the refresh-tokens-rebased branch from 4daf2e1 to 4a29e6f Compare February 16, 2019 05:14
@colemickens
Copy link
Author

Hey @hickey, sorry it took longer to get back to this than I'd hoped. I rebased my branch again, and also created a demo repository: https://github.com/colemickens/dex-portier-demo. It contains config files for Dex (configured to use oidc type with Portier) and for Portier itself, along with a demo video of it in action.

Small things first:

  1. You'll surely need to set scopes: ["id_token"] for your connector config, as I have done in my demo. If that doesn't happen, Portier (and presumably other conforming OIDC IdPs) will throw back an error about invalid scopes (Dex defaults to more, I don't recall exactly which). Also, unfortunately Dex more or less swallows that error and doesn't show it, though you can see it in Dev Tools in the browser.

  2. Yes, there was a bug in that responseType was not defaulted to code, per the comment. I've fixed that up so it defaults back to code. Note, that is not allowed with the implicit flow, so I wouldn't expect that to work.

  3. response_type=token is not allowed with the implicit grant type; the allowed response types are id_token and id_token token. Since this code has only ever consumed the id_token, and Portier only allows id_token, I didn't add support for id_token token. That could be done at a later time.

As for bad connectorData, here are some thoughts on causes, given that the connectorData is serialized on start of flow and deserialized on response from IdP to Dex:

  1. There is some corruption at the data layer or bug in my code that writes/reads it.
  2. You somehow got a session started before pulling my code, didn't clear cookies in browser, then pulled my code and started to complete the flow in your browser. Portier would push back a response, Dex would load your old flow (since the session_id is correct, and the nonce would be as well, though it's checked afterward), and then fail to load the connectorData.

I've added some code that will at least give a different error if the connectorData is empty. Let me know if this repros in a private browser session reliably, etc.

@colemickens colemickens force-pushed the refresh-tokens-rebased branch 2 times, most recently from 1e6df46 to 00a207d Compare February 16, 2019 09:20
@colemickens colemickens force-pushed the refresh-tokens-rebased branch from 00a207d to 9ff5eb1 Compare February 16, 2019 09:47
@colemickens
Copy link
Author

@hickey I added one more fix that resolved a test failure related to SQL. Maybe it was the cause of the issue you were seeing with connector Data.

I was able to get Okta working pretty easily, please check the demo repo.

I also resolved the final build failures.

@colemickens
Copy link
Author

I have no idea if this will ever be unblocked, but if it were, we might consider not merging it, as it seems like implicit flow is discouraged moving forward: https://developer.okta.com/blog/2019/05/01/is-the-oauth-implicit-flow-dead

@Brian-McM
Copy link

I have no idea if this will ever be unblocked, but if it were, we might consider not merging it, as it seems like implicit flow is discouraged moving forward: https://developer.okta.com/blog/2019/05/01/is-the-oauth-implicit-flow-dead

@colemickens I know it's discouraged but some custom IdPs might not support the explicit flow (I have customers in such a situation). It would be great if this could get through.

@arianvp
Copy link

arianvp commented Oct 15, 2021

Note that implicit flow with form_post does not suffer the same security issues https://auth0.com/docs/authorization/flows/implicit-flow-with-form-post

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants