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

Validate ES has_privileges response before trusting it #20682

Merged
merged 2 commits into from
Jul 12, 2018
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
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`with a malformed Elasticsearch response throws a validation error when an extra index privilege is present in the response 1`] = `[Error: Invalid response received from Elasticsearch has_privilege endpoint. ValidationError: child "index" fails because [child "default-index" fails because ["oopsAnExtraPrivilege" is not allowed]]]`;

exports[`with a malformed Elasticsearch response throws a validation error when an extra privilege is present in the response 1`] = `[Error: Invalid response received from Elasticsearch has_privilege endpoint. ValidationError: child "application" fails because [child "kibana-our_application" fails because [child "*" fails because ["oops-an-unexpected-privilege" is not allowed]]]]`;

exports[`with a malformed Elasticsearch response throws a validation error when index privileges are missing in the response 1`] = `[Error: Invalid response received from Elasticsearch has_privilege endpoint. ValidationError: child "index" fails because [child "default-index" fails because [child "read" fails because ["read" is required]]]]`;

exports[`with a malformed Elasticsearch response throws a validation error when privileges are missing in the response 1`] = `[Error: Invalid response received from Elasticsearch has_privilege endpoint. ValidationError: child "application" fails because [child "kibana-our_application" fails because [child "*" fails because [child "mock-action:version" fails because ["mock-action:version" is required]]]]]`;

exports[`with index privileges throws error if missing version privilege and has login privilege 1`] = `[Error: Multiple versions of Kibana are running against the same Elasticsearch cluster, unable to authorize user.]`;

exports[`with no index privileges throws error if missing version privilege and has login privilege 1`] = `[Error: Multiple versions of Kibana are running against the same Elasticsearch cluster, unable to authorize user.]`;
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

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 an unexpected resource property 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 [\\"foo-resource\\" is required]]]"`;

exports[`validateEsPrivilegeResponse fails validation when 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 the expected resource property is missing from 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 [\\"foo-resource\\" is required]]]"`;

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 fails validation when the resource propertry 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 [\\"foo-resource\\" must be an object]]]"`;

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]]]"`;
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
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ const checkPrivilegesTest = (
])
}],
index: [{
names: [ defaultKibanaIndex ],
names: [defaultKibanaIndex],
privileges: ['create', 'delete', 'read', 'view_index_metadata']
}],
}
Expand Down Expand Up @@ -328,3 +328,75 @@ describe('with no application privileges', () => {
});
});
});

describe('with a malformed Elasticsearch response', () => {
const indexPrivilegesResponse = {
create: true,
delete: true,
read: true,
view_index_metadata: true,
};

checkPrivilegesTest('throws a validation error when an extra privilege is present in the response', {
username: 'foo-username',
privileges: [
`action:saved_objects/${savedObjectTypes[0]}/get`,
],
applicationPrivilegesResponse: {
[mockActions.version]: true,
[mockActions.login]: true,
[`action:saved_objects/${savedObjectTypes[0]}/get`]: true,
['oops-an-unexpected-privilege']: true,
},
indexPrivilegesResponse,
expectErrorThrown: true,
});

checkPrivilegesTest('throws a validation error when privileges are missing in the response', {
username: 'foo-username',
privileges: [
`action:saved_objects/${savedObjectTypes[0]}/get`,
],
applicationPrivilegesResponse: {
[`action:saved_objects/${savedObjectTypes[0]}/get`]: true,
},
indexPrivilegesResponse,
expectErrorThrown: true,
});

checkPrivilegesTest('throws a validation error when an extra index privilege is present in the response', {
username: 'foo-username',
privileges: [
`action:saved_objects/${savedObjectTypes[0]}/get`,
],
applicationPrivilegesResponse: {
[mockActions.version]: true,
[mockActions.login]: true,
[`action:saved_objects/${savedObjectTypes[0]}/get`]: true,
},
indexPrivilegesResponse: {
...indexPrivilegesResponse,
oopsAnExtraPrivilege: true,
},
expectErrorThrown: true,
});

const missingIndexPrivileges = {
...indexPrivilegesResponse
};
delete missingIndexPrivileges.read;

checkPrivilegesTest('throws a validation error when index privileges are missing in the response', {
username: 'foo-username',
privileges: [
`action:saved_objects/${savedObjectTypes[0]}/get`,
],
applicationPrivilegesResponse: {
[mockActions.version]: true,
[mockActions.login]: true,
[`action:saved_objects/${savedObjectTypes[0]}/get`]: true,
},
indexPrivilegesResponse: missingIndexPrivileges,
expectErrorThrown: true,
});
});
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'];
}
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 buildActionsValidationSchema(actions) {
return Joi.object({
...actions.reduce((acc, action) => {
return {
...acc,
[action]: Joi.bool().required()
};
}, {})
}).required();
}

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

const actionValidationSchema = buildActionsValidationSchema(actions);

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

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