From 3cbe6d533940536299464795587e2e1ef5e33835 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Mon, 24 Oct 2022 12:34:39 -0500 Subject: [PATCH 1/4] Prevent message collection from being updated after message count has been received (#2180) Also adds mechanism to detect if messages were missed so tests can be updated to appropriate counts. Signed-off-by: Peter Nied (cherry picked from commit ba9d82ef6a2c6da137c19fcaaddce6dabcf7160f) --- .../security/auditlog/impl/AuditMessage.java | 26 ++++-- .../compliance/ComplianceAuditlogTest.java | 80 +++++++++++-------- .../integration/BasicAuditlogTest.java | 8 +- .../integration/TestAuditlogImpl.java | 62 ++++++++++---- 4 files changed, 117 insertions(+), 59 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 7afe7eb5a6..6e17d073db 100644 --- a/src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java +++ b/src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java @@ -451,15 +451,27 @@ public String getExceptionStackTrace() { return (String) this.auditInfo.get(EXCEPTION); } - @Override - public String toString() { - try { - return org.opensearch.common.Strings.toString(JsonXContent.contentBuilder().map(getAsMap())); - } catch (final IOException e) { - throw ExceptionsHelper.convertToOpenSearchException(e); - } + public String getRequestBody() { + return (String) this.auditInfo.get(REQUEST_BODY); } + public String getNodeId() { + return (String) this.auditInfo.get(NODE_ID); + } + + public String getDocId() { + return (String) this.auditInfo.get(ID); + } + + @Override + public String toString() { + try { + return org.opensearch.common.Strings.toString(JsonXContent.contentBuilder().map(getAsMap())); + } catch (final IOException e) { + throw ExceptionsHelper.convertToOpenSearchException(e); + } + } + public String toPrettyString() { try { return org.opensearch.common.Strings.toString(JsonXContent.contentBuilder().prettyPrint().map(getAsMap())); diff --git a/src/test/java/org/opensearch/security/auditlog/compliance/ComplianceAuditlogTest.java b/src/test/java/org/opensearch/security/auditlog/compliance/ComplianceAuditlogTest.java index 85f5d10617..4f51e18aa2 100644 --- a/src/test/java/org/opensearch/security/auditlog/compliance/ComplianceAuditlogTest.java +++ b/src/test/java/org/opensearch/security/auditlog/compliance/ComplianceAuditlogTest.java @@ -43,6 +43,8 @@ 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.not; import static org.hamcrest.core.AnyOf.anyOf; import static org.hamcrest.core.IsEqual.equalTo; import static org.junit.Assert.assertThrows; @@ -90,10 +92,11 @@ public void testSourceFilter() throws Exception { Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode()); }); - Assert.assertTrue(TestAuditlogImpl.sb.toString().contains("COMPLIANCE_DOC_READ")); - Assert.assertFalse(TestAuditlogImpl.sb.toString().contains("Designation")); - Assert.assertFalse(TestAuditlogImpl.sb.toString().contains("Salary")); - Assert.assertTrue(TestAuditlogImpl.sb.toString().contains("Gender")); + assertThat(message.getCategory(), equalTo(AuditCategory.COMPLIANCE_DOC_READ)); + assertThat(message.getRequestBody(), not(containsString("Designation"))); + assertThat(message.getRequestBody(), not(containsString("Salary"))); + assertThat(message.getRequestBody(), containsString("Gender")); + Assert.assertTrue(validateMsgs(TestAuditlogImpl.messages)); } @@ -223,17 +226,24 @@ public void testSourceFilterMsearch() throws Exception { + "}" + System.lineSeparator(); - TestAuditlogImpl.doThenWaitForMessages(() -> { + final List messages = TestAuditlogImpl.doThenWaitForMessages(() -> { HttpResponse response = rh.executePostRequest("_msearch?pretty", search, encodeBasicHeader("admin", "admin")); assertNotContains(response, "*exception*"); Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode()); }, 2); - System.out.println(TestAuditlogImpl.sb.toString()); - Assert.assertTrue(TestAuditlogImpl.sb.toString().contains("COMPLIANCE_DOC_READ")); - Assert.assertFalse(TestAuditlogImpl.sb.toString().contains("Salary")); - Assert.assertTrue(TestAuditlogImpl.sb.toString().contains("Gender")); - Assert.assertTrue(TestAuditlogImpl.sb.toString().contains("Designation")); - Assert.assertTrue(validateMsgs(TestAuditlogImpl.messages)); + + + final AuditMessage desginationMsg = messages.stream().filter(msg -> msg.getRequestBody().contains("Designation")).findFirst().orElseThrow(); + assertThat(desginationMsg.getCategory(), equalTo(AuditCategory.COMPLIANCE_DOC_READ)); + assertThat(desginationMsg.getRequestBody(), containsString("Designation")); + assertThat(desginationMsg.getRequestBody(), not(containsString("Salary"))); + + final AuditMessage genderMsg = messages.stream().filter(msg -> msg.getRequestBody().contains("Gender")).findFirst().orElseThrow(); + assertThat(genderMsg.getCategory(), equalTo(AuditCategory.COMPLIANCE_DOC_READ)); + assertThat(genderMsg.getRequestBody(), containsString("Gender")); + assertThat(genderMsg.getRequestBody(), not(containsString("Salary"))); + + Assert.assertTrue(validateMsgs(messages)); } @Test @@ -253,6 +263,7 @@ public void testInternalConfig() throws Exception { setup(additionalSettings); + final List expectedDocumentsTypes = List.of("config", "actiongroups", "internalusers", "roles", "rolesmapping", "tenants", "audit"); final List messages = TestAuditlogImpl.doThenWaitForMessages(() -> { try (RestHighLevelClient restHighLevelClient = getRestClient(clusterInfo, "kirk-keystore.jks", "truststore.jks")) { for (IndexRequest ir : new DynamicSecurityConfig().setSecurityRoles("roles_2.yml").getDynamicConfig(getResourceFolder())) { @@ -268,23 +279,19 @@ public void testInternalConfig() throws Exception { assertThat(response.getStatusCode(), equalTo(HttpStatus.SC_OK)); }, 14); - 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("anonymous_auth_enabled")); - Assert.assertTrue(TestAuditlogImpl.sb.toString().contains("indices:data/read/suggest")); - Assert.assertTrue(TestAuditlogImpl.sb.toString().contains("internalusers")); - Assert.assertTrue(TestAuditlogImpl.sb.toString().contains("opendistro_security_all_access")); - Assert.assertTrue(TestAuditlogImpl.sb.toString().contains("indices:data/read/suggest")); - Assert.assertFalse(TestAuditlogImpl.sb.toString().contains("eyJzZWFyY2hndWFy")); - Assert.assertFalse(TestAuditlogImpl.sb.toString().contains("eyJBTEwiOlsiaW")); - Assert.assertFalse(TestAuditlogImpl.sb.toString().contains("eyJhZG1pbiI6e")); - Assert.assertFalse(TestAuditlogImpl.sb.toString().contains("eyJzZ19hb")); - Assert.assertFalse(TestAuditlogImpl.sb.toString().contains("eyJzZ19hbGx")); - Assert.assertFalse(TestAuditlogImpl.sb.toString().contains("dvcmYiOnsiY2x")); - Assert.assertTrue( - TestAuditlogImpl.sb.toString().contains("\\\"op\\\":\\\"remove\\\",\\\"path\\\":\\\"/opendistro_security_worf\\\"") - ); - Assert.assertTrue(validateMsgs(TestAuditlogImpl.messages)); + final List documentIds = messages.stream().map(AuditMessage::getDocId).distinct().collect(Collectors.toList()); + assertThat(documentIds, equalTo(expectedDocumentsTypes)); + + messages.stream().collect(Collectors.groupingBy(AuditMessage::getDocId)).entrySet().forEach((e) -> { + final String docId = e.getKey(); + final List messagesByDocId = e.getValue(); + assertThat("Doc " + docId + " should have a read/write config message", + messagesByDocId.stream().map(AuditMessage::getCategory).collect(Collectors.toList()), + equalTo(List.of(AuditCategory.COMPLIANCE_INTERNAL_CONFIG_WRITE, AuditCategory.COMPLIANCE_INTERNAL_CONFIG_READ)) + ); + }); + + Assert.assertTrue(validateMsgs(messages)); } @Test @@ -301,7 +308,7 @@ public void testExternalConfig() throws Exception { .put(ConfigConstants.OPENDISTRO_SECURITY_AUDIT_CONFIG_DISABLED_REST_CATEGORIES, "authenticated,GRANTED_PRIVILEGES") .build(); - TestAuditlogImpl.doThenWaitForMessages(() -> { + final List messages = TestAuditlogImpl.doThenWaitForMessages(() -> { try { setup(additionalSettings); } catch (final Exception ex) { @@ -318,10 +325,17 @@ public void testExternalConfig() throws Exception { Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode()); }, 4); - Assert.assertTrue(TestAuditlogImpl.sb.toString().contains("external_configuration")); - Assert.assertTrue(TestAuditlogImpl.sb.toString().contains("COMPLIANCE_EXTERNAL_CONFIG")); - Assert.assertTrue(TestAuditlogImpl.sb.toString().contains("opensearch_yml")); - Assert.assertTrue(validateMsgs(TestAuditlogImpl.messages)); + // Record the updated config, and then for each node record that the config was updated + assertThat(messages.get(0).getCategory(), equalTo(AuditCategory.COMPLIANCE_INTERNAL_CONFIG_WRITE)); + assertThat(messages.get(1).getCategory(), equalTo(AuditCategory.COMPLIANCE_EXTERNAL_CONFIG)); + assertThat(messages.get(2).getCategory(), equalTo(AuditCategory.COMPLIANCE_EXTERNAL_CONFIG)); + assertThat(messages.get(3).getCategory(), equalTo(AuditCategory.COMPLIANCE_EXTERNAL_CONFIG)); + + // Make sure that the config update messsages are for each node in the cluster + assertThat(messages.get(1).getNodeId(), not(equalTo(messages.get(2).getNodeId()))); + assertThat(messages.get(2).getNodeId(), not(equalTo(messages.get(3).getNodeId()))); + + Assert.assertTrue(validateMsgs(messages)); } @Test 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 c746b4ebcb..70b9cfc57e 100644 --- a/src/test/java/org/opensearch/security/auditlog/integration/BasicAuditlogTest.java +++ b/src/test/java/org/opensearch/security/auditlog/integration/BasicAuditlogTest.java @@ -137,12 +137,10 @@ public void testSSLPlainText() throws Exception { setup(additionalSettings); final List messages = TestAuditlogImpl.doThenWaitForMessages(() -> { - final RuntimeException ex = Assert.assertThrows( - RuntimeException.class, - () -> nonSslRestHelper().executeGetRequest("_search", encodeBasicHeader("admin", "admin")) - ); + final RuntimeException ex = Assert.assertThrows(RuntimeException.class, + () -> nonSslRestHelper().executeGetRequest("_search", encodeBasicHeader("admin", "admin"))); Assert.assertEquals("org.apache.http.NoHttpResponseException", ex.getCause().getClass().getName()); - }, 4); + }, 2); // All of the messages should be the same as the http client is attempting multiple times. messages.stream().forEach((message) -> { diff --git a/src/test/java/org/opensearch/security/auditlog/integration/TestAuditlogImpl.java b/src/test/java/org/opensearch/security/auditlog/integration/TestAuditlogImpl.java index a97d29732d..68fc47e19b 100644 --- a/src/test/java/org/opensearch/security/auditlog/integration/TestAuditlogImpl.java +++ b/src/test/java/org/opensearch/security/auditlog/integration/TestAuditlogImpl.java @@ -58,32 +58,66 @@ public static synchronized void clear() { * Perform an action and then wait until the expected number of messages have been found. */ public static List doThenWaitForMessages(final Runnable action, final int expectedCount) { - final CountDownLatch latch = new CountDownLatch(expectedCount); + final List missedMessages = new ArrayList<>(); final List messages = new ArrayList<>(); - countDownRef.set(latch); - messagesRef.set(messages); - - TestAuditlogImpl.sb = new StringBuffer(); - TestAuditlogImpl.messages = messages; - + final CountDownLatch latch = resetAuditStorage(expectedCount, messages); + try { action.run(); final int maxSecondsToWaitForMessages = 1; - final boolean foundAll = latch.await(maxSecondsToWaitForMessages, TimeUnit.SECONDS); - if (!foundAll) { + boolean foundAll = false; + foundAll = latch.await(maxSecondsToWaitForMessages, TimeUnit.SECONDS); + // After the wait has prevent any new messages from being recieved + resetAuditStorage(0, missedMessages); + if (!foundAll || messages.size() != expectedCount) { throw new MessagesNotFoundException(expectedCount, messages); } - if (messages.size() != expectedCount) { - throw new RuntimeException( - "Unexpected number of messages, was expecting " + expectedCount + ", received " + messages.size() - ); - } } catch (final InterruptedException e) { throw new RuntimeException("Unexpected exception", e); } + + // Do not check for missed messages if no messages were expected + if (expectedCount != 0) { + try { + Thread.sleep(100); + if (missedMessages.size() != 0) { + final String missedMessagesErrorMessage = new StringBuilder() + .append("Audit messages were missed! ") + .append("Found " + (missedMessages.size()) + " messages.") + .append("Messages found during this time: \n\n") + .append(missedMessages.stream() + .map(AuditMessage::toString) + .collect(Collectors.joining("\n"))) + .toString(); + + throw new RuntimeException(missedMessagesErrorMessage); + } + } catch (final Exception e) { + throw new RuntimeException("Unexpected exception", e); + } + } + + // Next usage of this class might be using raw stringbuilder / list so reset before that test might run + resetAuditStorage(0, new ArrayList<>()); return new ArrayList<>(messages); } + /** + * Resets all of the mechanics for fresh messages to be captured + * + * @param expectedMessageCount The number of messages before the latch is signalled, indicating all messages have been recieved + * @param message Where messages will be stored after being recieved + */ + private static CountDownLatch resetAuditStorage(int expectedMessageCount, List messages) { + final CountDownLatch latch = new CountDownLatch(expectedMessageCount); + countDownRef.set(latch); + messagesRef.set(messages); + + TestAuditlogImpl.sb = new StringBuffer(); + TestAuditlogImpl.messages = messages; + return latch; + } + /** * Perform an action and then wait until a single message has been found. */ From 774389232dd1a723681b392f19ad0d5f39807b28 Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Fri, 21 Jul 2023 10:43:23 -0400 Subject: [PATCH 2/4] Fixes failing citest task Signed-off-by: Darshit Chanpura --- .../org/opensearch/security/test/helper/rest/RestHelper.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/opensearch/security/test/helper/rest/RestHelper.java b/src/test/java/org/opensearch/security/test/helper/rest/RestHelper.java index 9e949a6ee0..9bdb8ef61b 100644 --- a/src/test/java/org/opensearch/security/test/helper/rest/RestHelper.java +++ b/src/test/java/org/opensearch/security/test/helper/rest/RestHelper.java @@ -303,7 +303,7 @@ protected final CloseableHttpClient getHTTPClient() throws Exception { hcb.setDefaultSocketConfig(SocketConfig.custom().setSoTimeout(60 * 1000).build()); - return hcb.build(); + return hcb.disableAutomaticRetries().build(); } public static class HttpResponse { From f0775fa799c7afaab972df42da778ca07011e551 Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Fri, 21 Jul 2023 10:44:58 -0400 Subject: [PATCH 3/4] Fixes spotlessChecks Signed-off-by: Darshit Chanpura --- .../security/auditlog/impl/AuditMessage.java | 14 +++++++------- .../compliance/ComplianceAuditlogTest.java | 19 +++++++++++++++---- .../integration/BasicAuditlogTest.java | 6 ++++-- .../integration/TestAuditlogImpl.java | 15 ++++++--------- 4 files changed, 32 insertions(+), 22 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 c84129ad10..c6fbcff7bc 100644 --- a/src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java +++ b/src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java @@ -463,14 +463,14 @@ public String getDocId() { return (String) this.auditInfo.get(ID); } - @Override - public String toString() { - try { + @Override + public String toString() { + try { return org.opensearch.common.Strings.toString(JsonXContent.contentBuilder().map(getAsMap())); - } catch (final IOException e) { - throw ExceptionsHelper.convertToOpenSearchException(e); - } - } + } catch (final IOException e) { + throw ExceptionsHelper.convertToOpenSearchException(e); + } + } public String toPrettyString() { try { diff --git a/src/test/java/org/opensearch/security/auditlog/compliance/ComplianceAuditlogTest.java b/src/test/java/org/opensearch/security/auditlog/compliance/ComplianceAuditlogTest.java index 4f51e18aa2..37f272e7bb 100644 --- a/src/test/java/org/opensearch/security/auditlog/compliance/ComplianceAuditlogTest.java +++ b/src/test/java/org/opensearch/security/auditlog/compliance/ComplianceAuditlogTest.java @@ -232,8 +232,10 @@ public void testSourceFilterMsearch() throws Exception { Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode()); }, 2); - - final AuditMessage desginationMsg = messages.stream().filter(msg -> msg.getRequestBody().contains("Designation")).findFirst().orElseThrow(); + final AuditMessage desginationMsg = messages.stream() + .filter(msg -> msg.getRequestBody().contains("Designation")) + .findFirst() + .orElseThrow(); assertThat(desginationMsg.getCategory(), equalTo(AuditCategory.COMPLIANCE_DOC_READ)); assertThat(desginationMsg.getRequestBody(), containsString("Designation")); assertThat(desginationMsg.getRequestBody(), not(containsString("Salary"))); @@ -263,7 +265,15 @@ public void testInternalConfig() throws Exception { setup(additionalSettings); - final List expectedDocumentsTypes = List.of("config", "actiongroups", "internalusers", "roles", "rolesmapping", "tenants", "audit"); + final List expectedDocumentsTypes = List.of( + "config", + "actiongroups", + "internalusers", + "roles", + "rolesmapping", + "tenants", + "audit" + ); final List messages = TestAuditlogImpl.doThenWaitForMessages(() -> { try (RestHighLevelClient restHighLevelClient = getRestClient(clusterInfo, "kirk-keystore.jks", "truststore.jks")) { for (IndexRequest ir : new DynamicSecurityConfig().setSecurityRoles("roles_2.yml").getDynamicConfig(getResourceFolder())) { @@ -285,7 +295,8 @@ public void testInternalConfig() throws Exception { messages.stream().collect(Collectors.groupingBy(AuditMessage::getDocId)).entrySet().forEach((e) -> { final String docId = e.getKey(); final List messagesByDocId = e.getValue(); - assertThat("Doc " + docId + " should have a read/write config message", + assertThat( + "Doc " + docId + " should have a read/write config message", messagesByDocId.stream().map(AuditMessage::getCategory).collect(Collectors.toList()), equalTo(List.of(AuditCategory.COMPLIANCE_INTERNAL_CONFIG_WRITE, AuditCategory.COMPLIANCE_INTERNAL_CONFIG_READ)) ); 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 70b9cfc57e..ba2297eede 100644 --- a/src/test/java/org/opensearch/security/auditlog/integration/BasicAuditlogTest.java +++ b/src/test/java/org/opensearch/security/auditlog/integration/BasicAuditlogTest.java @@ -137,8 +137,10 @@ public void testSSLPlainText() throws Exception { setup(additionalSettings); final List messages = TestAuditlogImpl.doThenWaitForMessages(() -> { - final RuntimeException ex = Assert.assertThrows(RuntimeException.class, - () -> nonSslRestHelper().executeGetRequest("_search", encodeBasicHeader("admin", "admin"))); + final RuntimeException ex = Assert.assertThrows( + RuntimeException.class, + () -> nonSslRestHelper().executeGetRequest("_search", encodeBasicHeader("admin", "admin")) + ); Assert.assertEquals("org.apache.http.NoHttpResponseException", ex.getCause().getClass().getName()); }, 2); diff --git a/src/test/java/org/opensearch/security/auditlog/integration/TestAuditlogImpl.java b/src/test/java/org/opensearch/security/auditlog/integration/TestAuditlogImpl.java index 68fc47e19b..7b77c1babc 100644 --- a/src/test/java/org/opensearch/security/auditlog/integration/TestAuditlogImpl.java +++ b/src/test/java/org/opensearch/security/auditlog/integration/TestAuditlogImpl.java @@ -61,7 +61,7 @@ public static List doThenWaitForMessages(final Runnable action, fi final List missedMessages = new ArrayList<>(); final List messages = new ArrayList<>(); final CountDownLatch latch = resetAuditStorage(expectedCount, messages); - + try { action.run(); final int maxSecondsToWaitForMessages = 1; @@ -81,14 +81,11 @@ public static List doThenWaitForMessages(final Runnable action, fi try { Thread.sleep(100); if (missedMessages.size() != 0) { - final String missedMessagesErrorMessage = new StringBuilder() - .append("Audit messages were missed! ") - .append("Found " + (missedMessages.size()) + " messages.") - .append("Messages found during this time: \n\n") - .append(missedMessages.stream() - .map(AuditMessage::toString) - .collect(Collectors.joining("\n"))) - .toString(); + final String missedMessagesErrorMessage = new StringBuilder().append("Audit messages were missed! ") + .append("Found " + (missedMessages.size()) + " messages.") + .append("Messages found during this time: \n\n") + .append(missedMessages.stream().map(AuditMessage::toString).collect(Collectors.joining("\n"))) + .toString(); throw new RuntimeException(missedMessagesErrorMessage); } From 458bce70b29f638044c33d6d455a2b03b207e32c Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Fri, 21 Jul 2023 10:47:37 -0400 Subject: [PATCH 4/4] Fixes test assertions to reflect correct number Signed-off-by: Darshit Chanpura --- .../security/auditlog/integration/BasicAuditlogTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 ba2297eede..812ff64c08 100644 --- a/src/test/java/org/opensearch/security/auditlog/integration/BasicAuditlogTest.java +++ b/src/test/java/org/opensearch/security/auditlog/integration/BasicAuditlogTest.java @@ -142,7 +142,7 @@ public void testSSLPlainText() throws Exception { () -> nonSslRestHelper().executeGetRequest("_search", encodeBasicHeader("admin", "admin")) ); Assert.assertEquals("org.apache.http.NoHttpResponseException", ex.getCause().getClass().getName()); - }, 2); + }, 1); // All of the messages should be the same as the http client is attempting multiple times. messages.stream().forEach((message) -> {