diff --git a/x-pack/plugins/security/__snapshots__/index.test.js.snap b/x-pack/plugins/security/__snapshots__/index.test.js.snap index dc07b4cce8c37..7dfd0824d69f4 100644 --- a/x-pack/plugins/security/__snapshots__/index.test.js.snap +++ b/x-pack/plugins/security/__snapshots__/index.test.js.snap @@ -1,5 +1,11 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP +exports[`config schema authc oidc realm realm is not allowed when authProviders is "['basic']" 1`] = `[ValidationError: child "authc" fails because ["oidc" is not allowed]]`; + +exports[`config schema authc oidc realm returns a validation error when authProviders is "['oidc', 'basic']" and realm is unspecified 1`] = `[ValidationError: child "authc" fails because [child "oidc" fails because [child "realm" fails because ["realm" is required]]]]`; + +exports[`config schema authc oidc realm returns a validation error when authProviders is "['oidc']" and realm is unspecified 1`] = `[ValidationError: child "authc" fails because [child "oidc" fails because [child "realm" fails because ["realm" is required]]]]`; + exports[`config schema with context {"dist":false} produces correct config 1`] = ` Object { "audit": Object { diff --git a/x-pack/plugins/security/index.js b/x-pack/plugins/security/index.js index cafbcb3e69b09..f0d016a658f61 100644 --- a/x-pack/plugins/security/index.js +++ b/x-pack/plugins/security/index.js @@ -65,11 +65,15 @@ export const security = (kibana) => new kibana.Plugin({ audit: Joi.object({ enabled: Joi.boolean().default(false) }).default(), - authc: Joi.object({ - oidc: Joi.object({ - realm: Joi.string() - }), - }), + authc: Joi.object({}) + .when('authProviders', { + is: Joi.array().items(Joi.string().valid('oidc').required(), Joi.string()), + then: Joi.object({ + oidc: Joi.object({ + realm: Joi.string().required(), + }).default() + }).default() + }) }).default(); }, diff --git a/x-pack/plugins/security/index.test.js b/x-pack/plugins/security/index.test.js index 4bc3f0e6da73b..035ada8dc4d18 100644 --- a/x-pack/plugins/security/index.test.js +++ b/x-pack/plugins/security/index.test.js @@ -7,16 +7,76 @@ import { security } from './index'; import { getConfigSchema } from '../../test_utils'; -const describeWithContext = describe.each([ - [{ dist: false }], - [{ dist: true }] -]); +const describeWithContext = describe.each([[{ dist: false }], [{ dist: true }]]); -describeWithContext('config schema with context %j', (context) => { +describeWithContext('config schema with context %j', context => { it('produces correct config', async () => { const schema = await getConfigSchema(security); - await expect( - schema.validate({}, { context }) - ).resolves.toMatchSnapshot(); + await expect(schema.validate({}, { context })).resolves.toMatchSnapshot(); + }); +}); + +describe('config schema', () => { + describe('authc', () => { + describe('oidc', () => { + describe('realm', () => { + it(`returns a validation error when authProviders is "['oidc']" and realm is unspecified`, async () => { + const schema = await getConfigSchema(security); + const validationResult = schema.validate({ + authProviders: ['oidc'], + }); + expect(validationResult.error).toMatchSnapshot(); + }); + + it(`is valid when authProviders is "['oidc']" and realm is specified`, async () => { + const schema = await getConfigSchema(security); + const validationResult = schema.validate({ + authProviders: ['oidc'], + authc: { + oidc: { + realm: 'realm-1', + }, + }, + }); + expect(validationResult.error).toBeNull(); + expect(validationResult.value).toHaveProperty('authc.oidc.realm', 'realm-1'); + }); + + it(`returns a validation error when authProviders is "['oidc', 'basic']" and realm is unspecified`, async () => { + const schema = await getConfigSchema(security); + const validationResult = schema.validate({ + authProviders: ['oidc', 'basic'], + }); + expect(validationResult.error).toMatchSnapshot(); + }); + + it(`is valid when authProviders is "['oidc', 'basic']" and realm is specified`, async () => { + const schema = await getConfigSchema(security); + const validationResult = schema.validate({ + authProviders: ['oidc', 'basic'], + authc: { + oidc: { + realm: 'realm-1', + }, + }, + }); + expect(validationResult.error).toBeNull(); + expect(validationResult.value).toHaveProperty('authc.oidc.realm', 'realm-1'); + }); + + it(`realm is not allowed when authProviders is "['basic']"`, async () => { + const schema = await getConfigSchema(security); + const validationResult = schema.validate({ + authProviders: ['basic'], + authc: { + oidc: { + realm: 'realm-1', + }, + }, + }); + expect(validationResult.error).toMatchSnapshot(); + }); + }); + }); }); }); diff --git a/x-pack/plugins/security/server/lib/authentication/authenticator.ts b/x-pack/plugins/security/server/lib/authentication/authenticator.ts index 82d9c305fad43..f2bbacbfb12c9 100644 --- a/x-pack/plugins/security/server/lib/authentication/authenticator.ts +++ b/x-pack/plugins/security/server/lib/authentication/authenticator.ts @@ -20,6 +20,7 @@ import { AuthenticationResult } from './authentication_result'; import { DeauthenticationResult } from './deauthentication_result'; import { Session } from './session'; import { LoginAttempt } from './login_attempt'; +import { AuthenticationProviderSpecificOptions } from './providers/base'; interface ProviderSession { provider: string; @@ -30,7 +31,10 @@ interface ProviderSession { // provider class that can handle specific authentication mechanism. const providerMap = new Map< string, - new (options: AuthenticationProviderOptions) => BaseAuthenticationProvider + new ( + options: AuthenticationProviderOptions, + providerSpecificOptions: AuthenticationProviderSpecificOptions + ) => BaseAuthenticationProvider >([ ['basic', BasicAuthenticationProvider], ['saml', SAMLAuthenticationProvider], @@ -65,16 +69,24 @@ function getProviderOptions(server: Legacy.Server) { } /** - * Prepares options object that is specific only to an authentication provider. This will be merged - * with the general options + * Prepares options object that is specific only to an authentication provider. * @param server Server instance. * @param providerType the type of the provider to get the options for. */ -function getProviderSpecificOptions(server: Legacy.Server, providerType: string) { +function getProviderSpecificOptions( + server: Legacy.Server, + providerType: string +): AuthenticationProviderSpecificOptions { const config = server.config(); - if (config.has(`xpack.security.authc.${providerType}`)) { - return config.get(`xpack.security.authc.${providerType}`); + // we can't use `config.has` here as it doesn't currently work with Joi's "alternatives" syntax which we + // are using to make the provider specific configuration required when the auth provider is specified + const authc = config.get>( + `xpack.security.authc` + ); + if (authc && authc[providerType] !== undefined) { + return authc[providerType] as AuthenticationProviderSpecificOptions; } + return {}; } @@ -83,13 +95,17 @@ function getProviderSpecificOptions(server: Legacy.Server, providerType: string) * @param providerType Provider type key. * @param options Options to pass to provider's constructor. */ -function instantiateProvider(providerType: string, options: AuthenticationProviderOptions) { +function instantiateProvider( + providerType: string, + options: AuthenticationProviderOptions, + providerSpecificOptions: AuthenticationProviderSpecificOptions +) { const ProviderClassName = providerMap.get(providerType); if (!ProviderClassName) { throw new Error(`Unsupported authentication provider name: ${providerType}.`); } - return new ProviderClassName(options); + return new ProviderClassName(options, providerSpecificOptions); } /** @@ -130,18 +146,15 @@ class Authenticator { ); } - const generalOptions = getProviderOptions(server); + const providerOptions = Object.freeze(getProviderOptions(server)); this.providers = new Map( authProviders.map(providerType => { - const providerOptions = Object.freeze({ - ...generalOptions, - ...getProviderSpecificOptions(server, providerType), - }); - return [providerType, instantiateProvider(providerType, providerOptions)] as [ - string, - BaseAuthenticationProvider - ]; + const providerSpecificOptions = getProviderSpecificOptions(server, providerType); + return [ + providerType, + instantiateProvider(providerType, providerOptions, providerSpecificOptions), + ] as [string, BaseAuthenticationProvider]; }) ); } diff --git a/x-pack/plugins/security/server/lib/authentication/providers/base.mock.ts b/x-pack/plugins/security/server/lib/authentication/providers/base.mock.ts index 2b5dff8ef7325..2317b04039017 100644 --- a/x-pack/plugins/security/server/lib/authentication/providers/base.mock.ts +++ b/x-pack/plugins/security/server/lib/authentication/providers/base.mock.ts @@ -8,8 +8,7 @@ import { stub } from 'sinon'; import { AuthenticationProviderOptions } from './base'; export function mockAuthenticationProviderOptions( - providerOptions: Partial = {}, - providerSpecificOptions: Record = {} + providerOptions: Partial = {} ) { return { hostname: 'test-hostname', @@ -19,6 +18,5 @@ export function mockAuthenticationProviderOptions( log: stub(), basePath: '/base-path', ...providerOptions, - ...providerSpecificOptions, }; } diff --git a/x-pack/plugins/security/server/lib/authentication/providers/base.ts b/x-pack/plugins/security/server/lib/authentication/providers/base.ts index 69e9729ce111a..01c8a260ee673 100644 --- a/x-pack/plugins/security/server/lib/authentication/providers/base.ts +++ b/x-pack/plugins/security/server/lib/authentication/providers/base.ts @@ -20,12 +20,15 @@ export interface AuthenticationProviderOptions { log: (tags: string[], message: string) => void; } +/** + * Represents available provider specific options. + */ +export type AuthenticationProviderSpecificOptions = Record; + /** * Base class that all authentication providers should extend. */ -export abstract class BaseAuthenticationProvider< - Options extends AuthenticationProviderOptions = AuthenticationProviderOptions -> { +export abstract class BaseAuthenticationProvider { /** * Instantiates AuthenticationProvider. * @param options Provider options object. diff --git a/x-pack/plugins/security/server/lib/authentication/providers/oidc.test.ts b/x-pack/plugins/security/server/lib/authentication/providers/oidc.test.ts index e4800b7d6fe0b..f7ecab4bcb8d6 100644 --- a/x-pack/plugins/security/server/lib/authentication/providers/oidc.test.ts +++ b/x-pack/plugins/security/server/lib/authentication/providers/oidc.test.ts @@ -17,14 +17,12 @@ describe('OIDCAuthenticationProvider', () => { let callWithRequest: sinon.SinonStub; let callWithInternalUser: sinon.SinonStub; beforeEach(() => { - const providerOptions = mockAuthenticationProviderOptions( - { basePath: '/test-base-path' }, - { realm: 'oidc1' } - ); + const providerOptions = mockAuthenticationProviderOptions({ basePath: '/test-base-path' }); + const providerSpecificOptions = { realm: 'oidc1' }; callWithRequest = providerOptions.client.callWithRequest as sinon.SinonStub; callWithInternalUser = providerOptions.client.callWithInternalUser as sinon.SinonStub; - provider = new OIDCAuthenticationProvider(providerOptions); + provider = new OIDCAuthenticationProvider(providerOptions, providerSpecificOptions); }); describe('`authenticate` method', () => { diff --git a/x-pack/plugins/security/server/lib/authentication/providers/oidc.ts b/x-pack/plugins/security/server/lib/authentication/providers/oidc.ts index fb627d0e94004..7ed9c11915a38 100644 --- a/x-pack/plugins/security/server/lib/authentication/providers/oidc.ts +++ b/x-pack/plugins/security/server/lib/authentication/providers/oidc.ts @@ -5,12 +5,17 @@ */ import Boom from 'boom'; +import type from 'type-detect'; import { Legacy } from 'kibana'; import { canRedirectRequest } from '../../can_redirect_request'; import { getErrorStatusCode } from '../../errors'; import { AuthenticationResult } from '../authentication_result'; import { DeauthenticationResult } from '../deauthentication_result'; -import { AuthenticationProviderOptions, BaseAuthenticationProvider } from './base'; +import { + AuthenticationProviderOptions, + BaseAuthenticationProvider, + AuthenticationProviderSpecificOptions, +} from './base'; /** * The state supported by the provider (for the OpenID Connect handshake or established session). @@ -64,13 +69,6 @@ type OIDCIncomingRequest = Legacy.Request & { }; }; -/** - * Defines the realm specific properties - */ -interface OIDCProviderOptions extends AuthenticationProviderOptions { - realm: string; -} - /** * Checks if the Request object represents an HTTP request regarding authentication with OpenID * Connect. This can be @@ -113,11 +111,22 @@ function isAccessTokenExpiredError(err?: any) { * Provider that supports authentication using an OpenID Connect realm in Elasticsearch. */ export class OIDCAuthenticationProvider extends BaseAuthenticationProvider { - constructor(protected readonly options: OIDCProviderOptions) { + private readonly realm: string; + + constructor( + protected readonly options: Readonly, + oidcOptions: Readonly + ) { super(options); - if (!this.options.realm) { + if (!oidcOptions.realm) { throw new Error('Realm name must be specified'); } + + if (type(oidcOptions.realm) !== 'string') { + throw new Error('Realm must be a string'); + } + + this.realm = oidcOptions.realm as string; } /** @@ -154,7 +163,7 @@ export class OIDCAuthenticationProvider extends BaseAuthenticationProvider { // We might already have a state and nonce generated by Elasticsearch (from an unfinished authentication in // another tab) return authenticationResult.notHandled() - ? await this.initiateOIDCAuthentication(request, { realm: this.options.realm }) + ? await this.initiateOIDCAuthentication(request, { realm: this.realm }) : authenticationResult; } @@ -441,7 +450,7 @@ export class OIDCAuthenticationProvider extends BaseAuthenticationProvider { this.debug( 'Both elasticsearch access and refresh tokens are expired. Re-initiating OpenID Connect authentication.' ); - return this.initiateOIDCAuthentication(request, { realm: this.options.realm }); + return this.initiateOIDCAuthentication(request, { realm: this.realm }); } return AuthenticationResult.failed( diff --git a/yarn.lock b/yarn.lock index 2ecf502fa4698..ea83cfb132cea 100644 --- a/yarn.lock +++ b/yarn.lock @@ -8457,9 +8457,9 @@ core-js@^2.5.3, core-js@^2.5.7: integrity sha512-RszJCAxg/PP6uzXVXL6BsxSXx/B05oJAQ2vkJRjyjrEcNVycaqOmNb5OTxZPE3xa5gwZduqza6L9JOCenh/Ecw== core-js@^2.6.5: - version "2.6.5" - resolved "https://registry.yarnpkg.com/core-js/-/core-js-2.6.5.tgz#44bc8d249e7fb2ff5d00e0341a7ffb94fbf67895" - integrity sha512-klh/kDpwX8hryYL14M9w/xei6vrv6sE8gTHDG7/T/+SEovB/G4ejwcfE/CBzO6Edsu+OETZMZ3wcX/EjUkrl5A== + version "2.6.6" + resolved "https://registry.yarnpkg.com/core-js/-/core-js-2.6.6.tgz#00eb6d6bf815471cc16d8563edd7d38786dec50b" + integrity sha512-Mt/LaAym54NXnrjEMdo918cT2h70tqb/Yl7T3uPHQHRm5SxVoqlKmerUy4mL11k8saSBDWQ7ULIHxmeFyT3pfg== core-util-is@1.0.2, core-util-is@~1.0.0: version "1.0.2"