PR comments

Signed-off-by: Derek Ho <[email protected]>
opensearch-project · Feb 4, 2025 · 7a20de3 · 7a20de3
1 parent 77ad6c3
commit 7a20de3
Show file tree

Hide file tree

Showing 6 changed files with 43 additions and 34 deletions.
diff --git a/src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java b/src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java
@@ -11,6 +11,8 @@
 
 package org.opensearch.security.authtoken.jwt;
 
+import java.security.AccessController;
+import java.security.PrivilegedAction;
 import java.text.ParseException;
 import java.util.Base64;
 import java.util.Date;
@@ -83,11 +85,14 @@ static Tuple<JWK, JWSSigner> createJwkFromSettings(final Settings settings) {
         }
     }
 
+    @SuppressWarnings("removal")
     public ExpiringBearerAuthToken createJwt(JwtClaimsBuilder claimsBuilder, String subject, Date expiryTime, Long expirySeconds)
         throws JOSEException, ParseException {
 
         final JWSHeader header = new JWSHeader.Builder(JWSAlgorithm.parse(signingKey.getAlgorithm().getName())).build();
-        final SignedJWT signedJwt = new SignedJWT(header, claimsBuilder.build());
+        final SignedJWT signedJwt = AccessController.doPrivileged(
+            (PrivilegedAction<SignedJWT>) () -> new SignedJWT(header, claimsBuilder.build())
+        );
 
         // Sign the JWT so it can be serialized
         signedJwt.sign(signer);

diff --git a/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java b/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();
+            if (privilegesEvaluationContext == null) {
                 return;
             }
 
             DlsFlsProcessedConfig config = this.dlsFlsProcessedConfig.get();
 
-            DlsRestriction dlsRestriction = config.getDocumentPrivileges().getRestriction(PrivilegesEvaluationContext, index);
+            DlsRestriction dlsRestriction = config.getDocumentPrivileges().getRestriction(privilegesEvaluationContext, index);
 
             if (log.isTraceEnabled()) {
                 log.trace("handleSearchContext(); index: {}; dlsRestriction: {}", index, dlsRestriction);
@@ -449,36 +449,36 @@ public DlsFlsProcessedConfig getCurrentConfig() {
 
     @Override
     public boolean hasFlsOrFieldMasking(String index) throws PrivilegesEvaluationException {
-        PrivilegesEvaluationContext PrivilegesEvaluationContext = this.dlsFlsBaseContext.getPrivilegesEvaluationContext();
-        if (PrivilegesEvaluationContext == null) {
+        PrivilegesEvaluationContext privilegesEvaluationContext = this.dlsFlsBaseContext.getPrivilegesEvaluationContext();
+        if (privilegesEvaluationContext == null) {
             return false;
         }
 
         DlsFlsProcessedConfig config = this.dlsFlsProcessedConfig.get();
-        return !config.getFieldPrivileges().isUnrestricted(PrivilegesEvaluationContext, index)
-            || !config.getFieldMasking().isUnrestricted(PrivilegesEvaluationContext, index);
+        return !config.getFieldPrivileges().isUnrestricted(privilegesEvaluationContext, index)
+            || !config.getFieldMasking().isUnrestricted(privilegesEvaluationContext, index);
     }
 
     @Override
     public boolean hasFieldMasking(String index) throws PrivilegesEvaluationException {
-        PrivilegesEvaluationContext PrivilegesEvaluationContext = this.dlsFlsBaseContext.getPrivilegesEvaluationContext();
-        if (PrivilegesEvaluationContext == null) {
+        PrivilegesEvaluationContext privilegesEvaluationContext = this.dlsFlsBaseContext.getPrivilegesEvaluationContext();
+        if (privilegesEvaluationContext == null) {
             return false;
         }
 
         DlsFlsProcessedConfig config = this.dlsFlsProcessedConfig.get();
-        return !config.getFieldMasking().isUnrestricted(PrivilegesEvaluationContext, index);
+        return !config.getFieldMasking().isUnrestricted(privilegesEvaluationContext, index);
     }
 
     @Override
     public boolean isFieldAllowed(String index, String field) throws PrivilegesEvaluationException {
-        PrivilegesEvaluationContext PrivilegesEvaluationContext = this.dlsFlsBaseContext.getPrivilegesEvaluationContext();
-        if (PrivilegesEvaluationContext == null) {
+        PrivilegesEvaluationContext privilegesEvaluationContext = this.dlsFlsBaseContext.getPrivilegesEvaluationContext();
+        if (privilegesEvaluationContext == null) {
             return true;
         }
 
         DlsFlsProcessedConfig config = this.dlsFlsProcessedConfig.get();
-        return config.getFieldPrivileges().getRestriction(PrivilegesEvaluationContext, index).isAllowed(field);
+        return config.getFieldPrivileges().getRestriction(privilegesEvaluationContext, index).isAllowed(field);
     }
 
     private static InternalAggregation aggregateBuckets(InternalAggregation aggregation) {

diff --git a/src/main/java/org/opensearch/security/configuration/SecurityFlsDlsIndexSearcherWrapper.java b/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();
 
         if (log.isTraceEnabled()) {
-            log.trace("dlsFlsWrap(); index: {}; privilegeEvaluationContext: {}", index.getName(), PrivilegesEvaluationContext);
+            log.trace("dlsFlsWrap(); index: {}; privilegeEvaluationContext: {}", index.getName(), privilegesEvaluationContext);
         }
 
-        if (isAdmin || PrivilegesEvaluationContext == null) {
+        if (isAdmin || privilegesEvaluationContext == null) {
             return new DlsFlsFilterLeafReader.DlsFlsDirectoryReader(
                 reader,
                 FieldPrivileges.FlsRule.ALLOW_ALL,
@@ -137,13 +137,13 @@ protected DirectoryReader dlsFlsWrap(final DirectoryReader reader, boolean isAdm
             DlsRestriction dlsRestriction;
 
             if (!this.dlsFlsBaseContext.isDlsDoneOnFilterLevel()) {
-                dlsRestriction = config.getDocumentPrivileges().getRestriction(PrivilegesEvaluationContext, index.getName());
+                dlsRestriction = config.getDocumentPrivileges().getRestriction(privilegesEvaluationContext, index.getName());
             } else {
                 dlsRestriction = DlsRestriction.NONE;
             }
 
-            FieldPrivileges.FlsRule flsRule = config.getFieldPrivileges().getRestriction(PrivilegesEvaluationContext, index.getName());
-            FieldMasking.FieldMaskingRule fmRule = config.getFieldMasking().getRestriction(PrivilegesEvaluationContext, index.getName());
+            FieldPrivileges.FlsRule flsRule = config.getFieldPrivileges().getRestriction(privilegesEvaluationContext, index.getName());
+            FieldMasking.FieldMaskingRule fmRule = config.getFieldMasking().getRestriction(privilegesEvaluationContext, index.getName());
 
             Query dlsQuery;
 

diff --git a/src/main/java/org/opensearch/security/privileges/ActionPrivileges.java b/src/main/java/org/opensearch/security/privileges/ActionPrivileges.java
@@ -251,10 +251,12 @@ public PrivilegesEvaluatorResponse hasIndexPrivilege(
                 indexMetadata
             );
         } else if (context instanceof PermissionBasedPrivilegesEvaluationContext) {
+            Map<String, IndexAbstraction> indexMetadata = this.indexMetadataSupplier.get();
             return this.index.apiTokenProvidesIndexPrivilege(
                 (PermissionBasedPrivilegesEvaluationContext) context,
                 resolvedIndices,
                 actions,
+                indexMetadata,
                 false
             );
         } else {
@@ -284,6 +286,7 @@ public PrivilegesEvaluatorResponse hasExplicitIndexPrivilege(
                 (PermissionBasedPrivilegesEvaluationContext) context,
                 resolvedIndices,
                 actions,
+                this.indexMetadataSupplier.get(),
                 true
             );
         } else {
@@ -1031,6 +1034,7 @@ PrivilegesEvaluatorResponse apiTokenProvidesIndexPrivilege(
             PermissionBasedPrivilegesEvaluationContext context,
             IndexResolverReplacer.Resolved resolvedIndices,
             Set<String> actions,
+            Map<String, IndexAbstraction> indexMetadata,
             Boolean explicit
         ) {
             Permissions permissions = context.getPermissions();
@@ -1042,14 +1046,14 @@ PrivilegesEvaluatorResponse apiTokenProvidesIndexPrivilege(
                 // Check each index permission
                 for (ApiToken.IndexPermission indexPermission : indexPermissions) {
                     // First check if this permission applies to this index
+                    IndexPattern indexPattern = IndexPattern.from(indexPermission.getIndexPatterns());
                     boolean indexMatched = false;
-                    for (String pattern : indexPermission.getIndexPatterns()) {
-                        if (WildcardMatcher.from(pattern).test(concreteIndex)) {
-                            indexMatched = true;
-                            break;
-                        }
+                    try {
+                        indexMatched = indexPattern.matches(concreteIndex, context, indexMetadata);
+                    } catch (PrivilegesEvaluationException e) {
+                        // We can ignore these errors, as this max leads to fewer privileges than available
+                        log.error("Error while evaluating index pattern. Ignoring entry");
                     }
-
                     if (!indexMatched) {
                         continue;
                     }

diff --git a/src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java b/src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java
@@ -128,7 +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;
+    private final ApiTokenRepository apiTokenRepository;
 
     SecurityDynamicConfiguration<?> config;
 
@@ -140,15 +140,15 @@ public DynamicConfigFactory(
         ThreadPool threadPool,
         ClusterInfoHolder cih,
         PasswordHasher passwordHasher,
-        ApiTokenRepository ar
+        ApiTokenRepository apiTokenRepository
     ) {
         super();
         this.cr = cr;
         this.opensearchSettings = opensearchSettings;
         this.configPath = configPath;
         this.cih = cih;
         this.iab = new InternalAuthenticationBackend(passwordHasher);
-        this.ar = ar;
+        this.apiTokenRepository = apiTokenRepository;
 
         if (opensearchSettings.getAsBoolean(ConfigConstants.SECURITY_UNSUPPORTED_LOAD_STATIC_RESOURCES, true)) {
             try {
@@ -273,7 +273,7 @@ public void onChange(ConfigurationMap typeToConfig) {
         );
 
         // rebuild v7 Models
-        dcm = new DynamicConfigModelV7(getConfigV7(config), opensearchSettings, configPath, iab, this.cih, ar);
+        dcm = new DynamicConfigModelV7(getConfigV7(config), opensearchSettings, configPath, iab, this.cih, apiTokenRepository);
         ium = new InternalUsersModelV7(internalusers, roles, rolesmapping);
         cm = new ConfigModelV7(roles, rolesmapping, actionGroups, tenants, dcm, opensearchSettings);
 

diff --git a/src/main/java/org/opensearch/security/securityconf/DynamicConfigModelV7.java b/src/main/java/org/opensearch/security/securityconf/DynamicConfigModelV7.java
@@ -87,23 +87,23 @@ public class DynamicConfigModelV7 extends DynamicConfigModel {
     private List<ClientBlockRegistry<InetAddress>> ipClientBlockRegistries;
     private Multimap<String, ClientBlockRegistry<String>> authBackendClientBlockRegistries;
     private final ClusterInfoHolder cih;
-    private final ApiTokenRepository ar;
+    private final ApiTokenRepository apiTokenRepository;
 
     public DynamicConfigModelV7(
         ConfigV7 config,
         Settings opensearchSettings,
         Path configPath,
         InternalAuthenticationBackend iab,
         ClusterInfoHolder cih,
-        ApiTokenRepository ar
+        ApiTokenRepository apiTokenRepository
     ) {
         super();
         this.config = config;
         this.opensearchSettings = opensearchSettings;
         this.configPath = configPath;
         this.iab = iab;
         this.cih = cih;
-        this.ar = ar;
+        this.apiTokenRepository = apiTokenRepository;
         buildAAA();
     }
 
@@ -392,7 +392,7 @@ private void buildAAA() {
         if (!isKeyNull(apiTokenSettings, "signing_key")) {
             final AuthDomain _ad = new AuthDomain(
                 new NoOpAuthenticationBackend(Settings.EMPTY, null),
-                new ApiTokenAuthenticator(getDynamicApiTokenSettings(), this.cih.getClusterName(), ar),
+                new ApiTokenAuthenticator(getDynamicApiTokenSettings(), this.cih.getClusterName(), apiTokenRepository),
                 false,
                 -2
             );