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

Reduce saved objects authorization checks #82204

Merged
merged 8 commits into from
Nov 4, 2020
Merged

Reduce saved objects authorization checks #82204

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
15b57dc
Avoid redundant saved objects authorization checks
jportner Oct 30, 2020
868600c
Merge branch 'master' into issue-82203-saved-objects-authz-checks
kibanamachine Nov 2, 2020
63a35e0
Merge branch 'master' into issue-82203-saved-objects-authz-checks
jportner Nov 3, 2020
bd23001
Changes for review feedback
jportner Nov 3, 2020
e97acf0
Remove redundant conditional
jportner Nov 3, 2020
594ed92
Merge branch 'master' into issue-82203-saved-objects-authz-checks
jportner Nov 4, 2020
1d438ea
More review feedback
jportner Nov 4, 2020
553a0b9
Merge branch 'master' into issue-82203-saved-objects-authz-checks
kibanamachine Nov 4, 2020
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
41 changes: 26 additions & 15 deletions x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts
View file
Open in desktop
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,17 @@ 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.namespaces
: args.options.namespace
? [args.options.namespace]
: ['default'];
jportner marked this conversation as resolved.
Show resolved Hide resolved
const returnValue = {
saved_objects: [
{ namespaces: ['foo'] },
{ namespaces: [authorizedNamespace] },
{ namespaces: ['foo', authorizedNamespace] },
{ namespaces: ['*'] },
{ namespaces: authorizedNamespaces },
{ namespaces: ['some-other-namespace', ...authorizedNamespaces] },
],
};

Expand All @@ -224,17 +233,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 +975,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
84 changes: 52 additions & 32 deletions x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,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 @@ -113,7 +113,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 @@ -135,15 +135,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 @@ -170,7 +167,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 @@ -249,7 +246,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 @@ -290,7 +287,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 @@ -317,7 +314,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 @@ -348,7 +345,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 @@ -357,9 +354,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 @@ -395,7 +392,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 @@ -432,20 +429,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 @@ -473,7 +470,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);
}

private async checkPrivileges(
Expand Down Expand Up @@ -596,14 +593,21 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
return map;
}

private redactAndSortNamespaces(spaceIds: string[], privilegeMap: Record<string, boolean>) {
private redactAndSortNamespaces(
spaceIds: string[],
privilegeMap: Record<string, boolean>,
authorizedSpaceIds: string[]
) {
return spaceIds
.map((x) => (x === ALL_SPACES_ID || privilegeMap[x] ? x : UNKNOWN_SPACE))
.map((x) =>
x === ALL_SPACES_ID || privilegeMap[x] || authorizedSpaceIds.includes(x) ? x : UNKNOWN_SPACE
jportner marked this conversation as resolved.
Show resolved Hide resolved
)
.sort(namespaceComparator);
}

private async redactSavedObjectNamespaces<T extends SavedObjectNamespaces>(
savedObject: T
savedObject: T,
authorizedNamespaces: Array<string | undefined>
): Promise<T> {
if (
this.getSpacesService() === undefined ||
Expand All @@ -613,7 +617,13 @@ 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
const authorizedSpaceIds = authorizedNamespaces.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 && !authorizedSpaceIds.includes(x)
);
if (namespaces.length === 0) {
return savedObject;
}
Expand All @@ -622,20 +632,30 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra

return {
...savedObject,
namespaces: this.redactAndSortNamespaces(savedObject.namespaces, privilegeMap),
namespaces: this.redactAndSortNamespaces(
savedObject.namespaces,
privilegeMap,
authorizedSpaceIds
),
};
}

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

const authorizedSpaceIds = authorizedNamespaces.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
).filter((x) => x !== ALL_SPACES_ID && !authorizedSpaceIds.includes(x));
if (namespaces.length === 0) {
return response;
}
Expand All @@ -648,7 +668,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
...savedObject,
namespaces:
savedObject.namespaces &&
this.redactAndSortNamespaces(savedObject.namespaces, privilegeMap),
this.redactAndSortNamespaces(savedObject.namespaces, privilegeMap, authorizedSpaceIds),
})),
};
}
Expand Down