Skip to content

Commit

Permalink
Reduce saved objects authorization checks (elastic#82204)
Browse files Browse the repository at this point in the history
  • Loading branch information
@jportner
jportner committed Nov 4, 2020
1 parent f53ef78 commit 4470a90
Show file tree
Hide file tree
Showing 2 changed files with 131 additions and 66 deletions.
75 changes: 60 additions & 15 deletions x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,9 @@ const createSecureSavedObjectsClientWrapperOptions = () => {
createBadRequestError: jest.fn().mockImplementation((message) => new Error(message)),
isNotFoundError: jest.fn().mockReturnValue(false),
} as unknown) as jest.Mocked<SavedObjectsClientContract['errors']>;
const getSpacesService = jest.fn().mockReturnValue(true);
const getSpacesService = jest.fn().mockReturnValue({
namespaceToSpaceId: (namespace?: string) => (namespace ? namespace : 'default'),
});

return {
actions,
Expand Down Expand Up @@ -174,7 +176,9 @@ const expectObjectNamespaceFiltering = async (
);
expect(clientOpts.checkSavedObjectsPrivilegesAsCurrentUser).toHaveBeenLastCalledWith(
'login:',
namespaces.filter((x) => x !== '*') // when we check what namespaces to redact, we don't check privileges for '*', only actual space IDs
['some-other-namespace']
// when we check what namespaces to redact, we don't check privileges for '*', only actual space IDs
// we don't check privileges for authorizedNamespace either, as that was already checked earlier in the operation
);
};

Expand Down Expand Up @@ -206,12 +210,14 @@ const expectObjectsNamespaceFiltering = async (fn: Function, args: Record<string
getMockCheckPrivilegesFailure // privilege check for namespace filtering
);

const authorizedNamespace = args.options.namespace || 'default';
// the 'find' operation has options.namespaces, the others have options.namespace
const authorizedNamespaces =
args.options.namespaces ?? (args.options.namespace ? [args.options.namespace] : ['default']);
const returnValue = {
saved_objects: [
{ namespaces: ['foo'] },
{ namespaces: [authorizedNamespace] },
{ namespaces: ['foo', authorizedNamespace] },
{ namespaces: ['*'] },
{ namespaces: authorizedNamespaces },
{ namespaces: ['some-other-namespace', ...authorizedNamespaces] },
],
};

Expand All @@ -224,17 +230,19 @@ const expectObjectsNamespaceFiltering = async (fn: Function, args: Record<string
const result = await fn.bind(client)(...Object.values(args));
expect(result).toEqual({
saved_objects: [
{ namespaces: ['?'] },
{ namespaces: [authorizedNamespace] },
{ namespaces: [authorizedNamespace, '?'] },
{ namespaces: ['*'] },
{ namespaces: authorizedNamespaces },
{ namespaces: [...authorizedNamespaces, '?'] },
],
});

expect(clientOpts.checkSavedObjectsPrivilegesAsCurrentUser).toHaveBeenCalledTimes(2);
expect(clientOpts.checkSavedObjectsPrivilegesAsCurrentUser).toHaveBeenLastCalledWith('login:', [
'foo',
authorizedNamespace,
]);
expect(clientOpts.checkSavedObjectsPrivilegesAsCurrentUser).toHaveBeenLastCalledWith(
'login:',
['some-other-namespace']
// when we check what namespaces to redact, we don't check privileges for '*', only actual space IDs
// we don't check privileges for authorizedNamespaces either, as that was already checked earlier in the operation
);
};

function getMockCheckPrivilegesSuccess(actions: string | string[], namespaces?: string | string[]) {
Expand Down Expand Up @@ -964,8 +972,8 @@ describe('#get', () => {
describe('#deleteFromNamespaces', () => {
const type = 'foo';
const id = `${type}-id`;
const namespace1 = 'foo-namespace';
const namespace2 = 'bar-namespace';
const namespace1 = 'default';
const namespace2 = 'another-namespace';
const namespaces = [namespace1, namespace2];
const privilege = `mock-saved_object:${type}/share_to_space`;

Expand Down Expand Up @@ -1153,4 +1161,41 @@ describe('other', () => {
test(`assigns errors from constructor to .errors`, () => {
expect(client.errors).toBe(clientOpts.errors);
});

test(`namespace redaction fails safe`, async () => {
const type = 'foo';
const id = `${type}-id`;
const namespace = 'some-ns';
const namespaces = ['some-other-namespace', '*', namespace];
const returnValue = { namespaces, foo: 'bar' };
clientOpts.baseClient.get.mockReturnValue(returnValue as any);

clientOpts.checkSavedObjectsPrivilegesAsCurrentUser.mockImplementationOnce(
getMockCheckPrivilegesSuccess // privilege check for authorization
);
clientOpts.checkSavedObjectsPrivilegesAsCurrentUser.mockImplementation(
// privilege check for namespace filtering
(_actions: string | string[], _namespaces?: string | string[]) => ({
hasAllRequested: false,
username: USERNAME,
privileges: {
kibana: [
// this is a contrived scenario as we *shouldn't* get both an unauthorized and authorized result for a given resource...
// however, in case we do, we should fail-safe (authorized + unauthorized = unauthorized)
{ resource: 'some-other-namespace', privilege: 'login:', authorized: false },
{ resource: 'some-other-namespace', privilege: 'login:', authorized: true },
],
},
})
);

const result = await client.get(type, id, { namespace });
// we will never redact the "All Spaces" ID
expect(result).toEqual(expect.objectContaining({ namespaces: ['*', namespace, '?'] }));

expect(clientOpts.checkSavedObjectsPrivilegesAsCurrentUser).toHaveBeenCalledTimes(2);
expect(clientOpts.checkSavedObjectsPrivilegesAsCurrentUser).toHaveBeenLastCalledWith('login:', [
'some-other-namespace',
]);
});
});
122 changes: 71 additions & 51 deletions x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,9 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
attributes: T = {} as T,
options: SavedObjectsCreateOptions = {}
) {
const namespaces = [options.namespace, ...(options.initialNamespaces || [])];
try {
const args = { type, attributes, options };
const namespaces = [options.namespace, ...(options.initialNamespaces || [])];
await this.ensureAuthorized(type, 'create', namespaces, { args });
} catch (error) {
this.auditLogger.log(
Expand All @@ -119,7 +119,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
);

const savedObject = await this.baseClient.create(type, attributes, options);
return await this.redactSavedObjectNamespaces(savedObject);
return await this.redactSavedObjectNamespaces(savedObject, namespaces);
}

public async checkConflicts(
Expand All @@ -141,15 +141,12 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
objects: Array<SavedObjectsBulkCreateObject<T>>,
options: SavedObjectsBaseOptions = {}
) {
const namespaces = objects.reduce(
(acc, { initialNamespaces = [] }) => acc.concat(initialNamespaces),
[options.namespace]
);
try {
const args = { objects, options };
const namespaces = objects.reduce(
(acc, { initialNamespaces = [] }) => {
return acc.concat(initialNamespaces);
},
[options.namespace]
);

await this.ensureAuthorized(this.getUniqueObjectTypes(objects), 'bulk_create', namespaces, {
args,
});
Expand All @@ -176,7 +173,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
);

const response = await this.baseClient.bulkCreate(objects, options);
return await this.redactSavedObjectsNamespaces(response);
return await this.redactSavedObjectsNamespaces(response, namespaces);
}

public async delete(type: string, id: string, options: SavedObjectsBaseOptions = {}) {
Expand Down Expand Up @@ -255,7 +252,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
)
);

return await this.redactSavedObjectsNamespaces(response);
return await this.redactSavedObjectsNamespaces(response, options.namespaces ?? [undefined]);
}

public async bulkGet<T = unknown>(
Expand Down Expand Up @@ -296,7 +293,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
)
);

return await this.redactSavedObjectsNamespaces(response);
return await this.redactSavedObjectsNamespaces(response, [options.namespace]);
}

public async get<T = unknown>(type: string, id: string, options: SavedObjectsBaseOptions = {}) {
Expand All @@ -323,7 +320,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
})
);

return await this.redactSavedObjectNamespaces(savedObject);
return await this.redactSavedObjectNamespaces(savedObject, [options.namespace]);
}

public async update<T = unknown>(
Expand Down Expand Up @@ -354,7 +351,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
);

const savedObject = await this.baseClient.update(type, id, attributes, options);
return await this.redactSavedObjectNamespaces(savedObject);
return await this.redactSavedObjectNamespaces(savedObject, [options.namespace]);
}

public async addToNamespaces(
Expand All @@ -363,9 +360,9 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
namespaces: string[],
options: SavedObjectsAddToNamespacesOptions = {}
) {
const { namespace } = options;
try {
const args = { type, id, namespaces, options };
const { namespace } = options;
// To share an object, the user must have the "share_to_space" permission in each of the destination namespaces.
await this.ensureAuthorized(type, 'share_to_space', namespaces, {
args,
Expand Down Expand Up @@ -401,7 +398,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
);

const response = await this.baseClient.addToNamespaces(type, id, namespaces, options);
return await this.redactSavedObjectNamespaces(response);
return await this.redactSavedObjectNamespaces(response, [namespace, ...namespaces]);
}

public async deleteFromNamespaces(
Expand Down Expand Up @@ -438,20 +435,20 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
);

const response = await this.baseClient.deleteFromNamespaces(type, id, namespaces, options);
return await this.redactSavedObjectNamespaces(response);
return await this.redactSavedObjectNamespaces(response, namespaces);
}

public async bulkUpdate<T = unknown>(
objects: Array<SavedObjectsBulkUpdateObject<T>> = [],
options: SavedObjectsBaseOptions = {}
) {
const objectNamespaces = objects
// The repository treats an `undefined` object namespace is treated as the absence of a namespace, falling back to options.namespace;
// in this case, filter it out here so we don't accidentally check for privileges in the Default space when we shouldn't be doing so.
.filter(({ namespace }) => namespace !== undefined)
.map(({ namespace }) => namespace!);
const namespaces = [options?.namespace, ...objectNamespaces];
try {
const objectNamespaces = objects
// The repository treats an `undefined` object namespace is treated as the absence of a namespace, falling back to options.namespace;
// in this case, filter it out here so we don't accidentally check for privileges in the Default space when we shouldn't be doing so.
.filter(({ namespace }) => namespace !== undefined)
.map(({ namespace }) => namespace!);
const namespaces = [options?.namespace, ...objectNamespaces];
const args = { objects, options };
await this.ensureAuthorized(this.getUniqueObjectTypes(objects), 'bulk_update', namespaces, {
args,
Expand Down Expand Up @@ -479,7 +476,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
);

const response = await this.baseClient.bulkUpdate<T>(objects, options);
return await this.redactSavedObjectsNamespaces(response);
return await this.redactSavedObjectsNamespaces(response, namespaces);
}

public async removeReferencesTo(
Expand Down Expand Up @@ -617,32 +614,43 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
return uniq(objects.map((o) => o.type));
}

private async getNamespacesPrivilegeMap(namespaces: string[]) {
private async getNamespacesPrivilegeMap(
namespaces: string[],
previouslyAuthorizedSpaceIds: string[]
) {
const namespacesToCheck = namespaces.filter(
(namespace) => !previouslyAuthorizedSpaceIds.includes(namespace)
);
const initialPrivilegeMap = previouslyAuthorizedSpaceIds.reduce(
(acc, spaceId) => acc.set(spaceId, true),
new Map<string, boolean>()
);
if (namespacesToCheck.length === 0) {
return initialPrivilegeMap;
}
const action = this.actions.login;
const checkPrivilegesResult = await this.checkPrivileges(action, namespaces);
const checkPrivilegesResult = await this.checkPrivileges(action, namespacesToCheck);
// check if the user can log into each namespace
const map = checkPrivilegesResult.privileges.kibana.reduce(
(acc: Record<string, boolean>, { resource, authorized }) => {
// there should never be a case where more than one privilege is returned for a given space
// if there is, fail-safe (authorized + unauthorized = unauthorized)
if (resource && (!authorized || !acc.hasOwnProperty(resource))) {
acc[resource] = authorized;
}
return acc;
},
{}
);
const map = checkPrivilegesResult.privileges.kibana.reduce((acc, { resource, authorized }) => {
// there should never be a case where more than one privilege is returned for a given space
// if there is, fail-safe (authorized + unauthorized = unauthorized)
if (resource && (!authorized || !acc.has(resource))) {
acc.set(resource, authorized);
}
return acc;
}, initialPrivilegeMap);
return map;
}

private redactAndSortNamespaces(spaceIds: string[], privilegeMap: Record<string, boolean>) {
private redactAndSortNamespaces(spaceIds: string[], privilegeMap: Map<string, boolean>) {
return spaceIds
.map((x) => (x === ALL_SPACES_ID || privilegeMap[x] ? x : UNKNOWN_SPACE))
.map((x) => (x === ALL_SPACES_ID || privilegeMap.get(x) ? x : UNKNOWN_SPACE))
.sort(namespaceComparator);
}

private async redactSavedObjectNamespaces<T extends SavedObjectNamespaces>(
savedObject: T
savedObject: T,
previouslyAuthorizedNamespaces: Array<string | undefined>
): Promise<T> {
if (
this.getSpacesService() === undefined ||
Expand All @@ -652,12 +660,18 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
return savedObject;
}

const namespaces = savedObject.namespaces.filter((x) => x !== ALL_SPACES_ID); // all users can see the "all spaces" ID
if (namespaces.length === 0) {
return savedObject;
}
const previouslyAuthorizedSpaceIds = previouslyAuthorizedNamespaces.map((x) =>
this.getSpacesService()!.namespaceToSpaceId(x)
);
// all users can see the "all spaces" ID, and we don't need to recheck authorization for any namespaces that we just checked earlier
const namespaces = savedObject.namespaces.filter(
(x) => x !== ALL_SPACES_ID && !previouslyAuthorizedSpaceIds.includes(x)
);

const privilegeMap = await this.getNamespacesPrivilegeMap(namespaces);
const privilegeMap = await this.getNamespacesPrivilegeMap(
namespaces,
previouslyAuthorizedSpaceIds
);

return {
...savedObject,
Expand All @@ -666,20 +680,26 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
}

private async redactSavedObjectsNamespaces<T extends SavedObjectsNamespaces>(
response: T
response: T,
previouslyAuthorizedNamespaces: Array<string | undefined>
): Promise<T> {
if (this.getSpacesService() === undefined) {
return response;
}

const previouslyAuthorizedSpaceIds = previouslyAuthorizedNamespaces.map((x) =>
this.getSpacesService()!.namespaceToSpaceId(x)
);
const { saved_objects: savedObjects } = response;
// all users can see the "all spaces" ID, and we don't need to recheck authorization for any namespaces that we just checked earlier
const namespaces = uniq(
savedObjects.flatMap((savedObject) => savedObject.namespaces || [])
).filter((x) => x !== ALL_SPACES_ID); // all users can see the "all spaces" ID
if (namespaces.length === 0) {
return response;
}
).filter((x) => x !== ALL_SPACES_ID && !previouslyAuthorizedSpaceIds.includes(x));

const privilegeMap = await this.getNamespacesPrivilegeMap(namespaces);
const privilegeMap = await this.getNamespacesPrivilegeMap(
namespaces,
previouslyAuthorizedSpaceIds
);

return {
...response,
Expand Down

0 comments on commit 4470a90

Please sign in to comment.