diff --git a/CHANGELOG.md b/CHANGELOG.md index dbf8055648..b6651aba2a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,8 @@ 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)) - Improve array validator to also check for blank string in addition to null ([#5714](https://github.com/opensearch-project/security/pull/5714)) +- Use RestRequestFilter.getFilteredRequest to declare sensitive API params ([#5710](https://github.com/opensearch-project/security/pull/5710)) + ### Refactoring - [Resource Sharing] Make migrate api require default access level to be supplied and updates documentations + tests ([#5717](https://github.com/opensearch-project/security/pull/5717)) 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..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 @@ -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,11 @@ public void onFailure(Exception e) { @Override protected final RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { + 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, // not 400 consumeParameters(request); @@ -594,12 +600,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 +657,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 10934e9a57..9e686c33d6 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,11 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c } }); + RestRequest filteredRequest = maybeFilterRestRequest(request); + 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)) { @@ -177,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); @@ -187,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); @@ -196,9 +202,9 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c return; } - authorizeRequest(delegate, requestChannel, user); - if (requestChannel.getQueuedResponse().isPresent()) { - channel.sendResponse(requestChannel.getQueuedResponse().get().asRestResponse()); + authorizeRequest(delegate, filteredRequestChannel, user); + if (filteredRequestChannel.getQueuedResponse().isPresent()) { + channel.sendResponse(filteredRequestChannel.getQueuedResponse().get().asRestResponse()); return; } @@ -216,6 +222,14 @@ private void handleSuperAdminPermissionCheck(RestChannel channel) throws Excepti channel.sendResponse(new BytesRestResponse(RestStatus.OK, builder)); } + + 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 request; + } } /** 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..750df2d78c 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( + "/_opendistro/_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", "[{\"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)); } }