From b443b72b406a9409990676058b13f914bb6a2e30 Mon Sep 17 00:00:00 2001 From: kobelb Date: Fri, 6 Jul 2018 12:09:47 -0400 Subject: [PATCH 1/3] Specifying the application on the "authorization service" --- x-pack/plugins/security/index.js | 3 +- .../server/lib/__tests__/validate_config.js | 16 ------- .../__snapshots__/init.test.js.snap | 2 + .../lib/authorization/check_privileges.js | 3 +- .../authorization/check_privileges.test.js | 9 ++-- .../security/server/lib/authorization/init.js | 6 ++- .../server/lib/authorization/init.test.js | 27 +++++++++-- .../register_privileges_with_cluster.js | 10 ++-- .../register_privileges_with_cluster.test.js | 47 +++++++++---------- .../security/server/lib/validate_config.js | 8 ---- .../server/routes/api/v1/privileges.js | 4 +- .../apis/privileges/index.js | 4 +- .../apis/saved_objects/index.js | 5 +- 13 files changed, 66 insertions(+), 78 deletions(-) diff --git a/x-pack/plugins/security/index.js b/x-pack/plugins/security/index.js index 0e521589ac141..5671830dc4c18 100644 --- a/x-pack/plugins/security/index.js +++ b/x-pack/plugins/security/index.js @@ -76,10 +76,11 @@ export const security = (kibana) => new kibana.Plugin({ injectDefaultVars: function (server) { const config = server.config(); + const { authorization } = server.plugins.security; return { secureCookies: config.get('xpack.security.secureCookies'), sessionTimeout: config.get('xpack.security.sessionTimeout'), - rbacApplication: config.get('xpack.security.rbac.application'), + rbacApplication: authorization.application, }; } }, diff --git a/x-pack/plugins/security/server/lib/__tests__/validate_config.js b/x-pack/plugins/security/server/lib/__tests__/validate_config.js index e8ba58cf23aec..26d84f6f5559b 100644 --- a/x-pack/plugins/security/server/lib/__tests__/validate_config.js +++ b/x-pack/plugins/security/server/lib/__tests__/validate_config.js @@ -75,20 +75,4 @@ describe('Validate config', function () { sinon.assert.calledWith(config.set, 'xpack.security.secureCookies', true); sinon.assert.notCalled(log); }); - - it('should throw error if xpack.security.rbac.application is the default when kibana.index is set', function () { - // other valid keys that we need - config.get.withArgs('server.ssl.key').returns('foo'); - config.get.withArgs('server.ssl.certificate').returns('bar'); - config.get.withArgs('xpack.security.encryptionKey').returns(validKey); - - config.get.withArgs('kibana.index').returns('notDefaultIndex'); - config.get.withArgs('xpack.security.rbac.application').returns('defaultApplication'); - config.getDefault.withArgs('kibana.index').returns('defaultIndex'); - config.getDefault.withArgs('xpack.security.rbac.application').returns('defaultApplication'); - - expect(() => validateConfig(config, log)).to.throwError(); - - sinon.assert.notCalled(log); - }); }); diff --git a/x-pack/plugins/security/server/lib/authorization/__snapshots__/init.test.js.snap b/x-pack/plugins/security/server/lib/authorization/__snapshots__/init.test.js.snap index fd944032d2930..c65b0d2d6ae39 100644 --- a/x-pack/plugins/security/server/lib/authorization/__snapshots__/init.test.js.snap +++ b/x-pack/plugins/security/server/lib/authorization/__snapshots__/init.test.js.snap @@ -5,3 +5,5 @@ exports[`deep freezes exposed service 1`] = `"Cannot delete property 'checkPrivi exports[`deep freezes exposed service 2`] = `"Cannot add property foo, object is not extensible"`; exports[`deep freezes exposed service 3`] = `"Cannot assign to read only property 'login' of object '#'"`; + +exports[`deep freezes exposed service 4`] = `"Cannot assign to read only property 'application' of object '#'"`; diff --git a/x-pack/plugins/security/server/lib/authorization/check_privileges.js b/x-pack/plugins/security/server/lib/authorization/check_privileges.js index df8ee33ec2902..594966e5b3e4e 100644 --- a/x-pack/plugins/security/server/lib/authorization/check_privileges.js +++ b/x-pack/plugins/security/server/lib/authorization/check_privileges.js @@ -13,10 +13,9 @@ export const CHECK_PRIVILEGES_RESULT = { LEGACY: Symbol('Legacy'), }; -export function checkPrivilegesWithRequestFactory(shieldClient, config, actions) { +export function checkPrivilegesWithRequestFactory(shieldClient, config, actions, application) { const { callWithRequest } = shieldClient; - const application = config.get('xpack.security.rbac.application'); const kibanaIndex = config.get('kibana.index'); const hasIncompatibileVersion = (applicationPrivilegesResponse) => { diff --git a/x-pack/plugins/security/server/lib/authorization/check_privileges.test.js b/x-pack/plugins/security/server/lib/authorization/check_privileges.test.js index 3c414665c8258..ea829c61d90be 100644 --- a/x-pack/plugins/security/server/lib/authorization/check_privileges.test.js +++ b/x-pack/plugins/security/server/lib/authorization/check_privileges.test.js @@ -9,8 +9,8 @@ import { checkPrivilegesWithRequestFactory, CHECK_PRIVILEGES_RESULT } from './ch import { ALL_RESOURCE } from '../../../common/constants'; +const application = 'kibana-our_application'; const defaultVersion = 'default-version'; -const defaultApplication = 'default-application'; const defaultKibanaIndex = 'default-index'; const savedObjectTypes = ['foo-type', 'bar-type']; @@ -26,7 +26,6 @@ const createMockConfig = ({ settings = {} } = {}) => { const defaultSettings = { 'pkg.version': defaultVersion, - 'xpack.security.rbac.application': defaultApplication, 'kibana.index': defaultKibanaIndex, }; @@ -63,7 +62,7 @@ const checkPrivilegesTest = ( const mockShieldClient = createMockShieldClient({ username, application: { - [defaultApplication]: { + [application]: { [ALL_RESOURCE]: applicationPrivilegesResponse } }, @@ -72,7 +71,7 @@ const checkPrivilegesTest = ( }, }); - const checkPrivilegesWithRequest = checkPrivilegesWithRequestFactory(mockShieldClient, mockConfig, mockActions); + const checkPrivilegesWithRequest = checkPrivilegesWithRequestFactory(mockShieldClient, mockConfig, mockActions, application); const request = Symbol(); const checkPrivileges = checkPrivilegesWithRequest(request); @@ -88,7 +87,7 @@ const checkPrivilegesTest = ( expect(mockShieldClient.callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { body: { applications: [{ - application: defaultApplication, + application: application, resources: [ALL_RESOURCE], privileges: uniq([ mockActions.version, mockActions.login, ...privileges diff --git a/x-pack/plugins/security/server/lib/authorization/init.js b/x-pack/plugins/security/server/lib/authorization/init.js index 27ca3f7e84b55..f99bf6d25d26f 100644 --- a/x-pack/plugins/security/server/lib/authorization/init.js +++ b/x-pack/plugins/security/server/lib/authorization/init.js @@ -14,9 +14,11 @@ export function initAuthorizationService(server) { const config = server.config(); const actions = actionsFactory(config); + const application = `kibana-${config.get('kibana.index')}`; server.expose('authorization', deepFreeze({ - checkPrivilegesWithRequest: checkPrivilegesWithRequestFactory(shieldClient, config, actions), - actions + actions, + application, + checkPrivilegesWithRequest: checkPrivilegesWithRequestFactory(shieldClient, config, actions, application), })); } diff --git a/x-pack/plugins/security/server/lib/authorization/init.test.js b/x-pack/plugins/security/server/lib/authorization/init.test.js index b72813809896f..d70e08934c131 100644 --- a/x-pack/plugins/security/server/lib/authorization/init.test.js +++ b/x-pack/plugins/security/server/lib/authorization/init.test.js @@ -21,8 +21,21 @@ jest.mock('./actions', () => ({ actionsFactory: jest.fn(), })); +const createMockConfig = (settings = {}) => { + const mockConfig = { + get: jest.fn() + }; + + mockConfig.get.mockImplementation(key => settings[key]); + + return mockConfig; +}; + test(`calls server.expose with exposed services`, () => { - const mockConfig = Symbol(); + const kibanaIndex = '.a-kibana-index'; + const mockConfig = createMockConfig({ + 'kibana.index': kibanaIndex + }); const mockServer = { expose: jest.fn(), config: jest.fn().mockReturnValue(mockConfig) @@ -33,20 +46,25 @@ test(`calls server.expose with exposed services`, () => { checkPrivilegesWithRequestFactory.mockReturnValue(mockCheckPrivilegesWithRequest); const mockActions = Symbol(); actionsFactory.mockReturnValue(mockActions); + mockConfig.get.mock; initAuthorizationService(mockServer); + const application = `kibana-${kibanaIndex}`; expect(getClient).toHaveBeenCalledWith(mockServer); expect(actionsFactory).toHaveBeenCalledWith(mockConfig); - expect(checkPrivilegesWithRequestFactory).toHaveBeenCalledWith(mockShieldClient, mockConfig, mockActions); + expect(checkPrivilegesWithRequestFactory).toHaveBeenCalledWith(mockShieldClient, mockConfig, mockActions, application); expect(mockServer.expose).toHaveBeenCalledWith('authorization', { - checkPrivilegesWithRequest: mockCheckPrivilegesWithRequest, actions: mockActions, + application, + checkPrivilegesWithRequest: mockCheckPrivilegesWithRequest, }); }); test(`deep freezes exposed service`, () => { - const mockConfig = Symbol(); + const mockConfig = createMockConfig({ + 'kibana.index': '' + }); const mockServer = { expose: jest.fn(), config: jest.fn().mockReturnValue(mockConfig) @@ -61,4 +79,5 @@ test(`deep freezes exposed service`, () => { expect(() => delete exposed.checkPrivilegesWithRequest).toThrowErrorMatchingSnapshot(); expect(() => exposed.foo = 'bar').toThrowErrorMatchingSnapshot(); expect(() => exposed.actions.login = 'not-login').toThrowErrorMatchingSnapshot(); + expect(() => exposed.application = 'changed').toThrowErrorMatchingSnapshot(); }); diff --git a/x-pack/plugins/security/server/lib/authorization/register_privileges_with_cluster.js b/x-pack/plugins/security/server/lib/authorization/register_privileges_with_cluster.js index 432d4647dc1ee..826cdab4b4204 100644 --- a/x-pack/plugins/security/server/lib/authorization/register_privileges_with_cluster.js +++ b/x-pack/plugins/security/server/lib/authorization/register_privileges_with_cluster.js @@ -7,16 +7,12 @@ import { difference, isEmpty, isEqual } from 'lodash'; import { buildPrivilegeMap } from './privileges'; import { getClient } from '../../../../../server/lib/get_client_shield'; -import { actionsFactory } from './actions'; - - export async function registerPrivilegesWithCluster(server) { - const config = server.config(); - const actions = actionsFactory(config); - const application = config.get('xpack.security.rbac.application'); - const savedObjectTypes = server.savedObjects.types; + const { authorization } = server.plugins.security; + const { types: savedObjectTypes } = server.savedObjects; + const { actions, application } = authorization; const shouldRemovePrivileges = (existingPrivileges, expectedPrivileges) => { if (isEmpty(existingPrivileges)) { diff --git a/x-pack/plugins/security/server/lib/authorization/register_privileges_with_cluster.test.js b/x-pack/plugins/security/server/lib/authorization/register_privileges_with_cluster.test.js index 9e4f2fa237e7a..f326d85fdeee3 100644 --- a/x-pack/plugins/security/server/lib/authorization/register_privileges_with_cluster.test.js +++ b/x-pack/plugins/security/server/lib/authorization/register_privileges_with_cluster.test.js @@ -7,21 +7,16 @@ import { registerPrivilegesWithCluster } from './register_privileges_with_cluster'; import { getClient } from '../../../../../server/lib/get_client_shield'; import { buildPrivilegeMap } from './privileges'; -import { actionsFactory } from './actions'; jest.mock('../../../../../server/lib/get_client_shield', () => ({ getClient: jest.fn(), })); jest.mock('./privileges', () => ({ buildPrivilegeMap: jest.fn(), })); -jest.mock('./actions', () => ({ - actionsFactory: jest.fn(), -})); const registerPrivilegesWithClusterTest = (description, { settings = {}, savedObjectTypes, - actions, expectedPrivileges, existingPrivileges, throwErrorWhenGettingPrivileges, @@ -37,7 +32,7 @@ const registerPrivilegesWithClusterTest = (description, { }; const defaultVersion = 'default-version'; - const defaultApplication = 'default-application'; + const application = 'default-application'; const createMockServer = () => { const mockServer = { @@ -45,11 +40,18 @@ const registerPrivilegesWithClusterTest = (description, { get: jest.fn(), }), log: jest.fn(), + plugins: { + security: { + authorization: { + actions: Symbol(), + application + } + } + } }; const defaultSettings = { 'pkg.version': defaultVersion, - 'xpack.security.rbac.application': defaultApplication, }; mockServer.config().get.mockImplementation(key => { @@ -68,18 +70,17 @@ const registerPrivilegesWithClusterTest = (description, { expect(error).toBeUndefined(); expect(mockCallWithInternalUser).toHaveBeenCalledTimes(2); expect(mockCallWithInternalUser).toHaveBeenCalledWith('shield.getPrivilege', { - privilege: defaultApplication, + privilege: application, }); expect(mockCallWithInternalUser).toHaveBeenCalledWith( 'shield.postPrivileges', { body: { - [defaultApplication]: privileges + [application]: privileges }, } ); - const application = settings['xpack.security.rbac.application'] || defaultApplication; expect(mockServer.log).toHaveBeenCalledWith( ['security', 'debug'], `Registering Kibana Privileges with Elasticsearch for ${application}` @@ -96,10 +97,9 @@ const registerPrivilegesWithClusterTest = (description, { expect(error).toBeUndefined(); expect(mockCallWithInternalUser).toHaveBeenCalledTimes(1); expect(mockCallWithInternalUser).toHaveBeenLastCalledWith('shield.getPrivilege', { - privilege: defaultApplication + privilege: application }); - const application = settings['xpack.security.rbac.application'] || defaultApplication; expect(mockServer.log).toHaveBeenCalledWith( ['security', 'debug'], `Registering Kibana Privileges with Elasticsearch for ${application}` @@ -117,7 +117,6 @@ const registerPrivilegesWithClusterTest = (description, { expect(actualError).toBeInstanceOf(Error); expect(actualError.message).toEqual(expectedErrorMessage); - const application = settings['xpack.security.rbac.application'] || defaultApplication; expect(mockServer.log).toHaveBeenCalledWith( ['security', 'error'], `Error registering Kibana Privileges with Elasticsearch for ${application}: ${expectedErrorMessage}` @@ -139,7 +138,7 @@ const registerPrivilegesWithClusterTest = (description, { } return { - [defaultApplication]: existingPrivileges + [application]: existingPrivileges }; }) .mockImplementationOnce(async () => { @@ -148,7 +147,6 @@ const registerPrivilegesWithClusterTest = (description, { } }); - actionsFactory.mockReturnValue(actions); buildPrivilegeMap.mockReturnValue(expectedPrivileges); let error; @@ -163,7 +161,8 @@ const registerPrivilegesWithClusterTest = (description, { expectDidntUpdatePrivileges: createExpectDidntUpdatePrivileges(mockServer, mockCallWithInternalUser, error), expectErrorThrown: createExpectErrorThrown(mockServer, error), mocks: { - buildPrivilegeMap + buildPrivilegeMap, + server: mockServer, } }); }); @@ -171,22 +170,18 @@ const registerPrivilegesWithClusterTest = (description, { registerPrivilegesWithClusterTest(`passes saved object types, application and actions to buildPrivilegeMap`, { settings: { - 'pkg.version': 'foo-version', - 'xpack.security.rbac.application': 'foo-application', + 'pkg.version': 'foo-version' }, savedObjectTypes: [ 'foo-type', 'bar-type', ], - actions: { - login: 'mock-action:login', - version: 'mock-action:version', - }, assert: ({ mocks }) => { - expect(mocks.buildPrivilegeMap).toHaveBeenCalledWith(['foo-type', 'bar-type'], 'foo-application', { - login: 'mock-action:login', - version: 'mock-action:version', - }); + expect(mocks.buildPrivilegeMap).toHaveBeenCalledWith( + ['foo-type', 'bar-type'], + mocks.server.plugins.security.authorization.application, + mocks.server.plugins.security.authorization.actions, + ); }, }); diff --git a/x-pack/plugins/security/server/lib/validate_config.js b/x-pack/plugins/security/server/lib/validate_config.js index d1f80cb65b7ac..49c9ba94ffd57 100644 --- a/x-pack/plugins/security/server/lib/validate_config.js +++ b/x-pack/plugins/security/server/lib/validate_config.js @@ -6,10 +6,6 @@ const crypto = require('crypto'); -const isDefault = (config, key) => { - return config.getDefault(key) === config.get(key); -}; - export function validateConfig(config, log) { if (config.get('xpack.security.encryptionKey') == null) { log('Generating a random key for xpack.security.encryptionKey. To prevent sessions from being invalidated on ' + @@ -31,8 +27,4 @@ export function validateConfig(config, log) { } else { config.set('xpack.security.secureCookies', true); } - - if (!isDefault(config, 'kibana.index') && isDefault(config, 'xpack.security.rbac.application')) { - throw new Error('When setting kibana.index, xpack.security.rbac.application must be set as well.'); - } } diff --git a/x-pack/plugins/security/server/routes/api/v1/privileges.js b/x-pack/plugins/security/server/routes/api/v1/privileges.js index 375f687997270..4bf1b2c5cc7a5 100644 --- a/x-pack/plugins/security/server/routes/api/v1/privileges.js +++ b/x-pack/plugins/security/server/routes/api/v1/privileges.js @@ -7,9 +7,7 @@ import { buildPrivilegeMap } from '../../../lib/authorization'; export function initPrivilegesApi(server) { - const config = server.config(); const { authorization } = server.plugins.security; - const application = config.get('xpack.security.rbac.application'); const savedObjectTypes = server.savedObjects.types; server.route({ @@ -20,7 +18,7 @@ export function initPrivilegesApi(server) { // in Elasticsearch because our current thinking is that we'll associate additional structure/metadata // with our view of them to allow users to more efficiently edit privileges for roles, and serialize it // into a different structure for enforcement within Elasticsearch - const privileges = buildPrivilegeMap(savedObjectTypes, application, authorization.actions); + const privileges = buildPrivilegeMap(savedObjectTypes, authorization.application, authorization.actions); reply(Object.values(privileges)); } }); diff --git a/x-pack/test/rbac_api_integration/apis/privileges/index.js b/x-pack/test/rbac_api_integration/apis/privileges/index.js index bc5bc62f60838..74e30e5616c03 100644 --- a/x-pack/test/rbac_api_integration/apis/privileges/index.js +++ b/x-pack/test/rbac_api_integration/apis/privileges/index.js @@ -16,13 +16,13 @@ export default function ({ getService }) { .then(resp => { expect(resp.body).to.eql([ { - application: 'kibana', + application: 'kibana-.kibana', name: 'all', actions: ['version:7.0.0-alpha1', 'action:*'], metadata: {}, }, { - application: 'kibana', + application: 'kibana-.kibana', name: 'read', actions: [ 'version:7.0.0-alpha1', diff --git a/x-pack/test/rbac_api_integration/apis/saved_objects/index.js b/x-pack/test/rbac_api_integration/apis/saved_objects/index.js index 6336d4bcd47ec..2c11949d8bb47 100644 --- a/x-pack/test/rbac_api_integration/apis/saved_objects/index.js +++ b/x-pack/test/rbac_api_integration/apis/saved_objects/index.js @@ -6,6 +6,7 @@ import { AUTHENTICATION } from "./lib/authentication"; +const application = 'kibana-.kibana'; export default function ({ loadTestFile, getService }) { const es = getService('es'); @@ -42,7 +43,7 @@ export default function ({ loadTestFile, getService }) { index: [], applications: [ { - application: 'kibana', + application, privileges: [ 'all' ], resources: [ '*' ] } @@ -57,7 +58,7 @@ export default function ({ loadTestFile, getService }) { index: [], applications: [ { - application: 'kibana', + application, privileges: [ 'read' ], resources: [ '*' ] } From 26cc5d3c62282b30344c96bf35510297e2e48d73 Mon Sep 17 00:00:00 2001 From: kobelb Date: Fri, 6 Jul 2018 12:13:42 -0400 Subject: [PATCH 2/3] Moving watchStatusAndLicenseToInitialize to be below initAuthorizationService --- x-pack/plugins/security/index.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/x-pack/plugins/security/index.js b/x-pack/plugins/security/index.js index 5671830dc4c18..8e0c5ad68c64c 100644 --- a/x-pack/plugins/security/index.js +++ b/x-pack/plugins/security/index.js @@ -98,12 +98,6 @@ export const security = (kibana) => new kibana.Plugin({ // to re-compute the license check results for this plugin xpackInfoFeature.registerLicenseCheckResultsGenerator(checkLicense); - watchStatusAndLicenseToInitialize(xpackMainPlugin, plugin, async (license) => { - if (license.allowRbac) { - await registerPrivilegesWithCluster(server); - } - }); - validateConfig(config, message => server.log(['security', 'warning'], message)); // Create a Hapi auth scheme that should be applied to each request. @@ -113,11 +107,17 @@ export const security = (kibana) => new kibana.Plugin({ // automatically assigned to all routes that don't contain an auth config. server.auth.strategy('session', 'login', 'required'); - const auditLogger = new SecurityAuditLogger(server.config(), new AuditLogger(server, 'security')); - // exposes server.plugins.security.authorization initAuthorizationService(server); + watchStatusAndLicenseToInitialize(xpackMainPlugin, plugin, async (license) => { + if (license.allowRbac) { + await registerPrivilegesWithCluster(server); + } + }); + + const auditLogger = new SecurityAuditLogger(server.config(), new AuditLogger(server, 'security')); + const { savedObjects } = server; savedObjects.setScopedSavedObjectsClientFactory(({ request, From 801370f62ca01abd6057a09a96e105351211a381 Mon Sep 17 00:00:00 2001 From: kobelb Date: Tue, 10 Jul 2018 11:06:10 -0400 Subject: [PATCH 3/3] Using short-hand propery assignment --- .../security/server/lib/authorization/check_privileges.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/security/server/lib/authorization/check_privileges.test.js b/x-pack/plugins/security/server/lib/authorization/check_privileges.test.js index ea829c61d90be..edf3b42d11c28 100644 --- a/x-pack/plugins/security/server/lib/authorization/check_privileges.test.js +++ b/x-pack/plugins/security/server/lib/authorization/check_privileges.test.js @@ -87,7 +87,7 @@ const checkPrivilegesTest = ( expect(mockShieldClient.callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { body: { applications: [{ - application: application, + application, resources: [ALL_RESOURCE], privileges: uniq([ mockActions.version, mockActions.login, ...privileges