From 771bd236ab6f8713d01b14a4dd7ff10777aa8582 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 18 Sep 2024 10:26:22 -0400 Subject: [PATCH 01/10] Add ensureCustomSerialization to ensure that headers are serialized correctly with multiple transport hops Signed-off-by: Craig Perkins --- .../security/support/Base64Helper.java | 24 +++++++++++++++++++ .../transport/SecurityInterceptor.java | 8 +++---- .../security/support/Base64HelperTest.java | 9 +++++++ 3 files changed, 37 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/opensearch/security/support/Base64Helper.java b/src/main/java/org/opensearch/security/support/Base64Helper.java index a5fbab8515..a147ad919d 100644 --- a/src/main/java/org/opensearch/security/support/Base64Helper.java +++ b/src/main/java/org/opensearch/security/support/Base64Helper.java @@ -69,4 +69,28 @@ public static String ensureJDKSerialized(final String string) { // If we see an exception now, we want the caller to see it - return Base64Helper.serializeObject(serializable, true); } + + /** + * Ensures that the returned string is custom serialized. + * + * If the supplied string is a JDK serialized representation, will deserialize it and further serialize using + * custom, otherwise returns the string as is. + * + * @param string original string, can be JDK or custom serialized + * @return custom serialized string + */ + public static String ensureCustomSerialized(final String string) { + Serializable serializable; + try { + serializable = Base64Helper.deserializeObject(string, true); + } catch (Exception e) { + // We received an exception when de-serializing the given string. It is probably custom serialized. + // Try to deserialize using custom + Base64Helper.deserializeObject(string, false); + // Since we could deserialize the object using custom, the string is already custom serialized, return as is + return string; + } + // If we see an exception now, we want the caller to see it - + return Base64Helper.serializeObject(serializable, false); + } } diff --git a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java index f55d9ac338..3a7560f718 100644 --- a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java +++ b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java @@ -231,13 +231,13 @@ && getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROL } try { - if (serializationFormat == SerializationFormat.JDK) { - Map jdkSerializedHeaders = new HashMap<>(); + if (serializationFormat == SerializationFormat.CustomSerializer_2_11) { + Map customSerializedHeaders = new HashMap<>(); HeaderHelper.getAllSerializedHeaderNames() .stream() .filter(k -> headerMap.get(k) != null) - .forEach(k -> jdkSerializedHeaders.put(k, Base64Helper.ensureJDKSerialized(headerMap.get(k)))); - headerMap.putAll(jdkSerializedHeaders); + .forEach(k -> customSerializedHeaders.put(k, Base64Helper.ensureCustomSerialized(headerMap.get(k)))); + headerMap.putAll(customSerializedHeaders); } getThreadContext().putHeader(headerMap); } catch (IllegalArgumentException iae) { diff --git a/src/test/java/org/opensearch/security/support/Base64HelperTest.java b/src/test/java/org/opensearch/security/support/Base64HelperTest.java index de21c67d52..7c7e68b342 100644 --- a/src/test/java/org/opensearch/security/support/Base64HelperTest.java +++ b/src/test/java/org/opensearch/security/support/Base64HelperTest.java @@ -53,6 +53,15 @@ public void testEnsureJDKSerialized() { assertThat(Base64Helper.ensureJDKSerialized(customSerialized), is(jdkSerialized)); } + @Test + public void testEnsureCustomSerialized() { + String test = "string"; + String jdkSerialized = Base64Helper.serializeObject(test, true); + String customSerialized = Base64Helper.serializeObject(test, false); + assertThat(Base64Helper.ensureCustomSerialized(jdkSerialized), is(customSerialized)); + assertThat(Base64Helper.ensureCustomSerialized(customSerialized), is(customSerialized)); + } + @Test public void testDuplicatedItemSizes() { var largeObject = new HashMap(); From 03f89523459e3bb5a70139ce18b3ef3146f288fa Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 18 Sep 2024 10:59:39 -0400 Subject: [PATCH 02/10] Add check for min node version Signed-off-by: Craig Perkins --- .../configuration/ClusterInfoHolder.java | 12 ++++++++++ .../transport/SecurityInterceptor.java | 24 +++++++++++++------ 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/opensearch/security/configuration/ClusterInfoHolder.java b/src/main/java/org/opensearch/security/configuration/ClusterInfoHolder.java index d7429c5d1d..a9f08eb5f1 100644 --- a/src/main/java/org/opensearch/security/configuration/ClusterInfoHolder.java +++ b/src/main/java/org/opensearch/security/configuration/ClusterInfoHolder.java @@ -29,6 +29,7 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.opensearch.Version; import org.opensearch.cluster.ClusterChangedEvent; import org.opensearch.cluster.ClusterStateListener; import org.opensearch.cluster.node.DiscoveryNode; @@ -67,6 +68,17 @@ public boolean isInitialized() { return initialized; } + public Version getMinNodeVersion() { + if (nodes == null) { + if (log.isDebugEnabled()) { + log.debug("Cluster Info Holder not initialized yet for 'nodes'"); + } + return null; + } + + return nodes.getMinNodeVersion(); + } + public Boolean hasNode(DiscoveryNode node) { if (nodes == null) { if (log.isDebugEnabled()) { diff --git a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java index 3a7560f718..257563218f 100644 --- a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java +++ b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java @@ -39,6 +39,7 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.opensearch.Version; import org.opensearch.action.admin.cluster.shards.ClusterSearchShardsAction; import org.opensearch.action.admin.cluster.shards.ClusterSearchShardsResponse; import org.opensearch.action.get.GetRequest; @@ -231,13 +232,22 @@ && getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROL } try { - if (serializationFormat == SerializationFormat.CustomSerializer_2_11) { - Map customSerializedHeaders = new HashMap<>(); - HeaderHelper.getAllSerializedHeaderNames() - .stream() - .filter(k -> headerMap.get(k) != null) - .forEach(k -> customSerializedHeaders.put(k, Base64Helper.ensureCustomSerialized(headerMap.get(k)))); - headerMap.putAll(customSerializedHeaders); + if (clusterInfoHolder.getMinNodeVersion().before(Version.V_2_14_0)) { + if (serializationFormat == SerializationFormat.JDK) { + Map jdkSerializedHeaders = new HashMap<>(); + HeaderHelper.getAllSerializedHeaderNames() + .stream() + .filter(k -> headerMap.get(k) != null) + .forEach(k -> jdkSerializedHeaders.put(k, Base64Helper.ensureJDKSerialized(headerMap.get(k)))); + headerMap.putAll(jdkSerializedHeaders); + } else if (serializationFormat == SerializationFormat.CustomSerializer_2_11) { + Map customSerializedHeaders = new HashMap<>(); + HeaderHelper.getAllSerializedHeaderNames() + .stream() + .filter(k -> headerMap.get(k) != null) + .forEach(k -> customSerializedHeaders.put(k, Base64Helper.ensureCustomSerialized(headerMap.get(k)))); + headerMap.putAll(customSerializedHeaders); + } } getThreadContext().putHeader(headerMap); } catch (IllegalArgumentException iae) { From cb43777f60f98301504366a71cd5e1bc4dcc9ea3 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 18 Sep 2024 11:06:00 -0400 Subject: [PATCH 03/10] Add null check Signed-off-by: Craig Perkins --- .../org/opensearch/security/transport/SecurityInterceptor.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java index 257563218f..9741014fda 100644 --- a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java +++ b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java @@ -232,7 +232,7 @@ && getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROL } try { - if (clusterInfoHolder.getMinNodeVersion().before(Version.V_2_14_0)) { + if (clusterInfoHolder.getMinNodeVersion() == null || clusterInfoHolder.getMinNodeVersion().before(Version.V_2_14_0)) { if (serializationFormat == SerializationFormat.JDK) { Map jdkSerializedHeaders = new HashMap<>(); HeaderHelper.getAllSerializedHeaderNames() From 93480e767a88e9b77a47443810af0dcfc46cfed6 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 18 Sep 2024 11:36:23 -0400 Subject: [PATCH 04/10] Make JDK serialization default Signed-off-by: Craig Perkins --- src/main/java/org/opensearch/security/support/Base64Helper.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/opensearch/security/support/Base64Helper.java b/src/main/java/org/opensearch/security/support/Base64Helper.java index a147ad919d..a8603e044a 100644 --- a/src/main/java/org/opensearch/security/support/Base64Helper.java +++ b/src/main/java/org/opensearch/security/support/Base64Helper.java @@ -35,7 +35,7 @@ public static String serializeObject(final Serializable object, final boolean us } public static String serializeObject(final Serializable object) { - return serializeObject(object, false); + return serializeObject(object, true); } public static Serializable deserializeObject(final String string) { From b178c20a66222bb9537d76d55bf3d3c265e059e4 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 18 Sep 2024 12:07:34 -0400 Subject: [PATCH 05/10] Change default for Base64Helper.deserializeObject Signed-off-by: Craig Perkins --- src/main/java/org/opensearch/security/support/Base64Helper.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/opensearch/security/support/Base64Helper.java b/src/main/java/org/opensearch/security/support/Base64Helper.java index a8603e044a..7e104ace54 100644 --- a/src/main/java/org/opensearch/security/support/Base64Helper.java +++ b/src/main/java/org/opensearch/security/support/Base64Helper.java @@ -39,7 +39,7 @@ public static String serializeObject(final Serializable object) { } public static Serializable deserializeObject(final String string) { - return deserializeObject(string, false); + return deserializeObject(string, true); } public static Serializable deserializeObject(final String string, final boolean useJDKDeserialization) { From 1220b003667ed7b44991f30bde872da1de464b5f Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 18 Sep 2024 12:27:40 -0400 Subject: [PATCH 06/10] Make JDK serialization default Signed-off-by: Craig Perkins --- .../java/org/opensearch/security/filter/SecurityFilter.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/opensearch/security/filter/SecurityFilter.java b/src/main/java/org/opensearch/security/filter/SecurityFilter.java index 1116e70845..f0ab7bb487 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityFilter.java +++ b/src/main/java/org/opensearch/security/filter/SecurityFilter.java @@ -185,7 +185,7 @@ private void ap } if (threadContext.getTransient(ConfigConstants.USE_JDK_SERIALIZATION) == null) { - threadContext.putTransient(ConfigConstants.USE_JDK_SERIALIZATION, false); + threadContext.putTransient(ConfigConstants.USE_JDK_SERIALIZATION, true); } final ComplianceConfig complianceConfig = auditLog.getComplianceConfig(); From 692439adb94d2eed38405c8c588716c7e47de956 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 19 Sep 2024 10:42:42 -0400 Subject: [PATCH 07/10] Add UT to demonstrate ensureCustomSerialization logic Signed-off-by: Craig Perkins --- .../transport/SecurityInterceptorTests.java | 79 ++++++++++++++++--- 1 file changed, 70 insertions(+), 9 deletions(-) diff --git a/src/test/java/org/opensearch/security/transport/SecurityInterceptorTests.java b/src/test/java/org/opensearch/security/transport/SecurityInterceptorTests.java index 42884862a2..6dfb2fa9d7 100644 --- a/src/test/java/org/opensearch/security/transport/SecurityInterceptorTests.java +++ b/src/test/java/org/opensearch/security/transport/SecurityInterceptorTests.java @@ -119,9 +119,12 @@ public class SecurityInterceptorTests { private Connection connection3; private DiscoveryNode otherRemoteNode; private Connection connection4; + private DiscoveryNode remoteNodeWithCustomSerialization; + private Connection connection5; private AsyncSender sender; - private AsyncSender serializedSender; + private AsyncSender jdkSerializedSender; + private AsyncSender customSerializedSender; private AtomicReference senderLatch = new AtomicReference<>(new CountDownLatch(1)); @Before @@ -199,7 +202,14 @@ public void setup() { otherRemoteNode = new DiscoveryNode("remote-node2", new TransportAddress(remoteAddress, 9876), remoteNodeVersion); connection4 = transportService.getConnection(otherRemoteNode); - serializedSender = new AsyncSender() { + remoteNodeWithCustomSerialization = new DiscoveryNode( + "remote-node-with-custom-serialization", + new TransportAddress(localAddress, 7456), + Version.V_2_12_0 + ); + connection5 = transportService.getConnection(remoteNodeWithCustomSerialization); + + jdkSerializedSender = new AsyncSender() { @Override public void sendRequest( Connection connection, @@ -209,11 +219,27 @@ public void sendRequest( TransportResponseHandler handler ) { String serializedUserHeader = threadPool.getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER); + System.out.println("serializedUserHeader: " + serializedUserHeader); assertThat(serializedUserHeader, is(Base64Helper.serializeObject(user, true))); senderLatch.get().countDown(); } }; + customSerializedSender = new AsyncSender() { + @Override + public void sendRequest( + Connection connection, + String action, + TransportRequest request, + TransportRequestOptions options, + TransportResponseHandler handler + ) { + String serializedUserHeader = threadPool.getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER); + assertThat(serializedUserHeader, is(Base64Helper.serializeObject(user, false))); + senderLatch.get().countDown(); + } + }; + sender = new AsyncSender() { @Override public void sendRequest( @@ -265,6 +291,27 @@ final void completableRequestDecorate( senderLatch.set(new CountDownLatch(1)); } + @SuppressWarnings({ "rawtypes", "unchecked" }) + final void completableRequestDecorateWithPreviouslyPopulatedHeaders( + AsyncSender sender, + Connection connection, + String action, + TransportRequest request, + TransportRequestOptions options, + TransportResponseHandler handler, + DiscoveryNode localNode + ) { + securityInterceptor.sendRequestDecorate(sender, connection, action, request, options, handler, localNode); + try { + senderLatch.get().await(1, TimeUnit.SECONDS); + } catch (final InterruptedException e) { + throw new RuntimeException(e); + } + + // Reset the latch so another request can be processed + senderLatch.set(new CountDownLatch(1)); + } + @Test public void testSendRequestDecorateLocalConnection() { @@ -278,16 +325,30 @@ public void testSendRequestDecorateLocalConnection() { public void testSendRequestDecorateRemoteConnection() { // this is a remote request - completableRequestDecorate(serializedSender, connection3, action, request, options, handler, localNode); + completableRequestDecorate(jdkSerializedSender, connection3, action, request, options, handler, localNode); // this is a remote request where the transport address is different - completableRequestDecorate(serializedSender, connection4, action, request, options, handler, localNode); + completableRequestDecorate(jdkSerializedSender, connection4, action, request, options, handler, localNode); + } + + @Test + public void testSendRequestDecorateRemoteConnectionUsesCustomSerialization() { + threadPool.getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER, Base64Helper.serializeObject(user)); + completableRequestDecorateWithPreviouslyPopulatedHeaders( + customSerializedSender, + connection5, + action, + request, + options, + handler, + localNode + ); } @Test public void testSendNoOriginNodeCausesSerialization() { // this is a request where the local node is null; have to use the remote connection since the serialization will fail - completableRequestDecorate(serializedSender, connection3, action, request, options, handler, null); + completableRequestDecorate(jdkSerializedSender, connection3, action, request, options, handler, null); } @Test @@ -296,7 +357,7 @@ public void testSendNoConnectionShouldThrowNPE() { // The completable version swallows the NPE so have to call actual method assertThrows( java.lang.NullPointerException.class, - () -> securityInterceptor.sendRequestDecorate(serializedSender, null, action, request, options, handler, localNode) + () -> securityInterceptor.sendRequestDecorate(jdkSerializedSender, null, action, request, options, handler, localNode) ); } @@ -328,7 +389,7 @@ public void testCustomRemoteAddressCausesSerialization() { ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS, String.valueOf(new TransportAddress(new InetSocketAddress("8.8.8.8", 80))) ); - completableRequestDecorate(serializedSender, connection3, action, request, options, handler, localNode); + completableRequestDecorate(jdkSerializedSender, connection3, action, request, options, handler, localNode); } @Test @@ -351,7 +412,7 @@ public void testFakeHeaderIsIgnored() { // this is a local request completableRequestDecorate(sender, connection1, action, request, options, handler, localNode); // this is a remote request - completableRequestDecorate(serializedSender, connection3, action, request, options, handler, localNode); + completableRequestDecorate(jdkSerializedSender, connection3, action, request, options, handler, localNode); } @Test @@ -363,7 +424,7 @@ public void testNullHeaderIsIgnored() { // this is a local request completableRequestDecorate(sender, connection1, action, request, options, handler, localNode); // this is a remote request - completableRequestDecorate(serializedSender, connection3, action, request, options, handler, localNode); + completableRequestDecorate(jdkSerializedSender, connection3, action, request, options, handler, localNode); } @Test From 0ef2911a18a873b597c7964188410cd14a8fbae2 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 19 Sep 2024 10:44:25 -0400 Subject: [PATCH 08/10] Remove sysout Signed-off-by: Craig Perkins --- .../opensearch/security/transport/SecurityInterceptorTests.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/test/java/org/opensearch/security/transport/SecurityInterceptorTests.java b/src/test/java/org/opensearch/security/transport/SecurityInterceptorTests.java index 6dfb2fa9d7..6159a6276d 100644 --- a/src/test/java/org/opensearch/security/transport/SecurityInterceptorTests.java +++ b/src/test/java/org/opensearch/security/transport/SecurityInterceptorTests.java @@ -219,7 +219,6 @@ public void sendRequest( TransportResponseHandler handler ) { String serializedUserHeader = threadPool.getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER); - System.out.println("serializedUserHeader: " + serializedUserHeader); assertThat(serializedUserHeader, is(Base64Helper.serializeObject(user, true))); senderLatch.get().countDown(); } From 807374b07772d4db9e34fb41b46116075593a277 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 19 Sep 2024 11:04:06 -0400 Subject: [PATCH 09/10] Also add test to ensureJdkSerialization Signed-off-by: Craig Perkins --- .../transport/SecurityInterceptorTests.java | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/test/java/org/opensearch/security/transport/SecurityInterceptorTests.java b/src/test/java/org/opensearch/security/transport/SecurityInterceptorTests.java index 6159a6276d..3093874e67 100644 --- a/src/test/java/org/opensearch/security/transport/SecurityInterceptorTests.java +++ b/src/test/java/org/opensearch/security/transport/SecurityInterceptorTests.java @@ -219,7 +219,8 @@ public void sendRequest( TransportResponseHandler handler ) { String serializedUserHeader = threadPool.getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER); - assertThat(serializedUserHeader, is(Base64Helper.serializeObject(user, true))); + User deserializedUser = (User) Base64Helper.deserializeObject(serializedUserHeader, true); + assertThat(deserializedUser, is(user)); senderLatch.get().countDown(); } }; @@ -329,9 +330,23 @@ public void testSendRequestDecorateRemoteConnection() { completableRequestDecorate(jdkSerializedSender, connection4, action, request, options, handler, localNode); } + @Test + public void testSendRequestDecorateRemoteConnectionUsesJDKSerialization() { + threadPool.getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER, Base64Helper.serializeObject(user, false)); + completableRequestDecorateWithPreviouslyPopulatedHeaders( + jdkSerializedSender, + connection3, + action, + request, + options, + handler, + localNode + ); + } + @Test public void testSendRequestDecorateRemoteConnectionUsesCustomSerialization() { - threadPool.getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER, Base64Helper.serializeObject(user)); + threadPool.getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER, Base64Helper.serializeObject(user, true)); completableRequestDecorateWithPreviouslyPopulatedHeaders( customSerializedSender, connection5, From efa5c1e53578121fe343f7b116f5a0561d7982e0 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 19 Sep 2024 11:42:22 -0400 Subject: [PATCH 10/10] spotlessApply Signed-off-by: Craig Perkins --- .../transport/SecurityInterceptorTests.java | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/test/java/org/opensearch/security/transport/SecurityInterceptorTests.java b/src/test/java/org/opensearch/security/transport/SecurityInterceptorTests.java index 3093874e67..d12fafb247 100644 --- a/src/test/java/org/opensearch/security/transport/SecurityInterceptorTests.java +++ b/src/test/java/org/opensearch/security/transport/SecurityInterceptorTests.java @@ -334,13 +334,13 @@ public void testSendRequestDecorateRemoteConnection() { public void testSendRequestDecorateRemoteConnectionUsesJDKSerialization() { threadPool.getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER, Base64Helper.serializeObject(user, false)); completableRequestDecorateWithPreviouslyPopulatedHeaders( - jdkSerializedSender, - connection3, - action, - request, - options, - handler, - localNode + jdkSerializedSender, + connection3, + action, + request, + options, + handler, + localNode ); }