From 091fa97fefdafe8a2d3f67ef91844290e7be13e2 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Mon, 13 Oct 2025 09:39:01 -0400 Subject: [PATCH 1/6] Use getFilteredRequest to omit sensitive values Signed-off-by: Craig Perkins --- .../security/auditlog/impl/AuditMessage.java | 18 +----------------- .../security/filter/SecurityRestFilter.java | 13 ++++++++++++- 2 files changed, 13 insertions(+), 18 deletions(-) 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 81b701bd7f..0c22182c2b 100644 --- a/src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java +++ b/src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java @@ -56,21 +56,12 @@ import org.joda.time.format.DateTimeFormat; import org.joda.time.format.DateTimeFormatter; -import static org.opensearch.security.OpenSearchSecurityPlugin.LEGACY_OPENDISTRO_PREFIX; -import static org.opensearch.security.OpenSearchSecurityPlugin.PLUGINS_PREFIX; - public final class AuditMessage { private static final Logger log = LogManager.getLogger(AuditMessage.class); // clustername and cluster uuid private static final WildcardMatcher AUTHORIZATION_HEADER = WildcardMatcher.from("Authorization").ignoreCase(); - private static final String SENSITIVE_KEY = "password"; - private static final String SENSITIVE_REPLACEMENT_VALUE = "__SENSITIVE__"; - - private static final Pattern SENSITIVE_PATHS = Pattern.compile( - "/(" + LEGACY_OPENDISTRO_PREFIX + "|" + PLUGINS_PREFIX + ")/api/(account.*|internalusers.*|user.*)" - ); @VisibleForTesting public static final String BCRYPT_REGEX = "\\$2[ayb]\\$.{56}"; @@ -417,14 +408,7 @@ void addRestRequestInfo(final SecurityRequest request, final AuditConfig.Filter try { final Tuple xContentTuple = restRequest.contentOrSourceParam(); final String requestBody = XContentHelper.convertToJson(xContentTuple.v2(), false, xContentTuple.v1()); - if (path != null - && requestBody != null - && SENSITIVE_PATHS.matcher(path).matches() - && requestBody.contains(SENSITIVE_KEY)) { - auditInfo.put(REQUEST_BODY, SENSITIVE_REPLACEMENT_VALUE); - } else { - auditInfo.put(REQUEST_BODY, requestBody); - } + auditInfo.put(REQUEST_BODY, requestBody); } catch (Exception e) { auditInfo.put(REQUEST_BODY, "ERROR: Unable to generate request body"); log.error("Error while generating request body for audit log", e); diff --git a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java index 10934e9a57..35f62c3fe0 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java +++ b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java @@ -26,6 +26,7 @@ package org.opensearch.security.filter; +import java.io.IOException; import java.nio.file.Path; import java.util.Collections; import java.util.List; @@ -49,6 +50,7 @@ import org.opensearch.rest.RestChannel; import org.opensearch.rest.RestHandler; import org.opensearch.rest.RestRequest; +import org.opensearch.rest.RestRequestFilter; import org.opensearch.security.auditlog.AuditLog; import org.opensearch.security.auditlog.AuditLog.Origin; import org.opensearch.security.auth.BackendRegistry; @@ -156,7 +158,9 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c } }); - final SecurityRequestChannel requestChannel = SecurityRequestFactory.from(request, channel); + RestRequest filteredRequest = maybeFilterRestRequest(request); + + final SecurityRequestChannel requestChannel = SecurityRequestFactory.from(filteredRequest, channel); // Authenticate request if (!NettyAttribute.popFrom(request, Netty4HttpRequestHeaderVerifier.IS_AUTHENTICATED).orElse(false)) { @@ -216,6 +220,13 @@ private void handleSuperAdminPermissionCheck(RestChannel channel) throws Excepti channel.sendResponse(new BytesRestResponse(RestStatus.OK, builder)); } + + RestRequest maybeFilterRestRequest(RestRequest restRequest) throws IOException { + if (delegate instanceof RestRequestFilter) { + return ((RestRequestFilter) delegate).getFilteredRequest(restRequest); + } + return restRequest; + } } /** From d7165a0dafc8835c7582d0314526f2e6d57836d1 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Mon, 13 Oct 2025 13:08:44 -0400 Subject: [PATCH 2/6] Use RestRequestFilter to filter out sensitive params Signed-off-by: Craig Perkins --- .../security/auditlog/impl/AuditMessage.java | 18 ++++++++++- .../dlic/rest/api/AbstractApiAction.java | 16 ++++++++-- .../security/filter/SecurityRestFilter.java | 10 ++++--- .../integration/BasicAuditlogTest.java | 30 ++++++++++++------- 4 files changed, 56 insertions(+), 18 deletions(-) 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 0c22182c2b..81b701bd7f 100644 --- a/src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java +++ b/src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java @@ -56,12 +56,21 @@ import org.joda.time.format.DateTimeFormat; import org.joda.time.format.DateTimeFormatter; +import static org.opensearch.security.OpenSearchSecurityPlugin.LEGACY_OPENDISTRO_PREFIX; +import static org.opensearch.security.OpenSearchSecurityPlugin.PLUGINS_PREFIX; + public final class AuditMessage { private static final Logger log = LogManager.getLogger(AuditMessage.class); // clustername and cluster uuid private static final WildcardMatcher AUTHORIZATION_HEADER = WildcardMatcher.from("Authorization").ignoreCase(); + private static final String SENSITIVE_KEY = "password"; + private static final String SENSITIVE_REPLACEMENT_VALUE = "__SENSITIVE__"; + + private static final Pattern SENSITIVE_PATHS = Pattern.compile( + "/(" + LEGACY_OPENDISTRO_PREFIX + "|" + PLUGINS_PREFIX + ")/api/(account.*|internalusers.*|user.*)" + ); @VisibleForTesting public static final String BCRYPT_REGEX = "\\$2[ayb]\\$.{56}"; @@ -408,7 +417,14 @@ void addRestRequestInfo(final SecurityRequest request, final AuditConfig.Filter try { final Tuple xContentTuple = restRequest.contentOrSourceParam(); final String requestBody = XContentHelper.convertToJson(xContentTuple.v2(), false, xContentTuple.v1()); - auditInfo.put(REQUEST_BODY, requestBody); + if (path != null + && requestBody != null + && SENSITIVE_PATHS.matcher(path).matches() + && requestBody.contains(SENSITIVE_KEY)) { + auditInfo.put(REQUEST_BODY, SENSITIVE_REPLACEMENT_VALUE); + } else { + auditInfo.put(REQUEST_BODY, requestBody); + } } catch (Exception e) { auditInfo.put(REQUEST_BODY, "ERROR: Unable to generate request body"); log.error("Error while generating request body for audit log", e); diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java index d235e53680..a36b0b85f5 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java +++ b/src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java @@ -49,6 +49,7 @@ import org.opensearch.rest.RestChannel; import org.opensearch.rest.RestRequest; import org.opensearch.rest.RestRequest.Method; +import org.opensearch.rest.RestRequestFilter; import org.opensearch.security.action.configupdate.ConfigUpdateAction; import org.opensearch.security.action.configupdate.ConfigUpdateRequest; import org.opensearch.security.action.configupdate.ConfigUpdateResponse; @@ -79,7 +80,7 @@ import static org.opensearch.security.dlic.rest.api.Responses.payload; import static org.opensearch.security.dlic.rest.support.Utils.withIOException; -public abstract class AbstractApiAction extends BaseRestHandler { +public abstract class AbstractApiAction extends BaseRestHandler implements RestRequestFilter { private final static Logger LOGGER = LogManager.getLogger(AbstractApiAction.class); @@ -578,6 +579,10 @@ public void onFailure(Exception e) { @Override protected final RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { + RestRequest filteredRequest = getFilteredRequest(request); + + RestRequest auditLogRequest = (request.method() != Method.PATCH) ? filteredRequest : request; + // consume all parameters first so we can return a correct HTTP status, // not 400 consumeParameters(request); @@ -594,12 +599,12 @@ protected final RestChannelConsumer prepareRequest(RestRequest request, NodeClie final String userName = user == null ? null : user.getName(); if (authError != null) { LOGGER.error("No permission to access REST API: " + authError); - securityApiDependencies.auditLog().logMissingPrivileges(authError, userName, SecurityRequestFactory.from(request)); + securityApiDependencies.auditLog().logMissingPrivileges(authError, userName, SecurityRequestFactory.from(auditLogRequest)); // for rest request request.params().clear(); return channel -> forbidden(channel, "No permission to access REST API: " + authError); } else { - securityApiDependencies.auditLog().logGrantedPrivileges(userName, SecurityRequestFactory.from(request)); + securityApiDependencies.auditLog().logGrantedPrivileges(userName, SecurityRequestFactory.from(auditLogRequest)); } final var originalUserAndRemoteAddress = Utils.userAndRemoteAddressFrom(threadPool.getThreadContext()); @@ -651,4 +656,9 @@ public boolean canTripCircuitBreaker() { return false; } + @Override + public Set getFilteredFields() { + return Set.of("password", "current_password"); + } + } diff --git a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java index 35f62c3fe0..5b791e1471 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java +++ b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java @@ -160,7 +160,9 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c RestRequest filteredRequest = maybeFilterRestRequest(request); - final SecurityRequestChannel requestChannel = SecurityRequestFactory.from(filteredRequest, channel); + final SecurityRequestChannel requestChannel = SecurityRequestFactory.from(request, channel); + // for audit logging + final SecurityRequestChannel filteredRequestChannel = SecurityRequestFactory.from(filteredRequest, channel); // Authenticate request if (!NettyAttribute.popFrom(request, Netty4HttpRequestHeaderVerifier.IS_AUTHENTICATED).orElse(false)) { @@ -181,7 +183,7 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c String intiatingUser = threadContext.getTransient(OPENDISTRO_SECURITY_INITIATING_USER); if (userIsSuperAdmin(user, adminDNs)) { // Super admins are always authorized - auditLog.logSucceededLogin(user.getName(), true, intiatingUser, requestChannel); + auditLog.logSucceededLogin(user.getName(), true, intiatingUser, filteredRequestChannel); if (performPermissionCheck) { log.debug("Permission check skipped: Super admin has full access"); handleSuperAdminPermissionCheck(channel); @@ -191,7 +193,7 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c return; } if (user != null) { - auditLog.logSucceededLogin(user.getName(), false, intiatingUser, requestChannel); + auditLog.logSucceededLogin(user.getName(), false, intiatingUser, filteredRequestChannel); } final Optional deniedResponse = allowlistingSettings.checkRequestIsAllowed(requestChannel); @@ -200,7 +202,7 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c return; } - authorizeRequest(delegate, requestChannel, user); + authorizeRequest(delegate, filteredRequestChannel, user); if (requestChannel.getQueuedResponse().isPresent()) { channel.sendResponse(requestChannel.getQueuedResponse().get().asRestResponse()); return; diff --git a/src/test/java/org/opensearch/security/auditlog/integration/BasicAuditlogTest.java b/src/test/java/org/opensearch/security/auditlog/integration/BasicAuditlogTest.java index 319c455a40..e3c6246c7b 100644 --- a/src/test/java/org/opensearch/security/auditlog/integration/BasicAuditlogTest.java +++ b/src/test/java/org/opensearch/security/auditlog/integration/BasicAuditlogTest.java @@ -962,40 +962,50 @@ public void testSensitiveMethodRedaction() throws Exception { .put(ConfigConstants.OPENDISTRO_SECURITY_AUDIT_ENABLE_TRANSPORT, false) .build(); setup(settings); - rh.sendAdminCertificate = true; - final String expectedRequestBody = "\"audit_request_body\" : \"__SENSITIVE__\""; + final String expectedChangePasswordRequestBody = "{}"; // test PUT accounts API TestAuditlogImpl.clear(); - rh.executePutRequest("/_opendistro/_security/api/account", "{\"password\":\"new-pass\", \"current_password\":\"curr-passs\"}"); + rh.sendAdminCertificate = false; + final Header adminHeader = encodeBasicHeader("admin", "admin"); + rh.executePutRequest( + "/_plugins/_security/api/account", + "{\"password\":\"myNewPassword123!\", \"current_password\":\"myCurrentPassword123!\"}", + adminHeader + ); assertThat(TestAuditlogImpl.messages.size(), is(1)); - Assert.assertTrue(TestAuditlogImpl.sb.toString().contains(expectedRequestBody)); + Assert.assertTrue(TestAuditlogImpl.sb.toString().contains(expectedChangePasswordRequestBody)); + final String expectedUpdateUserRequestBody = "{\\\"backend_roles\\\":[],\\\"attributes\\\":{}}"; // test PUT internal users API TestAuditlogImpl.clear(); rh.executePutRequest( "/_opendistro/_security/api/internalusers/test1", - "{\"password\":\"new-pass\", \"backend_roles\":[], \"attributes\": {}}" + "{\"password\":\"new-pass\", \"backend_roles\":[], \"attributes\": {}}", + adminHeader ); assertThat(TestAuditlogImpl.messages.size(), is(1)); - Assert.assertTrue(TestAuditlogImpl.sb.toString().contains(expectedRequestBody)); + Assert.assertTrue(TestAuditlogImpl.sb.toString().contains(expectedUpdateUserRequestBody)); // test PATCH internal users API + rh.sendAdminCertificate = true; TestAuditlogImpl.clear(); rh.executePatchRequest( - "/_opendistro/_security/api/internalusers/test1", + "/_plugins/_security/api/internalusers/test1", "[{\"op\":\"add\", \"path\":\"/password\", \"value\": \"test-pass\"}]" ); assertThat(TestAuditlogImpl.messages.size(), is(1)); - Assert.assertTrue(TestAuditlogImpl.sb.toString().contains(expectedRequestBody)); + Assert.assertTrue(TestAuditlogImpl.sb.toString().contains("__SENSITIVE__")); // test PUT users API + rh.sendAdminCertificate = false; TestAuditlogImpl.clear(); rh.executePutRequest( "/_opendistro/_security/api/user/test2", - "{\"password\":\"new-pass\", \"backend_roles\":[], \"attributes\": {}}" + "{\"password\":\"new-pass\", \"backend_roles\":[], \"attributes\": {}}", + adminHeader ); assertThat(TestAuditlogImpl.messages.size(), is(1)); - Assert.assertTrue(TestAuditlogImpl.sb.toString().contains(expectedRequestBody)); + Assert.assertTrue(TestAuditlogImpl.sb.toString().contains(expectedUpdateUserRequestBody)); } } From 75463993794715103122c9e9c48677e21ce58ff8 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Mon, 13 Oct 2025 16:00:27 -0400 Subject: [PATCH 3/6] Fix authz in rest layer tests Signed-off-by: Craig Perkins --- .../org/opensearch/security/filter/SecurityRestFilter.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java index 5b791e1471..09fad21067 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java +++ b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java @@ -203,8 +203,8 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c } authorizeRequest(delegate, filteredRequestChannel, user); - if (requestChannel.getQueuedResponse().isPresent()) { - channel.sendResponse(requestChannel.getQueuedResponse().get().asRestResponse()); + if (filteredRequestChannel.getQueuedResponse().isPresent()) { + channel.sendResponse(filteredRequestChannel.getQueuedResponse().get().asRestResponse()); return; } From 00761ce3942d75e70a5c32164c8a3b2b301055ff Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Mon, 13 Oct 2025 16:30:40 -0400 Subject: [PATCH 4/6] Use opendistro endpoint like before Signed-off-by: Craig Perkins --- .../security/auditlog/integration/BasicAuditlogTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/java/org/opensearch/security/auditlog/integration/BasicAuditlogTest.java b/src/test/java/org/opensearch/security/auditlog/integration/BasicAuditlogTest.java index e3c6246c7b..750df2d78c 100644 --- a/src/test/java/org/opensearch/security/auditlog/integration/BasicAuditlogTest.java +++ b/src/test/java/org/opensearch/security/auditlog/integration/BasicAuditlogTest.java @@ -969,7 +969,7 @@ public void testSensitiveMethodRedaction() throws Exception { rh.sendAdminCertificate = false; final Header adminHeader = encodeBasicHeader("admin", "admin"); rh.executePutRequest( - "/_plugins/_security/api/account", + "/_opendistro/_security/api/account", "{\"password\":\"myNewPassword123!\", \"current_password\":\"myCurrentPassword123!\"}", adminHeader ); @@ -991,7 +991,7 @@ public void testSensitiveMethodRedaction() throws Exception { rh.sendAdminCertificate = true; TestAuditlogImpl.clear(); rh.executePatchRequest( - "/_plugins/_security/api/internalusers/test1", + "/_opendistro/_security/api/internalusers/test1", "[{\"op\":\"add\", \"path\":\"/password\", \"value\": \"test-pass\"}]" ); assertThat(TestAuditlogImpl.messages.size(), is(1)); From 185d8b4987613bf2a48f209620067a57117a8823 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Mon, 13 Oct 2025 16:36:48 -0400 Subject: [PATCH 5/6] Add CHANGELOG entry Signed-off-by: Craig Perkins --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b321857f17..feb125077d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ### Bug Fixes - Create a WildcardMatcher.NONE when creating a WildcardMatcher with an empty string ([#5694](https://github.com/opensearch-project/security/pull/5694)) +- Use RestRequestFilter.getFilteredRequest to declare sensitive API params ([#5710](https://github.com/opensearch-project/security/pull/5710)) ### Refactoring From 3cfbbb283c1c702e532be3afad94562e4cf7e184 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 15 Oct 2025 14:15:08 -0400 Subject: [PATCH 6/6] Add comment about PATCH Signed-off-by: Craig Perkins --- .../security/dlic/rest/api/AbstractApiAction.java | 1 + .../opensearch/security/filter/SecurityRestFilter.java | 9 +++++---- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java index a36b0b85f5..c24705e260 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java +++ b/src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java @@ -581,6 +581,7 @@ protected final RestChannelConsumer prepareRequest(RestRequest request, NodeClie RestRequest filteredRequest = getFilteredRequest(request); + // Skip PATCH because filtering only supports JSON object bodies, not arrays. RestRequest auditLogRequest = (request.method() != Method.PATCH) ? filteredRequest : request; // consume all parameters first so we can return a correct HTTP status, diff --git a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java index 09fad21067..9e686c33d6 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java +++ b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java @@ -223,11 +223,12 @@ private void handleSuperAdminPermissionCheck(RestChannel channel) throws Excepti channel.sendResponse(new BytesRestResponse(RestStatus.OK, builder)); } - RestRequest maybeFilterRestRequest(RestRequest restRequest) throws IOException { - if (delegate instanceof RestRequestFilter) { - return ((RestRequestFilter) delegate).getFilteredRequest(restRequest); + RestRequest maybeFilterRestRequest(RestRequest request) throws IOException { + // Skip PATCH because filtering only supports JSON object bodies, not arrays. + if (delegate instanceof RestRequestFilter && (request.method() != RestRequest.Method.PATCH)) { + return ((RestRequestFilter) delegate).getFilteredRequest(request); } - return restRequest; + return request; } }