Skip to content

Commit

Permalink
Make xpack.security.authc.saml.realm mandatory and completely remov…
Browse files Browse the repository at this point in the history
…e `xpack.security.authProviders` and `xpack.security.public`. (elastic#38657)
  • Loading branch information
azasypkin authored Jun 13, 2019
1 parent 120b060 commit ffb0b06
Show file tree
Hide file tree
Showing 11 changed files with 75 additions and 140 deletions.
20 changes: 20 additions & 0 deletions docs/migration/migrate_8_0.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,24 @@ for example, `logstash-*`.

*Impact:* To restore the previous behavior, in kibana.yml set `logging.timezone: UTC`.

[float]
==== `xpack.security.authProviders` is no longer valid
*Details:* The deprecated `xpack.security.authProviders` setting in the `kibana.yml` file has been removed.

*Impact:* Use `xpack.security.authc.providers` instead.

[float]
==== `xpack.security.authc.saml.realm` is now mandatory when using the SAML authentication provider
*Details:* Previously Kibana was choosing the appropriate Elasticsearch SAML realm automatically using the `Assertion Consumer Service`
URL that it derived from the actual server address. Starting in 8.0.0, the Elasticsearch SAML realm name that Kibana will use should be
specified explicitly.

*Impact:* Always define `xpack.security.authc.saml.realm` when using the SAML authentication provider.

[float]
==== `xpack.security.public` is no longer valid
*Details:* The deprecated `xpack.security.public` setting in the `kibana.yml` file has been removed.

*Impact:* Define `xpack.security.authc.saml.realm` when using the SAML authentication provider instead.

// end::notable-breaking-changes[]
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,6 @@ kibana_vars=(
xpack.reporting.queue.timeout
xpack.reporting.roles.allow
xpack.searchprofiler.enabled
xpack.security.authProviders
xpack.security.authc.providers
xpack.security.cookieName
xpack.security.enabled
Expand Down
4 changes: 0 additions & 4 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.

22 changes: 2 additions & 20 deletions x-pack/plugins/security/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
*/

import { resolve } from 'path';
import { get, has } from 'lodash';
import { getUserProvider } from './server/lib/get_user';
import { initAuthenticateApi } from './server/routes/api/v1/authenticate';
import { initUsersApi } from './server/routes/api/v1/users';
Expand Down Expand Up @@ -59,11 +58,6 @@ export const security = (kibana) => new kibana.Plugin({
}),
sessionTimeout: Joi.number().allow(null).default(null),
secureCookies: Joi.boolean().default(false),
public: Joi.object({
protocol: Joi.string().valid(['http', 'https']),
hostname: Joi.string().hostname(),
port: Joi.number().integer().min(0).max(65535)
}).default(),
authorization: Joi.object({
legacyFallback: Joi.object({
enabled: Joi.boolean().default(true) // deprecated
Expand All @@ -75,26 +69,14 @@ export const security = (kibana) => new kibana.Plugin({
authc: Joi.object({
providers: Joi.array().items(Joi.string()).default(['basic']),
oidc: providerOptionsSchema('oidc', Joi.object({ realm: Joi.string().required() }).required()),
saml: providerOptionsSchema('saml', Joi.object({ realm: Joi.string() })),
saml: providerOptionsSchema('saml', Joi.object({ realm: Joi.string().required() }).required()),
}).default()
}).default();
},

deprecations: function ({ unused, rename }) {
deprecations: function ({ unused }) {
return [
unused('authorization.legacyFallback.enabled'),
rename('authProviders', 'authc.providers'),
(settings, log) => {
const hasSAMLProvider = get(settings, 'authc.providers', []).includes('saml');
if (hasSAMLProvider && !get(settings, 'authc.saml.realm')) {
log('Config key "authc.saml.realm" will become mandatory when using the SAML authentication provider in the next major version.');
}

if (has(settings, 'public')) {
log('Config key "public" is deprecated and will be removed in the next major version. ' +
'Specify "authc.saml.realm" instead.');
}
}
];
},

Expand Down
41 changes: 20 additions & 21 deletions x-pack/plugins/security/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,24 +78,19 @@ describe('config schema', () => {
});

describe('saml', () => {
it('`realm` is optional', async () => {
it('fails if authc.providers includes `saml`, but `saml.realm` is not specified', async () => {
const schema = await getConfigSchema(security);

let validationResult = schema.validate({
authc: { providers: ['saml'] },
});

expect(validationResult.error).toBeNull();
expect(validationResult.value.authc.saml).toBeUndefined();

validationResult = schema.validate({
authc: { providers: ['saml'], saml: {} },
});
expect(schema.validate({ authc: { providers: ['saml'] } }).error).toMatchInlineSnapshot(
`[ValidationError: child "authc" fails because [child "saml" fails because ["saml" is required]]]`
);
expect(
schema.validate({ authc: { providers: ['saml'], saml: {} } }).error
).toMatchInlineSnapshot(
`[ValidationError: child "authc" fails because [child "saml" fails because [child "realm" fails because ["realm" is required]]]]`
);

expect(validationResult.error).toBeNull();
expect(validationResult.value.authc.saml.realm).toBeUndefined();

validationResult = schema.validate({
const validationResult = schema.validate({
authc: { providers: ['saml'], saml: { realm: 'realm-1' } },
});

Expand All @@ -105,12 +100,16 @@ describe('config schema', () => {

it('`realm` is not allowed if saml provider is not enabled', async () => {
const schema = await getConfigSchema(security);
expect(schema.validate({
authc: {
providers: ['basic'],
saml: { realm: 'realm-1' },
},
}).error).toMatchSnapshot();
expect(
schema.validate({
authc: {
providers: ['basic'],
saml: { realm: 'realm-1' },
},
}).error
).toMatchInlineSnapshot(
`[ValidationError: child "authc" fails because [child "saml" fails because ["saml" is not allowed]]]`
);
});
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,13 +60,7 @@ function getProviderOptions(server: Legacy.Server) {
return {
client: getClient(server),
log: server.log.bind(server),

protocol: server.info.protocol,
hostname: config.get<string>('server.host'),
port: config.get<number>('server.port'),
basePath: config.get<string>('server.basePath'),

...config.get('xpack.security.public'),
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,6 @@ export function mockAuthenticationProviderOptions(
providerOptions: Partial<AuthenticationProviderOptions> = {}
) {
return {
hostname: 'test-hostname',
port: 1234,
protocol: 'test-protocol',
client: { callWithRequest: stub(), callWithInternalUser: stub() },
log: stub(),
basePath: '/base-path',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,6 @@ export interface RequestWithLoginAttempt extends Legacy.Request {
* Represents available provider options.
*/
export interface AuthenticationProviderOptions {
protocol: string;
hostname: string;
port: number;
basePath: string;
client: Legacy.Plugins.elasticsearch.Cluster;
log: (tags: string[], message: string) => void;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,21 @@ describe('SAMLAuthenticationProvider', () => {
callWithRequest = providerOptions.client.callWithRequest as sinon.SinonStub;
callWithInternalUser = providerOptions.client.callWithInternalUser as sinon.SinonStub;

provider = new SAMLAuthenticationProvider(providerOptions);
provider = new SAMLAuthenticationProvider(providerOptions, { realm: 'test-realm' });
});

it('throws if `realm` option is not specified', () => {
const providerOptions = mockAuthenticationProviderOptions({ basePath: '/test-base-path' });

expect(() => new SAMLAuthenticationProvider(providerOptions)).toThrowError(
'Realm name must be specified'
);
expect(() => new SAMLAuthenticationProvider(providerOptions, {})).toThrowError(
'Realm name must be specified'
);
expect(() => new SAMLAuthenticationProvider(providerOptions, { realm: '' })).toThrowError(
'Realm name must be specified'
);
});

describe('`authenticate` method', () => {
Expand Down Expand Up @@ -73,36 +87,6 @@ describe('SAMLAuthenticationProvider', () => {

const authenticationResult = await provider.authenticate(request, null);

sinon.assert.calledWithExactly(callWithInternalUser, 'shield.samlPrepare', {
body: { acs: `test-protocol://test-hostname:1234/test-base-path/api/security/v1/saml` },
});

expect(authenticationResult.redirected()).toBe(true);
expect(authenticationResult.redirectURL).toBe(
'https://idp-host/path/login?SAMLRequest=some%20request%20'
);
expect(authenticationResult.state).toEqual({
requestId: 'some-request-id',
nextURL: `/s/foo/some-path`,
});
});

it('uses `realm` name instead of `acs` if it is specified for SAML prepare request.', async () => {
const request = requestFixture({ path: '/some-path', basePath: '/s/foo' });

// Create new provider instance with additional `realm` option.
const providerOptions = mockAuthenticationProviderOptions({ basePath: '/test-base-path' });
callWithRequest = providerOptions.client.callWithRequest as sinon.SinonStub;
callWithInternalUser = providerOptions.client.callWithInternalUser as sinon.SinonStub;
provider = new SAMLAuthenticationProvider(providerOptions, { realm: 'test-realm' });

callWithInternalUser.withArgs('shield.samlPrepare').resolves({
id: 'some-request-id',
redirect: 'https://idp-host/path/login?SAMLRequest=some%20request%20',
});

const authenticationResult = await provider.authenticate(request, null);

sinon.assert.calledWithExactly(callWithInternalUser, 'shield.samlPrepare', {
body: { realm: 'test-realm' },
});
Expand All @@ -126,7 +110,7 @@ describe('SAMLAuthenticationProvider', () => {
const authenticationResult = await provider.authenticate(request, null);

sinon.assert.calledWithExactly(callWithInternalUser, 'shield.samlPrepare', {
body: { acs: `test-protocol://test-hostname:1234/test-base-path/api/security/v1/saml` },
body: { realm: 'test-realm' },
});

expect(authenticationResult.failed()).toBe(true);
Expand Down Expand Up @@ -392,7 +376,7 @@ describe('SAMLAuthenticationProvider', () => {
});

sinon.assert.calledWithExactly(callWithInternalUser, 'shield.samlPrepare', {
body: { acs: `test-protocol://test-hostname:1234/test-base-path/api/security/v1/saml` },
body: { realm: 'test-realm' },
});

expect(authenticationResult.redirected()).toBe(true);
Expand Down Expand Up @@ -432,7 +416,7 @@ describe('SAMLAuthenticationProvider', () => {
});

sinon.assert.calledWithExactly(callWithInternalUser, 'shield.samlPrepare', {
body: { acs: `test-protocol://test-hostname:1234/test-base-path/api/security/v1/saml` },
body: { realm: 'test-realm' },
});

expect(authenticationResult.redirected()).toBe(true);
Expand Down Expand Up @@ -759,7 +743,7 @@ describe('SAMLAuthenticationProvider', () => {
sinon.assert.calledWithExactly(callWithInternalUser, 'shield.samlInvalidate', {
body: {
queryString: 'SAMLRequest=xxx%20yyy',
acs: 'test-protocol://test-hostname:1234/test-base-path/api/security/v1/saml',
realm: 'test-realm',
},
});

Expand Down Expand Up @@ -844,7 +828,7 @@ describe('SAMLAuthenticationProvider', () => {
sinon.assert.calledWithExactly(callWithInternalUser, 'shield.samlInvalidate', {
body: {
queryString: 'SAMLRequest=xxx%20yyy',
acs: 'test-protocol://test-hostname:1234/test-base-path/api/security/v1/saml',
realm: 'test-realm',
},
});

Expand All @@ -863,36 +847,14 @@ describe('SAMLAuthenticationProvider', () => {
sinon.assert.calledWithExactly(callWithInternalUser, 'shield.samlInvalidate', {
body: {
queryString: 'SAMLRequest=xxx%20yyy',
acs: 'test-protocol://test-hostname:1234/test-base-path/api/security/v1/saml',
realm: 'test-realm',
},
});

expect(authenticationResult.redirected()).toBe(true);
expect(authenticationResult.redirectURL).toBe('/logged_out');
});

it('uses `realm` name instead of `acs` if it is specified for SAML invalidate request.', async () => {
const request = requestFixture({ search: '?SAMLRequest=xxx%20yyy' });

// Create new provider instance with additional `realm` option.
const providerOptions = mockAuthenticationProviderOptions({ basePath: '/test-base-path' });
callWithRequest = providerOptions.client.callWithRequest as sinon.SinonStub;
callWithInternalUser = providerOptions.client.callWithInternalUser as sinon.SinonStub;
provider = new SAMLAuthenticationProvider(providerOptions, { realm: 'test-realm' });

callWithInternalUser.withArgs('shield.samlInvalidate').resolves({ redirect: undefined });

const authenticationResult = await provider.deauthenticate(request);

sinon.assert.calledOnce(callWithInternalUser);
sinon.assert.calledWithExactly(callWithInternalUser, 'shield.samlInvalidate', {
body: { queryString: 'SAMLRequest=xxx%20yyy', realm: 'test-realm' },
});

expect(authenticationResult.redirected()).toBe(true);
expect(authenticationResult.redirectURL).toBe('/logged_out');
});

it('redirects to /logged_out if `redirect` field in SAML invalidate response is not defined.', async () => {
const request = requestFixture({ search: '?SAMLRequest=xxx%20yyy' });

Expand All @@ -904,7 +866,7 @@ describe('SAMLAuthenticationProvider', () => {
sinon.assert.calledWithExactly(callWithInternalUser, 'shield.samlInvalidate', {
body: {
queryString: 'SAMLRequest=xxx%20yyy',
acs: 'test-protocol://test-hostname:1234/test-base-path/api/security/v1/saml',
realm: 'test-realm',
},
});

Expand Down
Loading

0 comments on commit ffb0b06

Please sign in to comment.