Subset of permissions check on creation

[derek-ho]

Description

This PR adds permission checks around API token creation, and cleans up misc. items from the feature branch, including authorizing API token endpoint properly and stashing the context in order to perform the API token refresh action without explicit permission to do so. This PR is dependent on #4992 to be merged first, and this PR only contains the following changes:

https://github.com/opensearch-project/security/pull/5012/files/aa506e78eb5699d2580e28d4bb0f0060971449e1..e13c055e063ae3e5ee40386a53155e5c65893b58

Issues Resolved

[List any issues this PR will resolve]

Is this a backport? If so, please add backport PR # and/or commits #, and remove backport-failed label from the original PR.

Do these changes introduce new permission(s) to be displayed in the static dropdown on the front-end? If so, please open a draft PR in the security dashboards plugin and link the draft PR here

Testing

[Please provide details of testing done: unit testing, integration testing and manual testing]

Check List

• New functionality includes testing
• New functionality has been documented
• New Roles/Permissions have a corresponding security dashboards plugin PR
• API changes companion pull request created
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[codecov]

Codecov Report

Attention: Patch coverage is 65.95745% with 16 lines in your changes missing coverage. Please review.

Project coverage is 71.15%. Comparing base (0edba23) to head (280e00c).
Report is 34 commits behind head on feature/api-tokens.

Files with missing lines	Patch %	Lines
...arch/security/action/apitokens/ApiTokenAction.java	40.74%	16 Missing ⚠️

Additional details and impacted files

@@                  Coverage Diff                   @@
##           feature/api-tokens    #5012      +/-   ##
======================================================
- Coverage               71.46%   71.15%   -0.31%     
======================================================
  Files                     334      354      +20     
  Lines                   22552    23275     +723     
  Branches                 3590     3680      +90     
======================================================
+ Hits                    16117    16562     +445     
- Misses                   4642     4881     +239     
- Partials                 1793     1832      +39     

Files with missing lines	Coverage Δ
.../opensearch/security/OpenSearchSecurityPlugin.java	83.92% <100.00%> (+0.36%)	⬆️
...ecurity/action/apitokens/ApiTokenIndexHandler.java	84.78% <100.00%> (ø)
.../security/action/apitokens/ApiTokenRepository.java	96.96% <ø> (ø)
...earch/security/auditlog/impl/AbstractAuditLog.java	76.88% <ø> (+0.15%)	⬆️
...rg/opensearch/security/dlic/rest/api/Endpoint.java	100.00% <100.00%> (ø)
...dlic/rest/api/RestApiAdminPrivilegesEvaluator.java	73.07% <100.00%> (+0.52%)	⬆️
...pensearch/security/http/ApiTokenAuthenticator.java	58.97% <ø> (ø)
...arch/security/action/apitokens/ApiTokenAction.java	24.82% <40.74%> (ø)

... and 9 files with indirect coverage changes

[nibix]

Assume that I have a role with the index_pattern /index_[0-9]+/. How can a requestedPattern look like that replicates this pattern?

At the moment, I have doubts that simple pattern matching can be used to support the full pattern semantics.

[nibix]

This also does not cover alias semantics.

[cwperks]

Assume that I have a role with the index_pattern /index_[0-9]+/. How can a requestedPattern look like that replicates this pattern?

Do role definitions support this? I have only seen this in a context of masked fields

Edit: Found an example: https://github.com/opensearch-project/security/blob/main/src/test/resources/roles.yml#L20-L33

Edit2: Is that the purpose of the renderedMatcher?

Looks like this is handled by WildcardMatcher itself.

[derek-ho]

Added alias support in this commit: bf31791. I also shimmed in a way to not pass in a privilegesEvaluationContext into the indexpattern class, as long as you pass in the used components out of the context. What do y'all think of this change?

[cwperks]

This also does not cover alias semantics.

How would this not cover aliases? i.e. if a user has a role mapping with read access to logs and requests a token with read access to logs (assuming logs is an alias) won't this work?

Agree on regexp support here, @derek-ho can we throw an IllegalArgumentException if the requested permission is for a regexp?

[derek-ho]

I think it doesn't cover alias support for underlying indices, i.e. Users with access to logs alias would not be able to request api tokens for a subset of any of the underlying indices, even when they have permission to do so.

[nibix]

Regarding the regexp issue:

I think, one of the fundamental issues here is the following:

Normal users generally do not have access to the roles.yml configuration. Thus, they do not know how they gain access to the indices - they just know that they have access to the indices they can observe.

Thus, they need to resort to trial-and-error process to see whether they can gain API tokens to an index or whether the roles.yml definition uses a construct that is not supported.

Obviously, this is sub-optimal from a UX point of view.

Ways around that issue - just from the top of my head:

• Just support API token creation for admin users only. These have full access anyway, and all the checks for intersection become unnecessary (as described in Subset of permissions check on creation #5012 (comment)).
• Just support API token creation for concrete index names, not for patterns. Then you do not have to match regexes against regexes.
• Move the process of determining the intersection of requested privileges and available privileges from the token creation to the privilege evaluation phase. Thus, one would change the check "does roles.yml provide privileges" into "does roles.yml provide privileges AND does the api token request the privileges". This would also solve - to a certain extent - the issue of changes to roles.yml.

[cwperks]

Obviously, this is sub-optimal from a UX point of view.

Can't this be handled another way though? i.e. prevent using tokens entirely if any role uses regex in index_permissions or skip the subset check for any role using regexes?

One other way this could be handled is creating an API that a user can call that helps them comprehend what actions and index patterns they are able to select. This API could also be used to power UX for the feature.

Just support API token creation for concrete index names, not for patterns. Then you do not have to match regexes against regexes.

What about glob-like patterns?

[nibix]

What about glob-like patterns?

It is quite a brain-twister to think about matching patterns against patterns 😅 .. but I thought this through again, and there might be another issue.

Consider:

• I have in roles.yml privileges for the pattern index_1?. This gives me privileges on index_10, index_11, etc., but not on index_1.

• I now try to request an API token for the index pattern index_1*. The pattern index_1? will positively match index_1*. The * character will be bound to the ?. However, the pattern index_1* is broader than index_1?. It will also match index_1, which was not matched by index_1?. Thus, we would have a privilege escalation vulnerability.

Of course, this might be quite a theoretical issue where its real world applicability is questionable. Still, IMHO, security software should be also safe of such scenarios.

Thus, I would strongly recommend against matching any patterns against patterns. It is just too fragile.

I would still recommend the approach sketched out above:

Move the process of determining the intersection of requested privileges and available privileges from the token creation to the privilege evaluation phase. Thus, one would change the check "does roles.yml provide privileges" into "does roles.yml provide privileges AND does the api token request the privileges". This would also solve - to a certain extent - the issue of changes to roles.yml.

[nibix]

What do you think about having a dedicated transport action backing the REST action? Then, one would get access control out of the box without needing explicit checks within the action code.

[cwperks]

Agree with this though it depends on the action.

If the security admin is going to a page in the security dashboards plugin to manage the existing tokens (listing them and performing actions such as revoke) then these should follow the authorization model of other security APIs.

If regular users are allowed to request a token (with a subset of their own permissions) then that could be authorized like other cluster actions in OpenSearch.

[derek-ho]

I think (for now) my operating assumption would be that this is an admin-level action to issue api tokens to connect with other services programmatically. Since API tokens (currently) do not follow the new optimized privileges evaluation, and thus have higher performance penalty than other means of authc/z, and thus we should restrict this as much as possible. However, if there is community feedback/requests for this feature, then we can consider it at a later time. Since we want to authorize these APIs like the others, I added this logic.

[nibix]

To be honest, it is a bit difficult for me to review this without a proper scope definition. During review, I learned from you about a couple of assumptions and assumed limitations (such as not using the optimized privilege evaluation code paths, not supporting regexes, etc). IMHO, there should be some definitions in the issue #1504 what's the initial goal to be achieved and what not. That will be then an information to guide the code review process. Otherwise, this review process will get quite inefficient.

If the assumption is indeed that issuing tokens is an admin-level action, I do wonder why we need the matching to existing privileges at all. Won't an admin-level action just be similar to granting new privileges?

[cwperks]

If its rolled out to admin users initially at first than I agree, but if the longer term plan is for other users to be able to request tokens then the logic to determine whether the requested permissions are a subset of the user's permissions would be needed. I was thinking to keep the logic simple and only consider: 1) cluster_permissions and 2) index_permissions. For index permissions, would it be sufficient to limit the checks to concrete indices and glob-like patterns and prohibit regexes?

In practice, I have not come across many roles definitions where cluster administrators use regex when defining the index patterns in a role. I'm sure its used, but not prevalent from my experience.

[cwperks]

Should we consider following a similar model to AbstractApiAction here?

I know this action is not a subset of AbstractApiAction (since it doesn't alter anything in the security index), but maybe it can be modeled similarly? Part of that block also registers an updateHandler to synchronize across the cluster.

[nibix]

I am a bit confused about returning WildcardMatcher.NONE here. Why do we do this?

[derek-ho]

Since I changed the index pattern class to take in a matcher generator, here I need to pass in a function to generate a wildcardmatcher (as I don't have a PrivilegesEvaluationContext to pass in). In the Index pattern class, the wildcardmatcher is used to evaluate pattern templates/user attributes, which we don't plan to support for API tokens, and thus I passed in a WildCardMatcher that doesn't match anything to not support that use case.

[cwperks]

Overall looks good to me. Left a comment about try to unify endpoint authorization of these new endpoints and existing endpoints that subclass AbstractApiAction.

[cwperks]

nit: we can start creating these inline.

[cwperks]

Should we consider following a similar model to AbstractApiAction here?

I know this action is not a subset of AbstractApiAction (since it doesn't alter anything in the security index), but maybe it can be modeled similarly? Part of that block also registers an updateHandler to synchronize across the cluster.

[cwperks]

This refactor looks ok, but may be unnecessary now.

[derek-ho]

Removed