From 4470a90e2f10537c0c0660f65034810c39baec5e Mon Sep 17 00:00:00 2001 From: Joe Portner <5295965+jportner@users.noreply.github.com> Date: Wed, 4 Nov 2020 14:13:05 -0500 Subject: [PATCH] Reduce saved objects authorization checks (#82204) --- ...ecure_saved_objects_client_wrapper.test.ts | 75 ++++++++--- .../secure_saved_objects_client_wrapper.ts | 122 ++++++++++-------- 2 files changed, 131 insertions(+), 66 deletions(-) diff --git a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts index 6b9592815dfc5..c6f4ca6dd8afe 100644 --- a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts +++ b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts @@ -31,7 +31,9 @@ const createSecureSavedObjectsClientWrapperOptions = () => { createBadRequestError: jest.fn().mockImplementation((message) => new Error(message)), isNotFoundError: jest.fn().mockReturnValue(false), } as unknown) as jest.Mocked; - const getSpacesService = jest.fn().mockReturnValue(true); + const getSpacesService = jest.fn().mockReturnValue({ + namespaceToSpaceId: (namespace?: string) => (namespace ? namespace : 'default'), + }); return { actions, @@ -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 ); }; @@ -206,12 +210,14 @@ const expectObjectsNamespaceFiltering = async (fn: Function, args: Record { 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`; @@ -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', + ]); + }); }); diff --git a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts index 2ef0cafcd6fdb..e6e34de4ac9ab 100644 --- a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts +++ b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts @@ -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( @@ -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( @@ -141,15 +141,12 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra objects: Array>, 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, }); @@ -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 = {}) { @@ -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( @@ -296,7 +293,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra ) ); - return await this.redactSavedObjectsNamespaces(response); + return await this.redactSavedObjectsNamespaces(response, [options.namespace]); } public async get(type: string, id: string, options: SavedObjectsBaseOptions = {}) { @@ -323,7 +320,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra }) ); - return await this.redactSavedObjectNamespaces(savedObject); + return await this.redactSavedObjectNamespaces(savedObject, [options.namespace]); } public async update( @@ -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( @@ -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, @@ -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( @@ -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( objects: Array> = [], 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, @@ -479,7 +476,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra ); const response = await this.baseClient.bulkUpdate(objects, options); - return await this.redactSavedObjectsNamespaces(response); + return await this.redactSavedObjectsNamespaces(response, namespaces); } public async removeReferencesTo( @@ -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() + ); + 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, { 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) { + private redactAndSortNamespaces(spaceIds: string[], privilegeMap: Map) { 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( - savedObject: T + savedObject: T, + previouslyAuthorizedNamespaces: Array ): Promise { if ( this.getSpacesService() === undefined || @@ -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, @@ -666,20 +680,26 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra } private async redactSavedObjectsNamespaces( - response: T + response: T, + previouslyAuthorizedNamespaces: Array ): Promise { 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,