Role changes to support enforcing workflow restrictions

[slobodanadamovic]

This PR implements necessary changes to Role classes in order to enforce
workflow restrictions for API keys. Main change is around resolving roles from
role descriptors, where a role can either be effectively empty (or not)
depending on its workflow restriction.

Currently, the only workflow supported is search_application_query which
allows restricting access only to Search Application Search API.

For simplicity, when creating an API key, it's possible to specify only a single
role descriptor with workflows restriction. For example:

POST /_security/api_key { "name": "my_restricted_api_key", "role_descriptors": { "my_restricted_role": { "indices": [ { "names": ["books"], "privileges": ["read"] } ], "restriction": { "workflows": ["search_application_query"] } } } } 

Relates to #96215

[ywangd]

I don't think this is an issue, but would like to write down for prosperity: v1 is the run-as user's role, v2 is the authenticating user's role. If v1 is empty and restricted while v2 is a proper role, one might argue that v2 should be able to run-as v1 (i.e. pass authorizeRunAs) then fail.

I think it can be argued both ways because:

We have a similar situation where request fails before authorization happens if run-as user is disabled.

At the same time, we also have request fails at authorization if the run-as user does not exist.

These two cases do not agree with each other. But more often we view the later one as an issue since it make an exception that realm can be null and has been causing issues in handling it. Therefore I overall feel it's OK to fail earlier here. Letting it passing through is not actually useful other than for argument sake.

The practical bottom line is that it is not possible to run-as API keys. So not a real issue anyway.

[slobodanadamovic]

Thanks for raising this topic. FWIW My reasoning was that both users (authenticating and run-as) should have access to a particular workflow. Allowing the authenticating user to invoke a workflow should be prevented early on, even if run-as user can access it.

Additionally, what we could potentially do is differentiate between which role was restricted, but this felt unnecessary at the moment.

[ywangd]

Allowing the authenticating user to invoke a workflow should be prevented early on, even if run-as user can access it.

My view is the opposite. When

1. The API is a workflow API
2. The authenticating user has the privilege to run-as the effective user
3. The effective user has a workflow role that allows it to call the workflow API

In this situation, the authorization should succeed. I believe that's what naturally would happen with the proposed change. Because Item 2 implies the authenticating user must have access to the workflow as well, either because the authenticating user has a workflow role or has a role without workflow restriction.

My original comment was about the failure case. That is, Item 1 & 2 remain unchanged, but in Item 3, the effective user does not have access to the workflow API. In this case, we could let the run-as (Item 2) to be successful (it generates an audit log entry runAsGranted), then fail due to Item 3. Or we can fail before even check Item 2 (and skip the runAsGranted audit entry), which is the behaviour of the proposed change. Both options are valid to some extend. But going with the later option seems to give us more flexibility in future (thinking moving rejection to the REST layer).

[slobodanadamovic]

@elasticmachine run elasticsearch-ci/bwc

[ywangd]

LGTM

I left quite a few comments. I'd appreciate if you could address them. Three of them are of some importance:

• Allow empty list of workflow names in WorkflowRestriction to mean nothing is permitted (workflow or not)
• Call Role.forWorkflow(...) in CompositeRolesStore.getRole on the final role built from role reference.
• Audit log or not

Please ping me if you need any clarifications. Thanks!

In addition, I think we need the >feature label instead of >non-issue for this PR since the doc PR will likely land after FF. So it is better for another PR to carry the >feature label and I think this PR is likely the best candidate.

[ywangd]

I have a slight BWC concern for adding this audit event if we later move role resolution to the REST layer. We don't have transportRequest or action name at the REST layer. Therefore it will not be possible to call accessDenied as is today, i.e. we will need to modify this event, probably in a non-BWC compatible way. But that change may take a long time to come, so having this even does have short term benefit.

I slightly prefer to leave it out. What do you think?

PS: If you decided to keep it, you'll want to add auditing tests for it, probably in a separate PR.

[ywangd]

Instead of calling Role.forWorkflow here and in buildThenMaybeCacheRole, I think we should call it once in the getRole(..) method, i.e. something like:

roleReferenceIntersection.buildRole(this::buildRoleFromRoleReference, ActionListener.wrap( role -> roleActionListener.onResponse(role.forWorkflow(workflow)), roleActionListener::onFailure));

The reason is that for LimitedRole, the proposed change actually ends up not calling LimitedRole.forWorkflow on it. It calls the method on its baseRole and limitedByRole, but never calls it on the final LimitedRole. It works in practice because Role.limitedBy short-circuits and returns an "empty-restricted" if either of them is "empty-restricted". But there is a potential inconsistency: the LimitedRole constructor allows baseRole to be empty-restricted. If that ever happens, without calling LimitedRole.forWorkflows, the check in RBACEngine (role == EMPTY_RESTRICTED_BY_WORKFLOW) can misbehave. In summary, we should ensure that we call forWorkflow on the final role that is built from the role reference.

[slobodanadamovic]

Good catch. I missed this, as I was making changes bottom-up. Hence why I end-up needing a short-circuit in Role.limitedBy.

[ywangd]

Nit: Should this be named ElasticsearchRoleRestrictionException. Or are you intentionally keeping it generic? Being access-restricted seems to suggest it to be applied in many other places. But that is not really the case.

[slobodanadamovic]

Initially, I wanted it to be generic, but I think now that your suggestion is better.

[elasticsearchmachine]

Hi @slobodanadamovic, I've created a changelog YAML for you.