Api token authc/z implementation with Cache #4992

derek-ho · 2024-12-24T17:20:50Z

Description

This PR implements authc/z for api tokens. For authc it is based on token validity for as well as presence in a cache, which listens to index and delete operations on the api token index. For Authz, it onboards onto action privileges class, where instead of returning not authorized for cluster and index privileges, requests go through a final api token check, where if the user is an api token, it will evaluate whether the permissions in the cache are sufficient to execute the request.

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.

Signed-off-by: Derek Ho <[email protected]>

codecov · 2024-12-26T17:28:57Z

Codecov Report

Attention: Patch coverage is 61.69265% with 172 lines in your changes missing coverage. Please review.

Project coverage is 71.18%. Comparing base (0edba23) to head (77ad6c3).
Report is 32 commits behind head on feature/api-tokens.

Files with missing lines	Patch %	Lines
...arch/security/action/apitokens/ApiTokenAction.java	7.14%	39 Missing ⚠️
...pensearch/security/http/ApiTokenAuthenticator.java	58.97%	22 Missing and 10 partials ⚠️
...ensearch/security/privileges/ActionPrivileges.java	77.55%	11 Missing and 11 partials ⚠️
...urity/action/apitokens/ApiTokenUpdateResponse.java	0.00%	14 Missing ⚠️
...ction/apitokens/TransportApiTokenUpdateAction.java	22.22%	14 Missing ⚠️
...es/PermissionBasedPrivilegesEvaluationContext.java	36.36%	7 Missing ⚠️
...curity/action/apitokens/ApiTokenUpdateRequest.java	0.00%	6 Missing ⚠️
...earch/security/auditlog/impl/AbstractAuditLog.java	40.00%	3 Missing and 3 partials ⚠️
...ivileges/RoleBasedPrivilegesEvaluationContext.java	54.54%	5 Missing ⚠️
...y/action/apitokens/ApiTokenUpdateNodeResponse.java	0.00%	4 Missing ⚠️
... and 10 more

Additional details and impacted files

@@                  Coverage Diff                   @@
##           feature/api-tokens    #4992      +/-   ##
======================================================
- Coverage               71.46%   71.18%   -0.28%     
======================================================
  Files                     334      354      +20     
  Lines                   22552    23272     +720     
  Branches                 3590     3680      +90     
======================================================
+ Hits                    16117    16567     +450     
- Misses                   4642     4864     +222     
- Partials                 1793     1841      +48

Files with missing lines	Coverage Δ
.../opensearch/security/OpenSearchSecurityPlugin.java	`83.87% <100.00%> (+0.31%)`	⬆️
...opensearch/security/action/apitokens/ApiToken.java	`91.20% <100.00%> (ø)`
...ecurity/action/apitokens/ApiTokenUpdateAction.java	`100.00% <100.00%> (ø)`
...g/opensearch/security/authtoken/jwt/JwtVendor.java	`82.75% <100.00%> (-6.34%)`	⬇️
...rity/authtoken/jwt/claims/ApiJwtClaimsBuilder.java	`100.00% <100.00%> (ø)`
...curity/privileges/PrivilegesEvaluationContext.java	`100.00% <100.00%> (+2.70%)`	⬆️
...ch/security/securityconf/DynamicConfigFactory.java	`62.74% <100.00%> (+0.24%)`	⬆️
...g/opensearch/security/ssl/util/ExceptionUtils.java	`45.83% <100.00%> (+2.35%)`	⬆️
...a/org/opensearch/security/util/AuthTokenUtils.java	`66.66% <100.00%> (+4.16%)`	⬆️
.../security/action/apitokens/ApiTokenRepository.java	`96.96% <94.44%> (ø)`
... and 19 more

... and 12 files with indirect coverage changes

Signed-off-by: Derek Ho <[email protected]>

…rmissions Signed-off-by: Derek Ho <[email protected]>

Signed-off-by: Derek Ho <[email protected]>

derek-ho · 2024-12-31T20:32:04Z

src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java

-                            originalSource = (new String(BaseEncoding.base64().decode((String) base64), StandardCharsets.UTF_8));
-                        } else {
-                            originalSource = XContentHelper.convertToJson(originalResult.internalSourceRef(), false, XContentType.JSON);
+                    if (originalSource == null) {


Changes in this file are to correct a mis-merge that happened in prior PRs to this feature branch.

Signed-off-by: Derek Ho <[email protected]>

src/main/java/org/opensearch/security/action/apitokens/ApiTokenIndexListenerCache.java

src/main/java/org/opensearch/security/privileges/PrivilegesEvaluationContext.java

src/test/java/org/opensearch/security/action/apitokens/ApiTokenAuthenticatorTest.java

Signed-off-by: Derek Ho <[email protected]>

src/main/java/org/opensearch/security/privileges/PrivilegesEvaluationContext.java

Signed-off-by: Derek Ho <[email protected]>

src/main/java/org/opensearch/security/privileges/PrivilegesEvaluationContext.java

src/main/java/org/opensearch/security/privileges/ActionPrivileges.java

src/main/java/org/opensearch/security/authtoken/jwt/ApiTokenJwtVendor.java

src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java

src/main/java/org/opensearch/security/action/apitokens/ApiTokenRepository.java

…curity into auth Signed-off-by: Derek Ho <[email protected]>

Signed-off-by: Derek Ho <[email protected]>

src/main/java/org/opensearch/security/authtoken/jwt/claims/JwtClaimsBuilder.java

src/main/java/org/opensearch/security/privileges/PrivilegesEvaluationContext.java

src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java

src/main/java/org/opensearch/security/securityconf/DynamicConfigModelV7.java

src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java

Signed-off-by: Derek Ho <[email protected]>

src/main/java/org/opensearch/security/privileges/ActionPrivileges.java

Signed-off-by: Derek Ho <[email protected]>

cwperks · 2025-01-23T21:51:04Z

src/main/java/org/opensearch/security/privileges/ActionPrivileges.java

    }

    public PrivilegesEvaluatorResponse hasAnyClusterPrivilege(PrivilegesEvaluationContext context, Set<String> actions) {
-        return cluster.providesAnyPrivilege(context, actions, context.getMappedRoles());
+        if (context instanceof RoleBasedPrivilegesEvaluationContext) {


This looks like a good contender for polymorphism.

In order to avoid the if/else constructs which are very prominent in this PR, I would recommend to extract ActionPrivileges as an interface or abstract super class, and have one implementation which deals with RoleBasedPrivilegesEvaluationContext and another implementation which deals with PermissionBasedPrivilegesEvaluationContext.

nibix · 2025-01-24T09:45:07Z

src/integrationTest/java/org/opensearch/security/privileges/ActionPrivilegesTest.java

+        return ctxWithUserName("test-user", roles);
+    }
+
+    static RoleBasedPrivilegesEvaluationContext ctxWithUserName(String userName, String... roles) {


Can the name ctxWithUserName() be somehow refined? Considering that ctxForApiToken() also accepts a userName param, it is unclear by the name what the difference to ctxWithUserName() is.

nibix · 2025-01-24T09:50:10Z

src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java

@@ -373,14 +373,14 @@ public void handleSearchContext(SearchContext searchContext, ThreadPool threadPo
                return;
            }

-            PrivilegesEvaluationContext privilegesEvaluationContext = this.dlsFlsBaseContext.getPrivilegesEvaluationContext();
-            if (privilegesEvaluationContext == null) {
+            PrivilegesEvaluationContext PrivilegesEvaluationContext = this.dlsFlsBaseContext.getPrivilegesEvaluationContext();


Nit: PrivilegesEvaluationContext should be lower case

nibix · 2025-01-24T09:50:39Z

src/main/java/org/opensearch/security/configuration/SecurityFlsDlsIndexSearcherWrapper.java

@@ -110,13 +110,13 @@ public SecurityFlsDlsIndexSearcherWrapper(
    protected DirectoryReader dlsFlsWrap(final DirectoryReader reader, boolean isAdmin) throws IOException {

        final ShardId shardId = ShardUtils.extractShardId(reader);
-        PrivilegesEvaluationContext privilegesEvaluationContext = this.dlsFlsBaseContext.getPrivilegesEvaluationContext();
+        PrivilegesEvaluationContext PrivilegesEvaluationContext = this.dlsFlsBaseContext.getPrivilegesEvaluationContext();


Nit: PrivilegesEvaluationContext should be lower case

nibix · 2025-01-24T09:56:52Z

src/main/java/org/opensearch/security/privileges/ActionPrivileges.java

    }

    public PrivilegesEvaluatorResponse hasAnyClusterPrivilege(PrivilegesEvaluationContext context, Set<String> actions) {
-        return cluster.providesAnyPrivilege(context, actions, context.getMappedRoles());
+        if (context instanceof RoleBasedPrivilegesEvaluationContext) {


In order to avoid the if/else constructs which are very prominent in this PR, I would recommend to extract ActionPrivileges as an interface or abstract super class, and have one implementation which deals with RoleBasedPrivilegesEvaluationContext and another implementation which deals with PermissionBasedPrivilegesEvaluationContext.

nibix · 2025-01-24T10:02:19Z

src/main/java/org/opensearch/security/privileges/ActionPrivileges.java

+                }
+
+                if (!indexHasAllPermissions) {
+                    return PrivilegesEvaluatorResponse.insufficient("Insufficient permissions for the index" + concreteIndex);


This error message does not indicate which permission is missing. It also does not mention any other potential indices which also might have missing permissions. Thus, it is not really user friendly.

I would recommend to use the CheckTable class as it is also used by the other index privilege evaluation methods.

Additionally, the API contract for hasIndexPrivilege() which calls this method defines that situations where there are privileges available for sub-sets of indices are reported with a isPartiallyOk() response, combined with the set of available indices:

security/src/main/java/org/opensearch/security/privileges/ActionPrivileges.java

Lines 165 to 179 in 472f7c7

/**

* Checks whether this instance provides privileges for the combination of the provided action,

* the provided indices and the provided roles.

* <p>

* Returns a PrivilegesEvaluatorResponse with allowed=true if privileges are available.

* <p>

* If privileges are only available for a sub-set of indices, isPartiallyOk() will return true

* and the indices for which privileges are available are returned by getAvailableIndices(). This allows the

* do_not_fail_on_forbidden behaviour.

*/

public PrivilegesEvaluatorResponse hasIndexPrivilege(

PrivilegesEvaluationContext context,

Set<String> actions,

IndexResolverReplacer.Resolved resolvedIndices

) {

This is necessary for do_not_fail_on_forbidden functionality. Without that behaviour, that functionality won't work correctly for API tokens.

nibix · 2025-01-24T10:10:33Z

src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java

@@ -127,6 +128,7 @@ public final static <T> SecurityDynamicConfiguration<T> addStatics(SecurityDynam
    private final Path configPath;
    private final InternalAuthenticationBackend iab;
    private final ClusterInfoHolder cih;
+    private final ApiTokenRepository ar;


Nit: Despite the abbreviations that are used in the existing code, I would recommend to use non-abbreviated attribute names, as it strongly improves code readability.

nibix · 2025-01-24T10:13:07Z

src/main/java/org/opensearch/security/securityconf/DynamicConfigModelV7.java

+                new NoOpAuthenticationBackend(Settings.EMPTY, null),
+                new ApiTokenAuthenticator(getDynamicApiTokenSettings(), this.cih.getClusterName(), ar),
+                false,
+                -2


If a user has configured an auth domain with the priority -2, this will be removed in favor of the ApiTokenAuthenticator. This might be unexpected behavior.

nibix · 2025-01-24T10:17:32Z

src/main/java/org/opensearch/security/action/apitokens/ApiTokenRepository.java

+    private final Map<String, Permissions> jtis = new ConcurrentHashMap<>();
+
+    void reloadApiTokensFromIndex() {
+        Map<String, ApiToken> tokensFromIndex = apiTokenIndexHandler.getTokenMetadatas();


ApiTokenIndexHandler seems to do a actionGet, i.e., a sync operation. Whenever we're on transport operations, I'd strongly recommend to prefer async operations using action listeners, as otherwise there's a slight deadlock risk due to thread pool exhaustion.

nibix · 2025-01-24T10:24:14Z

src/main/java/org/opensearch/security/privileges/ActionPrivileges.java

+                    // First check if this permission applies to this index
+                    boolean indexMatched = false;
+                    for (String pattern : indexPermission.getIndexPatterns()) {
+                        if (WildcardMatcher.from(pattern).test(concreteIndex)) {


OpenSearch has the property that privileges on aliases also grant privileges on indices contained inside the alias. Similar properties hold for data streams and backing indices. I think this support is missing here.

nibix · 2025-01-24T10:33:26Z

...main/java/org/opensearch/security/privileges/PermissionBasedPrivilegesEvaluationContext.java

+
+    @Override
+    public ImmutableSet<String> getMappedRoles() {
+        return ImmutableSet.of();


There are quite a few dependencies on mapped roles.

There's ProtectedIndexEvaluator which uses the setting plugins.security.protected_indices.roles to define roles which can access protected indices

PrivilegeEvaluator uses mapped roles to realize the Dashboards multi tenancy implementation

User attributes exposes the mapped roles via ${user.securityRoles}, thus they can be used in DLS statements

Is it anticipated that this functionality won't be available with API tokens?

derek-ho added 4 commits December 24, 2024 12:20

Naive cluster permission authz and authc based on token validity

9f695fa

Signed-off-by: Derek Ho <[email protected]>

Crude index permissions authz

d3fcc4a

Signed-off-by: Derek Ho <[email protected]>

Fix tests

6904317

Signed-off-by: Derek Ho <[email protected]>

Revert mis-merge in abstractauditlog

17bca93

Signed-off-by: Derek Ho <[email protected]>

derek-ho changed the title ~~Naive cluster permission authz and authc based on token validity~~ Naive authc+z api token implementation Dec 26, 2024

derek-ho added 6 commits December 27, 2024 12:34

Add allowlist for authc, add basic test showing it works

92d4e60

Signed-off-by: Derek Ho <[email protected]>

Add more extensive tests for authenticator, switch to list of indexPe…

22cfbe8

…rmissions Signed-off-by: Derek Ho <[email protected]>

Directly store permissions in the cache

665b9e9

Signed-off-by: Derek Ho <[email protected]>

Remove permissions from jti

e39df0d

Signed-off-by: Derek Ho <[email protected]>

Onboard onto clusterPrivileges

ad63974

Signed-off-by: Derek Ho <[email protected]>

Add index permissions api token eval

73eb2ab

Signed-off-by: Derek Ho <[email protected]>

derek-ho commented Dec 31, 2024

View reviewed changes

Add testing for cluster and index priv

6418226

Signed-off-by: Derek Ho <[email protected]>

derek-ho marked this pull request as ready for review December 31, 2024 21:57

derek-ho requested review from cwperks, DarshitChanpura, nibix, peternied, RyanL1997, reta and willyborankin as code owners December 31, 2024 21:57

derek-ho changed the title ~~Naive authc+z api token implementation~~ Api token authc/z implementation with Cache Dec 31, 2024

cwperks reviewed Jan 2, 2025

View reviewed changes

src/main/java/org/opensearch/security/action/apitokens/ApiTokenIndexListenerCache.java Outdated Show resolved Hide resolved

cwperks reviewed Jan 2, 2025

View reviewed changes

src/main/java/org/opensearch/security/privileges/PrivilegesEvaluationContext.java Outdated Show resolved Hide resolved

cwperks reviewed Jan 2, 2025

View reviewed changes

src/test/java/org/opensearch/security/action/apitokens/ApiTokenAuthenticatorTest.java Outdated Show resolved Hide resolved

cwperks mentioned this pull request Jan 3, 2025

Revert Optimized Privilege Evaluation (#4380) on main #5002

Closed

5 tasks

derek-ho added 3 commits January 6, 2025 12:42

Use transport action

bc8aacf

Signed-off-by: Derek Ho <[email protected]>

Cleanup tests and constants

b90bae9

Signed-off-by: Derek Ho <[email protected]>

Fix test

552aeda

Signed-off-by: Derek Ho <[email protected]>

cwperks reviewed Jan 13, 2025

View reviewed changes

src/main/java/org/opensearch/security/privileges/PrivilegesEvaluationContext.java Outdated Show resolved Hide resolved

derek-ho added 8 commits January 14, 2025 11:19

PR review

d9cf78d

Signed-off-by: Derek Ho <[email protected]>

Spotless

d7b7e47

Signed-off-by: Derek Ho <[email protected]>

Fix tests

6571d9d

Signed-off-by: Derek Ho <[email protected]>

PR review

9fcb720

Signed-off-by: Derek Ho <[email protected]>

Inject

e44072c

Signed-off-by: Derek Ho <[email protected]>

Fix

0927d99

Signed-off-by: Derek Ho <[email protected]>

Fix tests

fb79014

Signed-off-by: Derek Ho <[email protected]>

Final cleanup

0f60569

Signed-off-by: Derek Ho <[email protected]>

cwperks reviewed Jan 17, 2025

View reviewed changes

src/main/java/org/opensearch/security/privileges/PrivilegesEvaluationContext.java Outdated Show resolved Hide resolved

cwperks reviewed Jan 17, 2025

View reviewed changes

src/main/java/org/opensearch/security/privileges/ActionPrivileges.java Outdated Show resolved Hide resolved

cwperks reviewed Jan 17, 2025

View reviewed changes

src/main/java/org/opensearch/security/authtoken/jwt/ApiTokenJwtVendor.java Outdated Show resolved Hide resolved

cwperks reviewed Jan 17, 2025

View reviewed changes

src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java Outdated Show resolved Hide resolved

cwperks reviewed Jan 17, 2025

View reviewed changes

src/main/java/org/opensearch/security/action/apitokens/ApiTokenRepository.java Show resolved Hide resolved

derek-ho added 4 commits January 17, 2025 17:21

Merge branch 'feature/api-tokens' of github.com:opensearch-project/se…

6634896

…curity into auth Signed-off-by: Derek Ho <[email protected]>

Fix tests

83769b1

Signed-off-by: Derek Ho <[email protected]>

PR feedback

685dcfc

Signed-off-by: Derek Ho <[email protected]>

Mock api tokenrepository behavior

6ffc060

Signed-off-by: Derek Ho <[email protected]>

cwperks reviewed Jan 21, 2025

View reviewed changes

src/main/java/org/opensearch/security/authtoken/jwt/claims/JwtClaimsBuilder.java Outdated Show resolved Hide resolved

cwperks reviewed Jan 21, 2025

View reviewed changes

src/main/java/org/opensearch/security/privileges/PrivilegesEvaluationContext.java Outdated Show resolved Hide resolved

cwperks reviewed Jan 21, 2025

View reviewed changes

src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java Outdated Show resolved Hide resolved

cwperks reviewed Jan 21, 2025

View reviewed changes

src/main/java/org/opensearch/security/securityconf/DynamicConfigModelV7.java Outdated Show resolved Hide resolved

cwperks reviewed Jan 21, 2025

View reviewed changes

src/main/java/org/opensearch/security/securityconf/DynamicConfigModelV7.java Outdated Show resolved Hide resolved

cwperks reviewed Jan 21, 2025

View reviewed changes

src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java Show resolved Hide resolved

derek-ho added 2 commits January 22, 2025 16:56

Refactor to abstract class

763a9ff

Signed-off-by: Derek Ho <[email protected]>

Fix test and cleanup

a685951

Signed-off-by: Derek Ho <[email protected]>

cwperks reviewed Jan 23, 2025

View reviewed changes

src/main/java/org/opensearch/security/privileges/ActionPrivileges.java Show resolved Hide resolved

Separate the calls out based on type

77ad6c3

Signed-off-by: Derek Ho <[email protected]>

cwperks reviewed Jan 23, 2025

View reviewed changes

nibix reviewed Jan 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Api token authc/z implementation with Cache #4992

Api token authc/z implementation with Cache #4992

derek-ho commented Dec 24, 2024 •

edited

Loading

codecov bot commented Dec 26, 2024 •

edited

Loading

derek-ho Dec 31, 2024

cwperks Jan 23, 2025

nibix Jan 24, 2025

nibix Jan 24, 2025

nibix Jan 24, 2025

nibix Jan 24, 2025

nibix Jan 24, 2025

nibix Jan 24, 2025

nibix Jan 24, 2025

nibix Jan 24, 2025

nibix Jan 24, 2025

nibix Jan 24, 2025

nibix Jan 24, 2025

nibix Jan 24, 2025

	/**
	* Checks whether this instance provides privileges for the combination of the provided action,
	* the provided indices and the provided roles.
	* <p>
	* Returns a PrivilegesEvaluatorResponse with allowed=true if privileges are available.
	* <p>
	* If privileges are only available for a sub-set of indices, isPartiallyOk() will return true
	* and the indices for which privileges are available are returned by getAvailableIndices(). This allows the
	* do_not_fail_on_forbidden behaviour.
	*/
	public PrivilegesEvaluatorResponse hasIndexPrivilege(
	PrivilegesEvaluationContext context,
	Set<String> actions,
	IndexResolverReplacer.Resolved resolvedIndices
	) {

Api token authc/z implementation with Cache #4992

Are you sure you want to change the base?

Api token authc/z implementation with Cache #4992

Conversation

derek-ho commented Dec 24, 2024 • edited Loading

Description

Issues Resolved

Testing

Check List

codecov bot commented Dec 26, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derek-ho commented Dec 24, 2024 •

edited

Loading

codecov bot commented Dec 26, 2024 •

edited

Loading