Implement HTTP Authentication provider and allow ApiKey authentication by default.#58126
Merged
azasypkin merged 4 commits intoelastic:masterfrom Feb 28, 2020
Merged
Conversation
Contributor
|
Pinging @elastic/kibana-security (Team:Security) |
ff60dbe to
a531d9b
Compare
a531d9b to
dd03c4f
Compare
azasypkin
commented
Feb 21, 2020
| return authc; | ||
| } | ||
|
|
||
| export const authenticationMock = { |
Contributor
Author
There was a problem hiding this comment.
note: consumers are supposed to use mocks provided by the plugin itself and to not redefine it to be as much in sync with real types as possible.
azasypkin
commented
Feb 21, 2020
| throw error; | ||
| } | ||
| this.log.debug(`Attempting to authenticate a user`); | ||
| const user = authentication!.getCurrentUser(request); |
Contributor
Author
There was a problem hiding this comment.
note: getCurrentUser is synchronous and doesn't throw errors anymore, but it can return null.
azasypkin
commented
Feb 21, 2020
| ['Basic xxx yyy', 'basic'], | ||
| ['basic xxx', 'basic'], | ||
| ['basic', 'basic'], | ||
| // We don't trim leading whitespaces in scheme. |
Contributor
Author
There was a problem hiding this comment.
note: we never trimmed so we're not changing anything here (not sure if Core HTTP service does this though).
azasypkin
commented
Feb 21, 2020
| * you may not use this file except in compliance with the Elastic License. | ||
| */ | ||
|
|
||
| import sinon from 'sinon'; |
Contributor
Author
There was a problem hiding this comment.
note: that's what caused 50%+ of the changes in this PR - I finally decided to drop support for sinon in auth provider tests as more changes in auth providers are coming (Login Selector) and I don't want to increase technical debt.
azasypkin
commented
Feb 21, 2020
|
|
||
| const authenticationResult = await provider.login( | ||
| httpServerMock.createKibanaRequest(), | ||
| httpServerMock.createKibanaRequest({ headers: {} }), |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
azasypkin
commented
Feb 21, 2020
| }, | ||
| { | ||
| validate(value) { | ||
| if (value.providers.includes('http')) { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
azasypkin
commented
Feb 21, 2020
|
|
||
| if (hasProvider('basic') && hasProvider('token')) { | ||
| log( | ||
| 'Enabling both `basic` and `token` authentication providers in `xpack.security.authc.providers` is deprecated. Login page will only use `token` provider.' |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
azasypkin
commented
Feb 21, 2020
|
|
||
| export default function({ getService }: FtrProviderContext) { | ||
| const supertest = getService('supertest'); | ||
| const supertestWithoutAuth = getService('supertestWithoutAuth'); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
azasypkin
commented
Feb 21, 2020
| ), | ||
| http: schema.object({ | ||
| enabled: schema.boolean({ defaultValue: true }), | ||
| autoSchemesEnabled: schema.boolean({ defaultValue: true }), |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
Author
|
Hey @kobelb, this one is ready for the first review pass whenever you have time, thanks! |
Contributor
|
ACK: reviewing first thing tomorrow, apologies for the delay |
kobelb
reviewed
Feb 26, 2020
Contributor
kobelb
left a comment
There was a problem hiding this comment.
This is just great! I really like the approach that you took here.
|
|
||
| export default function({ getService }: FtrProviderContext) { | ||
| const supertest = getService('supertest'); | ||
| const supertestWithoutAuth = getService('supertestWithoutAuth'); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
|
||
| if (hasProvider('basic') && hasProvider('token')) { | ||
| log( | ||
| 'Enabling both `basic` and `token` authentication providers in `xpack.security.authc.providers` is deprecated. Login page will only use `token` provider.' |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| ), | ||
| http: schema.object({ | ||
| enabled: schema.boolean({ defaultValue: true }), | ||
| autoSchemesEnabled: schema.boolean({ defaultValue: true }), |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| }, | ||
| { | ||
| validate(value) { | ||
| if (value.providers.includes('http')) { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
|
||
| const authenticationResult = await provider.login( | ||
| httpServerMock.createKibanaRequest(), | ||
| httpServerMock.createKibanaRequest({ headers: {} }), |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
|
||
| constructor( | ||
| protected readonly options: Readonly<AuthenticationProviderOptions>, | ||
| proxyOptions?: Readonly<Partial<HTTPAuthenticationProviderOptions>> |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| mockOptions.client.asScoped.mockReturnValue(mockScopedClusterClient); | ||
|
|
||
| const authenticationResult = await provider.authenticate(request, null); | ||
|
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| [TokenAuthenticationProvider.type, TokenAuthenticationProvider], | ||
| [OIDCAuthenticationProvider.type, OIDCAuthenticationProvider], | ||
| [PKIAuthenticationProvider.type, PKIAuthenticationProvider], | ||
| [HTTPAuthenticationProvider.type, HTTPAuthenticationProvider], |
Contributor
There was a problem hiding this comment.
Do we get some benefit I'm not realizing from making the HTTPAuthenticationProvider part of the providerMap? If it wasn't part of the providerMap, could we get rid of the custom validate function? Separately, I was wondering whether we could pass all of the enabled providers into the HTTPAuthenticationProvider constructor, add a method to each provider to denote which auth header should be supported, and use this as the basis for isSchemeSupported...
Contributor
Author
There was a problem hiding this comment.
Do we get some benefit I'm not realizing from making the HTTPAuthenticationProvider part of the providerMap?
Not really, was experimenting with different options and just stopped at some point.
If it wasn't part of the providerMap, could we get rid of the custom validate function?
Yep, that would simplify the config part.
Separately, I was wondering whether we could pass all of the enabled providers into the HTTPAuthenticationProvider constructor, add a method to each provider to denote which auth header should be supported, and use this as the basis for isSchemeSupported...
That's a great idea! But I'd propose slightly modified version - Authenticator will calculate proper supportedSchemes on its own and just give them to HTTPAuthenticationProvider so that it doesn't have to deal with other providers or even know that they exist. I'll make that change and you'll tell if it looks good to you or not :)
… supported by the HTTP authentication provider.
Contributor
Author
|
@kobelb thanks for review! PR should be ready for another review pass. |
kobelb
approved these changes
Feb 27, 2020
Contributor
kobelb
left a comment
There was a problem hiding this comment.
The improvements to the tests and the changes to the HTTP authentication provider look great!!
This was referenced Feb 28, 2020
Contributor
Author
|
7.x/7.7.0: 7fd27c3 |
Contributor
|
Great to see this happening and having it enabled by default. @nchaulet We need to check in Fleet for the case this is disabled. |
This was referenced Mar 3, 2020
Contributor
💔 Build FailedTest FailuresKibana Pipeline / kibana-xpack-agent / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/machine_learning/anomaly_detection/single_metric_job·ts.machine learning anomaly detection single metric job cloning creates the job and finishes processingStandard OutStack TraceKibana Pipeline / kibana-xpack-agent / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/transform/creation_index_pattern·ts.transform creation_index_pattern batch transform with terms+date_histogram groups and avg agg adds the aggregation entriesStandard OutStack TraceKibana Pipeline / kibana-xpack-agent / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/transform/creation_index_pattern·ts.transform creation_index_pattern batch transform with terms+date_histogram groups and avg agg adds the aggregation entriesStandard OutStack Traceand 1 more failures, only showing the first 3. History
To update your PR or re-run it, just comment with: |
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR moves HTTP authentication logic (the one that relies on HTTP
Authorization: Basic/Bearer/ApiKeyheader) out of existing providers into a dedicated one. This newHTTP Authenticationprovider is enabled by default and also supports two additional features out of the box (configured viakibana.yml):It allows authentication of the requests with
AuthorizationHTTP header with schemes used by other currently enabled authentication providers (basic--->Authorization: Basic xxxx,saml--->Authorization: Bearer xxxetc.). It's needed for the BWC reasons, we may want to turn this off once all dependent code switches to API keys for authentication.It allows authentication of the requests using Elasticsearch API keys within
AuthorizationHTTP header (requirement for Authentication to Kibana using API Keys #56087)So if we expand default configuration it will look like this:
Also this new functionality can be used to allow requests to the Kibana APIs using
Authorization: Basic xxxeven ifbasicauthentication provider isn't enabled (requirement for #53910). Here is example config:The configuration above will allow user login only via SAML and additionally programmatic access to the Kibana APIs with authentication via
Authorization: ApiKey xxxandAuthorization: Basic xxxHTTP headers.Note to reviewers: a big chunk of the changes are related to the tests (refactored them to finally remove
sinondependency).Fixes: #56087, #53910
Prerequisite for: #53010
"Release Note: Kibana now allows authentication via Elasticsearch API keys by default."