Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Logging legacy fallback deprecation warning on login #20493

Merged
merged 15 commits into from
Jul 6, 2018
Merged
Show file tree
Hide file tree
Changes from 7 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
13 changes: 8 additions & 5 deletions x-pack/plugins/security/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,10 @@ import { authenticateFactory } from './server/lib/auth_redirect';
import { checkLicense } from './server/lib/check_license';
import { initAuthenticator } from './server/lib/authentication/authenticator';
import { initPrivilegesApi } from './server/routes/api/v1/privileges';
import { checkPrivilegesWithRequestFactory } from './server/lib/authorization/check_privileges';
import { SecurityAuditLogger } from './server/lib/audit_logger';
import { AuditLogger } from '../../server/lib/audit_logger';
import { SecureSavedObjectsClient } from './server/lib/saved_objects_client/secure_saved_objects_client';
import { registerPrivilegesWithCluster } from './server/lib/privileges';
import { initAuthorization, registerPrivilegesWithCluster } from './server/lib/authorization';
import { watchStatusAndLicenseToInitialize } from './server/lib/watch_status_and_license_to_initialize';

export const security = (kibana) => new kibana.Plugin({
Expand Down Expand Up @@ -114,9 +113,11 @@ export const security = (kibana) => new kibana.Plugin({
server.auth.strategy('session', 'login', 'required');

const auditLogger = new SecurityAuditLogger(server.config(), new AuditLogger(server, 'security'));
const checkPrivilegesWithRequest = checkPrivilegesWithRequestFactory(server);
const { savedObjects } = server;

// exposes server.plugins.security.authorization
initAuthorization(server);
Copy link
Member

Choose a reason for hiding this comment

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

nit: what about calling this something a little more descriptive, like initAuthorizationService? This more closely matches what you describe in init.test.js


const { savedObjects } = server;
savedObjects.setScopedSavedObjectsClientFactory(({
request,
}) => {
Expand All @@ -130,7 +131,8 @@ export const security = (kibana) => new kibana.Plugin({
return new savedObjects.SavedObjectsClient(callWithRequestRepository);
}

const checkPrivileges = checkPrivilegesWithRequest(request);
const { authorization } = server.plugins.security;
const checkPrivileges = authorization.checkPrivilegesWithRequest(request);
const internalRepository = savedObjects.getSavedObjectsRepository(callWithInternalUser);

return new SecureSavedObjectsClient({
Expand All @@ -140,6 +142,7 @@ export const security = (kibana) => new kibana.Plugin({
checkPrivileges,
auditLogger,
savedObjectTypes: savedObjects.types,
actions: authorization.actions,
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,13 @@ export function serverFixture() {
security: {
getUser: stub(),
authenticate: stub(),
deauthenticate: stub()
deauthenticate: stub(),
authorization: {
checkPrivilegesWithRequest: stub(),
actions: {
login: 'stub-login-action',
},
},
},

xpack_main: {
Expand Down
18 changes: 18 additions & 0 deletions x-pack/plugins/security/server/lib/authorization/actions.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/


export function actionsFactory(config) {
const kibanaVersion = config.get('pkg.version');

return {
getSavedObjectAction(type, action) {
return `action:saved_objects/${type}/${action}`;
Copy link
Member

Choose a reason for hiding this comment

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

Do we need validation here to assert that both type and action are provided?

},
login: `action:login`,
version: `version:${kibanaVersion}`,
};
}
51 changes: 51 additions & 0 deletions x-pack/plugins/security/server/lib/authorization/actions.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { actionsFactory } from './actions';

const createMockConfig = (settings = {}) => {
const mockConfig = {
get: jest.fn()
};

mockConfig.get.mockImplementation(key => settings[key]);

return mockConfig;
};

describe('#login', () => {
test('returns action:login', () => {
const mockConfig = createMockConfig();

const actions = actionsFactory(mockConfig);

expect(actions.login).toEqual('action:login');
});
});

describe('#version', () => {
test(`returns version:\${config.get('pkg.version')}`, () => {
const version = 'mock-version';
const mockConfig = createMockConfig({ 'pkg.version': version });

const actions = actionsFactory(mockConfig);

expect(actions.version).toEqual(`version:${version}`);
});
});

describe('#getSavedObjectAction()', () => {
test('uses type and action to build action', () => {
const mockConfig = createMockConfig();
const actions = actionsFactory(mockConfig);
const type = 'saved-object-type';
const action = 'saved-object-action';

const result = actions.getSavedObjectAction(type, action);

expect(result).toEqual(`action:saved_objects/${type}/${action}`);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -4,27 +4,21 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { getClient } from '../../../../../server/lib/get_client_shield';
import { uniq } from 'lodash';
import { ALL_RESOURCE } from '../../../common/constants';
import { getVersionAction, getLoginAction } from '../privileges';

export const CHECK_PRIVILEGES_RESULT = {
UNAUTHORIZED: Symbol(),
AUTHORIZED: Symbol(),
LEGACY: Symbol(),
};

export function checkPrivilegesWithRequestFactory(server) {
const callWithRequest = getClient(server).callWithRequest;
export function checkPrivilegesWithRequestFactory(shieldClient, config, actions) {
const { callWithRequest } = shieldClient;

const config = server.config();
const kibanaVersion = config.get('pkg.version');
const application = config.get('xpack.security.rbac.application');
const kibanaIndex = config.get('kibana.index');

const loginAction = getLoginAction();
const versionAction = getVersionAction(kibanaVersion);

return function checkPrivilegesWithRequest(request) {

const checkApplicationPrivileges = async (privileges) => {
Expand All @@ -38,19 +32,19 @@ export function checkPrivilegesWithRequestFactory(server) {
}
});

const hasPrivileges = privilegeCheck.application[application][ALL_RESOURCE];
const privilegeCheckPrivileges = privilegeCheck.application[application][ALL_RESOURCE];

// We include the login action in all privileges, so the existence of it and not the version privilege
// lets us know that we're running in an incorrect configuration. Without the login privilege check, we wouldn't
// know whether the user just wasn't authorized for this instance of Kibana in general
if (!hasPrivileges[getVersionAction(kibanaVersion)] && hasPrivileges[getLoginAction()]) {
if (!privilegeCheckPrivileges[actions.version] && privilegeCheckPrivileges[actions.login]) {
throw new Error('Multiple versions of Kibana are running against the same Elasticsearch cluster, unable to authorize user.');
}

return {
username: privilegeCheck.username,
hasAllRequested: privilegeCheck.has_all_requested,
privileges: hasPrivileges
privileges: privilegeCheckPrivileges
};
};

Expand All @@ -68,15 +62,15 @@ export function checkPrivilegesWithRequestFactory(server) {
};

return async function checkPrivileges(privileges) {
const allPrivileges = [versionAction, loginAction, ...privileges];
const allPrivileges = uniq([actions.version, actions.login, ...privileges]);
const applicationPrivilegesCheck = await checkApplicationPrivileges(allPrivileges);

const username = applicationPrivilegesCheck.username;

// We don't want to expose the version privilege to consumers, as it's an implementation detail only to detect version mismatch
// we only return missing privileges that they're specifically checking for
const missing = Object.keys(applicationPrivilegesCheck.privileges)
.filter(p => !applicationPrivilegesCheck.privileges[p])
.filter(p => p !== versionAction);
.filter(privilege => privileges.includes(privilege))
.filter(privilege => !applicationPrivilegesCheck.privileges[privilege]);

if (applicationPrivilegesCheck.hasAllRequested) {
return {
Expand All @@ -86,10 +80,7 @@ export function checkPrivilegesWithRequestFactory(server) {
};
}

if (!applicationPrivilegesCheck.privileges[loginAction] && await hasPrivilegesOnKibanaIndex()) {
const msg = 'Relying on index privileges is deprecated and will be removed in Kibana 7.0';
server.log(['warning', 'deprecated', 'security'], msg);

if (!applicationPrivilegesCheck.privileges[actions.login] && await hasPrivilegesOnKibanaIndex()) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if it's worth it or not, but now that we don't have to log the deprecation warning on a per-request basis, we could potentially do the entire privilege check in a single round-trip. Something like:

const privilegeCheck = await callWithRequest(request, 'shield.hasPrivileges', {
 body: {
   index: [{
     names: [kibanaIndex],
     privileges: ['create', 'delete', 'read', 'view_index_metadata']
   }],
   applications: [{
     application,
     resources: [DEFAULT_RESOURCE],
     privileges
   }]
 }
});

A couple of thoughts on this approach:

  1. We wouldn't be able to rely on ES's has_all_requested, but it should also be fairly trivial to derive that ourselves based on the response.
  2. Non-legacy users will incur an additional overhead while ES checks the index privileges, which we will never use for them. This is likely negligible though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like it... I'll make it so.

return {
result: CHECK_PRIVILEGES_RESULT.LEGACY,
username,
Expand Down
Loading