Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions x-pack/plugins/security/__snapshots__/index.test.js.snap

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 9 additions & 5 deletions x-pack/plugins/security/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
},

Expand Down
76 changes: 68 additions & 8 deletions x-pack/plugins/security/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
});
});
});
});
47 changes: 30 additions & 17 deletions x-pack/plugins/security/server/lib/authentication/authenticator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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],
Expand Down Expand Up @@ -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<Record<string, AuthenticationProviderSpecificOptions | undefined>>(
`xpack.security.authc`
);
if (authc && authc[providerType] !== undefined) {
return authc[providerType] as AuthenticationProviderSpecificOptions;
}

return {};
}

Expand All @@ -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);
}

/**
Expand Down Expand Up @@ -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];
})
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,7 @@ import { stub } from 'sinon';
import { AuthenticationProviderOptions } from './base';

export function mockAuthenticationProviderOptions(
providerOptions: Partial<AuthenticationProviderOptions> = {},
providerSpecificOptions: Record<string, unknown> = {}
providerOptions: Partial<AuthenticationProviderOptions> = {}
) {
return {
hostname: 'test-hostname',
Expand All @@ -19,6 +18,5 @@ export function mockAuthenticationProviderOptions(
log: stub(),
basePath: '/base-path',
...providerOptions,
...providerSpecificOptions,
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,15 @@ export interface AuthenticationProviderOptions {
log: (tags: string[], message: string) => void;
}

/**
* Represents available provider specific options.
*/
export type AuthenticationProviderSpecificOptions = Record<string, unknown>;

/**
* 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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down
33 changes: 21 additions & 12 deletions x-pack/plugins/security/server/lib/authentication/providers/oidc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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<AuthenticationProviderOptions>,
oidcOptions: Readonly<AuthenticationProviderSpecificOptions>
) {
super(options);
if (!this.options.realm) {
if (!oidcOptions.realm) {
throw new Error('Realm name must be specified');
}

if (type(oidcOptions.realm) !== 'string') {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Not a criticism, just a question: what does this give us over the built in typeof operator?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

For string type checking, we could just use typeof directly, it mainly comes in handy for those “gotcha types”.

throw new Error('Realm must be a string');
}

this.realm = oidcOptions.realm as string;
}

/**
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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(
Expand Down
6 changes: 3 additions & 3 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down