From 406771f9ae276e45bcd8d214dbe59ad8c2d6c790 Mon Sep 17 00:00:00 2001 From: Elena Shostak Date: Wed, 16 Oct 2024 19:09:04 +0200 Subject: [PATCH 01/20] [Authz] Operator privileges --- .../src/authentication/authenticated_user.ts | 5 ++ .../authorization/authorization_service.ts | 4 ++ x-pack/plugins/security/common/index.ts | 2 + x-pack/plugins/security/common/types.ts | 5 ++ .../authorization/api_authorization.test.ts | 55 ++++++++++++++----- .../server/authorization/api_authorization.ts | 9 +++ .../authorization/authorization_service.tsx | 1 + .../server/authorization/index.mock.ts | 1 + 8 files changed, 68 insertions(+), 14 deletions(-) diff --git a/packages/core/security/core-security-common/src/authentication/authenticated_user.ts b/packages/core/security/core-security-common/src/authentication/authenticated_user.ts index 4aa871125052b..667c9688fe921 100644 --- a/packages/core/security/core-security-common/src/authentication/authenticated_user.ts +++ b/packages/core/security/core-security-common/src/authentication/authenticated_user.ts @@ -60,4 +60,9 @@ export interface AuthenticatedUser extends User { * User profile ID of this user. */ profile_uid?: string; + + /** + * Indicated whether user is an operator. + */ + operator?: boolean; } diff --git a/x-pack/packages/security/plugin_types_server/src/authorization/authorization_service.ts b/x-pack/packages/security/plugin_types_server/src/authorization/authorization_service.ts index 82fa3d3fcce5e..3755fb5674f7d 100644 --- a/x-pack/packages/security/plugin_types_server/src/authorization/authorization_service.ts +++ b/x-pack/packages/security/plugin_types_server/src/authorization/authorization_service.ts @@ -5,6 +5,9 @@ * 2.0. */ +import type { KibanaRequest } from '@kbn/core/server'; +import type { AuthenticatedUser } from '@kbn/security-plugin-types-common'; + import type { Actions } from './actions'; import type { CheckPrivilegesWithRequest } from './check_privileges'; import type { CheckPrivilegesDynamicallyWithRequest } from './check_privileges_dynamically'; @@ -25,4 +28,5 @@ export interface AuthorizationServiceSetup { checkPrivilegesDynamicallyWithRequest: CheckPrivilegesDynamicallyWithRequest; checkSavedObjectsPrivilegesWithRequest: CheckSavedObjectsPrivilegesWithRequest; mode: AuthorizationMode; + getCurrentUser: (request: KibanaRequest) => AuthenticatedUser | null; } diff --git a/x-pack/plugins/security/common/index.ts b/x-pack/plugins/security/common/index.ts index f61b923656efa..53908d7773088 100644 --- a/x-pack/plugins/security/common/index.ts +++ b/x-pack/plugins/security/common/index.ts @@ -48,3 +48,5 @@ export type { UserProfileLabels, UserProfileUserInfoWithSecurity, } from '@kbn/security-plugin-types-common'; + +export { ReservedPrivilegesSet } from './types'; diff --git a/x-pack/plugins/security/common/types.ts b/x-pack/plugins/security/common/types.ts index 1fc47aad365de..d166903554ab9 100644 --- a/x-pack/plugins/security/common/types.ts +++ b/x-pack/plugins/security/common/types.ts @@ -24,3 +24,8 @@ export enum LogoutReason { export interface SecurityCheckupState { displayAlert: boolean; } + +export enum ReservedPrivilegesSet { + Operator = 'operator', + Superuser = 'superuser', +} diff --git a/x-pack/plugins/security/server/authorization/api_authorization.test.ts b/x-pack/plugins/security/server/authorization/api_authorization.test.ts index 0181c98d6f1b1..57a1ad7f2844a 100644 --- a/x-pack/plugins/security/server/authorization/api_authorization.test.ts +++ b/x-pack/plugins/security/server/authorization/api_authorization.test.ts @@ -146,6 +146,7 @@ describe('initAPIAuthorization', () => { { security, kibanaPrivilegesResponse, + kibanaCurrentUserResponse, kibanaPrivilegesRequestActions, asserts, }: { @@ -155,6 +156,7 @@ describe('initAPIAuthorization', () => { hasAllRequested?: boolean; }; kibanaPrivilegesRequestActions?: string[]; + kibanaCurrentUserResponse?: { operator: boolean }; asserts: { forbidden?: boolean; authzResult?: Record; @@ -185,6 +187,7 @@ describe('initAPIAuthorization', () => { const mockPostAuthToolkit = httpServiceMock.createOnPostAuthToolkit(); const mockCheckPrivileges = jest.fn().mockReturnValue(kibanaPrivilegesResponse); + mockAuthz.getCurrentUser.mockReturnValue(kibanaCurrentUserResponse); mockAuthz.mode.useRbacForRequest.mockReturnValue(true); mockAuthz.checkPrivilegesDynamicallyWithRequest.mockImplementation((request) => { // hapi conceals the actual "request" from us, so we make sure that the headers are passed to @@ -356,28 +359,52 @@ describe('initAPIAuthorization', () => { ); testSecurityConfig( - `protected route returns forbidden if user has allRequired AND NONE of anyRequired privileges requested`, + `protected route returns "authzResult" if user has operator privileges requested and user is operator`, { security: { authz: { - requiredPrivileges: [ - { - allRequired: ['privilege1'], - anyRequired: ['privilege2', 'privilege3'], - }, - ], + requiredPrivileges: [ReservedPrivilegesSet.operator], }, }, - kibanaPrivilegesRequestActions: ['privilege1', 'privilege2', 'privilege3'], + kibanaCurrentUserResponse: { operator: true }, + asserts: { + authzResult: { + operator: true, + }, + }, + } + ); + + testSecurityConfig( + `protected route returns "authzResult" if user has anyRequired privileges requested and user is operator`, + { + security: { + authz: { + requiredPrivileges: [{ anyRequired: [ReservedPrivilegesSet.operator, 'privilege1'] }], + }, + }, + kibanaCurrentUserResponse: { operator: true }, kibanaPrivilegesResponse: { - privileges: { - kibana: [ - { privilege: 'api:privilege1', authorized: true }, - { privilege: 'api:privilege2', authorized: false }, - { privilege: 'api:privilege3', authorized: false }, - ], + privileges: { kibana: [{ privilege: 'api:privilege1', authorized: false }] }, + }, + asserts: { + authzResult: { + [ReservedPrivilegesSet.operator]: true, + privilege1: false, + }, + }, + } + ); + + testSecurityConfig( + `protected route returns forbidden if user has operator privileges requested and user is not operator`, + { + security: { + authz: { + requiredPrivileges: [ReservedPrivilegesSet.operator], }, }, + kibanaCurrentUserResponse: { operator: false }, asserts: { forbidden: true, }, diff --git a/x-pack/plugins/security/server/authorization/api_authorization.ts b/x-pack/plugins/security/server/authorization/api_authorization.ts index 888d74e7d7bb2..3e55108f26734 100644 --- a/x-pack/plugins/security/server/authorization/api_authorization.ts +++ b/x-pack/plugins/security/server/authorization/api_authorization.ts @@ -35,6 +35,7 @@ export function initAPIAuthorization( checkPrivilegesDynamicallyWithRequest, checkPrivilegesWithRequest, mode, + getCurrentUser, }: AuthorizationServiceSetup, logger: Logger ) { @@ -103,6 +104,14 @@ export function initAPIAuthorization( } } + for (const reservedPrivilege of requestedReservedPrivileges) { + if (reservedPrivilege === ReservedPrivilegesSet.operator) { + const currentUser = getCurrentUser(request); + + kibanaPrivileges[ReservedPrivilegesSet.operator] = currentUser?.operator ?? false; + } + } + const hasRequestedPrivilege = (kbPrivilege: Privilege | PrivilegeSet) => { if (typeof kbPrivilege === 'object') { const allRequired = kbPrivilege.allRequired ?? []; diff --git a/x-pack/plugins/security/server/authorization/authorization_service.tsx b/x-pack/plugins/security/server/authorization/authorization_service.tsx index c8e036b07679c..4e9ae87f27fc0 100644 --- a/x-pack/plugins/security/server/authorization/authorization_service.tsx +++ b/x-pack/plugins/security/server/authorization/authorization_service.tsx @@ -138,6 +138,7 @@ export class AuthorizationService { checkPrivilegesWithRequest, getSpacesService ), + getCurrentUser, }; capabilities.registerSwitcher( diff --git a/x-pack/plugins/security/server/authorization/index.mock.ts b/x-pack/plugins/security/server/authorization/index.mock.ts index c3b76a0908f13..dfd7b5a25b226 100644 --- a/x-pack/plugins/security/server/authorization/index.mock.ts +++ b/x-pack/plugins/security/server/authorization/index.mock.ts @@ -24,5 +24,6 @@ export const authorizationMock = { privileges: { get: jest.fn() }, registerPrivilegesWithCluster: jest.fn(), disableUnauthorizedCapabilities: jest.fn(), + getCurrentUser: jest.fn(), }), }; From feea4315953efb4ad358533bc6efb23aa8efd8c8 Mon Sep 17 00:00:00 2001 From: Elena Shostak Date: Wed, 27 Nov 2024 11:45:05 +0100 Subject: [PATCH 02/20] Fixes --- .../plugins/security/server/authorization/api_authorization.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/x-pack/plugins/security/server/authorization/api_authorization.ts b/x-pack/plugins/security/server/authorization/api_authorization.ts index 3e55108f26734..7f01cefb3bdf0 100644 --- a/x-pack/plugins/security/server/authorization/api_authorization.ts +++ b/x-pack/plugins/security/server/authorization/api_authorization.ts @@ -102,9 +102,7 @@ export function initAPIAuthorization( kibanaPrivileges[ReservedPrivilegesSet.superuser] = checkSuperuserPrivilegesResponse.hasAllRequested; } - } - for (const reservedPrivilege of requestedReservedPrivileges) { if (reservedPrivilege === ReservedPrivilegesSet.operator) { const currentUser = getCurrentUser(request); From 5e2cca248695c5e7bd10f3491d11c5347b2955eb Mon Sep 17 00:00:00 2001 From: Elena Shostak Date: Wed, 27 Nov 2024 15:14:38 +0100 Subject: [PATCH 03/20] Operator privileges refined implementation and tests --- .../security_route_config_validator.test.ts | 15 ++++ .../src/security_route_config_validator.ts | 4 + .../authorization/authorization_service.ts | 3 +- .../authorization/api_authorization.test.ts | 30 ++++++-- .../server/authorization/api_authorization.ts | 76 ++++++++++++++++--- .../authorization/authorization_service.tsx | 1 + .../server/authorization/index.mock.ts | 1 + 7 files changed, 112 insertions(+), 18 deletions(-) diff --git a/packages/core/http/core-http-router-server-internal/src/security_route_config_validator.test.ts b/packages/core/http/core-http-router-server-internal/src/security_route_config_validator.test.ts index f10d2cb3b3ac4..9051f463793c1 100644 --- a/packages/core/http/core-http-router-server-internal/src/security_route_config_validator.test.ts +++ b/packages/core/http/core-http-router-server-internal/src/security_route_config_validator.test.ts @@ -304,4 +304,19 @@ describe('RouteSecurity validation', () => { `"[authz.requiredPrivileges]: Combining superuser with other privileges is redundant, superuser privileges set can be only used as a standalone privilege."` ); }); + + it('should fail validation when anyRequired has operator privileges set', () => { + const invalidRouteSecurity = { + authz: { + requiredPrivileges: [ + { anyRequired: ['privilege1', 'privilege2'], allRequired: ['privilege4'] }, + { anyRequired: ['privilege5', ReservedPrivilegesSet.operator] }, + ], + }, + }; + + expect(() => validRouteSecurity(invalidRouteSecurity)).toThrowErrorMatchingInlineSnapshot( + `"[authz.requiredPrivileges]: Using operator privilege in anyRequired is not allowed"` + ); + }); }); diff --git a/packages/core/http/core-http-router-server-internal/src/security_route_config_validator.ts b/packages/core/http/core-http-router-server-internal/src/security_route_config_validator.ts index 65073f9a66ec6..3537a78e8ee03 100644 --- a/packages/core/http/core-http-router-server-internal/src/security_route_config_validator.ts +++ b/packages/core/http/core-http-router-server-internal/src/security_route_config_validator.ts @@ -59,6 +59,10 @@ const requiredPrivilegesSchema = schema.arrayOf( return 'Combining superuser with other privileges is redundant, superuser privileges set can be only used as a standalone privilege.'; } + if (anyRequired.includes(ReservedPrivilegesSet.operator)) { + return 'Using operator privilege in anyRequired is not allowed'; + } + if (anyRequired.length && allRequired.length) { for (const privilege of anyRequired) { if (allRequired.includes(privilege)) { diff --git a/x-pack/packages/security/plugin_types_server/src/authorization/authorization_service.ts b/x-pack/packages/security/plugin_types_server/src/authorization/authorization_service.ts index 3755fb5674f7d..53185c72212fb 100644 --- a/x-pack/packages/security/plugin_types_server/src/authorization/authorization_service.ts +++ b/x-pack/packages/security/plugin_types_server/src/authorization/authorization_service.ts @@ -5,7 +5,7 @@ * 2.0. */ -import type { KibanaRequest } from '@kbn/core/server'; +import type { IClusterClient, KibanaRequest } from '@kbn/core/server'; import type { AuthenticatedUser } from '@kbn/security-plugin-types-common'; import type { Actions } from './actions'; @@ -29,4 +29,5 @@ export interface AuthorizationServiceSetup { checkSavedObjectsPrivilegesWithRequest: CheckSavedObjectsPrivilegesWithRequest; mode: AuthorizationMode; getCurrentUser: (request: KibanaRequest) => AuthenticatedUser | null; + getClusterClient: () => Promise; } diff --git a/x-pack/plugins/security/server/authorization/api_authorization.test.ts b/x-pack/plugins/security/server/authorization/api_authorization.test.ts index 57a1ad7f2844a..09e4dc7b92968 100644 --- a/x-pack/plugins/security/server/authorization/api_authorization.test.ts +++ b/x-pack/plugins/security/server/authorization/api_authorization.test.ts @@ -149,6 +149,7 @@ describe('initAPIAuthorization', () => { kibanaCurrentUserResponse, kibanaPrivilegesRequestActions, asserts, + esXpackUsageResponse, }: { security?: RouteSecurity; kibanaPrivilegesResponse?: { @@ -157,6 +158,9 @@ describe('initAPIAuthorization', () => { }; kibanaPrivilegesRequestActions?: string[]; kibanaCurrentUserResponse?: { operator: boolean }; + esXpackUsageResponse?: { + security: { operator_privileges: { enabled: boolean; available: boolean } }; + }; asserts: { forbidden?: boolean; authzResult?: Record; @@ -188,6 +192,13 @@ describe('initAPIAuthorization', () => { const mockCheckPrivileges = jest.fn().mockReturnValue(kibanaPrivilegesResponse); mockAuthz.getCurrentUser.mockReturnValue(kibanaCurrentUserResponse); + mockAuthz.getClusterClient.mockResolvedValue({ + asInternalUser: { + transport: { + request: jest.fn().mockResolvedValue(esXpackUsageResponse), + }, + }, + }); mockAuthz.mode.useRbacForRequest.mockReturnValue(true); mockAuthz.checkPrivilegesDynamicallyWithRequest.mockImplementation((request) => { // hapi conceals the actual "request" from us, so we make sure that the headers are passed to @@ -367,6 +378,9 @@ describe('initAPIAuthorization', () => { }, }, kibanaCurrentUserResponse: { operator: true }, + esXpackUsageResponse: { + security: { operator_privileges: { enabled: true, available: true } }, + }, asserts: { authzResult: { operator: true, @@ -376,21 +390,20 @@ describe('initAPIAuthorization', () => { ); testSecurityConfig( - `protected route returns "authzResult" if user has anyRequired privileges requested and user is operator`, + `falls back to 'superuser' privileges check if 'operator' privileges are not enabled`, { security: { authz: { - requiredPrivileges: [{ anyRequired: [ReservedPrivilegesSet.operator, 'privilege1'] }], + requiredPrivileges: [ReservedPrivilegesSet.operator], }, }, - kibanaCurrentUserResponse: { operator: true }, - kibanaPrivilegesResponse: { - privileges: { kibana: [{ privilege: 'api:privilege1', authorized: false }] }, + esXpackUsageResponse: { + security: { operator_privileges: { enabled: false, available: false } }, }, + kibanaPrivilegesResponse: { privileges: { kibana: [] }, hasAllRequested: true }, asserts: { authzResult: { - [ReservedPrivilegesSet.operator]: true, - privilege1: false, + superuser: true, }, }, } @@ -404,6 +417,9 @@ describe('initAPIAuthorization', () => { requiredPrivileges: [ReservedPrivilegesSet.operator], }, }, + esXpackUsageResponse: { + security: { operator_privileges: { enabled: true, available: true } }, + }, kibanaCurrentUserResponse: { operator: false }, asserts: { forbidden: true, diff --git a/x-pack/plugins/security/server/authorization/api_authorization.ts b/x-pack/plugins/security/server/authorization/api_authorization.ts index 7f01cefb3bdf0..4cfe6deec3dde 100644 --- a/x-pack/plugins/security/server/authorization/api_authorization.ts +++ b/x-pack/plugins/security/server/authorization/api_authorization.ts @@ -36,6 +36,7 @@ export function initAPIAuthorization( checkPrivilegesWithRequest, mode, getCurrentUser, + getClusterClient, }: AuthorizationServiceSetup, logger: Logger ) { @@ -54,7 +55,60 @@ export function initAPIAuthorization( const authz = security.authz as AuthzEnabled; - const { requestedPrivileges, requestedReservedPrivileges } = authz.requiredPrivileges.reduce( + const normalizeRequiredPrivileges = async ( + privileges: AuthzEnabled['requiredPrivileges'] + ) => { + const hasOperatorPrivileges = privileges.some( + (privilege) => + privilege === ReservedPrivilegesSet.operator || + (typeof privilege === 'object' && + privilege.allRequired?.includes(ReservedPrivilegesSet.operator)) + ); + + // nothing to normalize + if (!hasOperatorPrivileges) { + return privileges; + } + + const esClient = await getClusterClient(); + const operatorPrivilegesConfig = await esClient.asInternalUser.transport.request<{ + security: { operator_privileges: { enabled: boolean; available: boolean } }; + }>({ + method: 'GET', + path: '/_xpack/usage?filter_path=security.operator_privileges', + }); + + // nothing to normalize + if (operatorPrivilegesConfig.security.operator_privileges.enabled) { + return privileges; + } + + return privileges.map((privilege) => { + if (typeof privilege === 'object') { + const operatorPrivilegeIndex = + privilege.allRequired?.findIndex((p) => p === ReservedPrivilegesSet.operator) ?? -1; + + return operatorPrivilegeIndex !== -1 + ? { + anyRequired: privilege.anyRequired, + allRequired: privilege.allRequired?.toSpliced( + operatorPrivilegeIndex, + 1, + ReservedPrivilegesSet.superuser + ), + } + : privilege; + } + + return privilege === ReservedPrivilegesSet.operator + ? ReservedPrivilegesSet.superuser + : privilege; + }); + }; + + const requiredPrivileges = await normalizeRequiredPrivileges(authz.requiredPrivileges); + + const { requestedPrivileges, requestedReservedPrivileges } = requiredPrivileges.reduce( (acc, privilegeEntry) => { const privileges = typeof privilegeEntry === 'object' @@ -93,19 +147,21 @@ export function initAPIAuthorization( } } + const hasSuperuserPrivileges = async () => { + const checkSuperuserPrivilegesResponse = await checkPrivilegesWithRequest(request).globally( + SUPERUSER_PRIVILEGES + ); + + return checkSuperuserPrivilegesResponse.hasAllRequested; + }; + for (const reservedPrivilege of requestedReservedPrivileges) { if (reservedPrivilege === ReservedPrivilegesSet.superuser) { - const checkSuperuserPrivilegesResponse = await checkPrivilegesWithRequest( - request - ).globally(SUPERUSER_PRIVILEGES); - - kibanaPrivileges[ReservedPrivilegesSet.superuser] = - checkSuperuserPrivilegesResponse.hasAllRequested; + kibanaPrivileges[ReservedPrivilegesSet.superuser] = await hasSuperuserPrivileges(); } if (reservedPrivilege === ReservedPrivilegesSet.operator) { const currentUser = getCurrentUser(request); - kibanaPrivileges[ReservedPrivilegesSet.operator] = currentUser?.operator ?? false; } } @@ -125,8 +181,8 @@ export function initAPIAuthorization( return kibanaPrivileges[kbPrivilege]; }; - for (const requiredPrivilege of authz.requiredPrivileges) { - if (!hasRequestedPrivilege(requiredPrivilege)) { + for (const privilege of requiredPrivileges) { + if (!hasRequestedPrivilege(privilege)) { const missingPrivileges = Object.keys(kibanaPrivileges).filter( (key) => !kibanaPrivileges[key] ); diff --git a/x-pack/plugins/security/server/authorization/authorization_service.tsx b/x-pack/plugins/security/server/authorization/authorization_service.tsx index 4e9ae87f27fc0..f9cbe7a8b1a37 100644 --- a/x-pack/plugins/security/server/authorization/authorization_service.tsx +++ b/x-pack/plugins/security/server/authorization/authorization_service.tsx @@ -139,6 +139,7 @@ export class AuthorizationService { getSpacesService ), getCurrentUser, + getClusterClient, }; capabilities.registerSwitcher( diff --git a/x-pack/plugins/security/server/authorization/index.mock.ts b/x-pack/plugins/security/server/authorization/index.mock.ts index dfd7b5a25b226..ba533c8d247a5 100644 --- a/x-pack/plugins/security/server/authorization/index.mock.ts +++ b/x-pack/plugins/security/server/authorization/index.mock.ts @@ -25,5 +25,6 @@ export const authorizationMock = { registerPrivilegesWithCluster: jest.fn(), disableUnauthorizedCapabilities: jest.fn(), getCurrentUser: jest.fn(), + getClusterClient: jest.fn(), }), }; From e3984e1de26a5a460556b0d8083c89d3901b9784 Mon Sep 17 00:00:00 2001 From: Elena Shostak Date: Wed, 27 Nov 2024 15:17:24 +0100 Subject: [PATCH 04/20] Fixes --- .../security/server/authorization/api_authorization.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/x-pack/plugins/security/server/authorization/api_authorization.ts b/x-pack/plugins/security/server/authorization/api_authorization.ts index 4cfe6deec3dde..73d68e361a360 100644 --- a/x-pack/plugins/security/server/authorization/api_authorization.ts +++ b/x-pack/plugins/security/server/authorization/api_authorization.ts @@ -116,7 +116,10 @@ export function initAPIAuthorization( : [privilegeEntry]; for (const privilege of privileges) { - if (isReservedPrivilegeSet(privilege)) { + if ( + isReservedPrivilegeSet(privilege) && + !acc.requestedReservedPrivileges.includes(privilege) + ) { acc.requestedReservedPrivileges.push(privilege); } else { acc.requestedPrivileges.push(privilege); From 9de75aaa1296e1d0088af12aedc60841c78a6093 Mon Sep 17 00:00:00 2001 From: Elena Shostak Date: Wed, 27 Nov 2024 18:04:38 +0100 Subject: [PATCH 05/20] Fixes --- .../security/server/authorization/api_authorization.ts | 1 + x-pack/plugins/security/server/plugin.ts | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/x-pack/plugins/security/server/authorization/api_authorization.ts b/x-pack/plugins/security/server/authorization/api_authorization.ts index 73d68e361a360..32ecc59ac423d 100644 --- a/x-pack/plugins/security/server/authorization/api_authorization.ts +++ b/x-pack/plugins/security/server/authorization/api_authorization.ts @@ -91,6 +91,7 @@ export function initAPIAuthorization( return operatorPrivilegeIndex !== -1 ? { anyRequired: privilege.anyRequired, + // @ts-expect-error wrong types for `toSpliced` allRequired: privilege.allRequired?.toSpliced( operatorPrivilegeIndex, 1, diff --git a/x-pack/plugins/security/server/plugin.ts b/x-pack/plugins/security/server/plugin.ts index afd21a83712ae..98de71735d00a 100644 --- a/x-pack/plugins/security/server/plugin.ts +++ b/x-pack/plugins/security/server/plugin.ts @@ -354,6 +354,9 @@ export class SecurityPlugin checkSavedObjectsPrivilegesWithRequest: this.authorizationSetup.checkSavedObjectsPrivilegesWithRequest, mode: this.authorizationSetup.mode, + getCurrentUser, + getClusterClient: () => + startServicesPromise.then(({ elasticsearch }) => elasticsearch.client), }, license, privilegeDeprecationsService: getPrivilegeDeprecationsService({ @@ -437,6 +440,8 @@ export class SecurityPlugin checkSavedObjectsPrivilegesWithRequest: this.authorizationSetup!.checkSavedObjectsPrivilegesWithRequest, mode: this.authorizationSetup!.mode, + getCurrentUser: this.authorizationSetup!.getCurrentUser, + getClusterClient: this.authorizationSetup!.getClusterClient, }, userProfiles: { getCurrent: this.userProfileStart.getCurrent, From 38b960e6b8f298343e6f2b4da7a21b8c35bb36d5 Mon Sep 17 00:00:00 2001 From: Elena Shostak Date: Wed, 27 Nov 2024 21:39:37 +0100 Subject: [PATCH 06/20] Fixes --- x-pack/plugins/security/server/mocks.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/x-pack/plugins/security/server/mocks.ts b/x-pack/plugins/security/server/mocks.ts index e47faeba525a0..8fee778efe32c 100644 --- a/x-pack/plugins/security/server/mocks.ts +++ b/x-pack/plugins/security/server/mocks.ts @@ -29,6 +29,8 @@ function createSetupMock() { checkPrivilegesDynamicallyWithRequest: mockAuthz.checkPrivilegesDynamicallyWithRequest, checkSavedObjectsPrivilegesWithRequest: mockAuthz.checkSavedObjectsPrivilegesWithRequest, mode: mockAuthz.mode, + getCurrentUser: jest.fn(), + getClusterClient: jest.fn(), }, registerSpacesService: jest.fn(), license: licenseMock.create(), From fc2db235c36471469eebf5ef148ef0d0341ea540 Mon Sep 17 00:00:00 2001 From: Elena Shostak Date: Wed, 27 Nov 2024 22:10:24 +0100 Subject: [PATCH 07/20] Fixes --- x-pack/plugins/security/server/mocks.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/security/server/mocks.ts b/x-pack/plugins/security/server/mocks.ts index 8fee778efe32c..d884fb7822388 100644 --- a/x-pack/plugins/security/server/mocks.ts +++ b/x-pack/plugins/security/server/mocks.ts @@ -29,8 +29,8 @@ function createSetupMock() { checkPrivilegesDynamicallyWithRequest: mockAuthz.checkPrivilegesDynamicallyWithRequest, checkSavedObjectsPrivilegesWithRequest: mockAuthz.checkSavedObjectsPrivilegesWithRequest, mode: mockAuthz.mode, - getCurrentUser: jest.fn(), - getClusterClient: jest.fn(), + getCurrentUser: mockAuthz.getCurrentUser, + getClusterClient: mockAuthz.getClusterClient, }, registerSpacesService: jest.fn(), license: licenseMock.create(), @@ -56,6 +56,8 @@ function createStartMock() { checkPrivilegesDynamicallyWithRequest: mockAuthz.checkPrivilegesDynamicallyWithRequest, checkSavedObjectsPrivilegesWithRequest: mockAuthz.checkSavedObjectsPrivilegesWithRequest, mode: mockAuthz.mode, + getClusterClient: mockAuthz.getClusterClient, + getCurrentUser: mockAuthz.getCurrentUser, }, userProfiles: { getCurrent: mockUserProfiles.getCurrent, From b4fc8006db2f9bef0c97665cb9b9baabc3f7fce0 Mon Sep 17 00:00:00 2001 From: Elena Shostak Date: Fri, 29 Nov 2024 12:20:04 +0100 Subject: [PATCH 08/20] Jest test snapshot update --- x-pack/plugins/security/server/plugin.test.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/x-pack/plugins/security/server/plugin.test.ts b/x-pack/plugins/security/server/plugin.test.ts index 4b9479f51a0f3..5bf7fa85c9bcc 100644 --- a/x-pack/plugins/security/server/plugin.test.ts +++ b/x-pack/plugins/security/server/plugin.test.ts @@ -113,6 +113,8 @@ describe('Security Plugin', () => { "checkPrivilegesDynamicallyWithRequest": [Function], "checkPrivilegesWithRequest": [Function], "checkSavedObjectsPrivilegesWithRequest": [Function], + "getClusterClient": [Function], + "getCurrentUser": [Function], "mode": Object { "useRbacForRequest": [Function], }, @@ -210,6 +212,8 @@ describe('Security Plugin', () => { "checkPrivilegesDynamicallyWithRequest": [Function], "checkPrivilegesWithRequest": [Function], "checkSavedObjectsPrivilegesWithRequest": [Function], + "getClusterClient": [Function], + "getCurrentUser": [Function], "mode": Object { "useRbacForRequest": [Function], }, From ad4e058071906d348f47d5dda3158affad78b4a6 Mon Sep 17 00:00:00 2001 From: Elena Shostak Date: Fri, 29 Nov 2024 12:54:44 +0100 Subject: [PATCH 09/20] Added documentation --- dev_docs/key_concepts/api_authorization.mdx | 31 +++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/dev_docs/key_concepts/api_authorization.mdx b/dev_docs/key_concepts/api_authorization.mdx index cda6ad5de21ce..0eb7e9df79fe3 100644 --- a/dev_docs/key_concepts/api_authorization.mdx +++ b/dev_docs/key_concepts/api_authorization.mdx @@ -102,6 +102,37 @@ router.get({ }, handler); ``` +### Configuring operator and superuser privileges +We have two special predefined privilege sets that can be used in security configuration: +1. Operator +```ts +router.get({ + path: '/api/path', + security: { + authz: { + requiredPrivileges: [ReservedPrivilegesSet.operator], + }, + }, + ... +}, handler); +``` +Operator privileges check is enforced only if operator privileges are enabled in the Elasticsearch configuration. +For more information on operator privileges, refer to the [Operator privileges documentation](https://www.elastic.co/guide/en/elasticsearch/reference/current/operator-privileges.html). +If operator privileges are disabled, we fall back to the superuser privileges check. + +2. Superuser +```ts +router.get({ + path: '/api/path', + security: { + authz: { + requiredPrivileges: [ReservedPrivilegesSet.superuser], + }, + }, + ... +}, handler); +``` + ### Opting out of authorization for specific routes **Before migration:** ```ts From d0c0a60d17dacca3215a278cbfb579a93eefa08b Mon Sep 17 00:00:00 2001 From: Elena Shostak Date: Fri, 29 Nov 2024 16:23:50 +0100 Subject: [PATCH 10/20] Small fix --- .../server/authorization/api_authorization.ts | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/x-pack/plugins/security/server/authorization/api_authorization.ts b/x-pack/plugins/security/server/authorization/api_authorization.ts index 32ecc59ac423d..9334b8b66fe6c 100644 --- a/x-pack/plugins/security/server/authorization/api_authorization.ts +++ b/x-pack/plugins/security/server/authorization/api_authorization.ts @@ -151,17 +151,13 @@ export function initAPIAuthorization( } } - const hasSuperuserPrivileges = async () => { - const checkSuperuserPrivilegesResponse = await checkPrivilegesWithRequest(request).globally( - SUPERUSER_PRIVILEGES - ); - - return checkSuperuserPrivilegesResponse.hasAllRequested; - }; - for (const reservedPrivilege of requestedReservedPrivileges) { if (reservedPrivilege === ReservedPrivilegesSet.superuser) { - kibanaPrivileges[ReservedPrivilegesSet.superuser] = await hasSuperuserPrivileges(); + const checkSuperuserPrivilegesResponse = await checkPrivilegesWithRequest( + request + ).globally(SUPERUSER_PRIVILEGES); + kibanaPrivileges[ReservedPrivilegesSet.superuser] = + checkSuperuserPrivilegesResponse.hasAllRequested; } if (reservedPrivilege === ReservedPrivilegesSet.operator) { From 7f614ea6a5b3321db1b70e3a9fe9bd0190173b76 Mon Sep 17 00:00:00 2001 From: Elena Shostak Date: Tue, 10 Dec 2024 11:01:18 +0100 Subject: [PATCH 11/20] Fixes --- dev_docs/key_concepts/api_authorization.mdx | 7 +- .../security_route_config_validator.test.ts | 36 +++++++++- .../src/security_route_config_validator.ts | 19 +++++- .../src/authentication/authenticated_user.ts | 2 +- .../security/plugin_types_server/index.ts | 1 + .../authorization/authorization_service.ts | 5 +- .../src/authorization/es_security_config.ts | 13 ++++ .../src/authorization/index.ts | 1 + .../plugin_types_server/src/plugin.ts | 2 +- x-pack/plugins/security/common/types.ts | 5 -- .../authorization/api_authorization.test.ts | 42 ++---------- .../server/authorization/api_authorization.ts | 65 +++---------------- .../authorization/authorization_service.tsx | 12 +++- .../server/authorization/index.mock.ts | 2 +- x-pack/plugins/security/server/plugin.ts | 9 +-- 15 files changed, 103 insertions(+), 118 deletions(-) create mode 100644 x-pack/packages/security/plugin_types_server/src/authorization/es_security_config.ts diff --git a/dev_docs/key_concepts/api_authorization.mdx b/dev_docs/key_concepts/api_authorization.mdx index 0eb7e9df79fe3..5615a20d0f4b5 100644 --- a/dev_docs/key_concepts/api_authorization.mdx +++ b/dev_docs/key_concepts/api_authorization.mdx @@ -110,7 +110,7 @@ router.get({ path: '/api/path', security: { authz: { - requiredPrivileges: [ReservedPrivilegesSet.operator], + requiredPrivileges: [ReservedPrivilegesSet.operator, ''], }, }, ... @@ -118,7 +118,8 @@ router.get({ ``` Operator privileges check is enforced only if operator privileges are enabled in the Elasticsearch configuration. For more information on operator privileges, refer to the [Operator privileges documentation](https://www.elastic.co/guide/en/elasticsearch/reference/current/operator-privileges.html). -If operator privileges are disabled, we fall back to the superuser privileges check. +If operator privileges are disabled, we skip the check for it, so the only privilege checked from the example above is ``. +Operator privileges cannot be used as standalone, it is required to explicitly specify additional privileges in the configuration to ensure that the route is protected even when operator privileges are disabled. 2. Superuser ```ts @@ -322,7 +323,7 @@ GET /api/oas?pathStartsWith=/your/api/path ## Migrating from `access` tags to `security` configuration We aim to use the same privileges that are currently defined with tags `access:`. To assist with this migration, we have created eslint rule `no_deprecated_authz_config`, that will automatically convert your `access` tags to the new `security` configuration. -It scans route definitions and converts `access` tags to the new `requiredPriviliges` configuration. +It scans route definitions and converts `access` tags to the new `requiredPrivileges` configuration. Note: The rule is disabled by default to avoid automatic migrations without an oversight. You can perform migrations by running: diff --git a/packages/core/http/core-http-router-server-internal/src/security_route_config_validator.test.ts b/packages/core/http/core-http-router-server-internal/src/security_route_config_validator.test.ts index 9051f463793c1..9e6fa95f20845 100644 --- a/packages/core/http/core-http-router-server-internal/src/security_route_config_validator.test.ts +++ b/packages/core/http/core-http-router-server-internal/src/security_route_config_validator.test.ts @@ -220,6 +220,16 @@ describe('RouteSecurity validation', () => { expect(() => validRouteSecurity(routeSecurity)).not.toThrow(); }); + it('should pass validation when operator privileges are combined with superuser', () => { + const routeSecurity = { + authz: { + requiredPrivileges: [ReservedPrivilegesSet.operator, ReservedPrivilegesSet.superuser], + }, + }; + + expect(() => validRouteSecurity(routeSecurity)).not.toThrow(); + }); + it('should fail validation when anyRequired and allRequired have the same values', () => { const invalidRouteSecurity = { authz: { @@ -289,7 +299,7 @@ describe('RouteSecurity validation', () => { }; expect(() => validRouteSecurity(invalidRouteSecurity)).toThrowErrorMatchingInlineSnapshot( - `"[authz.requiredPrivileges]: Combining superuser with other privileges is redundant, superuser privileges set can be only used as a standalone privilege."` + `"[authz.requiredPrivileges]: Using superuser privileges in anyRequired is not allowed"` ); }); @@ -316,7 +326,29 @@ describe('RouteSecurity validation', () => { }; expect(() => validRouteSecurity(invalidRouteSecurity)).toThrowErrorMatchingInlineSnapshot( - `"[authz.requiredPrivileges]: Using operator privilege in anyRequired is not allowed"` + `"[authz.requiredPrivileges]: Using operator privileges in anyRequired is not allowed"` + ); + }); + + it('should fail validation when operator privileges set is used as standalone', () => { + expect(() => + validRouteSecurity({ + authz: { + requiredPrivileges: [{ allRequired: [ReservedPrivilegesSet.operator] }], + }, + }) + ).toThrowErrorMatchingInlineSnapshot( + `"[authz.requiredPrivileges]: Operator privileges cannot be used standalone"` + ); + + expect(() => + validRouteSecurity({ + authz: { + requiredPrivileges: [ReservedPrivilegesSet.operator], + }, + }) + ).toThrowErrorMatchingInlineSnapshot( + `"[authz.requiredPrivileges]: Operator privileges cannot be used standalone"` ); }); }); diff --git a/packages/core/http/core-http-router-server-internal/src/security_route_config_validator.ts b/packages/core/http/core-http-router-server-internal/src/security_route_config_validator.ts index 3537a78e8ee03..b62ab46eee1f5 100644 --- a/packages/core/http/core-http-router-server-internal/src/security_route_config_validator.ts +++ b/packages/core/http/core-http-router-server-internal/src/security_route_config_validator.ts @@ -50,17 +50,30 @@ const requiredPrivilegesSchema = schema.arrayOf( } }); + if (anyRequired.includes(ReservedPrivilegesSet.superuser)) { + return 'Using superuser privileges in anyRequired is not allowed'; + } + + const hasSuperuserInAllRequired = allRequired.includes(ReservedPrivilegesSet.superuser); + const hasOperatorInAllRequired = allRequired.includes(ReservedPrivilegesSet.operator); + // Combining superuser with other privileges is redundant. // If user is a superuser, they inherently have access to all the privileges that may come with other roles. + // The exception is when superuser and operator are the only required privileges. if ( - anyRequired.includes(ReservedPrivilegesSet.superuser) || - (allRequired.includes(ReservedPrivilegesSet.superuser) && allRequired.length > 1) + hasSuperuserInAllRequired && + allRequired.length > 1 && + !(hasOperatorInAllRequired && allRequired.length === 2) ) { return 'Combining superuser with other privileges is redundant, superuser privileges set can be only used as a standalone privilege.'; } if (anyRequired.includes(ReservedPrivilegesSet.operator)) { - return 'Using operator privilege in anyRequired is not allowed'; + return 'Using operator privileges in anyRequired is not allowed'; + } + + if (hasOperatorInAllRequired && allRequired.length === 1) { + return 'Operator privileges cannot be used standalone'; } if (anyRequired.length && allRequired.length) { diff --git a/packages/core/security/core-security-common/src/authentication/authenticated_user.ts b/packages/core/security/core-security-common/src/authentication/authenticated_user.ts index 667c9688fe921..d80ff8f434a4f 100644 --- a/packages/core/security/core-security-common/src/authentication/authenticated_user.ts +++ b/packages/core/security/core-security-common/src/authentication/authenticated_user.ts @@ -62,7 +62,7 @@ export interface AuthenticatedUser extends User { profile_uid?: string; /** - * Indicated whether user is an operator. + * Indicates whether user is an operator. */ operator?: boolean; } diff --git a/x-pack/packages/security/plugin_types_server/index.ts b/x-pack/packages/security/plugin_types_server/index.ts index 2b46fa0146a2a..4f213fc6c9920 100644 --- a/x-pack/packages/security/plugin_types_server/index.ts +++ b/x-pack/packages/security/plugin_types_server/index.ts @@ -50,6 +50,7 @@ export type { CheckUserProfilesPrivileges, AuthorizationMode, AuthorizationServiceSetup, + EsSecurityConfig, } from './src/authorization'; export type { SecurityPluginSetup, SecurityPluginStart } from './src/plugin'; export type { diff --git a/x-pack/packages/security/plugin_types_server/src/authorization/authorization_service.ts b/x-pack/packages/security/plugin_types_server/src/authorization/authorization_service.ts index 53185c72212fb..af390daaab2c1 100644 --- a/x-pack/packages/security/plugin_types_server/src/authorization/authorization_service.ts +++ b/x-pack/packages/security/plugin_types_server/src/authorization/authorization_service.ts @@ -5,13 +5,14 @@ * 2.0. */ -import type { IClusterClient, KibanaRequest } from '@kbn/core/server'; +import type { KibanaRequest } from '@kbn/core/server'; import type { AuthenticatedUser } from '@kbn/security-plugin-types-common'; import type { Actions } from './actions'; import type { CheckPrivilegesWithRequest } from './check_privileges'; import type { CheckPrivilegesDynamicallyWithRequest } from './check_privileges_dynamically'; import type { CheckSavedObjectsPrivilegesWithRequest } from './check_saved_objects_privileges'; +import type { EsSecurityConfig } from './es_security_config'; import type { AuthorizationMode } from './mode'; /** @@ -29,5 +30,5 @@ export interface AuthorizationServiceSetup { checkSavedObjectsPrivilegesWithRequest: CheckSavedObjectsPrivilegesWithRequest; mode: AuthorizationMode; getCurrentUser: (request: KibanaRequest) => AuthenticatedUser | null; - getClusterClient: () => Promise; + getSecurityConfig: () => Promise; } diff --git a/x-pack/packages/security/plugin_types_server/src/authorization/es_security_config.ts b/x-pack/packages/security/plugin_types_server/src/authorization/es_security_config.ts new file mode 100644 index 0000000000000..3f4d244d5fb9c --- /dev/null +++ b/x-pack/packages/security/plugin_types_server/src/authorization/es_security_config.ts @@ -0,0 +1,13 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +import type { Client } from '@elastic/elasticsearch'; + +type XpackUsageResponse = Awaited>; +type XpackUsageSecurity = XpackUsageResponse['security']; + +export type EsSecurityConfig = Pick; diff --git a/x-pack/packages/security/plugin_types_server/src/authorization/index.ts b/x-pack/packages/security/plugin_types_server/src/authorization/index.ts index c48e797dc1d1b..0ffa0900fa2d3 100644 --- a/x-pack/packages/security/plugin_types_server/src/authorization/index.ts +++ b/x-pack/packages/security/plugin_types_server/src/authorization/index.ts @@ -43,6 +43,7 @@ export type { PrivilegeDeprecationsRolesByFeatureIdResponse, } from './deprecations'; export type { AuthorizationMode } from './mode'; +export type { EsSecurityConfig } from './es_security_config'; export { GLOBAL_RESOURCE } from './constants'; export { elasticsearchRoleSchema, getKibanaRoleSchema } from './role_schema'; diff --git a/x-pack/packages/security/plugin_types_server/src/plugin.ts b/x-pack/packages/security/plugin_types_server/src/plugin.ts index 7d37935ab760a..c834dc46225f3 100644 --- a/x-pack/packages/security/plugin_types_server/src/plugin.ts +++ b/x-pack/packages/security/plugin_types_server/src/plugin.ts @@ -45,7 +45,7 @@ export interface SecurityPluginStart { /** * Authorization services to manage and access the permissions a particular user has. */ - authz: AuthorizationServiceSetup; + authz: Omit; /** * User profiles services to retrieve user profiles. * diff --git a/x-pack/plugins/security/common/types.ts b/x-pack/plugins/security/common/types.ts index d166903554ab9..1fc47aad365de 100644 --- a/x-pack/plugins/security/common/types.ts +++ b/x-pack/plugins/security/common/types.ts @@ -24,8 +24,3 @@ export enum LogoutReason { export interface SecurityCheckupState { displayAlert: boolean; } - -export enum ReservedPrivilegesSet { - Operator = 'operator', - Superuser = 'superuser', -} diff --git a/x-pack/plugins/security/server/authorization/api_authorization.test.ts b/x-pack/plugins/security/server/authorization/api_authorization.test.ts index 09e4dc7b92968..bb728e644fae7 100644 --- a/x-pack/plugins/security/server/authorization/api_authorization.test.ts +++ b/x-pack/plugins/security/server/authorization/api_authorization.test.ts @@ -149,7 +149,7 @@ describe('initAPIAuthorization', () => { kibanaCurrentUserResponse, kibanaPrivilegesRequestActions, asserts, - esXpackUsageResponse, + esXpackSecurityUsageResponse, }: { security?: RouteSecurity; kibanaPrivilegesResponse?: { @@ -158,8 +158,8 @@ describe('initAPIAuthorization', () => { }; kibanaPrivilegesRequestActions?: string[]; kibanaCurrentUserResponse?: { operator: boolean }; - esXpackUsageResponse?: { - security: { operator_privileges: { enabled: boolean; available: boolean } }; + esXpackSecurityUsageResponse?: { + operator_privileges: { enabled: boolean; available: boolean }; }; asserts: { forbidden?: boolean; @@ -192,13 +192,7 @@ describe('initAPIAuthorization', () => { const mockCheckPrivileges = jest.fn().mockReturnValue(kibanaPrivilegesResponse); mockAuthz.getCurrentUser.mockReturnValue(kibanaCurrentUserResponse); - mockAuthz.getClusterClient.mockResolvedValue({ - asInternalUser: { - transport: { - request: jest.fn().mockResolvedValue(esXpackUsageResponse), - }, - }, - }); + mockAuthz.getSecurityConfig.mockResolvedValue(esXpackSecurityUsageResponse); mockAuthz.mode.useRbacForRequest.mockReturnValue(true); mockAuthz.checkPrivilegesDynamicallyWithRequest.mockImplementation((request) => { // hapi conceals the actual "request" from us, so we make sure that the headers are passed to @@ -378,9 +372,7 @@ describe('initAPIAuthorization', () => { }, }, kibanaCurrentUserResponse: { operator: true }, - esXpackUsageResponse: { - security: { operator_privileges: { enabled: true, available: true } }, - }, + esXpackSecurityUsageResponse: { operator_privileges: { enabled: true, available: true } }, asserts: { authzResult: { operator: true, @@ -389,26 +381,6 @@ describe('initAPIAuthorization', () => { } ); - testSecurityConfig( - `falls back to 'superuser' privileges check if 'operator' privileges are not enabled`, - { - security: { - authz: { - requiredPrivileges: [ReservedPrivilegesSet.operator], - }, - }, - esXpackUsageResponse: { - security: { operator_privileges: { enabled: false, available: false } }, - }, - kibanaPrivilegesResponse: { privileges: { kibana: [] }, hasAllRequested: true }, - asserts: { - authzResult: { - superuser: true, - }, - }, - } - ); - testSecurityConfig( `protected route returns forbidden if user has operator privileges requested and user is not operator`, { @@ -417,9 +389,7 @@ describe('initAPIAuthorization', () => { requiredPrivileges: [ReservedPrivilegesSet.operator], }, }, - esXpackUsageResponse: { - security: { operator_privileges: { enabled: true, available: true } }, - }, + esXpackSecurityUsageResponse: { operator_privileges: { enabled: true, available: true } }, kibanaCurrentUserResponse: { operator: false }, asserts: { forbidden: true, diff --git a/x-pack/plugins/security/server/authorization/api_authorization.ts b/x-pack/plugins/security/server/authorization/api_authorization.ts index 9334b8b66fe6c..6db2ad5a3e663 100644 --- a/x-pack/plugins/security/server/authorization/api_authorization.ts +++ b/x-pack/plugins/security/server/authorization/api_authorization.ts @@ -36,7 +36,7 @@ export function initAPIAuthorization( checkPrivilegesWithRequest, mode, getCurrentUser, - getClusterClient, + getSecurityConfig, }: AuthorizationServiceSetup, logger: Logger ) { @@ -54,60 +54,7 @@ export function initAPIAuthorization( } const authz = security.authz as AuthzEnabled; - - const normalizeRequiredPrivileges = async ( - privileges: AuthzEnabled['requiredPrivileges'] - ) => { - const hasOperatorPrivileges = privileges.some( - (privilege) => - privilege === ReservedPrivilegesSet.operator || - (typeof privilege === 'object' && - privilege.allRequired?.includes(ReservedPrivilegesSet.operator)) - ); - - // nothing to normalize - if (!hasOperatorPrivileges) { - return privileges; - } - - const esClient = await getClusterClient(); - const operatorPrivilegesConfig = await esClient.asInternalUser.transport.request<{ - security: { operator_privileges: { enabled: boolean; available: boolean } }; - }>({ - method: 'GET', - path: '/_xpack/usage?filter_path=security.operator_privileges', - }); - - // nothing to normalize - if (operatorPrivilegesConfig.security.operator_privileges.enabled) { - return privileges; - } - - return privileges.map((privilege) => { - if (typeof privilege === 'object') { - const operatorPrivilegeIndex = - privilege.allRequired?.findIndex((p) => p === ReservedPrivilegesSet.operator) ?? -1; - - return operatorPrivilegeIndex !== -1 - ? { - anyRequired: privilege.anyRequired, - // @ts-expect-error wrong types for `toSpliced` - allRequired: privilege.allRequired?.toSpliced( - operatorPrivilegeIndex, - 1, - ReservedPrivilegesSet.superuser - ), - } - : privilege; - } - - return privilege === ReservedPrivilegesSet.operator - ? ReservedPrivilegesSet.superuser - : privilege; - }); - }; - - const requiredPrivileges = await normalizeRequiredPrivileges(authz.requiredPrivileges); + const requiredPrivileges = authz.requiredPrivileges; const { requestedPrivileges, requestedReservedPrivileges } = requiredPrivileges.reduce( (acc, privilegeEntry) => { @@ -162,7 +109,13 @@ export function initAPIAuthorization( if (reservedPrivilege === ReservedPrivilegesSet.operator) { const currentUser = getCurrentUser(request); - kibanaPrivileges[ReservedPrivilegesSet.operator] = currentUser?.operator ?? false; + const securityConfig = await getSecurityConfig(); + const isOperator = currentUser?.operator ?? false; + + kibanaPrivileges[ReservedPrivilegesSet.operator] = securityConfig.operator_privileges + .enabled + ? isOperator + : false; } } diff --git a/x-pack/plugins/security/server/authorization/authorization_service.tsx b/x-pack/plugins/security/server/authorization/authorization_service.tsx index f9cbe7a8b1a37..62595d34f28b9 100644 --- a/x-pack/plugins/security/server/authorization/authorization_service.tsx +++ b/x-pack/plugins/security/server/authorization/authorization_service.tsx @@ -36,6 +36,7 @@ import type { CheckPrivilegesDynamicallyWithRequest, CheckSavedObjectsPrivilegesWithRequest, CheckUserProfilesPrivileges, + EsSecurityConfig, } from '@kbn/security-plugin-types-server'; import { initAPIAuthorization } from './api_authorization'; @@ -123,6 +124,15 @@ export class AuthorizationService { this.applicationName ); + const esSecurityConfig = getClusterClient() + .then((client) => + client.asInternalUser.transport.request<{ security: EsSecurityConfig }>({ + method: 'GET', + path: '/_xpack/usage?filter_path=security.operator_privileges', + }) + ) + .then(({ security }) => security); + const authz = { actions, applicationName: this.applicationName, @@ -139,7 +149,7 @@ export class AuthorizationService { getSpacesService ), getCurrentUser, - getClusterClient, + getSecurityConfig: () => esSecurityConfig, }; capabilities.registerSwitcher( diff --git a/x-pack/plugins/security/server/authorization/index.mock.ts b/x-pack/plugins/security/server/authorization/index.mock.ts index ba533c8d247a5..8972518addd9e 100644 --- a/x-pack/plugins/security/server/authorization/index.mock.ts +++ b/x-pack/plugins/security/server/authorization/index.mock.ts @@ -25,6 +25,6 @@ export const authorizationMock = { registerPrivilegesWithCluster: jest.fn(), disableUnauthorizedCapabilities: jest.fn(), getCurrentUser: jest.fn(), - getClusterClient: jest.fn(), + getSecurityConfig: jest.fn(), }), }; diff --git a/x-pack/plugins/security/server/plugin.ts b/x-pack/plugins/security/server/plugin.ts index 98de71735d00a..119dbcef13427 100644 --- a/x-pack/plugins/security/server/plugin.ts +++ b/x-pack/plugins/security/server/plugin.ts @@ -80,7 +80,7 @@ export interface SecurityPluginSetup extends SecurityPluginSetupWithoutDeprecate /** * @deprecated Use `authz` methods from the `SecurityServiceStart` contract instead. */ - authz: AuthorizationServiceSetup; + authz: Omit; } export interface PluginSetupDependencies { @@ -344,7 +344,7 @@ export class SecurityPlugin return Object.freeze({ audit: this.auditSetup, authc: { - getCurrentUser: (request) => this.getAuthentication().getCurrentUser(request), + getCurrentUser, }, authz: { actions: this.authorizationSetup.actions, @@ -354,9 +354,6 @@ export class SecurityPlugin checkSavedObjectsPrivilegesWithRequest: this.authorizationSetup.checkSavedObjectsPrivilegesWithRequest, mode: this.authorizationSetup.mode, - getCurrentUser, - getClusterClient: () => - startServicesPromise.then(({ elasticsearch }) => elasticsearch.client), }, license, privilegeDeprecationsService: getPrivilegeDeprecationsService({ @@ -440,8 +437,6 @@ export class SecurityPlugin checkSavedObjectsPrivilegesWithRequest: this.authorizationSetup!.checkSavedObjectsPrivilegesWithRequest, mode: this.authorizationSetup!.mode, - getCurrentUser: this.authorizationSetup!.getCurrentUser, - getClusterClient: this.authorizationSetup!.getClusterClient, }, userProfiles: { getCurrent: this.userProfileStart.getCurrent, From a6aa383f7fef7870c7ff2b6d660fa4aada541839 Mon Sep 17 00:00:00 2001 From: Elena Shostak Date: Tue, 10 Dec 2024 11:03:55 +0100 Subject: [PATCH 12/20] Types fix --- x-pack/plugins/security/server/mocks.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/x-pack/plugins/security/server/mocks.ts b/x-pack/plugins/security/server/mocks.ts index d884fb7822388..e47faeba525a0 100644 --- a/x-pack/plugins/security/server/mocks.ts +++ b/x-pack/plugins/security/server/mocks.ts @@ -29,8 +29,6 @@ function createSetupMock() { checkPrivilegesDynamicallyWithRequest: mockAuthz.checkPrivilegesDynamicallyWithRequest, checkSavedObjectsPrivilegesWithRequest: mockAuthz.checkSavedObjectsPrivilegesWithRequest, mode: mockAuthz.mode, - getCurrentUser: mockAuthz.getCurrentUser, - getClusterClient: mockAuthz.getClusterClient, }, registerSpacesService: jest.fn(), license: licenseMock.create(), @@ -56,8 +54,6 @@ function createStartMock() { checkPrivilegesDynamicallyWithRequest: mockAuthz.checkPrivilegesDynamicallyWithRequest, checkSavedObjectsPrivilegesWithRequest: mockAuthz.checkSavedObjectsPrivilegesWithRequest, mode: mockAuthz.mode, - getClusterClient: mockAuthz.getClusterClient, - getCurrentUser: mockAuthz.getCurrentUser, }, userProfiles: { getCurrent: mockUserProfiles.getCurrent, From d22eaff109360545b68d0bafc142946bdd81c08a Mon Sep 17 00:00:00 2001 From: Elena Shostak Date: Tue, 10 Dec 2024 11:43:59 +0100 Subject: [PATCH 13/20] Types fix --- x-pack/plugins/security/common/index.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/x-pack/plugins/security/common/index.ts b/x-pack/plugins/security/common/index.ts index 53908d7773088..f61b923656efa 100644 --- a/x-pack/plugins/security/common/index.ts +++ b/x-pack/plugins/security/common/index.ts @@ -48,5 +48,3 @@ export type { UserProfileLabels, UserProfileUserInfoWithSecurity, } from '@kbn/security-plugin-types-common'; - -export { ReservedPrivilegesSet } from './types'; From 42f517f58b8de1687a18b13879d21c8e20ffac5a Mon Sep 17 00:00:00 2001 From: Elena Shostak Date: Tue, 10 Dec 2024 14:45:42 +0100 Subject: [PATCH 14/20] Plugin test updates --- x-pack/plugins/security/server/plugin.test.ts | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/x-pack/plugins/security/server/plugin.test.ts b/x-pack/plugins/security/server/plugin.test.ts index 5bf7fa85c9bcc..68815cd75ffdc 100644 --- a/x-pack/plugins/security/server/plugin.test.ts +++ b/x-pack/plugins/security/server/plugin.test.ts @@ -46,6 +46,13 @@ describe('Security Plugin', () => { mockCoreSetup = coreMock.createSetup({ pluginStartContract: { userProfiles: userProfileServiceMock.createStart() }, }); + + const core = coreMock.createRequestHandlerContext(); + + core.elasticsearch.client.asInternalUser.transport.request.mockImplementation((async () => ({ + security: { operator_privileges: { enabled: false, available: false } }, + })) as any); + mockCoreSetup.http.getServerInfo.mockReturnValue({ hostname: 'localhost', name: 'kibana', @@ -64,6 +71,12 @@ describe('Security Plugin', () => { mockCoreStart = coreMock.createStart(); + mockCoreSetup.getStartServices.mockResolvedValue([ + // @ts-expect-error only mocking the client we use + { elasticsearch: core.elasticsearch }, + mockSetupDependencies.features, + ]); + mockStartDependencies = { features: featuresPluginMock.createStart(), licensing: licensingMock.createStart(), @@ -113,8 +126,6 @@ describe('Security Plugin', () => { "checkPrivilegesDynamicallyWithRequest": [Function], "checkPrivilegesWithRequest": [Function], "checkSavedObjectsPrivilegesWithRequest": [Function], - "getClusterClient": [Function], - "getCurrentUser": [Function], "mode": Object { "useRbacForRequest": [Function], }, @@ -212,8 +223,6 @@ describe('Security Plugin', () => { "checkPrivilegesDynamicallyWithRequest": [Function], "checkPrivilegesWithRequest": [Function], "checkSavedObjectsPrivilegesWithRequest": [Function], - "getClusterClient": [Function], - "getCurrentUser": [Function], "mode": Object { "useRbacForRequest": [Function], }, From d564f9b88e3e359278497539e25193be03dea4fa Mon Sep 17 00:00:00 2001 From: Elena Shostak Date: Tue, 10 Dec 2024 23:59:09 +0100 Subject: [PATCH 15/20] Refined approach and tests --- .../authorization/api_authorization.test.ts | 44 ++++++++++++++++ .../server/authorization/api_authorization.ts | 52 +++++++++++++++++-- 2 files changed, 91 insertions(+), 5 deletions(-) diff --git a/x-pack/plugins/security/server/authorization/api_authorization.test.ts b/x-pack/plugins/security/server/authorization/api_authorization.test.ts index bb728e644fae7..4e135cfed8d81 100644 --- a/x-pack/plugins/security/server/authorization/api_authorization.test.ts +++ b/x-pack/plugins/security/server/authorization/api_authorization.test.ts @@ -381,6 +381,50 @@ describe('initAPIAuthorization', () => { } ); + testSecurityConfig( + `protected route returns "authzResult" if user has requested operator privileges and operator privileges are disabled`, + { + security: { + authz: { + requiredPrivileges: [ReservedPrivilegesSet.operator, 'privilege1'], + }, + }, + kibanaPrivilegesResponse: { + privileges: { + kibana: [{ privilege: 'api:privilege1', authorized: true }], + }, + }, + kibanaCurrentUserResponse: { operator: false }, + esXpackSecurityUsageResponse: { operator_privileges: { enabled: false, available: false } }, + asserts: { + authzResult: { + privilege1: true, + }, + }, + } + ); + + testSecurityConfig( + `protected route returns forbidden if user operator privileges are disabled and user doesn't have additional privileges granted`, + { + security: { + authz: { + requiredPrivileges: [ReservedPrivilegesSet.operator, 'privilege1'], + }, + }, + kibanaPrivilegesResponse: { + privileges: { + kibana: [{ privilege: 'api:privilege1', authorized: false }], + }, + }, + kibanaCurrentUserResponse: { operator: false }, + esXpackSecurityUsageResponse: { operator_privileges: { enabled: false, available: false } }, + asserts: { + forbidden: true, + }, + } + ); + testSecurityConfig( `protected route returns forbidden if user has operator privileges requested and user is not operator`, { diff --git a/x-pack/plugins/security/server/authorization/api_authorization.ts b/x-pack/plugins/security/server/authorization/api_authorization.ts index 6db2ad5a3e663..587ae5c43da65 100644 --- a/x-pack/plugins/security/server/authorization/api_authorization.ts +++ b/x-pack/plugins/security/server/authorization/api_authorization.ts @@ -54,7 +54,52 @@ export function initAPIAuthorization( } const authz = security.authz as AuthzEnabled; - const requiredPrivileges = authz.requiredPrivileges; + const normalizeRequiredPrivileges = async ( + privileges: AuthzEnabled['requiredPrivileges'] + ) => { + const hasOperatorPrivileges = privileges.some( + (privilege) => + privilege === ReservedPrivilegesSet.operator || + (typeof privilege === 'object' && + privilege.allRequired?.includes(ReservedPrivilegesSet.operator)) + ); + + // nothing to normalize + if (!hasOperatorPrivileges) { + return privileges; + } + + const securityConfig = await getSecurityConfig(); + + // nothing to normalize + if (securityConfig.operator_privileges.enabled) { + return privileges; + } + + return privileges.reduce((acc, privilege) => { + if (typeof privilege === 'object') { + const operatorPrivilegeIndex = + privilege.allRequired?.findIndex((p) => p === ReservedPrivilegesSet.operator) ?? -1; + + acc.push( + operatorPrivilegeIndex !== -1 + ? { + anyRequired: privilege.anyRequired, + allRequired: privilege.allRequired?.toSpliced(operatorPrivilegeIndex, 1), + } + : privilege + ); + } + + if (privilege !== ReservedPrivilegesSet.operator) { + acc.push(privilege); + } + + return acc; + }, []); + }; + + const requiredPrivileges = await normalizeRequiredPrivileges(authz.requiredPrivileges); const { requestedPrivileges, requestedReservedPrivileges } = requiredPrivileges.reduce( (acc, privilegeEntry) => { @@ -64,10 +109,7 @@ export function initAPIAuthorization( : [privilegeEntry]; for (const privilege of privileges) { - if ( - isReservedPrivilegeSet(privilege) && - !acc.requestedReservedPrivileges.includes(privilege) - ) { + if (isReservedPrivilegeSet(privilege)) { acc.requestedReservedPrivileges.push(privilege); } else { acc.requestedPrivileges.push(privilege); From 0516fd9d740eb442904ec8ab469abce1b946b0f6 Mon Sep 17 00:00:00 2001 From: Elena Shostak Date: Wed, 11 Dec 2024 10:13:27 +0100 Subject: [PATCH 16/20] Types fix --- .../plugins/security/server/authorization/api_authorization.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/x-pack/plugins/security/server/authorization/api_authorization.ts b/x-pack/plugins/security/server/authorization/api_authorization.ts index 587ae5c43da65..8d232f9642ad4 100644 --- a/x-pack/plugins/security/server/authorization/api_authorization.ts +++ b/x-pack/plugins/security/server/authorization/api_authorization.ts @@ -85,6 +85,7 @@ export function initAPIAuthorization( operatorPrivilegeIndex !== -1 ? { anyRequired: privilege.anyRequired, + // @ts-ignore wrong types for `toSpliced` allRequired: privilege.allRequired?.toSpliced(operatorPrivilegeIndex, 1), } : privilege From bfeb5108be9bff6cdb018afff95845de78082574 Mon Sep 17 00:00:00 2001 From: Elena Shostak Date: Wed, 11 Dec 2024 11:57:35 +0100 Subject: [PATCH 17/20] Test fixes --- .../authorization/authorization_service.test.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/x-pack/plugins/security/server/authorization/authorization_service.test.ts b/x-pack/plugins/security/server/authorization/authorization_service.test.ts index de3646166d8f9..31401f4ae1f86 100644 --- a/x-pack/plugins/security/server/authorization/authorization_service.test.ts +++ b/x-pack/plugins/security/server/authorization/authorization_service.test.ts @@ -38,6 +38,9 @@ const mockCheckPrivilegesDynamicallyWithRequest = Symbol(); const mockCheckSavedObjectsPrivilegesWithRequest = Symbol(); const mockPrivilegesService = Symbol(); const mockAuthorizationMode = Symbol(); +const mockEsSecurityResponse = { + security: { operator_privileges: { enabled: false, available: false } }, +}; beforeEach(() => { mockCheckPrivilegesFactory.mockReturnValue({ checkPrivilegesWithRequest: mockCheckPrivilegesWithRequest, @@ -59,6 +62,9 @@ afterEach(() => { it(`#setup returns exposed services`, () => { const mockClusterClient = elasticsearchServiceMock.createClusterClient(); + mockClusterClient.asInternalUser.transport.request.mockImplementation( + async () => mockEsSecurityResponse + ); const mockGetSpacesService = jest .fn() .mockReturnValue({ getSpaceId: jest.fn(), namespaceToSpaceId: jest.fn() }); @@ -126,6 +132,9 @@ describe('#start', () => { statusSubject = new Subject(); const mockClusterClient = elasticsearchServiceMock.createClusterClient(); + mockClusterClient.asInternalUser.transport.request.mockImplementation( + async () => mockEsSecurityResponse + ); const mockCoreSetup = coreMock.createSetup(); const authorizationService = new AuthorizationService(); @@ -194,6 +203,9 @@ describe('#start', () => { it('#stop unsubscribes from license and ES updates.', async () => { const mockClusterClient = elasticsearchServiceMock.createClusterClient(); + mockClusterClient.asInternalUser.transport.request.mockImplementation( + async () => mockEsSecurityResponse + ); const statusSubject = new Subject(); const mockCoreSetup = coreMock.createSetup(); From b4ae823c1161373a780bf8f086a6fea555f9efdd Mon Sep 17 00:00:00 2001 From: Elena Shostak Date: Thu, 12 Dec 2024 15:08:30 +0100 Subject: [PATCH 18/20] Fixes for xpack.usage request --- .../authorization/authorization_service.test.ts | 13 +++++++------ .../server/authorization/authorization_service.tsx | 6 ++---- x-pack/plugins/security/server/plugin.test.ts | 5 +++-- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/x-pack/plugins/security/server/authorization/authorization_service.test.ts b/x-pack/plugins/security/server/authorization/authorization_service.test.ts index 31401f4ae1f86..3317fff03bd64 100644 --- a/x-pack/plugins/security/server/authorization/authorization_service.test.ts +++ b/x-pack/plugins/security/server/authorization/authorization_service.test.ts @@ -15,6 +15,7 @@ import { mockRegisterPrivilegesWithCluster, } from './service.test.mocks'; +import type { Client } from '@elastic/elasticsearch'; import { Subject } from 'rxjs'; import { coreMock, elasticsearchServiceMock, loggingSystemMock } from '@kbn/core/server/mocks'; @@ -62,8 +63,8 @@ afterEach(() => { it(`#setup returns exposed services`, () => { const mockClusterClient = elasticsearchServiceMock.createClusterClient(); - mockClusterClient.asInternalUser.transport.request.mockImplementation( - async () => mockEsSecurityResponse + mockClusterClient.asInternalUser.xpack.usage.mockResolvedValue( + mockEsSecurityResponse as Awaited> ); const mockGetSpacesService = jest .fn() @@ -132,8 +133,8 @@ describe('#start', () => { statusSubject = new Subject(); const mockClusterClient = elasticsearchServiceMock.createClusterClient(); - mockClusterClient.asInternalUser.transport.request.mockImplementation( - async () => mockEsSecurityResponse + mockClusterClient.asInternalUser.xpack.usage.mockResolvedValue( + mockEsSecurityResponse as Awaited> ); const mockCoreSetup = coreMock.createSetup(); @@ -203,8 +204,8 @@ describe('#start', () => { it('#stop unsubscribes from license and ES updates.', async () => { const mockClusterClient = elasticsearchServiceMock.createClusterClient(); - mockClusterClient.asInternalUser.transport.request.mockImplementation( - async () => mockEsSecurityResponse + mockClusterClient.asInternalUser.xpack.usage.mockResolvedValue( + mockEsSecurityResponse as Awaited> ); const statusSubject = new Subject(); const mockCoreSetup = coreMock.createSetup(); diff --git a/x-pack/plugins/security/server/authorization/authorization_service.tsx b/x-pack/plugins/security/server/authorization/authorization_service.tsx index 62595d34f28b9..56e66f9b3b148 100644 --- a/x-pack/plugins/security/server/authorization/authorization_service.tsx +++ b/x-pack/plugins/security/server/authorization/authorization_service.tsx @@ -36,7 +36,6 @@ import type { CheckPrivilegesDynamicallyWithRequest, CheckSavedObjectsPrivilegesWithRequest, CheckUserProfilesPrivileges, - EsSecurityConfig, } from '@kbn/security-plugin-types-server'; import { initAPIAuthorization } from './api_authorization'; @@ -126,9 +125,8 @@ export class AuthorizationService { const esSecurityConfig = getClusterClient() .then((client) => - client.asInternalUser.transport.request<{ security: EsSecurityConfig }>({ - method: 'GET', - path: '/_xpack/usage?filter_path=security.operator_privileges', + client.asInternalUser.xpack.usage({ + filter_path: 'security.operator_privileges', }) ) .then(({ security }) => security); diff --git a/x-pack/plugins/security/server/plugin.test.ts b/x-pack/plugins/security/server/plugin.test.ts index 68815cd75ffdc..958e22a3f2dd9 100644 --- a/x-pack/plugins/security/server/plugin.test.ts +++ b/x-pack/plugins/security/server/plugin.test.ts @@ -5,6 +5,7 @@ * 2.0. */ +import type { Client } from '@elastic/elasticsearch'; import { of } from 'rxjs'; import { ByteSizeValue } from '@kbn/config-schema'; @@ -49,9 +50,9 @@ describe('Security Plugin', () => { const core = coreMock.createRequestHandlerContext(); - core.elasticsearch.client.asInternalUser.transport.request.mockImplementation((async () => ({ + core.elasticsearch.client.asInternalUser.xpack.usage.mockResolvedValue({ security: { operator_privileges: { enabled: false, available: false } }, - })) as any); + } as Awaited>); mockCoreSetup.http.getServerInfo.mockReturnValue({ hostname: 'localhost', From 062ce7a29d3f4c120d79f3875279d8646d53404a Mon Sep 17 00:00:00 2001 From: Elena Shostak Date: Thu, 12 Dec 2024 17:41:19 +0100 Subject: [PATCH 19/20] Review fixes --- .../security_route_config_validator.test.ts | 18 +++++++++-- .../src/security_route_config_validator.ts | 15 +++++++-- .../authorization/authorization_service.ts | 6 ---- .../server/authorization/api_authorization.ts | 31 +++++++++++-------- .../authorization/authorization_service.tsx | 8 +++-- .../server/authorization/index.mock.ts | 2 -- 6 files changed, 51 insertions(+), 29 deletions(-) diff --git a/packages/core/http/core-http-router-server-internal/src/security_route_config_validator.test.ts b/packages/core/http/core-http-router-server-internal/src/security_route_config_validator.test.ts index 9e6fa95f20845..a0cadaacfbf7b 100644 --- a/packages/core/http/core-http-router-server-internal/src/security_route_config_validator.test.ts +++ b/packages/core/http/core-http-router-server-internal/src/security_route_config_validator.test.ts @@ -273,6 +273,20 @@ describe('RouteSecurity validation', () => { ); }); + it('should fail validation when allRequired has duplicate entries', () => { + const invalidRouteSecurity = { + authz: { + requiredPrivileges: [ + { anyRequired: ['privilege4', 'privilege5'], allRequired: ['privilege1', 'privilege1'] }, + ], + }, + }; + + expect(() => validRouteSecurity(invalidRouteSecurity)).toThrowErrorMatchingInlineSnapshot( + `"[authz.requiredPrivileges]: allRequired privileges must contain unique values"` + ); + }); + it('should fail validation when anyRequired has duplicates in multiple privilege entries', () => { const invalidRouteSecurity = { authz: { @@ -338,7 +352,7 @@ describe('RouteSecurity validation', () => { }, }) ).toThrowErrorMatchingInlineSnapshot( - `"[authz.requiredPrivileges]: Operator privileges cannot be used standalone"` + `"[authz.requiredPrivileges]: Operator privilege requires at least one additional non-operator privilege to be defined"` ); expect(() => @@ -348,7 +362,7 @@ describe('RouteSecurity validation', () => { }, }) ).toThrowErrorMatchingInlineSnapshot( - `"[authz.requiredPrivileges]: Operator privileges cannot be used standalone"` + `"[authz.requiredPrivileges]: Operator privilege requires at least one additional non-operator privilege to be defined"` ); }); }); diff --git a/packages/core/http/core-http-router-server-internal/src/security_route_config_validator.ts b/packages/core/http/core-http-router-server-internal/src/security_route_config_validator.ts index b62ab46eee1f5..ec54c5e61ce0d 100644 --- a/packages/core/http/core-http-router-server-internal/src/security_route_config_validator.ts +++ b/packages/core/http/core-http-router-server-internal/src/security_route_config_validator.ts @@ -68,12 +68,13 @@ const requiredPrivilegesSchema = schema.arrayOf( return 'Combining superuser with other privileges is redundant, superuser privileges set can be only used as a standalone privilege.'; } + // Operator privilege requires at least one additional non-operator privilege to be defined, that's why it's not allowed in anyRequired. if (anyRequired.includes(ReservedPrivilegesSet.operator)) { return 'Using operator privileges in anyRequired is not allowed'; } if (hasOperatorInAllRequired && allRequired.length === 1) { - return 'Operator privileges cannot be used standalone'; + return 'Operator privilege requires at least one additional non-operator privilege to be defined'; } if (anyRequired.length && allRequired.length) { @@ -85,12 +86,20 @@ const requiredPrivilegesSchema = schema.arrayOf( } if (anyRequired.length) { - const uniquePrivileges = new Set([...anyRequired]); + const uniqueAnyPrivileges = new Set([...anyRequired]); - if (anyRequired.length !== uniquePrivileges.size) { + if (anyRequired.length !== uniqueAnyPrivileges.size) { return 'anyRequired privileges must contain unique values'; } } + + if (allRequired.length) { + const uniqueAllPrivileges = new Set([...allRequired]); + + if (allRequired.length !== uniqueAllPrivileges.size) { + return 'allRequired privileges must contain unique values'; + } + } }, minSize: 1, } diff --git a/x-pack/packages/security/plugin_types_server/src/authorization/authorization_service.ts b/x-pack/packages/security/plugin_types_server/src/authorization/authorization_service.ts index af390daaab2c1..82fa3d3fcce5e 100644 --- a/x-pack/packages/security/plugin_types_server/src/authorization/authorization_service.ts +++ b/x-pack/packages/security/plugin_types_server/src/authorization/authorization_service.ts @@ -5,14 +5,10 @@ * 2.0. */ -import type { KibanaRequest } from '@kbn/core/server'; -import type { AuthenticatedUser } from '@kbn/security-plugin-types-common'; - import type { Actions } from './actions'; import type { CheckPrivilegesWithRequest } from './check_privileges'; import type { CheckPrivilegesDynamicallyWithRequest } from './check_privileges_dynamically'; import type { CheckSavedObjectsPrivilegesWithRequest } from './check_saved_objects_privileges'; -import type { EsSecurityConfig } from './es_security_config'; import type { AuthorizationMode } from './mode'; /** @@ -29,6 +25,4 @@ export interface AuthorizationServiceSetup { checkPrivilegesDynamicallyWithRequest: CheckPrivilegesDynamicallyWithRequest; checkSavedObjectsPrivilegesWithRequest: CheckSavedObjectsPrivilegesWithRequest; mode: AuthorizationMode; - getCurrentUser: (request: KibanaRequest) => AuthenticatedUser | null; - getSecurityConfig: () => Promise; } diff --git a/x-pack/plugins/security/server/authorization/api_authorization.ts b/x-pack/plugins/security/server/authorization/api_authorization.ts index 8d232f9642ad4..dbfc8d03000e4 100644 --- a/x-pack/plugins/security/server/authorization/api_authorization.ts +++ b/x-pack/plugins/security/server/authorization/api_authorization.ts @@ -5,17 +5,22 @@ * 2.0. */ +import { ReservedPrivilegesSet } from '@kbn/core/server'; import type { AuthzDisabled, AuthzEnabled, HttpServiceSetup, + KibanaRequest, Logger, Privilege, PrivilegeSet, RouteAuthz, } from '@kbn/core/server'; -import { ReservedPrivilegesSet } from '@kbn/core/server'; -import type { AuthorizationServiceSetup } from '@kbn/security-plugin-types-server'; +import type { AuthenticatedUser } from '@kbn/security-plugin-types-common'; +import type { + AuthorizationServiceSetup, + EsSecurityConfig, +} from '@kbn/security-plugin-types-server'; import type { RecursiveReadonly } from '@kbn/utility-types'; import { API_OPERATION_PREFIX, SUPERUSER_PRIVILEGES } from '../../common/constants'; @@ -28,6 +33,11 @@ const isReservedPrivilegeSet = (privilege: string): privilege is ReservedPrivile return Object.hasOwn(ReservedPrivilegesSet, privilege); }; +interface InitApiAuthorization extends AuthorizationServiceSetup { + getCurrentUser: (request: KibanaRequest) => AuthenticatedUser | null; + getSecurityConfig: () => Promise; +} + export function initAPIAuthorization( http: HttpServiceSetup, { @@ -37,7 +47,7 @@ export function initAPIAuthorization( mode, getCurrentUser, getSecurityConfig, - }: AuthorizationServiceSetup, + }: InitApiAuthorization, logger: Logger ) { http.registerOnPostAuth(async (request, response, toolkit) => { @@ -84,15 +94,13 @@ export function initAPIAuthorization( acc.push( operatorPrivilegeIndex !== -1 ? { - anyRequired: privilege.anyRequired, + ...privilege, // @ts-ignore wrong types for `toSpliced` allRequired: privilege.allRequired?.toSpliced(operatorPrivilegeIndex, 1), } : privilege ); - } - - if (privilege !== ReservedPrivilegesSet.operator) { + } else if (privilege !== ReservedPrivilegesSet.operator) { acc.push(privilege); } @@ -100,6 +108,8 @@ export function initAPIAuthorization( }, []); }; + // We need to normalize privileges to drop unintended privilege checks. + // Operator privileges check should be only performed if the `operator_privileges` are enabled in config. const requiredPrivileges = await normalizeRequiredPrivileges(authz.requiredPrivileges); const { requestedPrivileges, requestedReservedPrivileges } = requiredPrivileges.reduce( @@ -152,13 +162,8 @@ export function initAPIAuthorization( if (reservedPrivilege === ReservedPrivilegesSet.operator) { const currentUser = getCurrentUser(request); - const securityConfig = await getSecurityConfig(); - const isOperator = currentUser?.operator ?? false; - kibanaPrivileges[ReservedPrivilegesSet.operator] = securityConfig.operator_privileges - .enabled - ? isOperator - : false; + kibanaPrivileges[ReservedPrivilegesSet.operator] = currentUser?.operator ?? false; } } diff --git a/x-pack/plugins/security/server/authorization/authorization_service.tsx b/x-pack/plugins/security/server/authorization/authorization_service.tsx index 56e66f9b3b148..d58e24d3b84f6 100644 --- a/x-pack/plugins/security/server/authorization/authorization_service.tsx +++ b/x-pack/plugins/security/server/authorization/authorization_service.tsx @@ -146,8 +146,6 @@ export class AuthorizationService { checkPrivilegesWithRequest, getSpacesService ), - getCurrentUser, - getSecurityConfig: () => esSecurityConfig, }; capabilities.registerSwitcher( @@ -178,7 +176,11 @@ export class AuthorizationService { } ); - initAPIAuthorization(http, authz, loggers.get('api-authorization')); + initAPIAuthorization( + http, + { ...authz, getCurrentUser, getSecurityConfig: () => esSecurityConfig }, + loggers.get('api-authorization') + ); initAppAuthorization(http, authz, loggers.get('app-authorization'), features); http.registerOnPreResponse(async (request, preResponse, toolkit) => { diff --git a/x-pack/plugins/security/server/authorization/index.mock.ts b/x-pack/plugins/security/server/authorization/index.mock.ts index 8972518addd9e..c3b76a0908f13 100644 --- a/x-pack/plugins/security/server/authorization/index.mock.ts +++ b/x-pack/plugins/security/server/authorization/index.mock.ts @@ -24,7 +24,5 @@ export const authorizationMock = { privileges: { get: jest.fn() }, registerPrivilegesWithCluster: jest.fn(), disableUnauthorizedCapabilities: jest.fn(), - getCurrentUser: jest.fn(), - getSecurityConfig: jest.fn(), }), }; From b212b43bae4ff024613bbfb6dcb6ac04969e35c0 Mon Sep 17 00:00:00 2001 From: Elena Shostak Date: Thu, 12 Dec 2024 18:11:24 +0100 Subject: [PATCH 20/20] Fixes --- .../authorization/api_authorization.test.ts | 30 +++++++++++++++---- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/x-pack/plugins/security/server/authorization/api_authorization.test.ts b/x-pack/plugins/security/server/authorization/api_authorization.test.ts index 4e135cfed8d81..d2db2a535b1d1 100644 --- a/x-pack/plugins/security/server/authorization/api_authorization.test.ts +++ b/x-pack/plugins/security/server/authorization/api_authorization.test.ts @@ -20,7 +20,11 @@ import { authorizationMock } from './index.mock'; describe('initAPIAuthorization', () => { test(`protected route when "mode.useRbacForRequest()" returns false continues`, async () => { const mockHTTPSetup = coreMock.createSetup().http; - const mockAuthz = authorizationMock.create(); + const mockAuthz = { + ...authorizationMock.create(), + getCurrentUser: jest.fn(), + getSecurityConfig: jest.fn(), + }; initAPIAuthorization(mockHTTPSetup, mockAuthz, loggingSystemMock.create().get()); const [[postAuthHandler]] = mockHTTPSetup.registerOnPostAuth.mock.calls; @@ -44,7 +48,11 @@ describe('initAPIAuthorization', () => { test(`unprotected route when "mode.useRbacForRequest()" returns true continues`, async () => { const mockHTTPSetup = coreMock.createSetup().http; - const mockAuthz = authorizationMock.create(); + const mockAuthz = { + ...authorizationMock.create(), + getCurrentUser: jest.fn(), + getSecurityConfig: jest.fn(), + }; initAPIAuthorization(mockHTTPSetup, mockAuthz, loggingSystemMock.create().get()); const [[postAuthHandler]] = mockHTTPSetup.registerOnPostAuth.mock.calls; @@ -68,7 +76,11 @@ describe('initAPIAuthorization', () => { test(`protected route when "mode.useRbacForRequest()" returns true and user is authorized continues`, async () => { const mockHTTPSetup = coreMock.createSetup().http; - const mockAuthz = authorizationMock.create({ version: '1.0.0-zeta1' }); + const mockAuthz = { + ...authorizationMock.create({ version: '1.0.0-zeta1' }), + getCurrentUser: jest.fn(), + getSecurityConfig: jest.fn(), + }; initAPIAuthorization(mockHTTPSetup, mockAuthz, loggingSystemMock.create().get()); const [[postAuthHandler]] = mockHTTPSetup.registerOnPostAuth.mock.calls; @@ -105,7 +117,11 @@ describe('initAPIAuthorization', () => { test(`protected route when "mode.useRbacForRequest()" returns true and user isn't authorized responds with a 403`, async () => { const mockHTTPSetup = coreMock.createSetup().http; - const mockAuthz = authorizationMock.create({ version: '1.0.0-zeta1' }); + const mockAuthz = { + ...authorizationMock.create({ version: '1.0.0-zeta1' }), + getCurrentUser: jest.fn(), + getSecurityConfig: jest.fn(), + }; initAPIAuthorization(mockHTTPSetup, mockAuthz, loggingSystemMock.create().get()); const [[postAuthHandler]] = mockHTTPSetup.registerOnPostAuth.mock.calls; @@ -170,7 +186,11 @@ describe('initAPIAuthorization', () => { ) => { test(description, async () => { const mockHTTPSetup = coreMock.createSetup().http; - const mockAuthz = authorizationMock.create({ version: '1.0.0-zeta1' }); + const mockAuthz = { + ...authorizationMock.create({ version: '1.0.0-zeta1' }), + getCurrentUser: jest.fn(), + getSecurityConfig: jest.fn(), + }; initAPIAuthorization(mockHTTPSetup, mockAuthz, loggingSystemMock.create().get()); const [[postAuthHandler]] = mockHTTPSetup.registerOnPostAuth.mock.calls;