From 7a20de31c8b1a2db6f6dda51a1302afaebc8dc72 Mon Sep 17 00:00:00 2001 From: Derek Ho Date: Tue, 4 Feb 2025 14:56:16 -0500 Subject: [PATCH] PR comments Signed-off-by: Derek Ho --- .../security/authtoken/jwt/JwtVendor.java | 7 ++++- .../configuration/DlsFlsValveImpl.java | 26 +++++++++---------- .../SecurityFlsDlsIndexSearcherWrapper.java | 12 ++++----- .../security/privileges/ActionPrivileges.java | 16 +++++++----- .../securityconf/DynamicConfigFactory.java | 8 +++--- .../securityconf/DynamicConfigModelV7.java | 8 +++--- 6 files changed, 43 insertions(+), 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 index a522056812..8fd3589ebe 100644 --- 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 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) () -> 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 index 880a828f26..498b908e5d 100644 --- 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 index 8a258a1307..4f7a412097 100644 --- 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 index 3daa38ad98..dcb6cded2d 100644 --- 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 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 actions, + Map 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 index 307b40d328..1f86f459c8 100644 --- 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 SecurityDynamicConfiguration 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,7 +140,7 @@ public DynamicConfigFactory( ThreadPool threadPool, ClusterInfoHolder cih, PasswordHasher passwordHasher, - ApiTokenRepository ar + ApiTokenRepository apiTokenRepository ) { super(); this.cr = cr; @@ -148,7 +148,7 @@ public DynamicConfigFactory( 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 index 44cd041d72..55271e960b 100644 --- a/src/main/java/org/opensearch/security/securityconf/DynamicConfigModelV7.java +++ b/src/main/java/org/opensearch/security/securityconf/DynamicConfigModelV7.java @@ -87,7 +87,7 @@ public class DynamicConfigModelV7 extends DynamicConfigModel { private List> ipClientBlockRegistries; private Multimap> authBackendClientBlockRegistries; private final ClusterInfoHolder cih; - private final ApiTokenRepository ar; + private final ApiTokenRepository apiTokenRepository; public DynamicConfigModelV7( ConfigV7 config, @@ -95,7 +95,7 @@ public DynamicConfigModelV7( Path configPath, InternalAuthenticationBackend iab, ClusterInfoHolder cih, - ApiTokenRepository ar + ApiTokenRepository apiTokenRepository ) { super(); this.config = config; @@ -103,7 +103,7 @@ public DynamicConfigModelV7( 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 );