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.

Sign up for GitHub

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

Jump to bottom

Validate ES has_privileges response before trusting it #20682

Merged
merged 2 commits into from
Jul 12, 2018
Merged

Validate ES has_privileges response before trusting it #20682

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cc9c2a9
validate elasticsearch has_privileges response before trusting it
legrego Jul 11, 2018
32d6478
address feedback
legrego Jul 12, 2018
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
37 changes: 37 additions & 0 deletions ...plugins/security/server/lib/authorization/__snapshots__/validate_es_response.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`validateEsPrivilegeResponse fails validation then the "application" property is missing from the response 1`] = `"Invalid response received from Elasticsearch has_privilege endpoint. ValidationError: child \\"application\\" fails because [\\"application\\" is required]"`;

exports[`validateEsPrivilegeResponse fails validation when an action is malformed in the response 1`] = `"Invalid response received from Elasticsearch has_privilege endpoint. ValidationError: child \\"application\\" fails because [child \\"foo-application\\" fails because [child \\"foo-resource\\" fails because [child \\"action3\\" fails because [\\"action3\\" must be a boolean]]]]"`;

exports[`validateEsPrivilegeResponse fails validation when an action is missing in the response 1`] = `"Invalid response received from Elasticsearch has_privilege endpoint. ValidationError: child \\"application\\" fails because [child \\"foo-application\\" fails because [child \\"foo-resource\\" fails because [child \\"action2\\" fails because [\\"action2\\" is required]]]]"`;

exports[`validateEsPrivilegeResponse fails validation when an extra action is present in the response 1`] = `"Invalid response received from Elasticsearch has_privilege endpoint. ValidationError: child \\"application\\" fails because [child \\"foo-application\\" fails because [child \\"foo-resource\\" fails because [\\"action4\\" is not allowed]]]"`;

exports[`validateEsPrivilegeResponse fails validation when an extra application is present in the response 1`] = `"Invalid response received from Elasticsearch has_privilege endpoint. ValidationError: child \\"application\\" fails because [\\"otherApplication\\" is not allowed]"`;

exports[`validateEsPrivilegeResponse fails validation when the requested application is missing from the response 1`] = `"Invalid response received from Elasticsearch has_privilege endpoint. ValidationError: child \\"application\\" fails because [child \\"foo-application\\" fails because [\\"foo-application\\" is required]]"`;

exports[`validateEsPrivilegeResponse legacy should fail if the create index privilege is malformed 1`] = `"Invalid response received from Elasticsearch has_privilege endpoint. ValidationError: child \\"index\\" fails because [child \\".kibana\\" fails because [child \\"create\\" fails because [\\"create\\" must be a boolean]]]"`;

exports[`validateEsPrivilegeResponse legacy should fail if the create index privilege is missing from the response 1`] = `"Invalid response received from Elasticsearch has_privilege endpoint. ValidationError: child \\"index\\" fails because [child \\".kibana\\" fails because [child \\"create\\" fails because [\\"create\\" is required]]]"`;

exports[`validateEsPrivilegeResponse legacy should fail if the delete index privilege is malformed 1`] = `"Invalid response received from Elasticsearch has_privilege endpoint. ValidationError: child \\"index\\" fails because [child \\".kibana\\" fails because [child \\"delete\\" fails because [\\"delete\\" must be a boolean]]]"`;

exports[`validateEsPrivilegeResponse legacy should fail if the delete index privilege is missing from the response 1`] = `"Invalid response received from Elasticsearch has_privilege endpoint. ValidationError: child \\"index\\" fails because [child \\".kibana\\" fails because [child \\"delete\\" fails because [\\"delete\\" is required]]]"`;

exports[`validateEsPrivilegeResponse legacy should fail if the index privilege response contains an extra privilege 1`] = `"Invalid response received from Elasticsearch has_privilege endpoint. ValidationError: child \\"index\\" fails because [child \\".kibana\\" fails because [\\"foo-permission\\" is not allowed]]"`;

exports[`validateEsPrivilegeResponse legacy should fail if the index privilege response returns an extra index 1`] = `"Invalid response received from Elasticsearch has_privilege endpoint. ValidationError: child \\"index\\" fails because [\\"anotherIndex\\" is not allowed]"`;

exports[`validateEsPrivilegeResponse legacy should fail if the index property is missing 1`] = `"Invalid response received from Elasticsearch has_privilege endpoint. ValidationError: child \\"index\\" fails because [\\"index\\" is required]"`;

exports[`validateEsPrivilegeResponse legacy should fail if the kibana index is missing from the response 1`] = `"Invalid response received from Elasticsearch has_privilege endpoint. ValidationError: child \\"index\\" fails because [child \\".kibana\\" fails because [\\".kibana\\" is required]]"`;

exports[`validateEsPrivilegeResponse legacy should fail if the read index privilege is malformed 1`] = `"Invalid response received from Elasticsearch has_privilege endpoint. ValidationError: child \\"index\\" fails because [child \\".kibana\\" fails because [child \\"read\\" fails because [\\"read\\" must be a boolean]]]"`;

exports[`validateEsPrivilegeResponse legacy should fail if the read index privilege is missing from the response 1`] = `"Invalid response received from Elasticsearch has_privilege endpoint. ValidationError: child \\"index\\" fails because [child \\".kibana\\" fails because [child \\"read\\" fails because [\\"read\\" is required]]]"`;

exports[`validateEsPrivilegeResponse legacy should fail if the view_index_metadata index privilege is malformed 1`] = `"Invalid response received from Elasticsearch has_privilege endpoint. ValidationError: child \\"index\\" fails because [child \\".kibana\\" fails because [child \\"view_index_metadata\\" fails because [\\"view_index_metadata\\" must be a boolean]]]"`;

exports[`validateEsPrivilegeResponse legacy should fail if the view_index_metadata index privilege is missing from the response 1`] = `"Invalid response received from Elasticsearch has_privilege endpoint. ValidationError: child \\"index\\" fails because [child \\".kibana\\" fails because [child \\"view_index_metadata\\" fails because [\\"view_index_metadata\\" is required]]]"`;
8 changes: 6 additions & 2 deletions x-pack/plugins/security/server/lib/authorization/check_privileges.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

import { uniq } from 'lodash';
import { ALL_RESOURCE } from '../../../common/constants';
import { buildLegacyIndexPrivileges } from './privileges';
import { validateEsPrivilegeResponse } from './validate_es_response';

export const CHECK_PRIVILEGES_RESULT = {
UNAUTHORIZED: Symbol('Unauthorized'),
Expand Down Expand Up @@ -59,13 +61,15 @@ export function checkPrivilegesWithRequestFactory(shieldClient, config, actions,
}],
index: [{
names: [kibanaIndex],
privileges: ['create', 'delete', 'read', 'view_index_metadata']
privileges: buildLegacyIndexPrivileges()
}],
}
});

validateEsPrivilegeResponse(hasPrivilegesResponse, application, allApplicationPrivileges, [ALL_RESOURCE], kibanaIndex);
Copy link
Contributor

Choose a reason for hiding this comment

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

The tests aren't ensuring that we're calling this...

If we use jest.mock('') to stub out the static import, we can ensure that this is being called with the proper arguments; or we can mock out the ES response with some complete bogus responses and make sure that at least those are rejecting with the validation errors, and leave the more fine-grained permutation based testing for the validateEsPrivilegeResponse tests. I haven't really found a way that I prefer in all situations yet...

Copy link
Contributor

Choose a reason for hiding this comment

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

What if instead of introducing the buildLegacyIndexPrivileges abstraction, we passed the "index privileges" that we expect to be on the response to the validateEsPrivilegeResource function, similar to the way that we're passing the more dynamic allApplicationPrivileges?

Copy link
Member Author

Choose a reason for hiding this comment

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

I originally had it that way. I came up with two reasons to try this other approach:

  1. I didn't like the number of parameters this function was expecting, and one less felt a bit more sane to me
  2. The abstraction allows me to build the legacy validation schema once, rather than building it on every request. This is probably unnecessary given the timings that I ran, but I was worried about performance when I put this together.

I can move it back if you'd still prefer -- I don't have a strong opinion here

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense to me :) I actually intended to delete this comment...


const applicationPrivilegesResponse = hasPrivilegesResponse.application[application][ALL_RESOURCE];
const indexPrivilegesResponse = hasPrivilegesResponse.index[kibanaIndex];
const indexPrivilegesResponse = hasPrivilegesResponse.index[kibanaIndex];

if (hasIncompatibileVersion(applicationPrivilegesResponse)) {
throw new Error('Multiple versions of Kibana are running against the same Elasticsearch cluster, unable to authorize user.');
Expand Down
3 changes: 2 additions & 1 deletion x-pack/plugins/security/server/lib/authorization/check_privileges.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ const checkPrivilegesTest = (
const mockConfig = createMockConfig();
const mockShieldClient = createMockShieldClient({
username,
has_all_requested: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

I had removed this from the tests to ensure that we weren't relying upon it, since we're calculating our own "result" based on the actual application privilege and index privileges responses.

application: {
[application]: {
[ALL_RESOURCE]: applicationPrivilegesResponse
Expand Down Expand Up @@ -94,7 +95,7 @@ const checkPrivilegesTest = (
])
}],
index: [{
names: [ defaultKibanaIndex ],
names: [defaultKibanaIndex],
privileges: ['create', 'delete', 'read', 'view_index_metadata']
}],
}
Expand Down
4 changes: 4 additions & 0 deletions x-pack/plugins/security/server/lib/authorization/privileges.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,7 @@ export function buildPrivilegeMap(savedObjectTypes, application, actions) {
}
};
}

export function buildLegacyIndexPrivileges() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a constant, and we use it like a constant when creating the schema, I'm tempted to suggest that we break from the tradition of naming this buildLegacyIndexPrivileges and just using const LEGACY_INDEX_PRIVILEGES, what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about introducing a constant here, but wrapping it in a function ensures that other code won't attempt to modify the entries in that array, which the const won't protect against. This approach ensures that you can only tamper with the instance you explicitly ask for, rather than the singleton that "everyone" uses.

return ['create', 'delete', 'read', 'view_index_metadata'];
}
65 changes: 65 additions & 0 deletions x-pack/plugins/security/server/lib/authorization/validate_es_response.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/*
* 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 Joi from 'joi';
import { buildLegacyIndexPrivileges } from './privileges';

const legacyIndexPrivilegesSchema = Joi.object({
...buildLegacyIndexPrivileges().reduce((acc, privilege) => {
return {
...acc,
[privilege]: Joi.bool().required()
};
}, {})
}).required();

export function validateEsPrivilegeResponse(response, application, actions, resources, kibanaIndex) {
const schema = buildValidationSchema(application, actions, resources, kibanaIndex);
const { error, value } = schema.validate(response);

if (error) {
throw new Error(`Invalid response received from Elasticsearch has_privilege endpoint. ${error}`);
}

return value;
}

function buildResourceValidationSchema(actions) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this is more the buildActionsValidationSchema ...

return Joi.object({
...actions.reduce((acc, action) => {
return {
...acc,
[action]: Joi.bool().required()
};
}, {})
}).required();
}

function buildValidationSchema(application, actions, resources, kibanaIndex) {

const resourceValidationSchema = buildResourceValidationSchema(actions);

const appValidationSchema = Joi.object({
Copy link
Contributor

Choose a reason for hiding this comment

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

... and this is more the resourceValidationSchema

...resources.reduce((acc, resource) => {
return {
...acc,
[resource]: resourceValidationSchema
};
}, {})
}).required();

return Joi.object({
username: Joi.string().required(),
has_all_requested: Joi.bool(),
cluster: Joi.object(),
application: Joi.object({
[application]: appValidationSchema,
}).required(),
index: Joi.object({
[kibanaIndex]: legacyIndexPrivilegesSchema
}).required()
}).required();
}
Loading