-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Conversation
Pinging @elastic/kibana-security |
💔 Build Failed |
💔 Build Failed |
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.
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.
x-pack/plugins/security/server/lib/authentication/providers/oidc.js
Outdated
Show resolved
Hide resolved
x-pack/plugins/security/server/lib/authentication/providers/oidc.js
Outdated
Show resolved
Hide resolved
x-pack/plugins/security/server/lib/authentication/providers/oidc.js
Outdated
Show resolved
Hide resolved
x-pack/plugins/security/server/lib/authentication/providers/oidc.js
Outdated
Show resolved
Hide resolved
💔 Build Failed |
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
I added tests for whatever I could think off can be tested without an OpenID Connect Provider. |
💔 Build Failed |
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? |
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. |
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 ;) |
💔 Build Failed |
💔 Build Failed |
Closed in favor of #36201 |
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:
state
andnonce
) as we do withrequestId
in the SAML realm. Instead if an existing value ( forstate
andnonce
) 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.Third Party initiated authentication
inOIDC-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:
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
strikethroughsto remove checklist items you don't feel are applicable to this PR.Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n supportThis was checked for keyboard-only and screenreader accessibilityFor maintainers
This was checked for breaking API changes and was labeled appropriately