From 642ac3616e964bff5bb5f305b13501046c13ffc9 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Mon, 22 Jan 2024 13:42:20 -0500 Subject: [PATCH 01/20] Redact JWT URL Parameter from the audit log Signed-off-by: Craig Perkins --- .../auth/http/jwt/HTTPJwtAuthenticator.java | 4 ++ .../security/auditlog/config/AuditConfig.java | 68 +++++++++++++++++++ .../security/auditlog/impl/AuditMessage.java | 37 +++++++++- .../security/support/ConfigConstants.java | 2 + .../config/AuditConfigSerializeTest.java | 2 + 5 files changed, 111 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java b/src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java index 9bf22bf7f3..f4b9770089 100644 --- a/src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java +++ b/src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java @@ -194,6 +194,10 @@ public Optional reRequestAuthentication(final SecurityRequest ); } + public String getJwtUrlParameter() { + return jwtUrlParameter; + } + @Override public String getType() { return "jwt"; diff --git a/src/main/java/org/opensearch/security/auditlog/config/AuditConfig.java b/src/main/java/org/opensearch/security/auditlog/config/AuditConfig.java index 7b173099b5..40d69fc95a 100644 --- a/src/main/java/org/opensearch/security/auditlog/config/AuditConfig.java +++ b/src/main/java/org/opensearch/security/auditlog/config/AuditConfig.java @@ -12,9 +12,11 @@ package org.opensearch.security.auditlog.config; import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.SortedSet; import java.util.stream.Collectors; import com.google.common.annotations.VisibleForTesting; @@ -31,11 +33,16 @@ import org.opensearch.common.settings.Settings; import org.opensearch.security.DefaultObjectMapper; import org.opensearch.security.auditlog.impl.AuditCategory; +import org.opensearch.security.auth.AuthDomain; import org.opensearch.security.compliance.ComplianceConfig; import org.opensearch.security.dlic.rest.support.Utils; +import org.opensearch.security.securityconf.DynamicConfigModel; import org.opensearch.security.support.ConfigConstants; import org.opensearch.security.support.WildcardMatcher; +import com.amazon.dlic.auth.http.jwt.HTTPJwtAuthenticator; +import org.greenrobot.eventbus.Subscribe; + import static org.opensearch.security.DefaultObjectMapper.getOrDefault; import static org.opensearch.security.support.ConfigConstants.SECURITY_AUDIT_CONFIG_DEFAULT; @@ -86,6 +93,8 @@ public class AuditConfig { private static Set FIELDS = DefaultObjectMapper.getFields(AuditConfig.class); + private Set sensitiveUrlParams = new HashSet<>(); + private AuditConfig() { this(true, null, null); } @@ -136,6 +145,7 @@ public static class Filter { private final boolean logRequestBody; private final boolean resolveIndices; private final boolean excludeSensitiveHeaders; + private final boolean excludeSensitiveUrlParams; @JsonProperty("ignore_users") private final Set ignoredAuditUsers; @JsonProperty("ignore_requests") @@ -145,6 +155,7 @@ public static class Filter { private final WildcardMatcher ignoredAuditUsersMatcher; private final WildcardMatcher ignoredAuditRequestsMatcher; private final WildcardMatcher ignoredCustomHeadersMatcher; + private final WildcardMatcher ignoredUrlParamsMatcher; private final Set disabledRestCategories; private final Set disabledTransportCategories; @@ -156,9 +167,11 @@ public static class Filter { final boolean logRequestBody, final boolean resolveIndices, final boolean excludeSensitiveHeaders, + final boolean excludeSensitiveUrlParams, final Set ignoredAuditUsers, final Set ignoredAuditRequests, final Set ignoredCustomHeaders, + final Set ignoredUrlParams, final Set disabledRestCategories, final Set disabledTransportCategories ) { @@ -168,12 +181,14 @@ public static class Filter { this.logRequestBody = logRequestBody; this.resolveIndices = resolveIndices; this.excludeSensitiveHeaders = excludeSensitiveHeaders; + this.excludeSensitiveUrlParams = excludeSensitiveUrlParams; this.ignoredAuditUsers = ignoredAuditUsers; this.ignoredAuditUsersMatcher = WildcardMatcher.from(ignoredAuditUsers); this.ignoredAuditRequests = ignoredAuditRequests; this.ignoredAuditRequestsMatcher = WildcardMatcher.from(ignoredAuditRequests); this.ignoredCustomHeaders = ignoredCustomHeaders; this.ignoredCustomHeadersMatcher = WildcardMatcher.from(ignoredCustomHeaders); + this.ignoredUrlParamsMatcher = WildcardMatcher.from(ignoredUrlParams); this.disabledRestCategories = disabledRestCategories; this.disabledTransportCategories = disabledTransportCategories; } @@ -185,6 +200,10 @@ public enum FilterEntries { LOG_REQUEST_BODY("log_request_body", ConfigConstants.OPENDISTRO_SECURITY_AUDIT_LOG_REQUEST_BODY), RESOLVE_INDICES("resolve_indices", ConfigConstants.OPENDISTRO_SECURITY_AUDIT_RESOLVE_INDICES), EXCLUDE_SENSITIVE_HEADERS("exclude_sensitive_headers", ConfigConstants.OPENDISTRO_SECURITY_AUDIT_EXCLUDE_SENSITIVE_HEADERS), + EXCLUDE_SENSITIVE_URL_PARAMETERS( + "exclude_sensitive_url_params", + ConfigConstants.OPENDISTRO_SECURITY_AUDIT_EXCLUDE_SENSITIVE_URL_PARAMETERS + ), DISABLE_REST_CATEGORIES("disabled_rest_categories", ConfigConstants.OPENDISTRO_SECURITY_AUDIT_CONFIG_DISABLED_REST_CATEGORIES), DISABLE_TRANSPORT_CATEGORIES( "disabled_transport_categories", @@ -235,6 +254,11 @@ public static Filter from(Map properties) throws JsonProcessingE final boolean logRequestBody = getOrDefault(properties, FilterEntries.LOG_REQUEST_BODY.getKey(), true); final boolean resolveIndices = getOrDefault(properties, FilterEntries.RESOLVE_INDICES.getKey(), true); final boolean excludeSensitiveHeaders = getOrDefault(properties, FilterEntries.EXCLUDE_SENSITIVE_HEADERS.getKey(), true); + final boolean excludeSensitiveUrlParams = getOrDefault( + properties, + FilterEntries.EXCLUDE_SENSITIVE_URL_PARAMETERS.getKey(), + true + ); final Set disabledRestCategories = AuditCategory.parse( getOrDefault( properties, @@ -266,6 +290,7 @@ public static Filter from(Map properties) throws JsonProcessingE logRequestBody, resolveIndices, excludeSensitiveHeaders, + excludeSensitiveUrlParams, ignoredAuditUsers, ignoreAuditRequests, ignoreHeaders, @@ -287,6 +312,7 @@ public static Filter from(Settings settings) { final boolean logRequestBody = fromSettingBoolean(settings, FilterEntries.LOG_REQUEST_BODY, true); final boolean resolveIndices = fromSettingBoolean(settings, FilterEntries.RESOLVE_INDICES, true); final boolean excludeSensitiveHeaders = fromSettingBoolean(settings, FilterEntries.EXCLUDE_SENSITIVE_HEADERS, true); + final boolean excludeSensitiveUrlParams = fromSettingBoolean(settings, FilterEntries.EXCLUDE_SENSITIVE_URL_PARAMETERS, true); final Set disabledRestCategories = AuditCategory.parse( fromSettingStringSet( settings, @@ -311,6 +337,7 @@ public static Filter from(Settings settings) { logRequestBody, resolveIndices, excludeSensitiveHeaders, + excludeSensitiveUrlParams, ignoredAuditUsers, ignoreAuditRequests, ignoreHeaders, @@ -398,6 +425,15 @@ public boolean shouldExcludeSensitiveHeaders() { return excludeSensitiveHeaders; } + /** + * Checks if sensitive url params eg: jwtUrlParameter from jwt auth domain must be excluded in log messages + * @return true/false + */ + @JsonProperty("exclude_sensitive_url_params") + public boolean shouldExcludeSensitiveUrlParams() { + return excludeSensitiveUrlParams; + } + @VisibleForTesting WildcardMatcher getIgnoredAuditUsersMatcher() { return ignoredAuditUsersMatcher; @@ -422,6 +458,21 @@ WildcardMatcher getIgnoredCustomHeadersMatcher() { return ignoredCustomHeadersMatcher; } + @VisibleForTesting + WildcardMatcher getIgnoredUrlParamsMatcher() { + return ignoredUrlParamsMatcher; + } + + /** + * Check if the specified url param is excluded from the audit + * + * @param param + * @return true if header should be excluded + */ + public boolean shouldExcludeUrlParam(String param) { + return ignoredUrlParamsMatcher.test(param); + } + /** * Check if the specified header is excluded from the audit * @@ -470,6 +521,7 @@ public void log(Logger logger) { logger.info("Sensitive headers auditing is {}.", excludeSensitiveHeaders ? "enabled" : "disabled"); logger.info("Auditing requests from {} users is disabled.", ignoredAuditUsersMatcher); logger.info("Auditing request headers {} is disabled.", ignoredCustomHeadersMatcher); + logger.info("Auditing request url params {} is disabled.", ignoredUrlParamsMatcher); } @Override @@ -497,6 +549,8 @@ public String toString() { + ignoredAuditRequestsMatcher + ", ignoredCustomHeaders=" + ignoredCustomHeadersMatcher + + ", ignoredUrlParamsMatcher=" + + ignoredUrlParamsMatcher + '}'; } } @@ -537,4 +591,18 @@ public static Set getDeprecatedKeys(final Settings settings) { Utils.generateFieldResourcePaths(ComplianceConfig.FIELDS, "/compliance/") ) ); + + @Subscribe + public void onDynamicConfigModelChanged(DynamicConfigModel dcm) { + SortedSet authDomains = Collections.unmodifiableSortedSet(dcm.getRestAuthDomains()); + + for (AuthDomain authDomain : authDomains) { + if ("jwt".equals(authDomain.getHttpAuthenticator().getType())) { + HTTPJwtAuthenticator jwtAuthenticator = (HTTPJwtAuthenticator) authDomain.getHttpAuthenticator(); + if (jwtAuthenticator.getJwtUrlParameter() != null) { + sensitiveUrlParams.add(jwtAuthenticator.getJwtUrlParameter()); + } + } + } + } } diff --git a/src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java b/src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java index b57becc359..da8583ee95 100644 --- a/src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java +++ b/src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java @@ -12,6 +12,7 @@ package org.opensearch.security.auditlog.impl; import java.io.IOException; +import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.LinkOption; import java.nio.file.Path; @@ -23,10 +24,13 @@ import java.util.Map.Entry; import java.util.Objects; import java.util.regex.Pattern; +import java.util.stream.Collectors; import com.google.common.annotations.VisibleForTesting; import org.apache.commons.codec.digest.DigestUtils; +import org.apache.hc.core5.http.NameValuePair; import org.apache.hc.core5.net.URIBuilder; +import org.apache.hc.core5.net.URLEncodedUtils; import org.opensearch.ExceptionsHelper; import org.opensearch.cluster.service.ClusterService; @@ -183,8 +187,37 @@ public void addEffectiveUser(String user) { } } - public void addPath(String path) { + public static String redactUrlParams(String path, AuditConfig.Filter filter) { + String[] parts = path.split("\\?", 2); + String urlPath = parts[0]; + + if (parts.length > 1) { + String queryString = parts[1]; + + // Parse the query string + List params = URLEncodedUtils.parse(queryString, StandardCharsets.UTF_8); + + // Redact the specified parameter + String redactedQuery = params.stream() + .map( + param -> filter.shouldExcludeUrlParam(param.getName()) + ? param.getName() + "=REDACTED" + : param.getName() + "=" + param.getValue() + ) + .collect(Collectors.joining("&")); + + return urlPath + "?" + redactedQuery; + } + + return path; + } + + public void addPath(String path, boolean excludeSensitiveUrlParams, AuditConfig.Filter filter) { + // TODO redact jwtUrlParameter if (path != null) { + if (excludeSensitiveUrlParams) { + path = redactUrlParams(path, filter); + } auditInfo.put(REST_REQUEST_PATH, path); } } @@ -378,7 +411,7 @@ void addRestMethod(final RestRequest.Method method) { void addRestRequestInfo(final SecurityRequest request, final AuditConfig.Filter filter) { if (request != null) { final String path = request.path().toString(); - addPath(path); + addPath(path, filter.shouldExcludeSensitiveUrlParams(), filter); addRestHeaders(request.getHeaders(), filter.shouldExcludeSensitiveHeaders(), filter); addRestParams(request.params()); addRestMethod(request.method()); diff --git a/src/main/java/org/opensearch/security/support/ConfigConstants.java b/src/main/java/org/opensearch/security/support/ConfigConstants.java index 3060e1b2dc..61006c4cdd 100644 --- a/src/main/java/org/opensearch/security/support/ConfigConstants.java +++ b/src/main/java/org/opensearch/security/support/ConfigConstants.java @@ -170,6 +170,8 @@ public class ConfigConstants { public static final boolean OPENDISTRO_SECURITY_AUDIT_SSL_VERIFY_HOSTNAMES_DEFAULT = true; public static final boolean OPENDISTRO_SECURITY_AUDIT_SSL_ENABLE_SSL_CLIENT_AUTH_DEFAULT = false; public static final String OPENDISTRO_SECURITY_AUDIT_EXCLUDE_SENSITIVE_HEADERS = "opendistro_security.audit.exclude_sensitive_headers"; + public static final String OPENDISTRO_SECURITY_AUDIT_EXCLUDE_SENSITIVE_URL_PARAMETERS = + "opendistro_security.audit.exclude_sensitive_url_params"; public static final String SECURITY_AUDIT_CONFIG_DEFAULT_PREFIX = "plugins.security.audit.config."; diff --git a/src/test/java/org/opensearch/security/auditlog/config/AuditConfigSerializeTest.java b/src/test/java/org/opensearch/security/auditlog/config/AuditConfigSerializeTest.java index 04cea3dc05..6e35c42425 100644 --- a/src/test/java/org/opensearch/security/auditlog/config/AuditConfigSerializeTest.java +++ b/src/test/java/org/opensearch/security/auditlog/config/AuditConfigSerializeTest.java @@ -197,9 +197,11 @@ public void testSerialize() throws IOException { true, true, true, + true, ImmutableSet.of("ignore-user-1", "ignore-user-2"), ImmutableSet.of("ignore-request-1"), ImmutableSet.of("test-header"), + ImmutableSet.of("test-param"), EnumSet.of(AuditCategory.FAILED_LOGIN, AuditCategory.GRANTED_PRIVILEGES), EnumSet.of(AUTHENTICATED) ); From adab35970641a915cac8c9847f2a461c5bfcf855 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Mon, 22 Jan 2024 14:03:14 -0500 Subject: [PATCH 02/20] Make ignoreUrlParams static Signed-off-by: Craig Perkins --- .../security/auditlog/config/AuditConfig.java | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/opensearch/security/auditlog/config/AuditConfig.java b/src/main/java/org/opensearch/security/auditlog/config/AuditConfig.java index 40d69fc95a..59771fa378 100644 --- a/src/main/java/org/opensearch/security/auditlog/config/AuditConfig.java +++ b/src/main/java/org/opensearch/security/auditlog/config/AuditConfig.java @@ -93,7 +93,7 @@ public class AuditConfig { private static Set FIELDS = DefaultObjectMapper.getFields(AuditConfig.class); - private Set sensitiveUrlParams = new HashSet<>(); + static Set ignoreUrlParams = new HashSet<>(); private AuditConfig() { this(true, null, null); @@ -152,6 +152,8 @@ public static class Filter { private final Set ignoredAuditRequests; @JsonProperty("ignore_headers") private final Set ignoredCustomHeaders; + @JsonProperty("ignore_url_params") + private final Set ignoredUrlParams; private final WildcardMatcher ignoredAuditUsersMatcher; private final WildcardMatcher ignoredAuditRequestsMatcher; private final WildcardMatcher ignoredCustomHeadersMatcher; @@ -188,6 +190,7 @@ public static class Filter { this.ignoredAuditRequestsMatcher = WildcardMatcher.from(ignoredAuditRequests); this.ignoredCustomHeaders = ignoredCustomHeaders; this.ignoredCustomHeadersMatcher = WildcardMatcher.from(ignoredCustomHeaders); + this.ignoredUrlParams = ignoredUrlParams; this.ignoredUrlParamsMatcher = WildcardMatcher.from(ignoredUrlParams); this.disabledRestCategories = disabledRestCategories; this.disabledTransportCategories = disabledTransportCategories; @@ -294,6 +297,7 @@ public static Filter from(Map properties) throws JsonProcessingE ignoredAuditUsers, ignoreAuditRequests, ignoreHeaders, + ignoreUrlParams, disabledRestCategories, disabledTransportCategories ); @@ -341,6 +345,7 @@ public static Filter from(Settings settings) { ignoredAuditUsers, ignoreAuditRequests, ignoreHeaders, + ignoreUrlParams, disabledRestCategories, disabledTransportCategories ); @@ -595,12 +600,12 @@ public static Set getDeprecatedKeys(final Settings settings) { @Subscribe public void onDynamicConfigModelChanged(DynamicConfigModel dcm) { SortedSet authDomains = Collections.unmodifiableSortedSet(dcm.getRestAuthDomains()); - + ignoreUrlParams.clear(); for (AuthDomain authDomain : authDomains) { if ("jwt".equals(authDomain.getHttpAuthenticator().getType())) { HTTPJwtAuthenticator jwtAuthenticator = (HTTPJwtAuthenticator) authDomain.getHttpAuthenticator(); if (jwtAuthenticator.getJwtUrlParameter() != null) { - sensitiveUrlParams.add(jwtAuthenticator.getJwtUrlParameter()); + ignoreUrlParams.add(jwtAuthenticator.getJwtUrlParameter()); } } } From e82d39f80b49fc934c1c508ba917430570bf3d6b Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Mon, 22 Jan 2024 16:55:06 -0500 Subject: [PATCH 03/20] WIP on redacting jwtUrlParameter Signed-off-by: Craig Perkins --- .../security/http/JwtAuthenticationTests.java | 30 ++++++++++++++++++- .../test/framework/AuditFilters.java | 8 +++++ .../test/framework/JwtConfigBuilder.java | 9 ++++++ .../framework/cluster/TestRestClient.java | 7 +++++ .../security/OpenSearchSecurityPlugin.java | 1 + .../security/rest/SecurityInfoAction.java | 3 ++ .../config/AuditConfigSerializeTest.java | 9 ++++++ 7 files changed, 66 insertions(+), 1 deletion(-) diff --git a/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationTests.java b/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationTests.java index 659d7c178e..5decb3ef5a 100644 --- a/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationTests.java +++ b/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationTests.java @@ -28,9 +28,13 @@ import org.opensearch.action.search.SearchResponse; import org.opensearch.client.Client; import org.opensearch.client.RestHighLevelClient; +import org.opensearch.test.framework.AuditCompliance; +import org.opensearch.test.framework.AuditConfiguration; +import org.opensearch.test.framework.AuditFilters; import org.opensearch.test.framework.JwtConfigBuilder; import org.opensearch.test.framework.TestSecurityConfig; import org.opensearch.test.framework.TestSecurityConfig.Role; +import org.opensearch.test.framework.audit.AuditLogsRule; import org.opensearch.test.framework.cluster.ClusterManager; import org.opensearch.test.framework.cluster.LocalCluster; import org.opensearch.test.framework.cluster.TestRestClient; @@ -104,11 +108,18 @@ public class JwtAuthenticationTests { JWT_AUTH_HEADER ); + @Rule + public AuditLogsRule auditLogsRule = new AuditLogsRule(); + public static final TestSecurityConfig.AuthcDomain JWT_AUTH_DOMAIN = new TestSecurityConfig.AuthcDomain( "jwt", BASIC_AUTH_DOMAIN_ORDER - 1 ).jwtHttpAuthenticator( - new JwtConfigBuilder().jwtHeader(JWT_AUTH_HEADER).signingKey(PUBLIC_KEY).subjectKey(CLAIM_USERNAME).rolesKey(CLAIM_ROLES) + new JwtConfigBuilder().jwtHeader(JWT_AUTH_HEADER) + .jwtUrlParameter("token") + .signingKey(PUBLIC_KEY) + .subjectKey(CLAIM_USERNAME) + .rolesKey(CLAIM_ROLES) ).backend("noop"); public static final String SONG_ID_1 = "song-id-01"; @@ -126,6 +137,10 @@ public class JwtAuthenticationTests { .users(ADMIN_USER) .roles(DEPARTMENT_SONG_LISTENER_ROLE) .authc(JWT_AUTH_DOMAIN) + .audit( + new AuditConfiguration(true).compliance(new AuditCompliance().enabled(true)) + .filters(new AuditFilters().enabledRest(true).enabledTransport(true).resolveBulkRequests(true)) + ) .build(); @Rule @@ -153,6 +168,19 @@ public void shouldAuthenticateWithJwtToken_positive() { } } + @Test + public void shouldAuthenticateWithJwtTokenInUrl_positive() { + Header jwtToken = tokenFactory.generateValidToken(USER_SUPERHERO); + String jwtTokenValue = jwtToken.getValue(); + try (TestRestClient client = cluster.getRestClient()) { + HttpResponse response = client.getAuthInfo(Map.of("token", jwtTokenValue)); + + response.assertStatusCode(200); + String username = response.getTextFromJsonBody(POINTER_USERNAME); + assertThat(username, equalTo(USER_SUPERHERO)); + } + } + @Test public void shouldAuthenticateWithJwtToken_positiveWithAnotherUsername() { try (TestRestClient client = cluster.getRestClient(tokenFactory.generateValidToken(USERNAME_ROOT))) { diff --git a/src/integrationTest/java/org/opensearch/test/framework/AuditFilters.java b/src/integrationTest/java/org/opensearch/test/framework/AuditFilters.java index 087342eb6f..5e63665f41 100644 --- a/src/integrationTest/java/org/opensearch/test/framework/AuditFilters.java +++ b/src/integrationTest/java/org/opensearch/test/framework/AuditFilters.java @@ -35,6 +35,7 @@ public class AuditFilters implements ToXContentObject { private List ignoreRequests; private List ignoreHeaders; + private List ignoreUrlParams; private List disabledRestCategories; @@ -52,6 +53,7 @@ public AuditFilters() { this.ignoreUsers = Collections.emptyList(); this.ignoreRequests = Collections.emptyList(); this.ignoreHeaders = Collections.emptyList(); + this.ignoreUrlParams = Collections.emptyList(); this.disabledRestCategories = Collections.emptyList(); this.disabledTransportCategories = Collections.emptyList(); } @@ -101,6 +103,11 @@ public AuditFilters ignoreHeaders(List ignoreHeaders) { return this; } + public AuditFilters ignoreUrlParams(List ignoreUrlParams) { + this.ignoreUrlParams = ignoreUrlParams; + return this; + } + public AuditFilters disabledRestCategories(List disabledRestCategories) { this.disabledRestCategories = disabledRestCategories; return this; @@ -123,6 +130,7 @@ public XContentBuilder toXContent(XContentBuilder xContentBuilder, Params params xContentBuilder.field("ignore_users", ignoreUsers); xContentBuilder.field("ignore_requests", ignoreRequests); xContentBuilder.field("ignore_headers", ignoreHeaders); + xContentBuilder.field("ignore_url_params", ignoreUrlParams); xContentBuilder.field("disabled_rest_categories", disabledRestCategories); xContentBuilder.field("disabled_transport_categories", disabledTransportCategories); xContentBuilder.endObject(); diff --git a/src/integrationTest/java/org/opensearch/test/framework/JwtConfigBuilder.java b/src/integrationTest/java/org/opensearch/test/framework/JwtConfigBuilder.java index 48dfa128e0..2ab838a41d 100644 --- a/src/integrationTest/java/org/opensearch/test/framework/JwtConfigBuilder.java +++ b/src/integrationTest/java/org/opensearch/test/framework/JwtConfigBuilder.java @@ -21,12 +21,18 @@ public class JwtConfigBuilder { private String signingKey; private String subjectKey; private String rolesKey; + private String jwtUrlParameter; public JwtConfigBuilder jwtHeader(String jwtHeader) { this.jwtHeader = jwtHeader; return this; } + public JwtConfigBuilder jwtUrlParameter(String jwtUrlParameter) { + this.jwtUrlParameter = jwtUrlParameter; + return this; + } + public JwtConfigBuilder signingKey(String signingKey) { this.signingKey = signingKey; return this; @@ -51,6 +57,9 @@ public Map build() { if (isNoneBlank(jwtHeader)) { builder.put("jwt_header", jwtHeader); } + if (isNoneBlank(jwtUrlParameter)) { + builder.put("jwt_url_parameter", jwtUrlParameter); + } if (isNoneBlank(subjectKey)) { builder.put("subject_key", subjectKey); } diff --git a/src/integrationTest/java/org/opensearch/test/framework/cluster/TestRestClient.java b/src/integrationTest/java/org/opensearch/test/framework/cluster/TestRestClient.java index c2e01bb338..b7da92b270 100644 --- a/src/integrationTest/java/org/opensearch/test/framework/cluster/TestRestClient.java +++ b/src/integrationTest/java/org/opensearch/test/framework/cluster/TestRestClient.java @@ -36,6 +36,7 @@ import java.util.Arrays; import java.util.Collections; import java.util.List; +import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.stream.Collectors; @@ -113,6 +114,12 @@ public HttpResponse getAuthInfo(Header... headers) { return executeRequest(new HttpGet(getHttpServerUri() + "/_opendistro/_security/authinfo?pretty"), headers); } + public HttpResponse getAuthInfo(Map urlParams, Header... headers) { + String urlParamsString = "?" + + urlParams.entrySet().stream().map(e -> e.getKey() + "=" + e.getValue()).collect(Collectors.joining("&")); + return executeRequest(new HttpGet(getHttpServerUri() + "/_opendistro/_security/authinfo" + urlParamsString), headers); + } + public void confirmCorrectCredentials(String expectedUserName) { HttpResponse response = getAuthInfo(); assertThat(response, notNullValue()); diff --git a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java index 569380582b..b9fc3f6eca 100644 --- a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java +++ b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java @@ -1419,6 +1419,7 @@ public List> getSettings() { case ENABLE_REST: case ENABLE_TRANSPORT: case EXCLUDE_SENSITIVE_HEADERS: + case EXCLUDE_SENSITIVE_URL_PARAMETERS: case LOG_REQUEST_BODY: case RESOLVE_INDICES: return boolSettingNodeScopeFiltered.apply(filterEntry.getKeyWithNamespace(), true); diff --git a/src/main/java/org/opensearch/security/rest/SecurityInfoAction.java b/src/main/java/org/opensearch/security/rest/SecurityInfoAction.java index 9300cf72f2..bb62987084 100644 --- a/src/main/java/org/opensearch/security/rest/SecurityInfoAction.java +++ b/src/main/java/org/opensearch/security/rest/SecurityInfoAction.java @@ -88,6 +88,9 @@ public List routes() { @Override protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { + // TODO Consume the jwtUrlParameter in the authenticator. This was broken after authentication moving up netty pipeline + System.out.println("prepareRequest"); + request.param("token"); return new RestChannelConsumer() { @Override diff --git a/src/test/java/org/opensearch/security/auditlog/config/AuditConfigSerializeTest.java b/src/test/java/org/opensearch/security/auditlog/config/AuditConfigSerializeTest.java index 6e35c42425..e5036cd219 100644 --- a/src/test/java/org/opensearch/security/auditlog/config/AuditConfigSerializeTest.java +++ b/src/test/java/org/opensearch/security/auditlog/config/AuditConfigSerializeTest.java @@ -70,9 +70,11 @@ public void testDefaultSerialize() throws IOException { .field("log_request_body", true) .field("resolve_indices", true) .field("exclude_sensitive_headers", true) + .field("exclude_sensitive_url_params", true) .field("ignore_users", Collections.singletonList("kibanaserver")) .field("ignore_requests", Collections.emptyList()) .field("ignore_headers", Collections.emptyList()) + .field("ignore_url_params", Collections.emptyList()) .endObject() .startObject("compliance") .field("enabled", true) @@ -133,9 +135,11 @@ public void testDeserialize() throws IOException { .field("log_request_body", true) .field("resolve_indices", true) .field("exclude_sensitive_headers", true) + .field("exclude_sensitive_url_params", true) .field("ignore_users", Collections.singletonList("test-user-1")) .field("ignore_requests", Collections.singletonList("test-request")) .field("ignore_headers", Collections.singletonList("test-headers")) + .field("ignore_url_params", Collections.singletonList("test-param")) .endObject() .startObject("compliance") .field("enabled", true) @@ -165,6 +169,7 @@ public void testDeserialize() throws IOException { assertTrue(audit.shouldLogRequestBody()); assertTrue(audit.shouldResolveIndices()); assertTrue(audit.shouldExcludeSensitiveHeaders()); + assertTrue(audit.shouldExcludeSensitiveUrlParams()); assertTrue(configCompliance.shouldLogExternalConfig()); assertTrue(configCompliance.shouldLogInternalConfig()); assertEquals(WildcardMatcher.from(Collections.singleton("test-user-1")), audit.getIgnoredAuditUsersMatcher()); @@ -232,9 +237,11 @@ public void testSerialize() throws IOException { .field("log_request_body", true) .field("resolve_indices", true) .field("exclude_sensitive_headers", true) + .field("exclude_sensitive_url_params", true) .field("ignore_users", ImmutableList.of("ignore-user-1", "ignore-user-2")) .field("ignore_requests", Collections.singletonList("ignore-request-1")) .field("ignore_headers", Collections.singletonList("test-header")) + .field("ignore_url_params", Collections.singletonList("test-param")) .endObject() .startObject("compliance") .field("enabled", true) @@ -275,9 +282,11 @@ public void testNullSerialize() throws IOException { .field("log_request_body", true) .field("resolve_indices", true) .field("exclude_sensitive_headers", true) + .field("exclude_sensitive_url_params", true) .field("ignore_users", ImmutableList.of("kibanaserver")) .field("ignore_requests", Collections.emptyList()) .field("ignore_headers", Collections.emptyList()) + .field("ignore_url_params", Collections.emptyList()) .endObject() .startObject("compliance") .field("enabled", true) From 7ce47d394f6bcabeea9c65968e4e0d2cf862a498 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Mon, 22 Jan 2024 17:13:16 -0500 Subject: [PATCH 04/20] Consume jwtUrlParameter Signed-off-by: Craig Perkins --- .../security/auth/BackendRegistry.java | 18 +++++++++++++++++- .../security/filter/NettyRequest.java | 5 +++++ .../security/filter/OpenSearchRequest.java | 5 +++++ .../security/filter/SecurityRequest.java | 3 +++ .../security/filter/SecurityRestFilter.java | 4 ++++ .../security/rest/SecurityInfoAction.java | 3 --- 6 files changed, 34 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/opensearch/security/auth/BackendRegistry.java b/src/main/java/org/opensearch/security/auth/BackendRegistry.java index 3ab9a2afc9..3864c0b96b 100644 --- a/src/main/java/org/opensearch/security/auth/BackendRegistry.java +++ b/src/main/java/org/opensearch/security/auth/BackendRegistry.java @@ -66,6 +66,7 @@ import org.opensearch.security.user.User; import org.opensearch.threadpool.ThreadPool; +import com.amazon.dlic.auth.http.jwt.HTTPJwtAuthenticator; import org.greenrobot.eventbus.Subscribe; import static org.apache.http.HttpStatus.SC_FORBIDDEN; @@ -77,7 +78,7 @@ public class BackendRegistry { protected final Logger log = LogManager.getLogger(this.getClass()); private SortedSet restAuthDomains; private Set restAuthorizers; - + private Set urlParamsToConsume = new HashSet<>(); private List ipAuthFailureListeners; private Multimap authBackendFailureListeners; private List> ipClientBlockRegistries; @@ -156,6 +157,10 @@ public BackendRegistry( createCaches(); } + public Set getUrlParamsToConsume() { + return urlParamsToConsume; + } + public boolean isInitialized() { return initialized; } @@ -181,6 +186,17 @@ public void onDynamicConfigModelChanged(DynamicConfigModel dcm) { ipClientBlockRegistries = dcm.getIpClientBlockRegistries(); authBackendClientBlockRegistries = dcm.getAuthBackendClientBlockRegistries(); + SortedSet authDomains = Collections.unmodifiableSortedSet(dcm.getRestAuthDomains()); + urlParamsToConsume.clear(); + for (AuthDomain authDomain : authDomains) { + if ("jwt".equals(authDomain.getHttpAuthenticator().getType())) { + HTTPJwtAuthenticator jwtAuthenticator = (HTTPJwtAuthenticator) authDomain.getHttpAuthenticator(); + if (jwtAuthenticator.getJwtUrlParameter() != null) { + urlParamsToConsume.add(jwtAuthenticator.getJwtUrlParameter()); + } + } + } + // OpenSearch Security no default authc initialized = !restAuthDomains.isEmpty() || anonymousAuthEnabled || injectedUserEnabled; } diff --git a/src/main/java/org/opensearch/security/filter/NettyRequest.java b/src/main/java/org/opensearch/security/filter/NettyRequest.java index 7b65e4e0de..50a28f84aa 100644 --- a/src/main/java/org/opensearch/security/filter/NettyRequest.java +++ b/src/main/java/org/opensearch/security/filter/NettyRequest.java @@ -85,6 +85,11 @@ public Map params() { return params(underlyingRequest.uri()); } + @Override + public String param(String key) { + return params().get(key); + } + private static Map params(String uri) { // Sourced from // https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java#L419-L422 diff --git a/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java b/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java index e86012f594..c64f7af881 100644 --- a/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java +++ b/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java @@ -72,6 +72,11 @@ public Map params() { return underlyingRequest.params(); } + @Override + public String param(String key) { + return underlyingRequest.param(key); + } + /** Gets access to the underlying request object */ public RestRequest breakEncapsulationForRequest() { return underlyingRequest; diff --git a/src/main/java/org/opensearch/security/filter/SecurityRequest.java b/src/main/java/org/opensearch/security/filter/SecurityRequest.java index 4c7ea27a87..259113269e 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityRequest.java +++ b/src/main/java/org/opensearch/security/filter/SecurityRequest.java @@ -49,4 +49,7 @@ default String header(final String headerName) { /** The parameters associated with this request */ Map params(); + + /** Get param by key */ + String param(String key); } diff --git a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java index d52c5109fc..f927a5b692 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java +++ b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java @@ -146,6 +146,10 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c final SecurityRequestChannel requestChannel = SecurityRequestFactory.from(request, channel); + for (String paramToConsume : registry.getUrlParamsToConsume()) { + requestChannel.param(paramToConsume); + } + // Authenticate request if (!NettyAttribute.popFrom(request, IS_AUTHENTICATED).orElse(false)) { // we aren't authenticated so we should skip this step diff --git a/src/main/java/org/opensearch/security/rest/SecurityInfoAction.java b/src/main/java/org/opensearch/security/rest/SecurityInfoAction.java index bb62987084..9300cf72f2 100644 --- a/src/main/java/org/opensearch/security/rest/SecurityInfoAction.java +++ b/src/main/java/org/opensearch/security/rest/SecurityInfoAction.java @@ -88,9 +88,6 @@ public List routes() { @Override protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { - // TODO Consume the jwtUrlParameter in the authenticator. This was broken after authentication moving up netty pipeline - System.out.println("prepareRequest"); - request.param("token"); return new RestChannelConsumer() { @Override From cb880364afd93f105e51a330bc52d277c596b0fc Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 23 Jan 2024 10:59:02 -0500 Subject: [PATCH 05/20] Fix unconsumed parameter exception when authentication with jwtUrlParameter Signed-off-by: Craig Perkins --- .../security/http/JwtAuthenticationTests.java | 19 ++++++++++++++++++- .../test/framework/JwtConfigBuilder.java | 9 +++++++++ .../framework/cluster/TestRestClient.java | 7 +++++++ .../auth/http/jwt/HTTPJwtAuthenticator.java | 4 ++-- .../security/filter/NettyRequest.java | 16 ++++++++++++++++ .../security/filter/OpenSearchRequest.java | 5 +++++ .../security/filter/SecurityRequest.java | 10 ++++++++++ .../filter/SecurityRequestChannel.java | 4 ++-- .../security/filter/SecurityRestFilter.java | 8 ++++++++ .../http/SecurityHttpServerTransport.java | 3 +++ .../Netty4HttpRequestHeaderVerifier.java | 3 +++ 11 files changed, 83 insertions(+), 5 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationTests.java b/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationTests.java index 659d7c178e..a046534ecb 100644 --- a/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationTests.java +++ b/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationTests.java @@ -108,7 +108,11 @@ public class JwtAuthenticationTests { "jwt", BASIC_AUTH_DOMAIN_ORDER - 1 ).jwtHttpAuthenticator( - new JwtConfigBuilder().jwtHeader(JWT_AUTH_HEADER).signingKey(PUBLIC_KEY).subjectKey(CLAIM_USERNAME).rolesKey(CLAIM_ROLES) + new JwtConfigBuilder().jwtHeader(JWT_AUTH_HEADER) + .jwtUrlParameter("token") + .signingKey(PUBLIC_KEY) + .subjectKey(CLAIM_USERNAME) + .rolesKey(CLAIM_ROLES) ).backend("noop"); public static final String SONG_ID_1 = "song-id-01"; @@ -153,6 +157,19 @@ public void shouldAuthenticateWithJwtToken_positive() { } } + @Test + public void shouldAuthenticateWithJwtTokenInUrl_positive() { + Header jwtToken = tokenFactory.generateValidToken(USER_SUPERHERO); + String jwtTokenValue = jwtToken.getValue(); + try (TestRestClient client = cluster.getRestClient()) { + HttpResponse response = client.getAuthInfo(Map.of("token", jwtTokenValue)); + + response.assertStatusCode(200); + String username = response.getTextFromJsonBody(POINTER_USERNAME); + assertThat(username, equalTo(USER_SUPERHERO)); + } + } + @Test public void shouldAuthenticateWithJwtToken_positiveWithAnotherUsername() { try (TestRestClient client = cluster.getRestClient(tokenFactory.generateValidToken(USERNAME_ROOT))) { diff --git a/src/integrationTest/java/org/opensearch/test/framework/JwtConfigBuilder.java b/src/integrationTest/java/org/opensearch/test/framework/JwtConfigBuilder.java index 48dfa128e0..88297bacd2 100644 --- a/src/integrationTest/java/org/opensearch/test/framework/JwtConfigBuilder.java +++ b/src/integrationTest/java/org/opensearch/test/framework/JwtConfigBuilder.java @@ -18,6 +18,7 @@ public class JwtConfigBuilder { private String jwtHeader; + private String jwtUrlParameter; private String signingKey; private String subjectKey; private String rolesKey; @@ -27,6 +28,11 @@ public JwtConfigBuilder jwtHeader(String jwtHeader) { return this; } + public JwtConfigBuilder jwtUrlParameter(String jwtUrlParameter) { + this.jwtUrlParameter = jwtUrlParameter; + return this; + } + public JwtConfigBuilder signingKey(String signingKey) { this.signingKey = signingKey; return this; @@ -51,6 +57,9 @@ public Map build() { if (isNoneBlank(jwtHeader)) { builder.put("jwt_header", jwtHeader); } + if (isNoneBlank(jwtUrlParameter)) { + builder.put("jwt_url_parameter", jwtUrlParameter); + } if (isNoneBlank(subjectKey)) { builder.put("subject_key", subjectKey); } diff --git a/src/integrationTest/java/org/opensearch/test/framework/cluster/TestRestClient.java b/src/integrationTest/java/org/opensearch/test/framework/cluster/TestRestClient.java index c2e01bb338..b7da92b270 100644 --- a/src/integrationTest/java/org/opensearch/test/framework/cluster/TestRestClient.java +++ b/src/integrationTest/java/org/opensearch/test/framework/cluster/TestRestClient.java @@ -36,6 +36,7 @@ import java.util.Arrays; import java.util.Collections; import java.util.List; +import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.stream.Collectors; @@ -113,6 +114,12 @@ public HttpResponse getAuthInfo(Header... headers) { return executeRequest(new HttpGet(getHttpServerUri() + "/_opendistro/_security/authinfo?pretty"), headers); } + public HttpResponse getAuthInfo(Map urlParams, Header... headers) { + String urlParamsString = "?" + + urlParams.entrySet().stream().map(e -> e.getKey() + "=" + e.getValue()).collect(Collectors.joining("&")); + return executeRequest(new HttpGet(getHttpServerUri() + "/_opendistro/_security/authinfo" + urlParamsString), headers); + } + public void confirmCorrectCredentials(String expectedUserName) { HttpResponse response = getAuthInfo(); assertThat(response, notNullValue()); diff --git a/src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java b/src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java index 9bf22bf7f3..d0d68c74bc 100644 --- a/src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java +++ b/src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java @@ -130,10 +130,10 @@ private AuthCredentials extractCredentials0(final SecurityRequest request) { } if ((jwtToken == null || jwtToken.isEmpty()) && jwtUrlParameter != null) { - jwtToken = request.params().get(jwtUrlParameter); + jwtToken = request.param(jwtUrlParameter); } else { // just consume to avoid "contains unrecognized parameter" - request.params().get(jwtUrlParameter); + request.param(jwtUrlParameter); } if (jwtToken == null || jwtToken.length() == 0) { diff --git a/src/main/java/org/opensearch/security/filter/NettyRequest.java b/src/main/java/org/opensearch/security/filter/NettyRequest.java index 7b65e4e0de..e0fb37142a 100644 --- a/src/main/java/org/opensearch/security/filter/NettyRequest.java +++ b/src/main/java/org/opensearch/security/filter/NettyRequest.java @@ -14,9 +14,11 @@ import java.net.InetSocketAddress; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.Set; import java.util.TreeMap; import javax.net.ssl.SSLEngine; @@ -35,6 +37,8 @@ public class NettyRequest implements SecurityRequest { protected final HttpRequest underlyingRequest; protected final Netty4HttpChannel underlyingChannel; + private final Set consumedParams = new HashSet<>(); + NettyRequest(final HttpRequest request, final Netty4HttpChannel channel) { this.underlyingRequest = request; this.underlyingChannel = channel; @@ -85,6 +89,18 @@ public Map params() { return params(underlyingRequest.uri()); } + @Override + public String param(String key) { + Map urlParams = params(); + consumedParams.add(key); + return urlParams != null ? params().get(key) : null; + } + + @Override + public Set getConsumedParams() { + return consumedParams; + } + private static Map params(String uri) { // Sourced from // https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java#L419-L422 diff --git a/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java b/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java index e86012f594..c64f7af881 100644 --- a/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java +++ b/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java @@ -72,6 +72,11 @@ public Map params() { return underlyingRequest.params(); } + @Override + public String param(String key) { + return underlyingRequest.param(key); + } + /** Gets access to the underlying request object */ public RestRequest breakEncapsulationForRequest() { return underlyingRequest; diff --git a/src/main/java/org/opensearch/security/filter/SecurityRequest.java b/src/main/java/org/opensearch/security/filter/SecurityRequest.java index 4c7ea27a87..ad46589e86 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityRequest.java +++ b/src/main/java/org/opensearch/security/filter/SecurityRequest.java @@ -12,9 +12,11 @@ package org.opensearch.security.filter; import java.net.InetSocketAddress; +import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.Set; import java.util.stream.Stream; import javax.net.ssl.SSLEngine; @@ -49,4 +51,12 @@ default String header(final String headerName) { /** The parameters associated with this request */ Map params(); + + /** Gets the parameter for the given key */ + String param(String key); + + /** Gets the parameters that have been consumed */ + default Set getConsumedParams() { + return Collections.emptySet(); + }; } diff --git a/src/main/java/org/opensearch/security/filter/SecurityRequestChannel.java b/src/main/java/org/opensearch/security/filter/SecurityRequestChannel.java index 66744d01dd..241c9009f4 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityRequestChannel.java +++ b/src/main/java/org/opensearch/security/filter/SecurityRequestChannel.java @@ -14,13 +14,13 @@ import java.util.Optional; /** - * When a request is recieved by the security plugin this governs getting information about the request and complete with with a response + * When a request is received by the security plugin this governs getting information about the request and complete with a response */ public interface SecurityRequestChannel extends SecurityRequest { /** Associate a response with this channel */ void queueForSending(final SecurityResponse response); - /** Acess the queued response */ + /** Access the queued response */ Optional getQueuedResponse(); } diff --git a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java index d52c5109fc..a15813eec7 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java +++ b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java @@ -69,6 +69,7 @@ import static org.opensearch.security.OpenSearchSecurityPlugin.LEGACY_OPENDISTRO_PREFIX; import static org.opensearch.security.OpenSearchSecurityPlugin.PLUGINS_PREFIX; +import static org.opensearch.security.http.SecurityHttpServerTransport.CONSUMED_PARAMS; import static org.opensearch.security.http.SecurityHttpServerTransport.CONTEXT_TO_RESTORE; import static org.opensearch.security.http.SecurityHttpServerTransport.EARLY_RESPONSE; import static org.opensearch.security.http.SecurityHttpServerTransport.IS_AUTHENTICATED; @@ -144,6 +145,13 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c } }); + NettyAttribute.popFrom(request, CONSUMED_PARAMS).ifPresent(consumedParams -> { + for (String param : consumedParams) { + // Consume the parameter on the RestRequest + request.param(param); + } + }); + final SecurityRequestChannel requestChannel = SecurityRequestFactory.from(request, channel); // Authenticate request diff --git a/src/main/java/org/opensearch/security/http/SecurityHttpServerTransport.java b/src/main/java/org/opensearch/security/http/SecurityHttpServerTransport.java index c5fbbfbbc6..0223632269 100644 --- a/src/main/java/org/opensearch/security/http/SecurityHttpServerTransport.java +++ b/src/main/java/org/opensearch/security/http/SecurityHttpServerTransport.java @@ -26,6 +26,8 @@ package org.opensearch.security.http; +import java.util.Set; + import org.opensearch.common.network.NetworkService; import org.opensearch.common.settings.ClusterSettings; import org.opensearch.common.settings.Settings; @@ -47,6 +49,7 @@ public class SecurityHttpServerTransport extends SecuritySSLNettyHttpServerTransport { public static final AttributeKey EARLY_RESPONSE = AttributeKey.newInstance("opensearch-http-early-response"); + public static final AttributeKey> CONSUMED_PARAMS = AttributeKey.newInstance("opensearch-http-request-consumed-params"); public static final AttributeKey CONTEXT_TO_RESTORE = AttributeKey.newInstance( "opensearch-http-request-thread-context" ); diff --git a/src/main/java/org/opensearch/security/ssl/http/netty/Netty4HttpRequestHeaderVerifier.java b/src/main/java/org/opensearch/security/ssl/http/netty/Netty4HttpRequestHeaderVerifier.java index 9adca0f377..978eb035e6 100644 --- a/src/main/java/org/opensearch/security/ssl/http/netty/Netty4HttpRequestHeaderVerifier.java +++ b/src/main/java/org/opensearch/security/ssl/http/netty/Netty4HttpRequestHeaderVerifier.java @@ -32,6 +32,7 @@ import io.netty.handler.codec.http.HttpRequest; import io.netty.util.ReferenceCountUtil; +import static org.opensearch.security.http.SecurityHttpServerTransport.CONSUMED_PARAMS; import static org.opensearch.security.http.SecurityHttpServerTransport.CONTEXT_TO_RESTORE; import static org.opensearch.security.http.SecurityHttpServerTransport.EARLY_RESPONSE; import static org.opensearch.security.http.SecurityHttpServerTransport.IS_AUTHENTICATED; @@ -84,6 +85,8 @@ public void channelRead0(ChannelHandlerContext ctx, DefaultHttpRequest msg) thro // If request channel is completed and a response is sent, then there was a failure during authentication restFilter.checkAndAuthenticateRequest(requestChannel); + ctx.channel().attr(CONSUMED_PARAMS).set(requestChannel.getConsumedParams()); + ThreadContext.StoredContext contextToRestore = threadPool.getThreadContext().newStoredContext(false); ctx.channel().attr(CONTEXT_TO_RESTORE).set(contextToRestore); From 61aa3c2cc1ece420a6bdfa9ee392ba4804027e1a Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 23 Jan 2024 11:29:15 -0500 Subject: [PATCH 06/20] Create separate test suite Signed-off-by: Craig Perkins --- .../security/http/JwtAuthenticationTests.java | 19 +--- .../JwtAuthenticationWithUrlParamTests.java | 97 +++++++++++++++++++ 2 files changed, 98 insertions(+), 18 deletions(-) create mode 100644 src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationWithUrlParamTests.java diff --git a/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationTests.java b/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationTests.java index a046534ecb..659d7c178e 100644 --- a/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationTests.java +++ b/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationTests.java @@ -108,11 +108,7 @@ public class JwtAuthenticationTests { "jwt", BASIC_AUTH_DOMAIN_ORDER - 1 ).jwtHttpAuthenticator( - new JwtConfigBuilder().jwtHeader(JWT_AUTH_HEADER) - .jwtUrlParameter("token") - .signingKey(PUBLIC_KEY) - .subjectKey(CLAIM_USERNAME) - .rolesKey(CLAIM_ROLES) + new JwtConfigBuilder().jwtHeader(JWT_AUTH_HEADER).signingKey(PUBLIC_KEY).subjectKey(CLAIM_USERNAME).rolesKey(CLAIM_ROLES) ).backend("noop"); public static final String SONG_ID_1 = "song-id-01"; @@ -157,19 +153,6 @@ public void shouldAuthenticateWithJwtToken_positive() { } } - @Test - public void shouldAuthenticateWithJwtTokenInUrl_positive() { - Header jwtToken = tokenFactory.generateValidToken(USER_SUPERHERO); - String jwtTokenValue = jwtToken.getValue(); - try (TestRestClient client = cluster.getRestClient()) { - HttpResponse response = client.getAuthInfo(Map.of("token", jwtTokenValue)); - - response.assertStatusCode(200); - String username = response.getTextFromJsonBody(POINTER_USERNAME); - assertThat(username, equalTo(USER_SUPERHERO)); - } - } - @Test public void shouldAuthenticateWithJwtToken_positiveWithAnotherUsername() { try (TestRestClient client = cluster.getRestClient(tokenFactory.generateValidToken(USERNAME_ROOT))) { diff --git a/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationWithUrlParamTests.java b/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationWithUrlParamTests.java new file mode 100644 index 0000000000..85dfec84aa --- /dev/null +++ b/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationWithUrlParamTests.java @@ -0,0 +1,97 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + */ +package org.opensearch.security.http; + +import java.security.KeyPair; +import java.util.Base64; +import java.util.List; +import java.util.Map; + +import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope; +import org.apache.hc.core5.http.Header; +import org.junit.ClassRule; +import org.junit.Rule; +import org.junit.Test; +import org.junit.runner.RunWith; + +import org.opensearch.test.framework.JwtConfigBuilder; +import org.opensearch.test.framework.TestSecurityConfig; +import org.opensearch.test.framework.cluster.ClusterManager; +import org.opensearch.test.framework.cluster.LocalCluster; +import org.opensearch.test.framework.cluster.TestRestClient; +import org.opensearch.test.framework.cluster.TestRestClient.HttpResponse; +import org.opensearch.test.framework.log.LogsRule; + +import io.jsonwebtoken.SignatureAlgorithm; +import io.jsonwebtoken.security.Keys; + +import static java.nio.charset.StandardCharsets.US_ASCII; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; +import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL; +import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.BASIC_AUTH_DOMAIN_ORDER; +import static org.opensearch.test.framework.TestSecurityConfig.Role.ALL_ACCESS; + +@RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class) +@ThreadLeakScope(ThreadLeakScope.Scope.NONE) +public class JwtAuthenticationWithUrlParamTests { + + public static final String CLAIM_USERNAME = "preferred-username"; + public static final String CLAIM_ROLES = "backend-user-roles"; + public static final String POINTER_USERNAME = "/user_name"; + + private static final KeyPair KEY_PAIR = Keys.keyPairFor(SignatureAlgorithm.RS256); + private static final String PUBLIC_KEY = new String(Base64.getEncoder().encode(KEY_PAIR.getPublic().getEncoded()), US_ASCII); + + static final TestSecurityConfig.User ADMIN_USER = new TestSecurityConfig.User("admin").roles(ALL_ACCESS); + + private static final String JWT_AUTH_HEADER = "jwt-auth"; + + private static final JwtAuthorizationHeaderFactory tokenFactory = new JwtAuthorizationHeaderFactory( + KEY_PAIR.getPrivate(), + CLAIM_USERNAME, + CLAIM_ROLES, + JWT_AUTH_HEADER + ); + + public static final TestSecurityConfig.AuthcDomain JWT_AUTH_DOMAIN = new TestSecurityConfig.AuthcDomain( + "jwt", + BASIC_AUTH_DOMAIN_ORDER - 1 + ).jwtHttpAuthenticator( + new JwtConfigBuilder().jwtUrlParameter("token").signingKey(PUBLIC_KEY).subjectKey(CLAIM_USERNAME).rolesKey(CLAIM_ROLES) + ).backend("noop"); + + @ClassRule + public static final LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.SINGLENODE) + .anonymousAuth(false) + .nodeSettings( + Map.of("plugins.security.restapi.roles_enabled", List.of("user_" + ADMIN_USER.getName() + "__" + ALL_ACCESS.getName())) + ) + .authc(AUTHC_HTTPBASIC_INTERNAL) + .authc(JWT_AUTH_DOMAIN) + .users(ADMIN_USER) + .build(); + + @Rule + public LogsRule logsRule = new LogsRule("com.amazon.dlic.auth.http.jwt.HTTPJwtAuthenticator"); + + @Test + public void shouldAuthenticateWithJwtTokenInUrl_positive() { + Header jwtToken = tokenFactory.generateValidToken(ADMIN_USER.getName()); + String jwtTokenValue = jwtToken.getValue(); + try (TestRestClient client = cluster.getRestClient()) { + HttpResponse response = client.getAuthInfo(Map.of("token", jwtTokenValue)); + + response.assertStatusCode(200); + String username = response.getTextFromJsonBody(POINTER_USERNAME); + assertThat(username, equalTo(ADMIN_USER.getName())); + } + } +} From 190df880c743502d2065ae70d7949d3571e6aa6e Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 23 Jan 2024 11:45:45 -0500 Subject: [PATCH 07/20] Add test that assert log entry for unauthed request Signed-off-by: Craig Perkins --- .../JwtAuthenticationWithUrlParamTests.java | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationWithUrlParamTests.java b/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationWithUrlParamTests.java index 85dfec84aa..6dfb3c3bbc 100644 --- a/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationWithUrlParamTests.java +++ b/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationWithUrlParamTests.java @@ -33,6 +33,7 @@ import io.jsonwebtoken.security.Keys; import static java.nio.charset.StandardCharsets.US_ASCII; +import static org.apache.http.HttpHeaders.AUTHORIZATION; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.equalTo; import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL; @@ -52,20 +53,20 @@ public class JwtAuthenticationWithUrlParamTests { static final TestSecurityConfig.User ADMIN_USER = new TestSecurityConfig.User("admin").roles(ALL_ACCESS); - private static final String JWT_AUTH_HEADER = "jwt-auth"; + private static final String TOKEN_URL_PARAM = "token"; private static final JwtAuthorizationHeaderFactory tokenFactory = new JwtAuthorizationHeaderFactory( KEY_PAIR.getPrivate(), CLAIM_USERNAME, CLAIM_ROLES, - JWT_AUTH_HEADER + AUTHORIZATION ); public static final TestSecurityConfig.AuthcDomain JWT_AUTH_DOMAIN = new TestSecurityConfig.AuthcDomain( "jwt", BASIC_AUTH_DOMAIN_ORDER - 1 ).jwtHttpAuthenticator( - new JwtConfigBuilder().jwtUrlParameter("token").signingKey(PUBLIC_KEY).subjectKey(CLAIM_USERNAME).rolesKey(CLAIM_ROLES) + new JwtConfigBuilder().jwtUrlParameter(TOKEN_URL_PARAM).signingKey(PUBLIC_KEY).subjectKey(CLAIM_USERNAME).rolesKey(CLAIM_ROLES) ).backend("noop"); @ClassRule @@ -87,11 +88,21 @@ public void shouldAuthenticateWithJwtTokenInUrl_positive() { Header jwtToken = tokenFactory.generateValidToken(ADMIN_USER.getName()); String jwtTokenValue = jwtToken.getValue(); try (TestRestClient client = cluster.getRestClient()) { - HttpResponse response = client.getAuthInfo(Map.of("token", jwtTokenValue)); + HttpResponse response = client.getAuthInfo(Map.of(TOKEN_URL_PARAM, jwtTokenValue)); response.assertStatusCode(200); String username = response.getTextFromJsonBody(POINTER_USERNAME); assertThat(username, equalTo(ADMIN_USER.getName())); } } + + @Test + public void testUnauthenticatedRequest() { + try (TestRestClient client = cluster.getRestClient()) { + HttpResponse response = client.getAuthInfo(); + + response.assertStatusCode(401); + logsRule.assertThatContainExactly(String.format("No JWT token found in '%s' url parameter header", TOKEN_URL_PARAM)); + } + } } From ab78f8e6e8637c9dee437e8c101b805793484b17 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 31 Jan 2024 14:17:57 -0500 Subject: [PATCH 08/20] getConsumedParams only on NettyRequest Signed-off-by: Craig Perkins --- .../java/org/opensearch/security/filter/NettyRequest.java | 1 - .../org/opensearch/security/filter/SecurityRequest.java | 7 ------- .../ssl/http/netty/Netty4HttpRequestHeaderVerifier.java | 5 ++++- 3 files changed, 4 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/opensearch/security/filter/NettyRequest.java b/src/main/java/org/opensearch/security/filter/NettyRequest.java index e0fb37142a..5890521e2c 100644 --- a/src/main/java/org/opensearch/security/filter/NettyRequest.java +++ b/src/main/java/org/opensearch/security/filter/NettyRequest.java @@ -96,7 +96,6 @@ public String param(String key) { return urlParams != null ? params().get(key) : null; } - @Override public Set getConsumedParams() { return consumedParams; } diff --git a/src/main/java/org/opensearch/security/filter/SecurityRequest.java b/src/main/java/org/opensearch/security/filter/SecurityRequest.java index ad46589e86..195c7d51a5 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityRequest.java +++ b/src/main/java/org/opensearch/security/filter/SecurityRequest.java @@ -12,11 +12,9 @@ package org.opensearch.security.filter; import java.net.InetSocketAddress; -import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Optional; -import java.util.Set; import java.util.stream.Stream; import javax.net.ssl.SSLEngine; @@ -54,9 +52,4 @@ default String header(final String headerName) { /** Gets the parameter for the given key */ String param(String key); - - /** Gets the parameters that have been consumed */ - default Set getConsumedParams() { - return Collections.emptySet(); - }; } diff --git a/src/main/java/org/opensearch/security/ssl/http/netty/Netty4HttpRequestHeaderVerifier.java b/src/main/java/org/opensearch/security/ssl/http/netty/Netty4HttpRequestHeaderVerifier.java index 978eb035e6..b6ca4ca79c 100644 --- a/src/main/java/org/opensearch/security/ssl/http/netty/Netty4HttpRequestHeaderVerifier.java +++ b/src/main/java/org/opensearch/security/ssl/http/netty/Netty4HttpRequestHeaderVerifier.java @@ -14,6 +14,7 @@ import org.opensearch.common.util.concurrent.ThreadContext; import org.opensearch.http.netty4.Netty4HttpChannel; import org.opensearch.http.netty4.Netty4HttpServerTransport; +import org.opensearch.security.filter.NettyRequest; import org.opensearch.security.filter.SecurityRequestChannel; import org.opensearch.security.filter.SecurityRequestChannelUnsupported; import org.opensearch.security.filter.SecurityRequestFactory; @@ -85,7 +86,9 @@ public void channelRead0(ChannelHandlerContext ctx, DefaultHttpRequest msg) thro // If request channel is completed and a response is sent, then there was a failure during authentication restFilter.checkAndAuthenticateRequest(requestChannel); - ctx.channel().attr(CONSUMED_PARAMS).set(requestChannel.getConsumedParams()); + if (requestChannel instanceof NettyRequest) { + ctx.channel().attr(CONSUMED_PARAMS).set(((NettyRequest) requestChannel).getConsumedParams()); + } ThreadContext.StoredContext contextToRestore = threadPool.getThreadContext().newStoredContext(false); ctx.channel().attr(CONTEXT_TO_RESTORE).set(contextToRestore); From df8fe5936329ef7bfe9ec543545d3c00b27cd7d9 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 1 Feb 2024 12:03:10 -0500 Subject: [PATCH 09/20] Update src/main/java/org/opensearch/security/filter/OpenSearchRequest.java Co-authored-by: Peter Nied Signed-off-by: Craig Perkins --- .../org/opensearch/security/filter/OpenSearchRequest.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java b/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java index c64f7af881..a54dfe744c 100644 --- a/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java +++ b/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java @@ -69,7 +69,12 @@ public String uri() { @Override public Map params() { - return underlyingRequest.params(); + return new Map<>(underlyingRequest.params()) { + @Override + public String get(String key) { + return underlyingRequest.param(key); + } + }; } @Override From 6a9330cfed878fdf48dadae01623384708cbe8e1 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 1 Feb 2024 12:11:08 -0500 Subject: [PATCH 10/20] Use HashMap Signed-off-by: Craig Perkins --- .../org/opensearch/security/filter/OpenSearchRequest.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java b/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java index a54dfe744c..3827c3a8ab 100644 --- a/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java +++ b/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java @@ -12,6 +12,7 @@ package org.opensearch.security.filter; import java.net.InetSocketAddress; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Optional; @@ -69,10 +70,10 @@ public String uri() { @Override public Map params() { - return new Map<>(underlyingRequest.params()) { + return new HashMap<>(underlyingRequest.params()) { @Override - public String get(String key) { - return underlyingRequest.param(key); + public String get(Object key) { + return underlyingRequest.param((String) key); } }; } From aaa23a558dc955f0f4f9d129c1e7f6538b92bb47 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 6 Feb 2024 11:24:08 -0500 Subject: [PATCH 11/20] Resolve merge conflict Signed-off-by: Craig Perkins --- .../org/opensearch/security/filter/OpenSearchRequest.java | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java b/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java index 1ecea29cc8..3827c3a8ab 100644 --- a/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java +++ b/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java @@ -83,11 +83,6 @@ public String param(String key) { return underlyingRequest.param(key); } - @Override - public String param(String key) { - return underlyingRequest.param(key); - } - /** Gets access to the underlying request object */ public RestRequest breakEncapsulationForRequest() { return underlyingRequest; From e3ed57479195e1155233993686c877b44d1491ba Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 6 Feb 2024 12:56:21 -0500 Subject: [PATCH 12/20] Remove option to disable url param redaction Signed-off-by: Craig Perkins --- .../security/OpenSearchSecurityPlugin.java | 1 - .../security/auditlog/config/AuditConfig.java | 24 ------------------- .../security/auditlog/impl/AuditMessage.java | 8 +++---- .../config/AuditConfigSerializeTest.java | 2 -- 4 files changed, 3 insertions(+), 32 deletions(-) diff --git a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java index b9fc3f6eca..569380582b 100644 --- a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java +++ b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java @@ -1419,7 +1419,6 @@ public List> getSettings() { case ENABLE_REST: case ENABLE_TRANSPORT: case EXCLUDE_SENSITIVE_HEADERS: - case EXCLUDE_SENSITIVE_URL_PARAMETERS: case LOG_REQUEST_BODY: case RESOLVE_INDICES: return boolSettingNodeScopeFiltered.apply(filterEntry.getKeyWithNamespace(), true); diff --git a/src/main/java/org/opensearch/security/auditlog/config/AuditConfig.java b/src/main/java/org/opensearch/security/auditlog/config/AuditConfig.java index 59771fa378..3fdb5e0143 100644 --- a/src/main/java/org/opensearch/security/auditlog/config/AuditConfig.java +++ b/src/main/java/org/opensearch/security/auditlog/config/AuditConfig.java @@ -145,7 +145,6 @@ public static class Filter { private final boolean logRequestBody; private final boolean resolveIndices; private final boolean excludeSensitiveHeaders; - private final boolean excludeSensitiveUrlParams; @JsonProperty("ignore_users") private final Set ignoredAuditUsers; @JsonProperty("ignore_requests") @@ -169,7 +168,6 @@ public static class Filter { final boolean logRequestBody, final boolean resolveIndices, final boolean excludeSensitiveHeaders, - final boolean excludeSensitiveUrlParams, final Set ignoredAuditUsers, final Set ignoredAuditRequests, final Set ignoredCustomHeaders, @@ -183,7 +181,6 @@ public static class Filter { this.logRequestBody = logRequestBody; this.resolveIndices = resolveIndices; this.excludeSensitiveHeaders = excludeSensitiveHeaders; - this.excludeSensitiveUrlParams = excludeSensitiveUrlParams; this.ignoredAuditUsers = ignoredAuditUsers; this.ignoredAuditUsersMatcher = WildcardMatcher.from(ignoredAuditUsers); this.ignoredAuditRequests = ignoredAuditRequests; @@ -203,10 +200,6 @@ public enum FilterEntries { LOG_REQUEST_BODY("log_request_body", ConfigConstants.OPENDISTRO_SECURITY_AUDIT_LOG_REQUEST_BODY), RESOLVE_INDICES("resolve_indices", ConfigConstants.OPENDISTRO_SECURITY_AUDIT_RESOLVE_INDICES), EXCLUDE_SENSITIVE_HEADERS("exclude_sensitive_headers", ConfigConstants.OPENDISTRO_SECURITY_AUDIT_EXCLUDE_SENSITIVE_HEADERS), - EXCLUDE_SENSITIVE_URL_PARAMETERS( - "exclude_sensitive_url_params", - ConfigConstants.OPENDISTRO_SECURITY_AUDIT_EXCLUDE_SENSITIVE_URL_PARAMETERS - ), DISABLE_REST_CATEGORIES("disabled_rest_categories", ConfigConstants.OPENDISTRO_SECURITY_AUDIT_CONFIG_DISABLED_REST_CATEGORIES), DISABLE_TRANSPORT_CATEGORIES( "disabled_transport_categories", @@ -257,11 +250,6 @@ public static Filter from(Map properties) throws JsonProcessingE final boolean logRequestBody = getOrDefault(properties, FilterEntries.LOG_REQUEST_BODY.getKey(), true); final boolean resolveIndices = getOrDefault(properties, FilterEntries.RESOLVE_INDICES.getKey(), true); final boolean excludeSensitiveHeaders = getOrDefault(properties, FilterEntries.EXCLUDE_SENSITIVE_HEADERS.getKey(), true); - final boolean excludeSensitiveUrlParams = getOrDefault( - properties, - FilterEntries.EXCLUDE_SENSITIVE_URL_PARAMETERS.getKey(), - true - ); final Set disabledRestCategories = AuditCategory.parse( getOrDefault( properties, @@ -293,7 +281,6 @@ public static Filter from(Map properties) throws JsonProcessingE logRequestBody, resolveIndices, excludeSensitiveHeaders, - excludeSensitiveUrlParams, ignoredAuditUsers, ignoreAuditRequests, ignoreHeaders, @@ -316,7 +303,6 @@ public static Filter from(Settings settings) { final boolean logRequestBody = fromSettingBoolean(settings, FilterEntries.LOG_REQUEST_BODY, true); final boolean resolveIndices = fromSettingBoolean(settings, FilterEntries.RESOLVE_INDICES, true); final boolean excludeSensitiveHeaders = fromSettingBoolean(settings, FilterEntries.EXCLUDE_SENSITIVE_HEADERS, true); - final boolean excludeSensitiveUrlParams = fromSettingBoolean(settings, FilterEntries.EXCLUDE_SENSITIVE_URL_PARAMETERS, true); final Set disabledRestCategories = AuditCategory.parse( fromSettingStringSet( settings, @@ -341,7 +327,6 @@ public static Filter from(Settings settings) { logRequestBody, resolveIndices, excludeSensitiveHeaders, - excludeSensitiveUrlParams, ignoredAuditUsers, ignoreAuditRequests, ignoreHeaders, @@ -430,15 +415,6 @@ public boolean shouldExcludeSensitiveHeaders() { return excludeSensitiveHeaders; } - /** - * Checks if sensitive url params eg: jwtUrlParameter from jwt auth domain must be excluded in log messages - * @return true/false - */ - @JsonProperty("exclude_sensitive_url_params") - public boolean shouldExcludeSensitiveUrlParams() { - return excludeSensitiveUrlParams; - } - @VisibleForTesting WildcardMatcher getIgnoredAuditUsersMatcher() { return ignoredAuditUsersMatcher; diff --git a/src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java b/src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java index da8583ee95..b5247cd0a2 100644 --- a/src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java +++ b/src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java @@ -212,12 +212,10 @@ public static String redactUrlParams(String path, AuditConfig.Filter filter) { return path; } - public void addPath(String path, boolean excludeSensitiveUrlParams, AuditConfig.Filter filter) { + public void addPath(String path, AuditConfig.Filter filter) { // TODO redact jwtUrlParameter if (path != null) { - if (excludeSensitiveUrlParams) { - path = redactUrlParams(path, filter); - } + path = redactUrlParams(path, filter); auditInfo.put(REST_REQUEST_PATH, path); } } @@ -411,7 +409,7 @@ void addRestMethod(final RestRequest.Method method) { void addRestRequestInfo(final SecurityRequest request, final AuditConfig.Filter filter) { if (request != null) { final String path = request.path().toString(); - addPath(path, filter.shouldExcludeSensitiveUrlParams(), filter); + addPath(path, filter); addRestHeaders(request.getHeaders(), filter.shouldExcludeSensitiveHeaders(), filter); addRestParams(request.params()); addRestMethod(request.method()); diff --git a/src/test/java/org/opensearch/security/auditlog/config/AuditConfigSerializeTest.java b/src/test/java/org/opensearch/security/auditlog/config/AuditConfigSerializeTest.java index e5036cd219..74aa4219cb 100644 --- a/src/test/java/org/opensearch/security/auditlog/config/AuditConfigSerializeTest.java +++ b/src/test/java/org/opensearch/security/auditlog/config/AuditConfigSerializeTest.java @@ -169,7 +169,6 @@ public void testDeserialize() throws IOException { assertTrue(audit.shouldLogRequestBody()); assertTrue(audit.shouldResolveIndices()); assertTrue(audit.shouldExcludeSensitiveHeaders()); - assertTrue(audit.shouldExcludeSensitiveUrlParams()); assertTrue(configCompliance.shouldLogExternalConfig()); assertTrue(configCompliance.shouldLogInternalConfig()); assertEquals(WildcardMatcher.from(Collections.singleton("test-user-1")), audit.getIgnoredAuditUsersMatcher()); @@ -202,7 +201,6 @@ public void testSerialize() throws IOException { true, true, true, - true, ImmutableSet.of("ignore-user-1", "ignore-user-2"), ImmutableSet.of("ignore-request-1"), ImmutableSet.of("test-header"), From 56244cb013a07861f1683702d39527a766689ca5 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 6 Feb 2024 13:42:30 -0500 Subject: [PATCH 13/20] Add unit tests Signed-off-by: Craig Perkins --- .../config/AuditConfigFilterTest.java | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/src/test/java/org/opensearch/security/auditlog/config/AuditConfigFilterTest.java b/src/test/java/org/opensearch/security/auditlog/config/AuditConfigFilterTest.java index a28d940862..d149f4a4ce 100644 --- a/src/test/java/org/opensearch/security/auditlog/config/AuditConfigFilterTest.java +++ b/src/test/java/org/opensearch/security/auditlog/config/AuditConfigFilterTest.java @@ -18,6 +18,7 @@ import java.util.function.Function; import com.google.common.collect.ImmutableSet; +import org.apache.hc.core5.http.HttpHeaders; import org.junit.Test; import org.opensearch.common.settings.Settings; @@ -34,6 +35,7 @@ import static org.opensearch.security.auditlog.impl.AuditCategory.GRANTED_PRIVILEGES; import static org.opensearch.security.auditlog.impl.AuditCategory.MISSING_PRIVILEGES; import static org.opensearch.security.auditlog.impl.AuditCategory.SSL_EXCEPTION; +import static org.opensearch.security.auditlog.impl.AuditMessage.redactUrlParams; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertSame; @@ -41,6 +43,21 @@ public class AuditConfigFilterTest { + private final AuditConfig.Filter urlParamFilter = new AuditConfig.Filter( + false, + false, + false, + false, + false, + true, + Collections.emptySet(), + Collections.emptySet(), + ImmutableSet.of(HttpHeaders.AUTHORIZATION), + ImmutableSet.of("secretParam"), + Collections.emptySet(), + Collections.emptySet() + ); + @Test public void testDefault() { // arrange @@ -229,4 +246,29 @@ public void fromSettingParseAuditCategory() { .build(); assertThat(parse.apply(settingMultipleValues), equalTo(ImmutableSet.of(AUTHENTICATED, BAD_HEADERS))); } + + @Test + public void testRedactParams() { + String path = "/path/to/route?param=paramValue1&secretParam=secret"; + String filtered = redactUrlParams(path, urlParamFilter); + assertEquals("/path/to/route?param=paramValue1&secretParam=REDACTED", filtered); + System.out.println("PASSED TEST"); + System.out.println("PASSED TEST: " + filtered); + } + + @Test + public void testNoRedact() { + String path = "/path/to/route?param=paramValue1"; + String result = redactUrlParams(path, urlParamFilter); + assertEquals(path, result); + System.out.println("PASSED TEST"); + System.out.println("PASSED TEST: " + result); + } + + @Test + public void testNoRedactOnNoParams() { + String path = "/path/to/route"; + String result = redactUrlParams(path, urlParamFilter); + assertEquals(path, result); + } } From 75eba726651fc048179c671552132671b05dd497 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 6 Feb 2024 14:58:50 -0500 Subject: [PATCH 14/20] Redact sensitive url params from auditlog Signed-off-by: Craig Perkins --- .../JwtAuthenticationWithUrlParamTests.java | 18 +++++++ .../test/framework/JwtConfigBuilder.java | 1 - .../audit/AuditMessagePredicate.java | 30 +++++++++++- .../security/auditlog/config/AuditConfig.java | 41 ++++++---------- .../auditlog/impl/AbstractAuditLog.java | 24 +++++++++ .../security/auditlog/impl/AuditMessage.java | 49 +++++-------------- .../config/AuditConfigFilterTest.java | 42 ---------------- 7 files changed, 99 insertions(+), 106 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationWithUrlParamTests.java b/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationWithUrlParamTests.java index 6dfb3c3bbc..3964bb4552 100644 --- a/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationWithUrlParamTests.java +++ b/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationWithUrlParamTests.java @@ -21,8 +21,12 @@ import org.junit.Test; import org.junit.runner.RunWith; +import org.opensearch.test.framework.AuditCompliance; +import org.opensearch.test.framework.AuditConfiguration; +import org.opensearch.test.framework.AuditFilters; import org.opensearch.test.framework.JwtConfigBuilder; import org.opensearch.test.framework.TestSecurityConfig; +import org.opensearch.test.framework.audit.AuditLogsRule; import org.opensearch.test.framework.cluster.ClusterManager; import org.opensearch.test.framework.cluster.LocalCluster; import org.opensearch.test.framework.cluster.TestRestClient; @@ -36,9 +40,11 @@ import static org.apache.http.HttpHeaders.AUTHORIZATION; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.equalTo; +import static org.opensearch.rest.RestRequest.Method.GET; import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL; import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.BASIC_AUTH_DOMAIN_ORDER; import static org.opensearch.test.framework.TestSecurityConfig.Role.ALL_ACCESS; +import static org.opensearch.test.framework.audit.AuditMessagePredicate.userAuthenticated; @RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class) @ThreadLeakScope(ThreadLeakScope.Scope.NONE) @@ -69,12 +75,19 @@ public class JwtAuthenticationWithUrlParamTests { new JwtConfigBuilder().jwtUrlParameter(TOKEN_URL_PARAM).signingKey(PUBLIC_KEY).subjectKey(CLAIM_USERNAME).rolesKey(CLAIM_ROLES) ).backend("noop"); + @Rule + public AuditLogsRule auditLogsRule = new AuditLogsRule(); + @ClassRule public static final LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.SINGLENODE) .anonymousAuth(false) .nodeSettings( Map.of("plugins.security.restapi.roles_enabled", List.of("user_" + ADMIN_USER.getName() + "__" + ALL_ACCESS.getName())) ) + .audit( + new AuditConfiguration(true).compliance(new AuditCompliance().enabled(true)) + .filters(new AuditFilters().enabledRest(true).enabledTransport(true)) + ) .authc(AUTHC_HTTPBASIC_INTERNAL) .authc(JWT_AUTH_DOMAIN) .users(ADMIN_USER) @@ -93,6 +106,11 @@ public void shouldAuthenticateWithJwtTokenInUrl_positive() { response.assertStatusCode(200); String username = response.getTextFromJsonBody(POINTER_USERNAME); assertThat(username, equalTo(ADMIN_USER.getName())); + Map expectedParams = Map.of("token", "REDACTED"); + + auditLogsRule.assertExactlyOne( + userAuthenticated(ADMIN_USER).withRestRequest(GET, "/_opendistro/_security/authinfo").withRestParams(expectedParams) + ); } } diff --git a/src/integrationTest/java/org/opensearch/test/framework/JwtConfigBuilder.java b/src/integrationTest/java/org/opensearch/test/framework/JwtConfigBuilder.java index b6fd6e0224..88297bacd2 100644 --- a/src/integrationTest/java/org/opensearch/test/framework/JwtConfigBuilder.java +++ b/src/integrationTest/java/org/opensearch/test/framework/JwtConfigBuilder.java @@ -22,7 +22,6 @@ public class JwtConfigBuilder { private String signingKey; private String subjectKey; private String rolesKey; - private String jwtUrlParameter; public JwtConfigBuilder jwtHeader(String jwtHeader) { this.jwtHeader = jwtHeader; diff --git a/src/integrationTest/java/org/opensearch/test/framework/audit/AuditMessagePredicate.java b/src/integrationTest/java/org/opensearch/test/framework/audit/AuditMessagePredicate.java index 4935bf0387..34565e9926 100644 --- a/src/integrationTest/java/org/opensearch/test/framework/audit/AuditMessagePredicate.java +++ b/src/integrationTest/java/org/opensearch/test/framework/audit/AuditMessagePredicate.java @@ -29,6 +29,7 @@ import static org.opensearch.security.auditlog.impl.AuditCategory.MISSING_PRIVILEGES; import static org.opensearch.security.auditlog.impl.AuditMessage.REQUEST_LAYER; import static org.opensearch.security.auditlog.impl.AuditMessage.RESOLVED_INDICES; +import static org.opensearch.security.auditlog.impl.AuditMessage.REST_REQUEST_PARAMS; import static org.opensearch.security.auditlog.impl.AuditMessage.REST_REQUEST_PATH; public class AuditMessagePredicate implements Predicate { @@ -36,6 +37,7 @@ public class AuditMessagePredicate implements Predicate { private final AuditCategory category; private final Origin requestLayer; private final String restRequestPath; + private final Map restParams; private final String initiatingUser; private final Method requestMethod; private final String transportRequestType; @@ -47,6 +49,7 @@ private AuditMessagePredicate( AuditCategory category, Origin requestLayer, String restRequestPath, + Map restParams, String initiatingUser, Method requestMethod, String transportRequestType, @@ -57,6 +60,7 @@ private AuditMessagePredicate( this.category = category; this.requestLayer = requestLayer; this.restRequestPath = restRequestPath; + this.restParams = restParams; this.initiatingUser = initiatingUser; this.requestMethod = requestMethod; this.transportRequestType = transportRequestType; @@ -66,7 +70,7 @@ private AuditMessagePredicate( } private AuditMessagePredicate(AuditCategory category) { - this(category, null, null, null, null, null, null, null, null); + this(category, null, null, null, null, null, null, null, null, null); } public static AuditMessagePredicate auditPredicate(AuditCategory category) { @@ -110,6 +114,7 @@ public AuditMessagePredicate withLayer(Origin layer) { category, layer, restRequestPath, + restParams, initiatingUser, requestMethod, transportRequestType, @@ -124,6 +129,22 @@ public AuditMessagePredicate withRequestPath(String path) { category, requestLayer, path, + restParams, + initiatingUser, + requestMethod, + transportRequestType, + effectiveUser, + index, + privilege + ); + } + + public AuditMessagePredicate withRestParams(Map params) { + return new AuditMessagePredicate( + category, + requestLayer, + restRequestPath, + params, initiatingUser, requestMethod, transportRequestType, @@ -138,6 +159,7 @@ public AuditMessagePredicate withInitiatingUser(String user) { category, requestLayer, restRequestPath, + restParams, user, requestMethod, transportRequestType, @@ -156,6 +178,7 @@ public AuditMessagePredicate withRestMethod(Method method) { category, requestLayer, restRequestPath, + restParams, initiatingUser, method, transportRequestType, @@ -170,6 +193,7 @@ public AuditMessagePredicate withTransportRequestType(String type) { category, requestLayer, restRequestPath, + restParams, initiatingUser, requestMethod, type, @@ -184,6 +208,7 @@ public AuditMessagePredicate withEffectiveUser(String user) { category, requestLayer, restRequestPath, + restParams, initiatingUser, requestMethod, transportRequestType, @@ -206,6 +231,7 @@ public AuditMessagePredicate withIndex(String indexName) { category, requestLayer, restRequestPath, + restParams, initiatingUser, requestMethod, transportRequestType, @@ -220,6 +246,7 @@ public AuditMessagePredicate withPrivilege(String privilegeAction) { category, requestLayer, restRequestPath, + restParams, initiatingUser, requestMethod, transportRequestType, @@ -235,6 +262,7 @@ public boolean test(AuditMessage auditMessage) { predicates.add(audit -> Objects.isNull(category) || category.equals(audit.getCategory())); predicates.add(audit -> Objects.isNull(requestLayer) || requestLayer.equals(audit.getAsMap().get(REQUEST_LAYER))); predicates.add(audit -> Objects.isNull(restRequestPath) || restRequestPath.equals(audit.getAsMap().get(REST_REQUEST_PATH))); + predicates.add(audit -> Objects.isNull(restParams) || restParams.equals(auditMessage.getAsMap().get(REST_REQUEST_PARAMS))); predicates.add(audit -> Objects.isNull(initiatingUser) || initiatingUser.equals(audit.getInitiatingUser())); predicates.add(audit -> Objects.isNull(requestMethod) || requestMethod.equals(audit.getRequestMethod())); predicates.add(audit -> Objects.isNull(transportRequestType) || transportRequestType.equals(audit.getRequestType())); diff --git a/src/main/java/org/opensearch/security/auditlog/config/AuditConfig.java b/src/main/java/org/opensearch/security/auditlog/config/AuditConfig.java index 3fdb5e0143..3b3ee742b6 100644 --- a/src/main/java/org/opensearch/security/auditlog/config/AuditConfig.java +++ b/src/main/java/org/opensearch/security/auditlog/config/AuditConfig.java @@ -16,7 +16,6 @@ import java.util.List; import java.util.Map; import java.util.Set; -import java.util.SortedSet; import java.util.stream.Collectors; import com.google.common.annotations.VisibleForTesting; @@ -33,16 +32,11 @@ import org.opensearch.common.settings.Settings; import org.opensearch.security.DefaultObjectMapper; import org.opensearch.security.auditlog.impl.AuditCategory; -import org.opensearch.security.auth.AuthDomain; import org.opensearch.security.compliance.ComplianceConfig; import org.opensearch.security.dlic.rest.support.Utils; -import org.opensearch.security.securityconf.DynamicConfigModel; import org.opensearch.security.support.ConfigConstants; import org.opensearch.security.support.WildcardMatcher; -import com.amazon.dlic.auth.http.jwt.HTTPJwtAuthenticator; -import org.greenrobot.eventbus.Subscribe; - import static org.opensearch.security.DefaultObjectMapper.getOrDefault; import static org.opensearch.security.support.ConfigConstants.SECURITY_AUDIT_CONFIG_DEFAULT; @@ -93,8 +87,6 @@ public class AuditConfig { private static Set FIELDS = DefaultObjectMapper.getFields(AuditConfig.class); - static Set ignoreUrlParams = new HashSet<>(); - private AuditConfig() { this(true, null, null); } @@ -152,11 +144,11 @@ public static class Filter { @JsonProperty("ignore_headers") private final Set ignoredCustomHeaders; @JsonProperty("ignore_url_params") - private final Set ignoredUrlParams; + private Set ignoredUrlParams; private final WildcardMatcher ignoredAuditUsersMatcher; private final WildcardMatcher ignoredAuditRequestsMatcher; private final WildcardMatcher ignoredCustomHeadersMatcher; - private final WildcardMatcher ignoredUrlParamsMatcher; + private WildcardMatcher ignoredUrlParamsMatcher; private final Set disabledRestCategories; private final Set disabledTransportCategories; @@ -284,7 +276,7 @@ public static Filter from(Map properties) throws JsonProcessingE ignoredAuditUsers, ignoreAuditRequests, ignoreHeaders, - ignoreUrlParams, + new HashSet<>(), disabledRestCategories, disabledTransportCategories ); @@ -330,7 +322,7 @@ public static Filter from(Settings settings) { ignoredAuditUsers, ignoreAuditRequests, ignoreHeaders, - ignoreUrlParams, + new HashSet<>(), disabledRestCategories, disabledTransportCategories ); @@ -473,6 +465,17 @@ public boolean isRequestAuditDisabled(String action) { return ignoredAuditRequestsMatcher.test(action); } + /** + * URL Params to redact for auditing + */ + public void setIgnoredUrlParams(Set ignoredUrlParams) { + if (ignoredUrlParams == null) { + return; + } + this.ignoredUrlParamsMatcher = WildcardMatcher.from(ignoredUrlParams); + this.ignoredUrlParams = ignoredUrlParams; + } + /** * Disabled categories for REST API auditing * @return set of categories @@ -572,18 +575,4 @@ public static Set getDeprecatedKeys(final Settings settings) { Utils.generateFieldResourcePaths(ComplianceConfig.FIELDS, "/compliance/") ) ); - - @Subscribe - public void onDynamicConfigModelChanged(DynamicConfigModel dcm) { - SortedSet authDomains = Collections.unmodifiableSortedSet(dcm.getRestAuthDomains()); - ignoreUrlParams.clear(); - for (AuthDomain authDomain : authDomains) { - if ("jwt".equals(authDomain.getHttpAuthenticator().getType())) { - HTTPJwtAuthenticator jwtAuthenticator = (HTTPJwtAuthenticator) authDomain.getHttpAuthenticator(); - if (jwtAuthenticator.getJwtUrlParameter() != null) { - ignoreUrlParams.add(jwtAuthenticator.getJwtUrlParameter()); - } - } - } - } } diff --git a/src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java b/src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java index e5f314cd29..a36d94e75b 100644 --- a/src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java +++ b/src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java @@ -19,10 +19,14 @@ import java.security.AccessController; import java.security.PrivilegedAction; import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Properties; +import java.util.Set; +import java.util.SortedSet; import java.util.concurrent.atomic.AtomicBoolean; import java.util.stream.Collectors; @@ -62,9 +66,11 @@ import org.opensearch.security.DefaultObjectMapper; import org.opensearch.security.auditlog.AuditLog; import org.opensearch.security.auditlog.config.AuditConfig; +import org.opensearch.security.auth.AuthDomain; import org.opensearch.security.compliance.ComplianceConfig; import org.opensearch.security.dlic.rest.support.Utils; import org.opensearch.security.filter.SecurityRequest; +import org.opensearch.security.securityconf.DynamicConfigModel; import org.opensearch.security.support.Base64Helper; import org.opensearch.security.support.ConfigConstants; import org.opensearch.security.user.User; @@ -72,7 +78,9 @@ import org.opensearch.threadpool.ThreadPool; import org.opensearch.transport.TransportRequest; +import com.amazon.dlic.auth.http.jwt.HTTPJwtAuthenticator; import com.flipkart.zjsonpatch.JsonDiff; +import org.greenrobot.eventbus.Subscribe; import static org.opensearch.core.xcontent.DeprecationHandler.THROW_UNSUPPORTED_OPERATION; @@ -88,6 +96,7 @@ public abstract class AbstractAuditLog implements AuditLog { private volatile ComplianceConfig complianceConfig; private final Environment environment; private AtomicBoolean externalConfigLogged = new AtomicBoolean(); + private final Set ignoredUrlParams = new HashSet<>(); protected abstract void enableRoutes(); @@ -120,6 +129,7 @@ protected AbstractAuditLog( } protected void onAuditConfigFilterChanged(AuditConfig.Filter auditConfigFilter) { + auditConfigFilter.setIgnoredUrlParams(ignoredUrlParams); this.auditConfigFilter = auditConfigFilter; this.auditConfigFilter.log(log); } @@ -930,4 +940,18 @@ boolean checkRestFilter(final AuditCategory category, final String effectiveUser } protected abstract void save(final AuditMessage msg); + + @Subscribe + public void onDynamicConfigModelChanged(DynamicConfigModel dcm) { + SortedSet authDomains = Collections.unmodifiableSortedSet(dcm.getRestAuthDomains()); + ignoredUrlParams.clear(); + for (AuthDomain authDomain : authDomains) { + if ("jwt".equals(authDomain.getHttpAuthenticator().getType())) { + HTTPJwtAuthenticator jwtAuthenticator = (HTTPJwtAuthenticator) authDomain.getHttpAuthenticator(); + if (jwtAuthenticator.getJwtUrlParameter() != null) { + ignoredUrlParams.add(jwtAuthenticator.getJwtUrlParameter()); + } + } + } + } } diff --git a/src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java b/src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java index b5247cd0a2..716e141ffd 100644 --- a/src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java +++ b/src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java @@ -12,7 +12,6 @@ package org.opensearch.security.auditlog.impl; import java.io.IOException; -import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.LinkOption; import java.nio.file.Path; @@ -24,13 +23,10 @@ import java.util.Map.Entry; import java.util.Objects; import java.util.regex.Pattern; -import java.util.stream.Collectors; import com.google.common.annotations.VisibleForTesting; import org.apache.commons.codec.digest.DigestUtils; -import org.apache.hc.core5.http.NameValuePair; import org.apache.hc.core5.net.URIBuilder; -import org.apache.hc.core5.net.URLEncodedUtils; import org.opensearch.ExceptionsHelper; import org.opensearch.cluster.service.ClusterService; @@ -187,35 +183,8 @@ public void addEffectiveUser(String user) { } } - public static String redactUrlParams(String path, AuditConfig.Filter filter) { - String[] parts = path.split("\\?", 2); - String urlPath = parts[0]; - - if (parts.length > 1) { - String queryString = parts[1]; - - // Parse the query string - List params = URLEncodedUtils.parse(queryString, StandardCharsets.UTF_8); - - // Redact the specified parameter - String redactedQuery = params.stream() - .map( - param -> filter.shouldExcludeUrlParam(param.getName()) - ? param.getName() + "=REDACTED" - : param.getName() + "=" + param.getValue() - ) - .collect(Collectors.joining("&")); - - return urlPath + "?" + redactedQuery; - } - - return path; - } - - public void addPath(String path, AuditConfig.Filter filter) { - // TODO redact jwtUrlParameter + public void addPath(String path) { if (path != null) { - path = redactUrlParams(path, filter); auditInfo.put(REST_REQUEST_PATH, path); } } @@ -381,9 +350,17 @@ public void addTaskParentId(String id) { } } - public void addRestParams(Map params) { + public void addRestParams(Map params, AuditConfig.Filter filter) { if (params != null && !params.isEmpty()) { - auditInfo.put(REST_REQUEST_PARAMS, new HashMap<>(params)); + Map redactedParams = new HashMap<>(); + for (Entry param : params.entrySet()) { + if (filter != null && filter.shouldExcludeUrlParam(param.getKey())) { + redactedParams.put(param.getKey(), "REDACTED"); + } else { + redactedParams.put(param.getKey(), param.getValue()); + } + } + auditInfo.put(REST_REQUEST_PARAMS, redactedParams); } } @@ -409,9 +386,9 @@ void addRestMethod(final RestRequest.Method method) { void addRestRequestInfo(final SecurityRequest request, final AuditConfig.Filter filter) { if (request != null) { final String path = request.path().toString(); - addPath(path, filter); + addPath(path); addRestHeaders(request.getHeaders(), filter.shouldExcludeSensitiveHeaders(), filter); - addRestParams(request.params()); + addRestParams(request.params(), filter); addRestMethod(request.method()); if (filter.shouldLogRequestBody()) { diff --git a/src/test/java/org/opensearch/security/auditlog/config/AuditConfigFilterTest.java b/src/test/java/org/opensearch/security/auditlog/config/AuditConfigFilterTest.java index d149f4a4ce..a28d940862 100644 --- a/src/test/java/org/opensearch/security/auditlog/config/AuditConfigFilterTest.java +++ b/src/test/java/org/opensearch/security/auditlog/config/AuditConfigFilterTest.java @@ -18,7 +18,6 @@ import java.util.function.Function; import com.google.common.collect.ImmutableSet; -import org.apache.hc.core5.http.HttpHeaders; import org.junit.Test; import org.opensearch.common.settings.Settings; @@ -35,7 +34,6 @@ import static org.opensearch.security.auditlog.impl.AuditCategory.GRANTED_PRIVILEGES; import static org.opensearch.security.auditlog.impl.AuditCategory.MISSING_PRIVILEGES; import static org.opensearch.security.auditlog.impl.AuditCategory.SSL_EXCEPTION; -import static org.opensearch.security.auditlog.impl.AuditMessage.redactUrlParams; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertSame; @@ -43,21 +41,6 @@ public class AuditConfigFilterTest { - private final AuditConfig.Filter urlParamFilter = new AuditConfig.Filter( - false, - false, - false, - false, - false, - true, - Collections.emptySet(), - Collections.emptySet(), - ImmutableSet.of(HttpHeaders.AUTHORIZATION), - ImmutableSet.of("secretParam"), - Collections.emptySet(), - Collections.emptySet() - ); - @Test public void testDefault() { // arrange @@ -246,29 +229,4 @@ public void fromSettingParseAuditCategory() { .build(); assertThat(parse.apply(settingMultipleValues), equalTo(ImmutableSet.of(AUTHENTICATED, BAD_HEADERS))); } - - @Test - public void testRedactParams() { - String path = "/path/to/route?param=paramValue1&secretParam=secret"; - String filtered = redactUrlParams(path, urlParamFilter); - assertEquals("/path/to/route?param=paramValue1&secretParam=REDACTED", filtered); - System.out.println("PASSED TEST"); - System.out.println("PASSED TEST: " + filtered); - } - - @Test - public void testNoRedact() { - String path = "/path/to/route?param=paramValue1"; - String result = redactUrlParams(path, urlParamFilter); - assertEquals(path, result); - System.out.println("PASSED TEST"); - System.out.println("PASSED TEST: " + result); - } - - @Test - public void testNoRedactOnNoParams() { - String path = "/path/to/route"; - String result = redactUrlParams(path, urlParamFilter); - assertEquals(path, result); - } } From c4e49b2d875a7eabc9466317f29fdcb0089534fb Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 22 Feb 2024 10:57:10 -0500 Subject: [PATCH 15/20] Rebase Signed-off-by: Craig Perkins --- .../amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java | 4 ++-- .../java/org/opensearch/security/filter/NettyRequest.java | 7 ------- .../org/opensearch/security/filter/SecurityRestFilter.java | 5 ----- .../ssl/http/netty/Netty4HttpRequestHeaderVerifier.java | 2 -- 4 files changed, 2 insertions(+), 16 deletions(-) diff --git a/src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java b/src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java index f00e43cfc3..f4b9770089 100644 --- a/src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java +++ b/src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java @@ -130,10 +130,10 @@ private AuthCredentials extractCredentials0(final SecurityRequest request) { } if ((jwtToken == null || jwtToken.isEmpty()) && jwtUrlParameter != null) { - jwtToken = request.param(jwtUrlParameter); + jwtToken = request.params().get(jwtUrlParameter); } else { // just consume to avoid "contains unrecognized parameter" - request.param(jwtUrlParameter); + request.params().get(jwtUrlParameter); } if (jwtToken == null || jwtToken.length() == 0) { diff --git a/src/main/java/org/opensearch/security/filter/NettyRequest.java b/src/main/java/org/opensearch/security/filter/NettyRequest.java index a6a7785faf..53ed6d6798 100644 --- a/src/main/java/org/opensearch/security/filter/NettyRequest.java +++ b/src/main/java/org/opensearch/security/filter/NettyRequest.java @@ -98,13 +98,6 @@ public Set getUnconsumedParams() { return parameters.get().accessedKeys(); } - @Override - public String param(String key) { - Map urlParams = params(); - consumedParams.add(key); - return urlParams != null ? params().get(key) : null; - } - public Set getConsumedParams() { return consumedParams; } diff --git a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java index 97816c99b1..b649b8d71f 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java +++ b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java @@ -69,7 +69,6 @@ import static org.opensearch.security.OpenSearchSecurityPlugin.LEGACY_OPENDISTRO_PREFIX; import static org.opensearch.security.OpenSearchSecurityPlugin.PLUGINS_PREFIX; -import static org.opensearch.security.http.SecurityHttpServerTransport.CONSUMED_PARAMS; import static org.opensearch.security.http.SecurityHttpServerTransport.CONTEXT_TO_RESTORE; import static org.opensearch.security.http.SecurityHttpServerTransport.EARLY_RESPONSE; import static org.opensearch.security.http.SecurityHttpServerTransport.IS_AUTHENTICATED; @@ -155,10 +154,6 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c final SecurityRequestChannel requestChannel = SecurityRequestFactory.from(request, channel); - for (String paramToConsume : registry.getUrlParamsToConsume()) { - requestChannel.param(paramToConsume); - } - // Authenticate request if (!NettyAttribute.popFrom(request, IS_AUTHENTICATED).orElse(false)) { // we aren't authenticated so we should skip this step diff --git a/src/main/java/org/opensearch/security/ssl/http/netty/Netty4HttpRequestHeaderVerifier.java b/src/main/java/org/opensearch/security/ssl/http/netty/Netty4HttpRequestHeaderVerifier.java index 7fe4cd9267..e6dec0c213 100644 --- a/src/main/java/org/opensearch/security/ssl/http/netty/Netty4HttpRequestHeaderVerifier.java +++ b/src/main/java/org/opensearch/security/ssl/http/netty/Netty4HttpRequestHeaderVerifier.java @@ -14,7 +14,6 @@ import org.opensearch.common.util.concurrent.ThreadContext; import org.opensearch.http.netty4.Netty4HttpChannel; import org.opensearch.http.netty4.Netty4HttpServerTransport; -import org.opensearch.security.filter.NettyRequest; import org.opensearch.security.filter.SecurityRequestChannel; import org.opensearch.security.filter.SecurityRequestChannelUnsupported; import org.opensearch.security.filter.SecurityRequestFactory; @@ -33,7 +32,6 @@ import io.netty.handler.codec.http.HttpRequest; import io.netty.util.ReferenceCountUtil; -import static org.opensearch.security.http.SecurityHttpServerTransport.CONSUMED_PARAMS; import static org.opensearch.security.http.SecurityHttpServerTransport.CONTEXT_TO_RESTORE; import static org.opensearch.security.http.SecurityHttpServerTransport.EARLY_RESPONSE; import static org.opensearch.security.http.SecurityHttpServerTransport.IS_AUTHENTICATED; From 1939a03b1597f3249d876463e7492337154cda9d Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 22 Feb 2024 11:02:26 -0500 Subject: [PATCH 16/20] Remove unneeded code Signed-off-by: Craig Perkins --- .../opensearch/security/auth/BackendRegistry.java | 15 --------------- .../opensearch/security/filter/NettyRequest.java | 6 ------ .../security/support/ConfigConstants.java | 2 -- .../auditlog/config/AuditConfigSerializeTest.java | 4 ---- 4 files changed, 27 deletions(-) diff --git a/src/main/java/org/opensearch/security/auth/BackendRegistry.java b/src/main/java/org/opensearch/security/auth/BackendRegistry.java index 3864c0b96b..2e9922647f 100644 --- a/src/main/java/org/opensearch/security/auth/BackendRegistry.java +++ b/src/main/java/org/opensearch/security/auth/BackendRegistry.java @@ -66,7 +66,6 @@ import org.opensearch.security.user.User; import org.opensearch.threadpool.ThreadPool; -import com.amazon.dlic.auth.http.jwt.HTTPJwtAuthenticator; import org.greenrobot.eventbus.Subscribe; import static org.apache.http.HttpStatus.SC_FORBIDDEN; @@ -78,7 +77,6 @@ public class BackendRegistry { protected final Logger log = LogManager.getLogger(this.getClass()); private SortedSet restAuthDomains; private Set restAuthorizers; - private Set urlParamsToConsume = new HashSet<>(); private List ipAuthFailureListeners; private Multimap authBackendFailureListeners; private List> ipClientBlockRegistries; @@ -157,10 +155,6 @@ public BackendRegistry( createCaches(); } - public Set getUrlParamsToConsume() { - return urlParamsToConsume; - } - public boolean isInitialized() { return initialized; } @@ -187,15 +181,6 @@ public void onDynamicConfigModelChanged(DynamicConfigModel dcm) { authBackendClientBlockRegistries = dcm.getAuthBackendClientBlockRegistries(); SortedSet authDomains = Collections.unmodifiableSortedSet(dcm.getRestAuthDomains()); - urlParamsToConsume.clear(); - for (AuthDomain authDomain : authDomains) { - if ("jwt".equals(authDomain.getHttpAuthenticator().getType())) { - HTTPJwtAuthenticator jwtAuthenticator = (HTTPJwtAuthenticator) authDomain.getHttpAuthenticator(); - if (jwtAuthenticator.getJwtUrlParameter() != null) { - urlParamsToConsume.add(jwtAuthenticator.getJwtUrlParameter()); - } - } - } // OpenSearch Security no default authc initialized = !restAuthDomains.isEmpty() || anonymousAuthEnabled || injectedUserEnabled; diff --git a/src/main/java/org/opensearch/security/filter/NettyRequest.java b/src/main/java/org/opensearch/security/filter/NettyRequest.java index 53ed6d6798..c827ddc779 100644 --- a/src/main/java/org/opensearch/security/filter/NettyRequest.java +++ b/src/main/java/org/opensearch/security/filter/NettyRequest.java @@ -41,8 +41,6 @@ public class NettyRequest implements SecurityRequest { protected final Netty4HttpChannel underlyingChannel; protected final Supplier parameters = Suppliers.memoize(() -> new CheckedAccessMap(params(uri()))); - private final Set consumedParams = new HashSet<>(); - NettyRequest(final HttpRequest request, final Netty4HttpChannel channel) { this.underlyingRequest = request; this.underlyingChannel = channel; @@ -98,10 +96,6 @@ public Set getUnconsumedParams() { return parameters.get().accessedKeys(); } - public Set getConsumedParams() { - return consumedParams; - } - private static Map params(String uri) { // Sourced from // https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java#L419-L422 diff --git a/src/main/java/org/opensearch/security/support/ConfigConstants.java b/src/main/java/org/opensearch/security/support/ConfigConstants.java index 61006c4cdd..3060e1b2dc 100644 --- a/src/main/java/org/opensearch/security/support/ConfigConstants.java +++ b/src/main/java/org/opensearch/security/support/ConfigConstants.java @@ -170,8 +170,6 @@ public class ConfigConstants { public static final boolean OPENDISTRO_SECURITY_AUDIT_SSL_VERIFY_HOSTNAMES_DEFAULT = true; public static final boolean OPENDISTRO_SECURITY_AUDIT_SSL_ENABLE_SSL_CLIENT_AUTH_DEFAULT = false; public static final String OPENDISTRO_SECURITY_AUDIT_EXCLUDE_SENSITIVE_HEADERS = "opendistro_security.audit.exclude_sensitive_headers"; - public static final String OPENDISTRO_SECURITY_AUDIT_EXCLUDE_SENSITIVE_URL_PARAMETERS = - "opendistro_security.audit.exclude_sensitive_url_params"; public static final String SECURITY_AUDIT_CONFIG_DEFAULT_PREFIX = "plugins.security.audit.config."; diff --git a/src/test/java/org/opensearch/security/auditlog/config/AuditConfigSerializeTest.java b/src/test/java/org/opensearch/security/auditlog/config/AuditConfigSerializeTest.java index 74aa4219cb..52cb39f41e 100644 --- a/src/test/java/org/opensearch/security/auditlog/config/AuditConfigSerializeTest.java +++ b/src/test/java/org/opensearch/security/auditlog/config/AuditConfigSerializeTest.java @@ -70,7 +70,6 @@ public void testDefaultSerialize() throws IOException { .field("log_request_body", true) .field("resolve_indices", true) .field("exclude_sensitive_headers", true) - .field("exclude_sensitive_url_params", true) .field("ignore_users", Collections.singletonList("kibanaserver")) .field("ignore_requests", Collections.emptyList()) .field("ignore_headers", Collections.emptyList()) @@ -135,7 +134,6 @@ public void testDeserialize() throws IOException { .field("log_request_body", true) .field("resolve_indices", true) .field("exclude_sensitive_headers", true) - .field("exclude_sensitive_url_params", true) .field("ignore_users", Collections.singletonList("test-user-1")) .field("ignore_requests", Collections.singletonList("test-request")) .field("ignore_headers", Collections.singletonList("test-headers")) @@ -235,7 +233,6 @@ public void testSerialize() throws IOException { .field("log_request_body", true) .field("resolve_indices", true) .field("exclude_sensitive_headers", true) - .field("exclude_sensitive_url_params", true) .field("ignore_users", ImmutableList.of("ignore-user-1", "ignore-user-2")) .field("ignore_requests", Collections.singletonList("ignore-request-1")) .field("ignore_headers", Collections.singletonList("test-header")) @@ -280,7 +277,6 @@ public void testNullSerialize() throws IOException { .field("log_request_body", true) .field("resolve_indices", true) .field("exclude_sensitive_headers", true) - .field("exclude_sensitive_url_params", true) .field("ignore_users", ImmutableList.of("kibanaserver")) .field("ignore_requests", Collections.emptyList()) .field("ignore_headers", Collections.emptyList()) From 638599f6b1b986e8bf33e37c2da8be6ce8952d9b Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 22 Feb 2024 11:05:51 -0500 Subject: [PATCH 17/20] Back out JwtAuthenticationTests changes Signed-off-by: Craig Perkins --- .../security/http/JwtAuthenticationTests.java | 30 +------------------ 1 file changed, 1 insertion(+), 29 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationTests.java b/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationTests.java index 5decb3ef5a..659d7c178e 100644 --- a/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationTests.java +++ b/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationTests.java @@ -28,13 +28,9 @@ import org.opensearch.action.search.SearchResponse; import org.opensearch.client.Client; import org.opensearch.client.RestHighLevelClient; -import org.opensearch.test.framework.AuditCompliance; -import org.opensearch.test.framework.AuditConfiguration; -import org.opensearch.test.framework.AuditFilters; import org.opensearch.test.framework.JwtConfigBuilder; import org.opensearch.test.framework.TestSecurityConfig; import org.opensearch.test.framework.TestSecurityConfig.Role; -import org.opensearch.test.framework.audit.AuditLogsRule; import org.opensearch.test.framework.cluster.ClusterManager; import org.opensearch.test.framework.cluster.LocalCluster; import org.opensearch.test.framework.cluster.TestRestClient; @@ -108,18 +104,11 @@ public class JwtAuthenticationTests { JWT_AUTH_HEADER ); - @Rule - public AuditLogsRule auditLogsRule = new AuditLogsRule(); - public static final TestSecurityConfig.AuthcDomain JWT_AUTH_DOMAIN = new TestSecurityConfig.AuthcDomain( "jwt", BASIC_AUTH_DOMAIN_ORDER - 1 ).jwtHttpAuthenticator( - new JwtConfigBuilder().jwtHeader(JWT_AUTH_HEADER) - .jwtUrlParameter("token") - .signingKey(PUBLIC_KEY) - .subjectKey(CLAIM_USERNAME) - .rolesKey(CLAIM_ROLES) + new JwtConfigBuilder().jwtHeader(JWT_AUTH_HEADER).signingKey(PUBLIC_KEY).subjectKey(CLAIM_USERNAME).rolesKey(CLAIM_ROLES) ).backend("noop"); public static final String SONG_ID_1 = "song-id-01"; @@ -137,10 +126,6 @@ public class JwtAuthenticationTests { .users(ADMIN_USER) .roles(DEPARTMENT_SONG_LISTENER_ROLE) .authc(JWT_AUTH_DOMAIN) - .audit( - new AuditConfiguration(true).compliance(new AuditCompliance().enabled(true)) - .filters(new AuditFilters().enabledRest(true).enabledTransport(true).resolveBulkRequests(true)) - ) .build(); @Rule @@ -168,19 +153,6 @@ public void shouldAuthenticateWithJwtToken_positive() { } } - @Test - public void shouldAuthenticateWithJwtTokenInUrl_positive() { - Header jwtToken = tokenFactory.generateValidToken(USER_SUPERHERO); - String jwtTokenValue = jwtToken.getValue(); - try (TestRestClient client = cluster.getRestClient()) { - HttpResponse response = client.getAuthInfo(Map.of("token", jwtTokenValue)); - - response.assertStatusCode(200); - String username = response.getTextFromJsonBody(POINTER_USERNAME); - assertThat(username, equalTo(USER_SUPERHERO)); - } - } - @Test public void shouldAuthenticateWithJwtToken_positiveWithAnotherUsername() { try (TestRestClient client = cluster.getRestClient(tokenFactory.generateValidToken(USERNAME_ROOT))) { From 161639b0268884d63c9d6c6d96c9da1ab55a6270 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 22 Feb 2024 11:19:56 -0500 Subject: [PATCH 18/20] Add generic way to define sensitive url params on http authenticator Signed-off-by: Craig Perkins --- .../dlic/auth/http/jwt/HTTPJwtAuthenticator.java | 10 ++++++++-- .../security/auditlog/impl/AbstractAuditLog.java | 8 +------- .../opensearch/security/auth/HTTPAuthenticator.java | 12 ++++++++++++ 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java b/src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java index f4b9770089..a6ff27eb6b 100644 --- a/src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java +++ b/src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java @@ -15,9 +15,11 @@ import java.security.AccessController; import java.security.PrivilegedAction; import java.util.Collection; +import java.util.Collections; import java.util.Map; import java.util.Map.Entry; import java.util.Optional; +import java.util.Set; import java.util.regex.Pattern; import org.apache.http.HttpStatus; @@ -194,8 +196,12 @@ public Optional reRequestAuthentication(final SecurityRequest ); } - public String getJwtUrlParameter() { - return jwtUrlParameter; + @Override + public Set getSensitiveUrlParams() { + if (jwtUrlParameter != null) { + return Set.of(jwtUrlParameter); + } + return Collections.emptySet(); } @Override diff --git a/src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java b/src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java index a36d94e75b..a5dd5290f6 100644 --- a/src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java +++ b/src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java @@ -78,7 +78,6 @@ import org.opensearch.threadpool.ThreadPool; import org.opensearch.transport.TransportRequest; -import com.amazon.dlic.auth.http.jwt.HTTPJwtAuthenticator; import com.flipkart.zjsonpatch.JsonDiff; import org.greenrobot.eventbus.Subscribe; @@ -946,12 +945,7 @@ public void onDynamicConfigModelChanged(DynamicConfigModel dcm) { SortedSet authDomains = Collections.unmodifiableSortedSet(dcm.getRestAuthDomains()); ignoredUrlParams.clear(); for (AuthDomain authDomain : authDomains) { - if ("jwt".equals(authDomain.getHttpAuthenticator().getType())) { - HTTPJwtAuthenticator jwtAuthenticator = (HTTPJwtAuthenticator) authDomain.getHttpAuthenticator(); - if (jwtAuthenticator.getJwtUrlParameter() != null) { - ignoredUrlParams.add(jwtAuthenticator.getJwtUrlParameter()); - } - } + ignoredUrlParams.addAll(authDomain.getHttpAuthenticator().getSensitiveUrlParams()); } } } diff --git a/src/main/java/org/opensearch/security/auth/HTTPAuthenticator.java b/src/main/java/org/opensearch/security/auth/HTTPAuthenticator.java index c79576ef5f..927dc0e286 100644 --- a/src/main/java/org/opensearch/security/auth/HTTPAuthenticator.java +++ b/src/main/java/org/opensearch/security/auth/HTTPAuthenticator.java @@ -26,7 +26,9 @@ package org.opensearch.security.auth; +import java.util.Collections; import java.util.Optional; +import java.util.Set; import org.opensearch.OpenSearchSecurityException; import org.opensearch.common.util.concurrent.ThreadContext; @@ -92,4 +94,14 @@ public interface HTTPAuthenticator { default boolean supportsImpersonation() { return true; } + + /** + * Returns a set of URL parameters this authenticator supports that are considered sensitive + * and should be redacted in the audit logs + * + * @return The set of URL parameters considered sensitive for this authenticator. + */ + default Set getSensitiveUrlParams() { + return Collections.emptySet(); + } } From e96f5b7b64e582e66da5548dfe87a645d4438c52 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 22 Feb 2024 12:02:12 -0500 Subject: [PATCH 19/20] Ensure that other url params stay unredacted Signed-off-by: Craig Perkins --- .../security/http/JwtAuthenticationWithUrlParamTests.java | 4 ++-- .../java/org/opensearch/security/auth/BackendRegistry.java | 3 +-- .../java/org/opensearch/security/rest/SecurityInfoAction.java | 3 +-- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationWithUrlParamTests.java b/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationWithUrlParamTests.java index 3964bb4552..e10ad82e8c 100644 --- a/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationWithUrlParamTests.java +++ b/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationWithUrlParamTests.java @@ -101,12 +101,12 @@ public void shouldAuthenticateWithJwtTokenInUrl_positive() { Header jwtToken = tokenFactory.generateValidToken(ADMIN_USER.getName()); String jwtTokenValue = jwtToken.getValue(); try (TestRestClient client = cluster.getRestClient()) { - HttpResponse response = client.getAuthInfo(Map.of(TOKEN_URL_PARAM, jwtTokenValue)); + HttpResponse response = client.getAuthInfo(Map.of(TOKEN_URL_PARAM, jwtTokenValue, "verbose", "true")); response.assertStatusCode(200); String username = response.getTextFromJsonBody(POINTER_USERNAME); assertThat(username, equalTo(ADMIN_USER.getName())); - Map expectedParams = Map.of("token", "REDACTED"); + Map expectedParams = Map.of("token", "REDACTED", "verbose", "true"); auditLogsRule.assertExactlyOne( userAuthenticated(ADMIN_USER).withRestRequest(GET, "/_opendistro/_security/authinfo").withRestParams(expectedParams) diff --git a/src/main/java/org/opensearch/security/auth/BackendRegistry.java b/src/main/java/org/opensearch/security/auth/BackendRegistry.java index 2e9922647f..3ab9a2afc9 100644 --- a/src/main/java/org/opensearch/security/auth/BackendRegistry.java +++ b/src/main/java/org/opensearch/security/auth/BackendRegistry.java @@ -77,6 +77,7 @@ public class BackendRegistry { protected final Logger log = LogManager.getLogger(this.getClass()); private SortedSet restAuthDomains; private Set restAuthorizers; + private List ipAuthFailureListeners; private Multimap authBackendFailureListeners; private List> ipClientBlockRegistries; @@ -180,8 +181,6 @@ public void onDynamicConfigModelChanged(DynamicConfigModel dcm) { ipClientBlockRegistries = dcm.getIpClientBlockRegistries(); authBackendClientBlockRegistries = dcm.getAuthBackendClientBlockRegistries(); - SortedSet authDomains = Collections.unmodifiableSortedSet(dcm.getRestAuthDomains()); - // OpenSearch Security no default authc initialized = !restAuthDomains.isEmpty() || anonymousAuthEnabled || injectedUserEnabled; } diff --git a/src/main/java/org/opensearch/security/rest/SecurityInfoAction.java b/src/main/java/org/opensearch/security/rest/SecurityInfoAction.java index 9300cf72f2..469c7f81b4 100644 --- a/src/main/java/org/opensearch/security/rest/SecurityInfoAction.java +++ b/src/main/java/org/opensearch/security/rest/SecurityInfoAction.java @@ -88,6 +88,7 @@ public List routes() { @Override protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { + final boolean verbose = request.paramAsBoolean("verbose", false); return new RestChannelConsumer() { @Override @@ -97,8 +98,6 @@ public void accept(RestChannel channel) throws Exception { try { - final boolean verbose = request.paramAsBoolean("verbose", false); - final X509Certificate[] certs = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_SSL_PEER_CERTIFICATES); final User user = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER); final TransportAddress remoteAddress = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS); From be70a9cde392a377d9f207face25c23a7fb26252 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 22 Feb 2024 13:21:15 -0500 Subject: [PATCH 20/20] Update expected payload Signed-off-by: Craig Perkins --- .../opensearch/security/dlic/rest/api/AuditApiActionTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/AuditApiActionTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/AuditApiActionTest.java index b3d916e8ed..92ce7c9112 100644 --- a/src/test/java/org/opensearch/security/dlic/rest/api/AuditApiActionTest.java +++ b/src/test/java/org/opensearch/security/dlic/rest/api/AuditApiActionTest.java @@ -682,7 +682,7 @@ private String getTestPayload() { + "\"enable_rest\":true,\"disabled_rest_categories\":[\"AUTHENTICATED\"]," + "\"enable_transport\":true,\"disabled_transport_categories\":[\"SSL_EXCEPTION\"]," + "\"resolve_bulk_requests\":true,\"log_request_body\":true,\"resolve_indices\":true,\"exclude_sensitive_headers\":true," - + "\"ignore_users\":[\"test-user-1\"],\"ignore_requests\":[\"test-request\"], \"ignore_headers\":[\"\"]}," + + "\"ignore_users\":[\"test-user-1\"],\"ignore_requests\":[\"test-request\"], \"ignore_headers\":[\"\"], \"ignore_url_params\":[]}," + "\"compliance\":{" + "\"enabled\":true," + "\"internal_config\":true,\"external_config\":true,"