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

Add OpenID Connect auth provider #29943

Closed
wants to merge 5 commits into from
Closed

Add OpenID Connect auth provider #29943

wants to merge 5 commits into from

Conversation

jkakavas
Copy link
Member

@jkakavas jkakavas commented Feb 4, 2019

Summary

The OpenID Connect authProvider is the accompanying authProvider for the OpenID Connect authentication realm in Elasticsearch. This is very similar to the saml authProvider in most ways with three noticeable differences:

  • We require explicit configuration regarding the Elasticsearch realm name instead of trying to build an environment aware string (like ACS URL in saml) and pass that to Elasticsearch for it to resolve the realm.
  • We do not support multiple values for the realm specific nonces (state and nonce) as we do with requestId in the SAML realm. Instead if an existing value ( for state and nonce) is present in the user's session, we pass that to Elasticsearch to be reused. The end goal is the same, allow a better UX for users attempting many requests over different tabs in the same browser context.
  • IDP initiated SSO ( Third Party initiated authentication in OIDC-speak ) is implemented but starts as an unsolicited request to initiate the handshake, instead of an unsolicited request with an authentication response (which is not supported here)

Missing:

  • Documentation

Limitations

This does not support the OpenID Connect Implicit flow as that depends on fragment handling/processing as described for instance in the spec

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@jkakavas jkakavas added release_note:enhancement review v7.0.0 Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! labels Feb 4, 2019
@jkakavas jkakavas requested a review from kobelb February 4, 2019 12:19
@jkakavas jkakavas requested a review from a team as a code owner February 4, 2019 12:19
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Contributor

@kobelb kobelb left a comment

Choose a reason for hiding this comment

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

The code for this is looking great, almost all nits. I haven't had a chance to try it out with the OIDC realm in ES yet, and ideally we'd have some API integration tests for this. If you'd like for us to add these, I don't mind at all, but I'm starting to think that 6.7/7.0 is looking challenging.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@jkakavas
Copy link
Member Author

jkakavas commented Feb 5, 2019

Thanks for the review @kobelb. Sorry for the left over debug logging, my "search and replace"-fu failed me.

Regarding the API integration tests, you are right. The relevant branch in Elasticsearch will hopefully be merged today so we couldn't have run them even if we had some. The real problem here is that

  • Kibana doesn't support the implicit flow in which the tokens are sent in the front channel via the browser and which would allow us to generate and consume synthetic ID tokens in a way similar to what we do for SAML responses for example.
  • The authorization code grant flow requires backchannel comms so it needs either a working OP or a mocked one so that the Elasticsearch instance in the kibana integration tests would make HTTP requests to the Token Endpoint to exchange the code for an ID Token and possibly to the UserInfo endpoint to get additional claims for the user. I was not sure how to approach this.

I added tests for whatever I could think off can be tested without an OpenID Connect Provider.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@kobelb
Copy link
Contributor

kobelb commented Feb 5, 2019

Kibana doesn't support the implicit flow in which the tokens are sent in the front channel via the browser and which would allow us to generate and consume synthetic ID tokens in a way similar to what we do for SAML responses for example.
The authorization code grant flow requires backchannel comms so it needs either a working OP or a mocked one so that the Elasticsearch instance in the kibana integration tests would make HTTP requests to the Token Endpoint to exchange the code for an ID Token and possibly to the UserInfo endpoint to get additional claims for the user. I was not sure how to approach this.

Gotcha, we're in a similar position with Kerberos and we're intending to borrow the Kerberos Vagrant test fixture from ES and run it along-side Kibana/ES for our api integration tests. Do you know if we have the same capability with this auth provider?

@jkakavas
Copy link
Member Author

jkakavas commented Feb 5, 2019

You mean as discussed in #18188 ? Yes, there are a few options for an on prem OP that we can run in there. I'll look into it

@kobelb
Copy link
Contributor

kobelb commented Feb 5, 2019

You mean as discussed in #18188 ? Yes, there are a few options for an on prem OP that we can run in there. I'll look into it

Yup! I feel bad putting this burden on you, as you've already done so much to help us out, but merging a auth provider without integration tests puts a lot of burden on our QA people because it's such a laborious task to test this manually. If you'd at any point in time like to hand this off to us, we can definitely take it from here.

@jkakavas
Copy link
Member Author

jkakavas commented Feb 5, 2019

Yup! I feel bad putting this burden on you, as you've already done so much to help us out, but merging a auth provider without integration tests puts a lot of burden on our QA people

No worries, I'd be wary of merging without integration tests also. I'll take a look and we can reshuffle ownership as needed for the next minor release ;)

@elasticmachine
Copy link
Contributor

💔 Build Failed

@snide snide removed v7.0.0 labels Apr 10, 2019
@jkakavas jkakavas requested a review from azasypkin May 7, 2019 14:56
@jkakavas jkakavas changed the title Add OpenID Connect auth provider and unit tests Add OpenID Connect auth provider May 7, 2019
@jkakavas jkakavas closed this May 7, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@jkakavas
Copy link
Member Author

jkakavas commented May 7, 2019

Closed in favor of #36201

@azasypkin azasypkin removed their request for review May 13, 2019 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:enhancement review Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants