From 6b8a3e494c7933b93f6ee4f1a7956b728ed48a43 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 28 Nov 2023 14:22:17 -0500 Subject: [PATCH] Refactor RestApiComplianceAuditlogTest to use TestAuditLogImpl.doThenWaitForMessages (#3733) ### Description Refactors RestApiComplianceAuditlogTest to make this test suite more stable and re-enable an ignored test. The primary problem in this test suite is that messages that are auto populated on startup get polluted with other audit log messages generated when a user calls REST APIs. To make this more stable, a sleep was added after `setup` to ignore the auto init messages and only isolate messages generated on the API calls in each test. `testAutoInit` is also updated to make it stable. There are 4 automatic messages generated on cluster startup that have to do with the seeding of the security index and audit log index. * Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation) Test Fix ### Issues Resolved https://github.com/opensearch-project/security/issues/3730 ### Check List - [ ] New functionality includes testing - [ ] New functionality has been documented - [ ] Commits are signed per the DCO using --signoff By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check [here](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin). --------- Signed-off-by: Craig Perkins --- .../RestApiComplianceAuditlogTest.java | 211 +++++++++++------- 1 file changed, 134 insertions(+), 77 deletions(-) diff --git a/src/test/java/org/opensearch/security/auditlog/compliance/RestApiComplianceAuditlogTest.java b/src/test/java/org/opensearch/security/auditlog/compliance/RestApiComplianceAuditlogTest.java index e93229ee8b..784176e1dd 100644 --- a/src/test/java/org/opensearch/security/auditlog/compliance/RestApiComplianceAuditlogTest.java +++ b/src/test/java/org/opensearch/security/auditlog/compliance/RestApiComplianceAuditlogTest.java @@ -11,18 +11,26 @@ package org.opensearch.security.auditlog.compliance; +import java.util.List; +import java.util.stream.Collectors; + import org.apache.http.HttpStatus; import org.junit.Assert; -import org.junit.Ignore; import org.junit.Test; import org.opensearch.common.settings.Settings; import org.opensearch.security.auditlog.AbstractAuditlogiUnitTest; +import org.opensearch.security.auditlog.impl.AuditCategory; import org.opensearch.security.auditlog.impl.AuditMessage; import org.opensearch.security.auditlog.integration.TestAuditlogImpl; import org.opensearch.security.support.ConfigConstants; +import org.opensearch.security.test.helper.cluster.ClusterConfiguration; import org.opensearch.security.test.helper.rest.RestHelper.HttpResponse; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; + public class RestApiComplianceAuditlogTest extends AbstractAuditlogiUnitTest { @Test @@ -40,22 +48,19 @@ public void testRestApiRolesEnabled() throws Exception { .put(ConfigConstants.OPENDISTRO_SECURITY_AUDIT_CONFIG_DISABLED_REST_CATEGORIES, "authenticated,GRANTED_PRIVILEGES") .build(); - setup(additionalSettings); - TestAuditlogImpl.clear(); - String body = "{ \"password\":\"some new password\",\"backend_roles\":[\"role1\",\"role2\"] }"; - HttpResponse response = rh.executePutRequest( - "_opendistro/_security/api/internalusers/compuser?pretty", - body, - encodeBasicHeader("admin", "admin") - ); - Thread.sleep(1500); - Assert.assertEquals(HttpStatus.SC_CREATED, response.getStatusCode()); - Assert.assertTrue(TestAuditlogImpl.messages.size() + "", TestAuditlogImpl.messages.size() == 1); - Assert.assertTrue(TestAuditlogImpl.sb.toString().contains("audit_request_effective_user")); - Assert.assertFalse(TestAuditlogImpl.sb.toString().contains("COMPLIANCE_INTERNAL_CONFIG_READ")); - Assert.assertTrue(TestAuditlogImpl.sb.toString().contains("COMPLIANCE_INTERNAL_CONFIG_WRITE")); - Assert.assertTrue(TestAuditlogImpl.sb.toString().contains("UPDATE")); - validateMsgs(TestAuditlogImpl.messages); + setupAndReturnAuditMessages(additionalSettings); + final AuditMessage message = TestAuditlogImpl.doThenWaitForMessage(() -> { + String body = "{ \"password\":\"some new password\",\"backend_roles\":[\"role1\",\"role2\"] }"; + HttpResponse response = rh.executePutRequest( + "_opendistro/_security/api/internalusers/compuser?pretty", + body, + encodeBasicHeader("admin", "admin") + ); + Assert.assertEquals(HttpStatus.SC_CREATED, response.getStatusCode()); + }); + validateMsgs(List.of(message)); + + assertThat(message.toString(), containsString("UPDATE")); } @Test @@ -72,8 +77,7 @@ public void testRestApiRolesDisabled() throws Exception { .put(ConfigConstants.OPENDISTRO_SECURITY_AUDIT_CONFIG_DISABLED_REST_CATEGORIES, "authenticated,GRANTED_PRIVILEGES") .build(); - setup(additionalSettings); - TestAuditlogImpl.clear(); + setupAndReturnAuditMessages(additionalSettings); String body = "{ \"password\":\"some new password\",\"backend_roles\":[\"role1\",\"role2\"] }"; rh.enableHTTPClientSSL = true; @@ -81,19 +85,15 @@ public void testRestApiRolesDisabled() throws Exception { rh.sendAdminCertificate = true; rh.keystore = "kirk-keystore.jks"; - HttpResponse response = rh.executePutRequest("_opendistro/_security/api/internalusers/compuser?pretty", body); - Thread.sleep(1500); - Assert.assertEquals(HttpStatus.SC_CREATED, response.getStatusCode()); - Assert.assertTrue(TestAuditlogImpl.messages.size() + "", TestAuditlogImpl.messages.size() == 1); - Assert.assertTrue(TestAuditlogImpl.sb.toString().contains("audit_request_effective_user")); - Assert.assertFalse(TestAuditlogImpl.sb.toString().contains("COMPLIANCE_INTERNAL_CONFIG_READ")); - Assert.assertTrue(TestAuditlogImpl.sb.toString().contains("COMPLIANCE_INTERNAL_CONFIG_WRITE")); - Assert.assertTrue(TestAuditlogImpl.sb.toString().contains("UPDATE")); - validateMsgs(TestAuditlogImpl.messages); + final AuditMessage message = TestAuditlogImpl.doThenWaitForMessage(() -> { + HttpResponse response = rh.executePutRequest("_opendistro/_security/api/internalusers/compuser?pretty", body); + Assert.assertEquals(HttpStatus.SC_CREATED, response.getStatusCode()); + }); + validateMsgs(List.of(message)); + assertThat(message.toString(), containsString("COMPLIANCE_INTERNAL_CONFIG_WRITE")); } @Test - @Ignore public void testRestApiRolesDisabledGet() throws Exception { Settings additionalSettings = Settings.builder() @@ -107,22 +107,42 @@ public void testRestApiRolesDisabledGet() throws Exception { .put(ConfigConstants.OPENDISTRO_SECURITY_AUDIT_CONFIG_DISABLED_REST_CATEGORIES, "authenticated,GRANTED_PRIVILEGES") .build(); - setup(additionalSettings); - TestAuditlogImpl.clear(); + setupAndReturnAuditMessages(additionalSettings); rh.enableHTTPClientSSL = true; rh.trustHTTPServerCertificate = true; rh.sendAdminCertificate = true; rh.keystore = "kirk-keystore.jks"; - HttpResponse response = rh.executeGetRequest("_opendistro/_security/api/rolesmapping/opendistro_security_all_access?pretty"); - Thread.sleep(1500); - Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode()); - Assert.assertTrue(TestAuditlogImpl.messages.size() > 2); - Assert.assertTrue(TestAuditlogImpl.sb.toString().contains("audit_request_effective_user")); - Assert.assertTrue(TestAuditlogImpl.sb.toString().contains("COMPLIANCE_INTERNAL_CONFIG_READ")); - Assert.assertTrue(TestAuditlogImpl.sb.toString().contains("COMPLIANCE_INTERNAL_CONFIG_WRITE")); - Assert.assertTrue(TestAuditlogImpl.sb.toString().contains("UPDATE")); - validateMsgs(TestAuditlogImpl.messages); + final AuditMessage message = TestAuditlogImpl.doThenWaitForMessage(() -> { + HttpResponse response = rh.executeGetRequest("_opendistro/_security/api/rolesmapping/opendistro_security_all_access?pretty"); + Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode()); + }); + validateMsgs(List.of(message)); + assertThat(message.toString(), containsString("audit_request_effective_user")); + assertThat(message.toString(), containsString("COMPLIANCE_INTERNAL_CONFIG_READ")); + } + + @Test + public void testAutoInit() throws Exception { + + Settings additionalSettings = Settings.builder() + .put("plugins.security.audit.type", TestAuditlogImpl.class.getName()) + .put(ConfigConstants.OPENDISTRO_SECURITY_AUDIT_ENABLE_TRANSPORT, true) + .put(ConfigConstants.OPENDISTRO_SECURITY_AUDIT_ENABLE_REST, true) + .put(ConfigConstants.OPENDISTRO_SECURITY_AUDIT_RESOLVE_BULK_REQUESTS, false) + .put(ConfigConstants.OPENDISTRO_SECURITY_COMPLIANCE_HISTORY_EXTERNAL_CONFIG_ENABLED, true) + .put(ConfigConstants.SECURITY_COMPLIANCE_HISTORY_INTERNAL_CONFIG_ENABLED, true) + .put(ConfigConstants.OPENDISTRO_SECURITY_AUDIT_CONFIG_DISABLED_TRANSPORT_CATEGORIES, "authenticated,GRANTED_PRIVILEGES") + .put(ConfigConstants.OPENDISTRO_SECURITY_AUDIT_CONFIG_DISABLED_REST_CATEGORIES, "authenticated,GRANTED_PRIVILEGES") + .build(); + + final List messages = setupAndReturnAuditMessages(additionalSettings); + + validateMsgs(messages); + String allMessages = messages.stream().map(AuditMessage::toString).collect(Collectors.joining(",")); + assertThat(allMessages, containsString("audit_request_effective_user")); + assertThat(allMessages, containsString("COMPLIANCE_INTERNAL_CONFIG_WRITE")); + assertThat(allMessages, containsString("COMPLIANCE_EXTERNAL_CONFIG")); } @Test @@ -136,20 +156,22 @@ public void testRestApiNewUser() throws Exception { .put(ConfigConstants.OPENDISTRO_SECURITY_AUDIT_RESOLVE_BULK_REQUESTS, false) .put(ConfigConstants.OPENDISTRO_SECURITY_COMPLIANCE_HISTORY_EXTERNAL_CONFIG_ENABLED, false) .put(ConfigConstants.SECURITY_COMPLIANCE_HISTORY_INTERNAL_CONFIG_ENABLED, true) - .put(ConfigConstants.OPENDISTRO_SECURITY_COMPLIANCE_HISTORY_WRITE_IGNORE_USERS, "admin") .build(); - setup(additionalSettings); - TestAuditlogImpl.clear(); - String body = "{ \"password\":\"some new password\",\"backend_roles\":[\"role1\",\"role2\"] }"; - HttpResponse response = rh.executePutRequest( - "_opendistro/_security/api/internalusers/compuser?pretty", - body, - encodeBasicHeader("admin", "admin") - ); - Thread.sleep(1500); - Assert.assertEquals(response.getBody(), HttpStatus.SC_CREATED, response.getStatusCode()); - Assert.assertTrue(TestAuditlogImpl.messages.size() + "", TestAuditlogImpl.messages.isEmpty()); + setupAndReturnAuditMessages(additionalSettings); + + final AuditMessage message = TestAuditlogImpl.doThenWaitForMessage(() -> { + String body = "{ \"password\":\"some new password\",\"backend_roles\":[\"role1\",\"role2\"] }"; + HttpResponse response = rh.executePutRequest( + "_opendistro/_security/api/internalusers/compuser?pretty", + body, + encodeBasicHeader("admin", "admin") + ); + Assert.assertEquals(response.getBody(), HttpStatus.SC_CREATED, response.getStatusCode()); + }); + validateMsgs(List.of(message)); + assertThat(message.toString(), containsString("audit_request_effective_user")); + assertThat(message.toString(), containsString("COMPLIANCE_INTERNAL_CONFIG_WRITE")); } @Test @@ -167,23 +189,21 @@ public void testRestInternalConfigRead() throws Exception { .put(ConfigConstants.OPENDISTRO_SECURITY_AUDIT_CONFIG_DISABLED_REST_CATEGORIES, "authenticated,GRANTED_PRIVILEGES") .build(); - setup(additionalSettings); - TestAuditlogImpl.clear(); + setupAndReturnAuditMessages(additionalSettings); rh.enableHTTPClientSSL = true; rh.trustHTTPServerCertificate = true; rh.sendAdminCertificate = true; rh.keystore = "kirk-keystore.jks"; - HttpResponse response = rh.executeGetRequest("_opendistro/_security/api/internalusers/admin?pretty"); - Thread.sleep(1500); - String auditLogImpl = TestAuditlogImpl.sb.toString(); - Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode()); - Assert.assertTrue(TestAuditlogImpl.messages.size() + "", TestAuditlogImpl.messages.size() == 1); - Assert.assertTrue(auditLogImpl.contains("audit_request_effective_user")); - Assert.assertTrue(auditLogImpl.contains("COMPLIANCE_INTERNAL_CONFIG_READ")); - Assert.assertFalse(auditLogImpl.contains("COMPLIANCE_INTERNAL_CONFIG_WRITE")); - Assert.assertFalse(auditLogImpl.contains("UPDATE")); - validateMsgs(TestAuditlogImpl.messages); + + final AuditMessage message = TestAuditlogImpl.doThenWaitForMessage(() -> { + HttpResponse response = rh.executeGetRequest("_opendistro/_security/api/internalusers/admin?pretty"); + String auditLogImpl = TestAuditlogImpl.sb.toString(); + Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode()); + Assert.assertTrue(auditLogImpl.contains("COMPLIANCE_INTERNAL_CONFIG_READ")); + }); + validateMsgs(List.of(message)); + assertThat(message.toString(), containsString("COMPLIANCE_INTERNAL_CONFIG_READ")); } @Test @@ -196,26 +216,63 @@ public void testBCryptHashRedaction() throws Exception { .put(ConfigConstants.SECURITY_COMPLIANCE_HISTORY_INTERNAL_CONFIG_ENABLED, true) .put(ConfigConstants.OPENDISTRO_SECURITY_COMPLIANCE_HISTORY_WRITE_LOG_DIFFS, true) .build(); - setup(settings); + setupAndReturnAuditMessages(settings); rh.sendAdminCertificate = true; rh.keystore = "kirk-keystore.jks"; // read internal users and verify no BCrypt hash is present in audit logs - TestAuditlogImpl.clear(); - rh.executeGetRequest("/_opendistro/_security/api/internalusers"); - Assert.assertEquals(1, TestAuditlogImpl.messages.size()); - Assert.assertFalse(AuditMessage.BCRYPT_HASH.matcher(TestAuditlogImpl.sb.toString()).matches()); + final AuditMessage message1 = TestAuditlogImpl.doThenWaitForMessage(() -> { + rh.executeGetRequest("/_opendistro/_security/api/internalusers"); + }); + + Assert.assertFalse(AuditMessage.BCRYPT_HASH.matcher(message1.toString()).matches()); // read internal user worf and verify no BCrypt hash is present in audit logs - TestAuditlogImpl.clear(); - rh.executeGetRequest("/_opendistro/_security/api/internalusers/worf"); - Assert.assertEquals(1, TestAuditlogImpl.messages.size()); - Assert.assertFalse(AuditMessage.BCRYPT_HASH.matcher(TestAuditlogImpl.sb.toString()).matches()); + final AuditMessage message2 = TestAuditlogImpl.doThenWaitForMessage(() -> { + rh.executeGetRequest("/_opendistro/_security/api/internalusers/worf"); + Assert.assertFalse(AuditMessage.BCRYPT_HASH.matcher(TestAuditlogImpl.sb.toString()).matches()); + }); + + Assert.assertFalse(AuditMessage.BCRYPT_HASH.matcher(message2.toString()).matches()); // create internal user and verify no BCrypt hash is present in audit logs - TestAuditlogImpl.clear(); - rh.executePutRequest("/_opendistro/_security/api/internalusers/test", "{ \"password\":\"some new user password\"}"); - Assert.assertEquals(1, TestAuditlogImpl.messages.size()); - Assert.assertFalse(AuditMessage.BCRYPT_HASH.matcher(TestAuditlogImpl.sb.toString()).matches()); + final AuditMessage message3 = TestAuditlogImpl.doThenWaitForMessage(() -> { + rh.executePutRequest("/_opendistro/_security/api/internalusers/test", "{ \"password\":\"some new user password\"}"); + }); + + Assert.assertFalse(AuditMessage.BCRYPT_HASH.matcher(message3.toString()).matches()); + } + + private List setupAndReturnAuditMessages(Settings settings) { + // When OPENDISTRO_SECURITY_COMPLIANCE_HISTORY_EXTERNAL_CONFIG_ENABLED is set to true, there is: + // - 1 message with COMPLIANCE_INTERNAL_CONFIG_WRITE as category. + // - 1 message with COMPLIANCE_EXTERNAL_CONFIG as category for each node. + int numNodes = ClusterConfiguration.DEFAULT.getNodes(); + boolean externalConfigEnabled = settings.getAsBoolean( + ConfigConstants.OPENDISTRO_SECURITY_COMPLIANCE_HISTORY_EXTERNAL_CONFIG_ENABLED, + false + ); + int expectedMessageCount = externalConfigEnabled ? (numNodes + 1) : 1; + final List messages = TestAuditlogImpl.doThenWaitForMessages(() -> { + try { + setup(settings); + } catch (final Exception ex) { + throw new RuntimeException(ex); + } + }, expectedMessageCount); + int complianceInternalConfigWriteCount = 0; + int complianceExternalConfigCount = 0; + for (AuditMessage message : messages) { + if (AuditCategory.COMPLIANCE_INTERNAL_CONFIG_WRITE.equals(message.getCategory())) { + complianceInternalConfigWriteCount++; + } else if (AuditCategory.COMPLIANCE_EXTERNAL_CONFIG.equals(message.getCategory())) { + complianceExternalConfigCount++; + } + } + assertThat(complianceInternalConfigWriteCount, equalTo(1)); + if (externalConfigEnabled) { + assertThat(complianceExternalConfigCount, equalTo(numNodes)); + } + return messages; } }