[Authz] Operator privileges #196583
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,8 @@ export function initAPIAuthorization( | |
| checkPrivilegesDynamicallyWithRequest, | ||
| checkPrivilegesWithRequest, | ||
| mode, | ||
| getCurrentUser, | ||
| getClusterClient, | ||
| }: AuthorizationServiceSetup, | ||
| logger: Logger | ||
| ) { | ||
|
|
@@ -53,15 +55,72 @@ export function initAPIAuthorization( | |
|
|
||
| const authz = security.authz as AuthzEnabled; | ||
|
|
||
| const normalizeRequiredPrivileges = async ( | ||
elena-shostak marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| privileges: AuthzEnabled['requiredPrivileges'] | ||
|
There was a problem hiding this comment. optional nit: maybe we need to expose\export type Privileges = Array<Privilege | PrivilegeSet>;
export interface AuthzEnabled {
requiredPrivileges: Privileges;
} |
||
| ) => { | ||
| 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<{ | ||
elena-shostak marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Note It's probably fine for now, but this is a hot path and if ever need to a high-performance endpoint to require operator privilege we might need to cache this response for some time. There was a problem hiding this comment. IMO this call is too expensive to put in a hot path such as this. I think we should look into a way to cache this during startup. I recall SDHs where our frequent use of this endpoint elsewhere has caused performance issues on the cluster There was a problem hiding this comment. I'm a bit concerned that if we determine this only once during startup, admins will have to restart Kibana whenever this functionality is enabled or disabled in Elasticsearch. If we want to avoid that risk, we could either cache the result near the invocation point or implement a periodic background job to handle it (which is probably too much comparing to simple cache). Do you think requiring a Kibana restart along with Elasticsearch would be a reasonable approach (similar to how it works for ES, although for the config that doesn't belong to Kibana)? Alternatively, we can require explicit operator flag/config for Kibana as well, that we can compare with the response from ES at startup time and bail if they don't match... Not sure if I like this option though There was a problem hiding this comment. I'd share your concerns if this was a different ES setting. Operator Privileges are only used by our orchestrated offerings, and are configured via files that reside on each node. I do not expect the same cluster to switch modes at runtime, and we explicitly do not support direct usage of this feature: https://www.elastic.co/guide/en/elasticsearch/reference/current/operator-privileges.html Since we are in control over the configuration of Operator Privileges, it seems like it'd be sufficient to check this once on startup, and require a restart in the event the cluster somehow switched between using Operator Privileges and not using Operator Privileges. There was a problem hiding this comment. We make a call to operator privileges config only on startup now |
||
| 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 = | ||
elena-shostak marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 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 { requestedPrivileges, requestedReservedPrivileges } = requiredPrivileges.reduce( | ||
| (acc, privilegeEntry) => { | ||
| const privileges = | ||
| typeof privilegeEntry === 'object' | ||
| ? [...(privilegeEntry.allRequired ?? []), ...(privilegeEntry.anyRequired ?? [])] | ||
| : [privilegeEntry]; | ||
|
|
||
| for (const privilege of privileges) { | ||
| if ( | ||
| isReservedPrivilegeSet(privilege) && | ||
| !acc.requestedReservedPrivileges.includes(privilege) | ||
| ) { | ||
| acc.requestedReservedPrivileges.push(privilege); | ||
| } else { | ||
| acc.requestedPrivileges.push(privilege); | ||
|
|
@@ -97,10 +156,14 @@ export function initAPIAuthorization( | |
| const checkSuperuserPrivilegesResponse = await checkPrivilegesWithRequest( | ||
| request | ||
| ).globally(SUPERUSER_PRIVILEGES); | ||
| kibanaPrivileges[ReservedPrivilegesSet.superuser] = | ||
| checkSuperuserPrivilegesResponse.hasAllRequested; | ||
| } | ||
|
|
||
| if (reservedPrivilege === ReservedPrivilegesSet.operator) { | ||
| const currentUser = getCurrentUser(request); | ||
| kibanaPrivileges[ReservedPrivilegesSet.operator] = currentUser?.operator ?? false; | ||
| } | ||
| } | ||
|
|
||
| const hasRequestedPrivilege = (kbPrivilege: Privilege | PrivilegeSet) => { | ||
|
|
@@ -118,8 +181,8 @@ export function initAPIAuthorization( | |
| return kibanaPrivileges[kbPrivilege]; | ||
| }; | ||
|
|
||
| for (const privilege of requiredPrivileges) { | ||
| if (!hasRequestedPrivilege(privilege)) { | ||
| const missingPrivileges = Object.keys(kibanaPrivileges).filter( | ||
| (key) => !kibanaPrivileges[key] | ||
| ); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we discussed this here: #196271 (comment), but it seems we didn’t reach an agreement. This behavior deviates slightly from Elasticsearch’s approach, making it harder to reason about operator privileges across the Elastic project as a whole. This isn’t necessarily a bad thing, but we should explicitly state the benefits if we decide not to align with Elasticsearch’s treatment of operator privilege requirements.
Is there a reason we can’t or don't want to require developers to explicitly list additional privileges (beyond operator privileges) when exposing a Kibana endpoint to operator users? This would ensure privilege the same checks occur even if the operator user functionality is not enabled. The additional privilege could be superuser, but it could also be a role with fewer privileges if superuser access isn’t strictly necessary. This would discourage admins from granting superuser privileges to operator users unnecessarily.
The only potential issue I see with requiring additional privileges is if there’s a use case where no extra privilege check is needed at the API level because the handler solely relies on user-scoped Saved Objects or Elasticsearch services. If such a use case arises, we could explore solutions to handle it appropriately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was for implementing it in the beginning and then thought to make it more feasible in terms of DX. That was a bit premature 😄
Agree, that makes sense
Thanks for the feedback, will address the PR accordingly 👍