From b76e80414eb6af1351a1019134eef4f11c9fa8bb Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Thu, 11 May 2023 12:11:23 -0400 Subject: [PATCH 01/19] Adds a check to skip serialization-deserialization if user request is direct Signed-off-by: Darshit Chanpura --- .../transport/SecurityInterceptor.java | 29 +++++++++++---- .../transport/SecurityRequestHandler.java | 36 ++++++++----------- 2 files changed, 37 insertions(+), 28 deletions(-) diff --git a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java index 5456d36d9c..59e1ca7689 100644 --- a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java +++ b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java @@ -58,8 +58,10 @@ import org.opensearch.security.ssl.transport.SSLConfig; import org.opensearch.security.support.Base64Helper; import org.opensearch.security.support.ConfigConstants; +import org.opensearch.security.support.HeaderHelper; import org.opensearch.security.user.User; import org.opensearch.threadpool.ThreadPool; +import org.opensearch.transport.Transport; import org.opensearch.transport.Transport.Connection; import org.opensearch.transport.TransportException; import org.opensearch.transport.TransportInterceptor.AsyncSender; @@ -127,6 +129,8 @@ public void sendRequestDecorate(AsyncSender sender final String origCCSTransientMf = getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_MASKED_FIELD_CCS); final boolean isDebugEnabled = log.isDebugEnabled(); + final boolean isDirectRequest = cs.localNode().equals(connection.getNode()) || HeaderHelper.isDirectRequest(getThreadContext()); + try (ThreadContext.StoredContext stashedContext = getThreadContext().stashContext()) { final TransportResponseHandler restoringHandler = new RestoringTransportResponseHandler(handler, stashedContext); getThreadContext().putHeader("_opendistro_security_remotecn", cs.getClusterName().value()); @@ -150,9 +154,7 @@ public void sendRequestDecorate(AsyncSender sender if (OpenSearchSecurityPlugin.GuiceHolder.getRemoteClusterService().isCrossClusterSearchEnabled() && clusterInfoHolder.isInitialized() - && (action.equals(ClusterSearchShardsAction.NAME) - || action.equals(SearchAction.NAME) - ) + && (action.equals(ClusterSearchShardsAction.NAME) || action.equals(SearchAction.NAME)) && !clusterInfoHolder.hasNode(connection.getNode())) { if (isDebugEnabled) { log.debug("remove dls/fls/mf because we sent a ccs request to a remote cluster"); @@ -197,7 +199,7 @@ && getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROL getThreadContext().putHeader(headerMap); - ensureCorrectHeaders(remoteAddress0, user0, origin0, injectedUserString, injectedRolesString); + ensureCorrectHeaders(remoteAddress0, user0, origin0, injectedUserString, injectedRolesString, isDirectRequest); if (isActionTraceEnabled()) { getThreadContext().putHeader("_opendistro_security_trace"+System.currentTimeMillis()+"#"+UUID.randomUUID().toString(), Thread.currentThread().getName()+" IC -> "+action+" "+getThreadContext().getHeaders().entrySet().stream().filter(p->!p.getKey().startsWith("_opendistro_security_trace")).collect(Collectors.toMap(p -> p.getKey(), p -> p.getValue()))); @@ -208,7 +210,7 @@ && getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROL } private void ensureCorrectHeaders(final Object remoteAdr, final User origUser, final String origin, - final String injectedUserString, final String injectedRolesString) { + final String injectedUserString, final String injectedRolesString, boolean isDirectRequest) { // keep original address if(origin != null && !origin.isEmpty() /*&& !Origin.LOCAL.toString().equalsIgnoreCase(origin)*/ && getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_ORIGIN_HEADER) == null) { @@ -224,7 +226,15 @@ private void ensureCorrectHeaders(final Object remoteAdr, final User origUser, f String remoteAddressHeader = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS_HEADER); if(remoteAddressHeader == null) { - getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS_HEADER, Base64Helper.serializeObject(((TransportAddress) remoteAdr).address())); + TransportAddress transportAddress = (TransportAddress) remoteAdr; + + if(isDirectRequest) { + // if request is going to be handled by same node, we directly put transient value. + log.info("Tranport Addr dir channel: {}", transportAddress); + getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS, transportAddress); + } else { + getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS_HEADER, Base64Helper.serializeObject(transportAddress.address())); + } } } @@ -233,7 +243,12 @@ private void ensureCorrectHeaders(final Object remoteAdr, final User origUser, f if(userHeader == null) { if(origUser != null) { - getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER, Base64Helper.serializeObject(origUser)); + if(isDirectRequest) { + // if request is going to be handled by same node, we directly put transient value as the thread context is not going to be stah. + getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_USER, origUser); + } else { + getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER, Base64Helper.serializeObject(origUser)); + } } else if(StringUtils.isNotEmpty(injectedRolesString)) { getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES_HEADER, injectedRolesString); diff --git a/src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java b/src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java index 4a2919fdb2..b0c82aa536 100644 --- a/src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java +++ b/src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java @@ -138,10 +138,12 @@ protected void messageReceivedDecorate(final T request, final TransportRequestHa //bypass non-netty requests if(channelType.equals("direct")) { final String userHeader = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER); + // for direct channel requests we don't serialize the user object in sendRequestDecorate + final User user = getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER); final String injectedRolesHeader = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES_HEADER); final String injectedUserHeader = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_USER_HEADER); - if(Strings.isNullOrEmpty(userHeader)) { + if(user != null) { // Keeping role injection with higher priority as plugins under OpenSearch will be using this // on transport layer if(!Strings.isNullOrEmpty(injectedRolesHeader)) { @@ -150,14 +152,6 @@ protected void messageReceivedDecorate(final T request, final TransportRequestHa else if(!Strings.isNullOrEmpty(injectedUserHeader)) { getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_USER, injectedUserHeader); } - } else { - getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_USER, Objects.requireNonNull((User) Base64Helper.deserializeObject(userHeader))); - } - - final String originalRemoteAddress = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS_HEADER); - - if(!Strings.isNullOrEmpty(originalRemoteAddress)) { - getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS, new TransportAddress((InetSocketAddress) Base64Helper.deserializeObject(originalRemoteAddress))); } final String rolesValidation = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES_VALIDATION_HEADER); @@ -177,21 +171,21 @@ else if(!Strings.isNullOrEmpty(injectedUserHeader)) { boolean skipSecurityIfDualMode = getThreadContext().getTransient(ConfigConstants.SECURITY_SSL_DUAL_MODE_SKIP_SECURITY) == Boolean.TRUE; - if(skipSecurityIfDualMode) { - if(getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS) == null) { - getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS, request.remoteAddress()); - } + if(skipSecurityIfDualMode) { + if(getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS) == null) { + getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS, request.remoteAddress()); + } - if(getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_ORIGIN) == null) { - getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_ORIGIN, Origin.TRANSPORT.toString()); - } + if(getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_ORIGIN) == null) { + getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_ORIGIN, Origin.TRANSPORT.toString()); + } - if (getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_SSL_TRANSPORT_TRUSTED_CLUSTER_REQUEST) == null) { - getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_SSL_TRANSPORT_TRUSTED_CLUSTER_REQUEST, Boolean.TRUE); - } + if (getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_SSL_TRANSPORT_TRUSTED_CLUSTER_REQUEST) == null) { + getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_SSL_TRANSPORT_TRUSTED_CLUSTER_REQUEST, Boolean.TRUE); + } - super.messageReceivedDecorate(request, handler, transportChannel, task); - return; + super.messageReceivedDecorate(request, handler, transportChannel, task); + return; } //if the incoming request is an internal:* or a shard request allow only if request was sent by a server node From 4a8dda543ed21bb3af1a744a0f0e12a8b0bb6f21 Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Thu, 11 May 2023 12:21:24 -0400 Subject: [PATCH 02/19] Fixes unused import Signed-off-by: Darshit Chanpura --- .../org/opensearch/security/transport/SecurityInterceptor.java | 1 - 1 file changed, 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 59e1ca7689..0d20c76874 100644 --- a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java +++ b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java @@ -61,7 +61,6 @@ import org.opensearch.security.support.HeaderHelper; import org.opensearch.security.user.User; import org.opensearch.threadpool.ThreadPool; -import org.opensearch.transport.Transport; import org.opensearch.transport.Transport.Connection; import org.opensearch.transport.TransportException; import org.opensearch.transport.TransportInterceptor.AsyncSender; From 2e693cf7dc00a59e4db252f9b3ac8d26e5d07791 Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Thu, 11 May 2023 12:25:58 -0400 Subject: [PATCH 03/19] Simplifies direct request check Signed-off-by: Darshit Chanpura --- .../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 0d20c76874..bea955eb5f 100644 --- a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java +++ b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java @@ -128,7 +128,7 @@ public void sendRequestDecorate(AsyncSender sender final String origCCSTransientMf = getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_MASKED_FIELD_CCS); final boolean isDebugEnabled = log.isDebugEnabled(); - final boolean isDirectRequest = cs.localNode().equals(connection.getNode()) || HeaderHelper.isDirectRequest(getThreadContext()); + final boolean isDirectRequest = HeaderHelper.isDirectRequest(getThreadContext()); try (ThreadContext.StoredContext stashedContext = getThreadContext().stashContext()) { final TransportResponseHandler restoringHandler = new RestoringTransportResponseHandler(handler, stashedContext); From 25796cb4f27ba9c736460fa2ef3ecd4ec9c10659 Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Thu, 11 May 2023 12:31:24 -0400 Subject: [PATCH 04/19] Cleans up debug code Signed-off-by: Darshit Chanpura --- .../org/opensearch/security/transport/SecurityInterceptor.java | 1 - .../opensearch/security/transport/SecurityRequestHandler.java | 1 - 2 files changed, 2 deletions(-) diff --git a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java index bea955eb5f..e15ced18c7 100644 --- a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java +++ b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java @@ -229,7 +229,6 @@ private void ensureCorrectHeaders(final Object remoteAdr, final User origUser, f if(isDirectRequest) { // if request is going to be handled by same node, we directly put transient value. - log.info("Tranport Addr dir channel: {}", transportAddress); getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS, transportAddress); } else { getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS_HEADER, Base64Helper.serializeObject(transportAddress.address())); diff --git a/src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java b/src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java index b0c82aa536..10b98ad892 100644 --- a/src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java +++ b/src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java @@ -137,7 +137,6 @@ protected void messageReceivedDecorate(final T request, final TransportRequestHa //bypass non-netty requests if(channelType.equals("direct")) { - final String userHeader = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER); // for direct channel requests we don't serialize the user object in sendRequestDecorate final User user = getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER); final String injectedRolesHeader = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES_HEADER); From d8a4f9cca38a6c5ab304c953b2fdec75e7a0783a Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Fri, 12 May 2023 11:17:19 -0400 Subject: [PATCH 05/19] Changes direct request comparision to compare using localNode Signed-off-by: Darshit Chanpura --- .../transport/SecurityInterceptor.java | 34 +++++++++---------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java index e15ced18c7..eebddc8074 100644 --- a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java +++ b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java @@ -58,7 +58,6 @@ import org.opensearch.security.ssl.transport.SSLConfig; import org.opensearch.security.support.Base64Helper; import org.opensearch.security.support.ConfigConstants; -import org.opensearch.security.support.HeaderHelper; import org.opensearch.security.user.User; import org.opensearch.threadpool.ThreadPool; import org.opensearch.transport.Transport.Connection; @@ -128,7 +127,7 @@ public void sendRequestDecorate(AsyncSender sender final String origCCSTransientMf = getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_MASKED_FIELD_CCS); final boolean isDebugEnabled = log.isDebugEnabled(); - final boolean isDirectRequest = HeaderHelper.isDirectRequest(getThreadContext()); + final boolean isDirectRequest = cs.localNode().equals(connection.getNode()); // using DiscoveryNode equals comparison here try (ThreadContext.StoredContext stashedContext = getThreadContext().stashContext()) { final TransportResponseHandler restoringHandler = new RestoringTransportResponseHandler(handler, stashedContext); @@ -236,25 +235,26 @@ private void ensureCorrectHeaders(final Object remoteAdr, final User origUser, f } } - + User user = getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER); String userHeader = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER); - if(userHeader == null) { - if(origUser != null) { - if(isDirectRequest) { - // if request is going to be handled by same node, we directly put transient value as the thread context is not going to be stah. - getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_USER, origUser); - } else { - getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER, Base64Helper.serializeObject(origUser)); - } - } - else if(StringUtils.isNotEmpty(injectedRolesString)) { - getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES_HEADER, injectedRolesString); - } - else if(StringUtils.isNotEmpty(injectedUserString)) { - getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_USER_HEADER, injectedUserString); + + if(origUser != null) { + if(isDirectRequest) { + // if request is going to be handled by same node, we directly put transient value as the thread context is not going to be stah. + getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_USER, origUser); +// getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER, Base64Helper.serializeObject(origUser)); + } else { + getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER, Base64Helper.serializeObject(origUser)); } } + else if(StringUtils.isNotEmpty(injectedRolesString)) { + getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES_HEADER, injectedRolesString); + } + else if(StringUtils.isNotEmpty(injectedUserString)) { + getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_USER_HEADER, injectedUserString); + } + } From 44d5048eb36b9b0e8017587d821c317539babb46 Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Fri, 12 May 2023 12:29:09 -0400 Subject: [PATCH 06/19] Addresses PR comments Signed-off-by: Darshit Chanpura --- .../transport/SecurityInterceptor.java | 26 +++++++++++-------- .../transport/SecurityRequestHandler.java | 17 ++---------- 2 files changed, 17 insertions(+), 26 deletions(-) diff --git a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java index eebddc8074..e6b6a92eae 100644 --- a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java +++ b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java @@ -238,22 +238,26 @@ private void ensureCorrectHeaders(final Object remoteAdr, final User origUser, f User user = getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER); String userHeader = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER); - - if(origUser != null) { - if(isDirectRequest) { + if(isDirectRequest) { + // put as transient values for same node requests + if(origUser != null) { // if request is going to be handled by same node, we directly put transient value as the thread context is not going to be stah. getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_USER, origUser); -// getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER, Base64Helper.serializeObject(origUser)); - } else { + } else if(StringUtils.isNotEmpty(injectedRolesString)) { + getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES, injectedRolesString); + } else if(StringUtils.isNotEmpty(injectedUserString)) { + getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_USER, injectedUserString); + } + } else { + // put as headers for other requests + if(origUser != null) { getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER, Base64Helper.serializeObject(origUser)); + } else if(StringUtils.isNotEmpty(injectedRolesString)) { + getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES_HEADER, injectedRolesString); + } else if(StringUtils.isNotEmpty(injectedUserString)) { + getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_USER_HEADER, injectedUserString); } } - else if(StringUtils.isNotEmpty(injectedRolesString)) { - getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES_HEADER, injectedRolesString); - } - else if(StringUtils.isNotEmpty(injectedUserString)) { - getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_USER_HEADER, injectedUserString); - } } diff --git a/src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java b/src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java index 10b98ad892..c7799f576e 100644 --- a/src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java +++ b/src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java @@ -137,21 +137,8 @@ protected void messageReceivedDecorate(final T request, final TransportRequestHa //bypass non-netty requests if(channelType.equals("direct")) { - // for direct channel requests we don't serialize the user object in sendRequestDecorate - final User user = getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER); - final String injectedRolesHeader = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES_HEADER); - final String injectedUserHeader = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_USER_HEADER); - - if(user != null) { - // Keeping role injection with higher priority as plugins under OpenSearch will be using this - // on transport layer - if(!Strings.isNullOrEmpty(injectedRolesHeader)) { - getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES, injectedRolesHeader); - } - else if(!Strings.isNullOrEmpty(injectedUserHeader)) { - getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_USER, injectedUserHeader); - } - } + // for direct channel requests user, injected user and injected roles value are already present as transient headers + // so we don't place them here again final String rolesValidation = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES_VALIDATION_HEADER); if(!Strings.isNullOrEmpty(rolesValidation)) { From d60bbb2135ed601794a0143ed874468de5a03726 Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Mon, 15 May 2023 15:07:19 -0400 Subject: [PATCH 07/19] Cleans up logic a little Signed-off-by: Darshit Chanpura --- .../transport/SecurityInterceptor.java | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java index e6b6a92eae..8cbbc6b484 100644 --- a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java +++ b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java @@ -219,19 +219,12 @@ private void ensureCorrectHeaders(final Object remoteAdr, final User origUser, f getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_ORIGIN_HEADER, Origin.LOCAL.toString()); } + String remoteAddressHeader = null; + TransportAddress transportAddress = null; if (remoteAdr != null && remoteAdr instanceof TransportAddress) { - - String remoteAddressHeader = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS_HEADER); - + remoteAddressHeader = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS_HEADER); if(remoteAddressHeader == null) { - TransportAddress transportAddress = (TransportAddress) remoteAdr; - - if(isDirectRequest) { - // if request is going to be handled by same node, we directly put transient value. - getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS, transportAddress); - } else { - getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS_HEADER, Base64Helper.serializeObject(transportAddress.address())); - } + transportAddress = (TransportAddress) remoteAdr; } } @@ -240,6 +233,10 @@ private void ensureCorrectHeaders(final Object remoteAdr, final User origUser, f if(isDirectRequest) { // put as transient values for same node requests + if (transportAddress != null) { + getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS, transportAddress); + } + if(origUser != null) { // if request is going to be handled by same node, we directly put transient value as the thread context is not going to be stah. getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_USER, origUser); @@ -250,6 +247,10 @@ private void ensureCorrectHeaders(final Object remoteAdr, final User origUser, f } } else { // put as headers for other requests + if (transportAddress != null) { + getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS_HEADER, Base64Helper.serializeObject(transportAddress.address())); + } + if(origUser != null) { getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER, Base64Helper.serializeObject(origUser)); } else if(StringUtils.isNotEmpty(injectedRolesString)) { From a55a0a98c67f0d8e8f169ac99c94cd2c51ed21bf Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Tue, 16 May 2023 12:53:38 -0400 Subject: [PATCH 08/19] Graceful error handling Signed-off-by: Darshit Chanpura --- .../transport/SecurityInterceptor.java | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java index 8cbbc6b484..059936a7c4 100644 --- a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java +++ b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java @@ -127,7 +127,12 @@ public void sendRequestDecorate(AsyncSender sender final String origCCSTransientMf = getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_MASKED_FIELD_CCS); final boolean isDebugEnabled = log.isDebugEnabled(); - final boolean isDirectRequest = cs.localNode().equals(connection.getNode()); // using DiscoveryNode equals comparison here + boolean isSameNodeRequest = false; + try { + isSameNodeRequest = cs.localNode().equals(connection.getNode()); // using DiscoveryNode equals comparison here + } catch (AssertionError e) { + // do nothing + } try (ThreadContext.StoredContext stashedContext = getThreadContext().stashContext()) { final TransportResponseHandler restoringHandler = new RestoringTransportResponseHandler(handler, stashedContext); @@ -197,7 +202,7 @@ && getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROL getThreadContext().putHeader(headerMap); - ensureCorrectHeaders(remoteAddress0, user0, origin0, injectedUserString, injectedRolesString, isDirectRequest); + ensureCorrectHeaders(remoteAddress0, user0, origin0, injectedUserString, injectedRolesString, isSameNodeRequest); if (isActionTraceEnabled()) { getThreadContext().putHeader("_opendistro_security_trace"+System.currentTimeMillis()+"#"+UUID.randomUUID().toString(), Thread.currentThread().getName()+" IC -> "+action+" "+getThreadContext().getHeaders().entrySet().stream().filter(p->!p.getKey().startsWith("_opendistro_security_trace")).collect(Collectors.toMap(p -> p.getKey(), p -> p.getValue()))); @@ -208,7 +213,7 @@ && getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROL } private void ensureCorrectHeaders(final Object remoteAdr, final User origUser, final String origin, - final String injectedUserString, final String injectedRolesString, boolean isDirectRequest) { + final String injectedUserString, final String injectedRolesString, boolean isSameNodeRequest) { // keep original address if(origin != null && !origin.isEmpty() /*&& !Origin.LOCAL.toString().equalsIgnoreCase(origin)*/ && getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_ORIGIN_HEADER) == null) { @@ -219,19 +224,15 @@ private void ensureCorrectHeaders(final Object remoteAdr, final User origUser, f getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_ORIGIN_HEADER, Origin.LOCAL.toString()); } - String remoteAddressHeader = null; TransportAddress transportAddress = null; if (remoteAdr != null && remoteAdr instanceof TransportAddress) { - remoteAddressHeader = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS_HEADER); + String remoteAddressHeader = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS_HEADER); if(remoteAddressHeader == null) { transportAddress = (TransportAddress) remoteAdr; } } - User user = getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER); - String userHeader = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER); - - if(isDirectRequest) { + if(isSameNodeRequest) { // put as transient values for same node requests if (transportAddress != null) { getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS, transportAddress); From 73ff9cb92c7d2aec1cbf0ff983914842fe0d9b9e Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Tue, 16 May 2023 15:32:51 -0400 Subject: [PATCH 09/19] Adds a null check to avoid rewriting persistent header Signed-off-by: Darshit Chanpura --- .../transport/SecurityInterceptor.java | 28 ++++++++++--------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java index 059936a7c4..0a8ca50d68 100644 --- a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java +++ b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java @@ -232,9 +232,10 @@ private void ensureCorrectHeaders(final Object remoteAdr, final User origUser, f } } - if(isSameNodeRequest) { - // put as transient values for same node requests - if (transportAddress != null) { + // we put headers as transient for same node requests + if (isSameNodeRequest) { + + if(transportAddress != null) { getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS, transportAddress); } @@ -247,21 +248,22 @@ private void ensureCorrectHeaders(final Object remoteAdr, final User origUser, f getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_USER, injectedUserString); } } else { - // put as headers for other requests - if (transportAddress != null) { + if(transportAddress != null) { getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS_HEADER, Base64Helper.serializeObject(transportAddress.address())); } - if(origUser != null) { - getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER, Base64Helper.serializeObject(origUser)); - } else if(StringUtils.isNotEmpty(injectedRolesString)) { - getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES_HEADER, injectedRolesString); - } else if(StringUtils.isNotEmpty(injectedUserString)) { - getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_USER_HEADER, injectedUserString); + final String userHeader = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER); + if(userHeader == null) { + // put as headers for other requests + if (origUser != null) { + getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER, Base64Helper.serializeObject(origUser)); + } else if (StringUtils.isNotEmpty(injectedRolesString)) { + getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES_HEADER, injectedRolesString); + } else if (StringUtils.isNotEmpty(injectedUserString)) { + getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_USER_HEADER, injectedUserString); + } } } - - } private ThreadContext getThreadContext() { From 62b6172acad5509b2b7cc2ab4cdb23571983e4b0 Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Tue, 23 May 2023 10:59:18 -0400 Subject: [PATCH 10/19] Changes same node evaluation logic to use TransportService instead of ClusterState Signed-off-by: Darshit Chanpura --- .../security/OpenSearchSecurityPlugin.java | 4 +++ .../transport/SecurityInterceptor.java | 33 +++++++++++-------- 2 files changed, 24 insertions(+), 13 deletions(-) diff --git a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java index 89e8ec31ac..6643cc3ace 100644 --- a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java +++ b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java @@ -73,6 +73,7 @@ import org.opensearch.action.support.ActionFilter; import org.opensearch.client.Client; import org.opensearch.cluster.metadata.IndexNameExpressionResolver; +import org.opensearch.cluster.node.DiscoveryNode; import org.opensearch.cluster.node.DiscoveryNodes; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.component.Lifecycle.State; @@ -1193,6 +1194,7 @@ public static class GuiceHolder implements LifecycleComponent { private static RemoteClusterService remoteClusterService; private static IndicesService indicesService; private static PitService pitService; + private static DiscoveryNode localNode; // CS-SUPPRESS-SINGLE: RegexpSingleline Extensions manager used to allow/disallow TLS connections to extensions private static ExtensionsManager extensionsManager; @@ -1205,6 +1207,7 @@ public GuiceHolder(final RepositoriesService repositoriesService, GuiceHolder.indicesService = indicesService; GuiceHolder.pitService = pitService; GuiceHolder.extensionsManager = extensionsManager; + GuiceHolder.localNode = remoteClusterService.getLocalNode(); } // CS-ENFORCE-SINGLE @@ -1226,6 +1229,7 @@ public static IndicesService getIndicesService() { public static ExtensionsManager getExtensionsManager() { return extensionsManager; } // CS-ENFORCE-SINGLE + public static DiscoveryNode getLocalNode() { return localNode; } @Override public void close() { diff --git a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java index 0a8ca50d68..be129de094 100644 --- a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java +++ b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java @@ -43,6 +43,7 @@ import org.opensearch.action.get.GetRequest; import org.opensearch.action.search.SearchAction; import org.opensearch.action.search.SearchRequest; +import org.opensearch.cluster.node.DiscoveryNode; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.io.stream.StreamInput; import org.opensearch.common.settings.Settings; @@ -68,6 +69,7 @@ import org.opensearch.transport.TransportRequestOptions; import org.opensearch.transport.TransportResponse; import org.opensearch.transport.TransportResponseHandler; +import org.opensearch.transport.TransportService; import static org.opensearch.security.OpenSearchSecurityPlugin.isActionTraceEnabled; @@ -85,14 +87,16 @@ public class SecurityInterceptor { private final ClusterInfoHolder clusterInfoHolder; private final SSLConfig SSLConfig; + private final DiscoveryNode localNode; + public SecurityInterceptor(final Settings settings, - final ThreadPool threadPool, final BackendRegistry backendRegistry, - final AuditLog auditLog, final PrincipalExtractor principalExtractor, - final InterClusterRequestEvaluator requestEvalProvider, - final ClusterService cs, - final SslExceptionHandler sslExceptionHandler, - final ClusterInfoHolder clusterInfoHolder, - final SSLConfig SSLConfig) { + final ThreadPool threadPool, final BackendRegistry backendRegistry, + final AuditLog auditLog, final PrincipalExtractor principalExtractor, + final InterClusterRequestEvaluator requestEvalProvider, + final ClusterService cs, + final SslExceptionHandler sslExceptionHandler, + final ClusterInfoHolder clusterInfoHolder, + final SSLConfig SSLConfig) { this.backendRegistry = backendRegistry; this.auditLog = auditLog; this.threadPool = threadPool; @@ -103,6 +107,7 @@ public SecurityInterceptor(final Settings settings, this.sslExceptionHandler = sslExceptionHandler; this.clusterInfoHolder = clusterInfoHolder; this.SSLConfig = SSLConfig; + this.localNode = OpenSearchSecurityPlugin.GuiceHolder.getLocalNode(); } public SecurityRequestHandler getHandler(String action, @@ -127,12 +132,14 @@ public void sendRequestDecorate(AsyncSender sender final String origCCSTransientMf = getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_MASKED_FIELD_CCS); final boolean isDebugEnabled = log.isDebugEnabled(); - boolean isSameNodeRequest = false; - try { - isSameNodeRequest = cs.localNode().equals(connection.getNode()); // using DiscoveryNode equals comparison here - } catch (AssertionError e) { - // do nothing - } + boolean isSameNodeRequest = localNode != null && localNode.equals(connection.getNode()); +// try { +// isSameNodeRequest = cs.localNode().equals(connection.getNode()); // using DiscoveryNode equals comparison here +// } catch (AssertionError e) { +// // do nothing +// log.info(e); +// } + try (ThreadContext.StoredContext stashedContext = getThreadContext().stashContext()) { final TransportResponseHandler restoringHandler = new RestoringTransportResponseHandler(handler, stashedContext); From 8a09ea8d5b6b75b0543ce0cad5632f4007e5d180 Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Wed, 24 May 2023 11:26:22 -0400 Subject: [PATCH 11/19] Fetches local node from TransportService instance via GuiceHolder Signed-off-by: Darshit Chanpura --- .../org/opensearch/security/OpenSearchSecurityPlugin.java | 6 +++--- .../opensearch/security/transport/SecurityInterceptor.java | 5 +---- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java index 6643cc3ace..25d392bbae 100644 --- a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java +++ b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java @@ -1194,7 +1194,7 @@ public static class GuiceHolder implements LifecycleComponent { private static RemoteClusterService remoteClusterService; private static IndicesService indicesService; private static PitService pitService; - private static DiscoveryNode localNode; + private static TransportService transportService; // CS-SUPPRESS-SINGLE: RegexpSingleline Extensions manager used to allow/disallow TLS connections to extensions private static ExtensionsManager extensionsManager; @@ -1207,7 +1207,7 @@ public GuiceHolder(final RepositoriesService repositoriesService, GuiceHolder.indicesService = indicesService; GuiceHolder.pitService = pitService; GuiceHolder.extensionsManager = extensionsManager; - GuiceHolder.localNode = remoteClusterService.getLocalNode(); + GuiceHolder.transportService = remoteClusterService; } // CS-ENFORCE-SINGLE @@ -1229,7 +1229,7 @@ public static IndicesService getIndicesService() { public static ExtensionsManager getExtensionsManager() { return extensionsManager; } // CS-ENFORCE-SINGLE - public static DiscoveryNode getLocalNode() { return localNode; } + public static TransportService getTransportService() { return transportService; } @Override public void close() { diff --git a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java index be129de094..b7a885608c 100644 --- a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java +++ b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java @@ -69,7 +69,6 @@ import org.opensearch.transport.TransportRequestOptions; import org.opensearch.transport.TransportResponse; import org.opensearch.transport.TransportResponseHandler; -import org.opensearch.transport.TransportService; import static org.opensearch.security.OpenSearchSecurityPlugin.isActionTraceEnabled; @@ -87,8 +86,6 @@ public class SecurityInterceptor { private final ClusterInfoHolder clusterInfoHolder; private final SSLConfig SSLConfig; - private final DiscoveryNode localNode; - public SecurityInterceptor(final Settings settings, final ThreadPool threadPool, final BackendRegistry backendRegistry, final AuditLog auditLog, final PrincipalExtractor principalExtractor, @@ -107,7 +104,6 @@ public SecurityInterceptor(final Settings settings, this.sslExceptionHandler = sslExceptionHandler; this.clusterInfoHolder = clusterInfoHolder; this.SSLConfig = SSLConfig; - this.localNode = OpenSearchSecurityPlugin.GuiceHolder.getLocalNode(); } public SecurityRequestHandler getHandler(String action, @@ -132,6 +128,7 @@ public void sendRequestDecorate(AsyncSender sender final String origCCSTransientMf = getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_MASKED_FIELD_CCS); final boolean isDebugEnabled = log.isDebugEnabled(); + final DiscoveryNode localNode = OpenSearchSecurityPlugin.GuiceHolder.getTransportService().getLocalNode(); boolean isSameNodeRequest = localNode != null && localNode.equals(connection.getNode()); // try { // isSameNodeRequest = cs.localNode().equals(connection.getNode()); // using DiscoveryNode equals comparison here From fbb60419d62ad0c16ba1b49a4a474ed826112a10 Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Wed, 24 May 2023 11:38:26 -0400 Subject: [PATCH 12/19] Fixes spotless errors Signed-off-by: Darshit Chanpura --- .../java/org/opensearch/security/OpenSearchSecurityPlugin.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java index 25d392bbae..3670ac8176 100644 --- a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java +++ b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java @@ -73,7 +73,6 @@ import org.opensearch.action.support.ActionFilter; import org.opensearch.client.Client; import org.opensearch.cluster.metadata.IndexNameExpressionResolver; -import org.opensearch.cluster.node.DiscoveryNode; import org.opensearch.cluster.node.DiscoveryNodes; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.component.Lifecycle.State; From ab7177db1bfd59ad19cc9a11d7094e27823dac94 Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Mon, 5 Jun 2023 20:08:41 -0400 Subject: [PATCH 13/19] Updates onNodeStarted to use the overloaded definition to accept localNode value passed from OpenSearch plugin Signed-off-by: Darshit Chanpura --- .../security/OpenSearchSecurityPlugin.java | 13 ++++++++----- .../security/transport/SecurityInterceptor.java | 8 +------- 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java index 3670ac8176..0735cf0de7 100644 --- a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java +++ b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java @@ -73,6 +73,7 @@ import org.opensearch.action.support.ActionFilter; import org.opensearch.client.Client; import org.opensearch.cluster.metadata.IndexNameExpressionResolver; +import org.opensearch.cluster.node.DiscoveryNode; import org.opensearch.cluster.node.DiscoveryNodes; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.component.Lifecycle.State; @@ -209,6 +210,7 @@ public final class OpenSearchSecurityPlugin extends OpenSearchSecuritySSLPlugin private volatile ConfigurationRepository cr; private volatile AdminDNs adminDns; private volatile ClusterService cs; + private static volatile DiscoveryNode localNode; private volatile AuditLog auditLog; private volatile BackendRegistry backendRegistry; private volatile SslExceptionHandler sslExceptionHandler; @@ -1107,11 +1109,12 @@ public List getSettingsFilter() { } @Override - public void onNodeStarted() { + public void onNodeStarted(DiscoveryNode localNode) { log.info("Node started"); if(!SSLConfig.isSslOnlyMode() && !client && !disabled) { cr.initOnNodeStart(); } + this.localNode = localNode; final Set securityModules = ReflectionHelper.getModulesLoaded(); log.info("{} OpenSearch Security modules loaded so far: {}", securityModules.size(), securityModules); } @@ -1187,13 +1190,16 @@ private static String handleKeyword(final String field) { return field; } + public static DiscoveryNode getLocalNode(){ + return localNode; + } + public static class GuiceHolder implements LifecycleComponent { private static RepositoriesService repositoriesService; private static RemoteClusterService remoteClusterService; private static IndicesService indicesService; private static PitService pitService; - private static TransportService transportService; // CS-SUPPRESS-SINGLE: RegexpSingleline Extensions manager used to allow/disallow TLS connections to extensions private static ExtensionsManager extensionsManager; @@ -1206,7 +1212,6 @@ public GuiceHolder(final RepositoriesService repositoriesService, GuiceHolder.indicesService = indicesService; GuiceHolder.pitService = pitService; GuiceHolder.extensionsManager = extensionsManager; - GuiceHolder.transportService = remoteClusterService; } // CS-ENFORCE-SINGLE @@ -1228,8 +1233,6 @@ public static IndicesService getIndicesService() { public static ExtensionsManager getExtensionsManager() { return extensionsManager; } // CS-ENFORCE-SINGLE - public static TransportService getTransportService() { return transportService; } - @Override public void close() { } diff --git a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java index b7a885608c..d89ce3e42c 100644 --- a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java +++ b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java @@ -128,14 +128,8 @@ public void sendRequestDecorate(AsyncSender sender final String origCCSTransientMf = getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_MASKED_FIELD_CCS); final boolean isDebugEnabled = log.isDebugEnabled(); - final DiscoveryNode localNode = OpenSearchSecurityPlugin.GuiceHolder.getTransportService().getLocalNode(); + final DiscoveryNode localNode = OpenSearchSecurityPlugin.getLocalNode(); boolean isSameNodeRequest = localNode != null && localNode.equals(connection.getNode()); -// try { -// isSameNodeRequest = cs.localNode().equals(connection.getNode()); // using DiscoveryNode equals comparison here -// } catch (AssertionError e) { -// // do nothing -// log.info(e); -// } try (ThreadContext.StoredContext stashedContext = getThreadContext().stashContext()) { From 79ad17f042b9449a6e87334489d5033357a764d6 Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Wed, 28 Jun 2023 14:58:43 -0400 Subject: [PATCH 14/19] Adds security interceptor tests Signed-off-by: Darshit Chanpura --- .../security/OpenSearchSecurityPlugin.java | 6 +- .../transport/SecurityInterceptor.java | 17 +- .../transport/SecurityInterceptorTests.java | 179 ++++++++++++++++++ 3 files changed, 194 insertions(+), 8 deletions(-) create mode 100644 src/test/java/org/opensearch/security/transport/SecurityInterceptorTests.java diff --git a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java index 8259ca7076..dafe2bf378 100644 --- a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java +++ b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java @@ -1879,10 +1879,14 @@ private static String handleKeyword(final String field) { return field; } - public static DiscoveryNode getLocalNode(){ + public static DiscoveryNode getLocalNode() { return localNode; } + public static void setLocalNode(DiscoveryNode node) { + localNode = node; + } + public static class GuiceHolder implements LifecycleComponent { private static RepositoriesService repositoriesService; diff --git a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java index 9c92782d0c..7bdf99f965 100644 --- a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java +++ b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java @@ -150,7 +150,6 @@ public void sendRequestDecorate( final DiscoveryNode localNode = OpenSearchSecurityPlugin.getLocalNode(); boolean isSameNodeRequest = localNode != null && localNode.equals(connection.getNode()); - try (ThreadContext.StoredContext stashedContext = getThreadContext().stashContext()) { final TransportResponseHandler restoringHandler = new RestoringTransportResponseHandler(handler, stashedContext); getThreadContext().putHeader("_opendistro_security_remotecn", cs.getClusterName().value()); @@ -272,7 +271,7 @@ && getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_ORIGIN_HEADE TransportAddress transportAddress = null; if (remoteAdr != null && remoteAdr instanceof TransportAddress) { String remoteAddressHeader = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS_HEADER); - if(remoteAddressHeader == null) { + if (remoteAddressHeader == null) { transportAddress = (TransportAddress) remoteAdr; } } @@ -284,16 +283,20 @@ && getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_ORIGIN_HEADE } if (origUser != null) { - // if request is going to be handled by same node, we directly put transient value as the thread context is not going to be stah. + // if request is going to be handled by same node, we directly put transient value as the thread context is not going to be + // stah. getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_USER, origUser); - } else if(StringUtils.isNotEmpty(injectedRolesString)) { + } else if (StringUtils.isNotEmpty(injectedRolesString)) { getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES, injectedRolesString); - } else if(StringUtils.isNotEmpty(injectedUserString)) { + } else if (StringUtils.isNotEmpty(injectedUserString)) { getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_USER, injectedUserString); } } else { - if(transportAddress != null) { - getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS_HEADER, Base64Helper.serializeObject(transportAddress.address())); + if (transportAddress != null) { + getThreadContext().putHeader( + ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS_HEADER, + Base64Helper.serializeObject(transportAddress.address()) + ); } final String userHeader = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER); diff --git a/src/test/java/org/opensearch/security/transport/SecurityInterceptorTests.java b/src/test/java/org/opensearch/security/transport/SecurityInterceptorTests.java new file mode 100644 index 0000000000..e00a193e67 --- /dev/null +++ b/src/test/java/org/opensearch/security/transport/SecurityInterceptorTests.java @@ -0,0 +1,179 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.security.transport; + +import org.junit.Before; +import org.junit.Test; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; +import org.opensearch.Version; +import org.opensearch.action.search.PitService; +import org.opensearch.cluster.ClusterName; +import org.opensearch.cluster.node.DiscoveryNode; +import org.opensearch.cluster.service.ClusterService; +import org.opensearch.common.settings.Settings; +import org.opensearch.extensions.ExtensionsManager; +import org.opensearch.indices.IndicesService; +import org.opensearch.repositories.RepositoriesService; +import org.opensearch.security.OpenSearchSecurityPlugin; +import org.opensearch.security.auditlog.AuditLog; +import org.opensearch.security.auth.BackendRegistry; +import org.opensearch.security.configuration.ClusterInfoHolder; +import org.opensearch.security.ssl.SslExceptionHandler; +import org.opensearch.security.ssl.transport.PrincipalExtractor; +import org.opensearch.security.ssl.transport.SSLConfig; +import org.opensearch.security.support.Base64Helper; +import org.opensearch.security.support.ConfigConstants; +import org.opensearch.security.user.User; +import org.opensearch.test.OpenSearchTestCase; +import org.opensearch.test.transport.MockTransport; +import org.opensearch.threadpool.ThreadPool; +import org.opensearch.transport.Transport.Connection; +import org.opensearch.transport.TransportInterceptor.AsyncSender; +import org.opensearch.transport.TransportRequest; +import org.opensearch.transport.TransportRequestOptions; +import org.opensearch.transport.TransportResponse; +import org.opensearch.transport.TransportResponseHandler; +import org.opensearch.transport.TransportService; + +import static java.util.Collections.emptySet; +import static org.junit.Assert.assertEquals; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +public class SecurityInterceptorTests { + + private SecurityInterceptor securityInterceptor; + + @Mock + private BackendRegistry backendRegistry; + + @Mock + private AuditLog auditLog; + + @Mock + private PrincipalExtractor principalExtractor; + + @Mock + private InterClusterRequestEvaluator requestEvalProvider; + + @Mock + private ClusterService clusterService; + + @Mock + private SslExceptionHandler sslExceptionHandler; + + @Mock + private ClusterInfoHolder clusterInfoHolder; + + @Mock + private SSLConfig sslConfig; + + private Settings settings; + + private ThreadPool threadPool; + + @Before + public void setup() { + MockitoAnnotations.openMocks(this); + settings = Settings.builder() + .put("node.name", SecurityInterceptorTests.class.getSimpleName()) + .put("request.headers.default", "1") + .build(); + threadPool = new ThreadPool(settings); + securityInterceptor = new SecurityInterceptor( + settings, + threadPool, + backendRegistry, + auditLog, + principalExtractor, + requestEvalProvider, + clusterService, + sslExceptionHandler, + clusterInfoHolder, + sslConfig + ); + } + + @Test + public void testSendRequestDecorate() { + + ClusterName clusterName = ClusterName.DEFAULT; + when(clusterService.getClusterName()).thenReturn(clusterName); + + MockTransport transport = new MockTransport(); + TransportService transportService = transport.createTransportService( + Settings.EMPTY, + threadPool, + TransportService.NOOP_TRANSPORT_INTERCEPTOR, + boundTransportAddress -> clusterService.state().nodes().get(SecurityInterceptor.class.getSimpleName()), + null, + emptySet() + ); + + OpenSearchSecurityPlugin.GuiceHolder guiceHolder = new OpenSearchSecurityPlugin.GuiceHolder( + mock(RepositoriesService.class), + transportService, + mock(IndicesService.class), + mock(PitService.class), + mock(ExtensionsManager.class) + ); + + User user = new User("John Doe"); + threadPool.getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_USER, user); + + AsyncSender sender = mock(AsyncSender.class); + String action = "testAction"; + TransportRequest request = mock(TransportRequest.class); + TransportRequestOptions options = mock(TransportRequestOptions.class); + TransportResponseHandler handler = mock(TransportResponseHandler.class); + + DiscoveryNode localNode = new DiscoveryNode("local-node", OpenSearchTestCase.buildNewFakeTransportAddress(), Version.CURRENT); + Connection connection1 = transportService.getConnection(localNode); + + DiscoveryNode otherNode = new DiscoveryNode("local-node", OpenSearchTestCase.buildNewFakeTransportAddress(), Version.CURRENT); + Connection connection2 = transportService.getConnection(otherNode); + + // setting localNode value explicitly + OpenSearchSecurityPlugin.setLocalNode(localNode); + + // isSameNodeRequest = true + securityInterceptor.sendRequestDecorate(sender, connection1, action, request, options, handler); + // from thread context inside sendRequestDecorate + doAnswer(i -> { + User transientUser = threadPool.getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER); + assertEquals(transientUser, user); + return null; + }).when(sender).sendRequest(any(Connection.class), eq(action), eq(request), eq(options), eq(handler)); + + // from original context + User transientUser = threadPool.getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER); + assertEquals(transientUser, user); + assertEquals(threadPool.getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER), null); + + // isSameNodeRequest = false + securityInterceptor.sendRequestDecorate(sender, connection2, action, request, options, handler); + // checking thread context inside sendRequestDecorate + doAnswer(i -> { + String serializedUserHeader = threadPool.getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER); + assertEquals(serializedUserHeader, Base64Helper.serializeObject(user)); + return null; + }).when(sender).sendRequest(any(Connection.class), eq(action), eq(request), eq(options), eq(handler)); + + // from original context + User transientUser2 = threadPool.getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER); + assertEquals(transientUser2, user); + assertEquals(threadPool.getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER), null); + + } + +} From 20af8198c65959aab175c3f1c404e7ef8e700961 Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Wed, 28 Jun 2023 15:06:02 -0400 Subject: [PATCH 15/19] Fixes checkstyle errors Signed-off-by: Darshit Chanpura --- .../security/transport/SecurityInterceptorTests.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/test/java/org/opensearch/security/transport/SecurityInterceptorTests.java b/src/test/java/org/opensearch/security/transport/SecurityInterceptorTests.java index e00a193e67..7291050d6e 100644 --- a/src/test/java/org/opensearch/security/transport/SecurityInterceptorTests.java +++ b/src/test/java/org/opensearch/security/transport/SecurityInterceptorTests.java @@ -8,6 +8,7 @@ package org.opensearch.security.transport; +// CS-SUPPRESS-SINGLE: RegexpSingleline Extensions manager used for creating a mock import org.junit.Before; import org.junit.Test; import org.mockito.Mock; @@ -49,6 +50,7 @@ import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +// CS-ENFORCE-SINGLE public class SecurityInterceptorTests { @@ -120,6 +122,7 @@ public void testSendRequestDecorate() { emptySet() ); + // CS-SUPPRESS-SINGLE: RegexpSingleline Extensions manager used for creating a mock OpenSearchSecurityPlugin.GuiceHolder guiceHolder = new OpenSearchSecurityPlugin.GuiceHolder( mock(RepositoriesService.class), transportService, @@ -127,6 +130,7 @@ public void testSendRequestDecorate() { mock(PitService.class), mock(ExtensionsManager.class) ); + // CS-ENFORCE-SINGLE User user = new User("John Doe"); threadPool.getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_USER, user); From fc1234d1c51bc4af44aabb560bed3e24d9a84767 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 30 Jun 2023 14:23:08 -0400 Subject: [PATCH 16/19] Check for transient headers in received Signed-off-by: Craig Perkins --- .../security/transport/SecurityInterceptor.java | 1 - .../security/transport/SecurityRequestHandler.java | 8 ++++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java index 45b56f2b41..66f4140d3e 100644 --- a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java +++ b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java @@ -132,7 +132,6 @@ public void sendRequestDecorate( TransportRequestOptions options, TransportResponseHandler handler ) { - final Map origHeaders0 = getThreadContext().getHeaders(); final User user0 = getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER); final String injectedUserString = getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_USER); diff --git a/src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java b/src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java index 0fdaa1f33e..fc9c37c59f 100644 --- a/src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java +++ b/src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java @@ -95,7 +95,6 @@ protected void messageReceivedDecorate( final TransportChannel transportChannel, Task task ) throws Exception { - String resolvedActionClass = request.getClass().getSimpleName(); if (request instanceof BulkShardRequest) { @@ -142,9 +141,10 @@ protected void messageReceivedDecorate( } // bypass non-netty requests - if (channelType.equals("direct")) { - // for direct channel requests user, injected user and injected roles value are already present as transient headers - // so we don't place them here again + if (getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER) != null + || getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_USER) != null + || getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES) != null + || getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS) != null) { final String rolesValidation = getThreadContext().getHeader( ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES_VALIDATION_HEADER From f114b72a894d47b9a085ae01b67a148b3b05cb42 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 30 Jun 2023 15:30:25 -0400 Subject: [PATCH 17/19] Attempt to make logic simpler by checking for transients or request headers Signed-off-by: Craig Perkins --- .../security/support/ConfigConstants.java | 2 + .../security/support/HeaderHelper.java | 6 ++ .../transport/SecurityInterceptor.java | 4 + .../transport/SecurityRequestHandler.java | 100 ++++++++---------- 4 files changed, 55 insertions(+), 57 deletions(-) diff --git a/src/main/java/org/opensearch/security/support/ConfigConstants.java b/src/main/java/org/opensearch/security/support/ConfigConstants.java index ee04ff62f3..d83dc56e7e 100644 --- a/src/main/java/org/opensearch/security/support/ConfigConstants.java +++ b/src/main/java/org/opensearch/security/support/ConfigConstants.java @@ -47,6 +47,8 @@ public class ConfigConstants { public static final String OPENDISTRO_SECURITY_ORIGIN = OPENDISTRO_SECURITY_CONFIG_PREFIX + "origin"; public static final String OPENDISTRO_SECURITY_ORIGIN_HEADER = OPENDISTRO_SECURITY_CONFIG_PREFIX + "origin_header"; + public static final String OPENDISTRO_SECURITY_SAME_NODE_REQUEST = OPENDISTRO_SECURITY_CONFIG_PREFIX + "is_same_node_request"; + public static final String OPENDISTRO_SECURITY_DLS_QUERY_HEADER = OPENDISTRO_SECURITY_CONFIG_PREFIX + "dls_query"; public static final String OPENDISTRO_SECURITY_DLS_FILTER_LEVEL_QUERY_HEADER = OPENDISTRO_SECURITY_CONFIG_PREFIX diff --git a/src/main/java/org/opensearch/security/support/HeaderHelper.java b/src/main/java/org/opensearch/security/support/HeaderHelper.java index e8d50346a8..c7489b8ff6 100644 --- a/src/main/java/org/opensearch/security/support/HeaderHelper.java +++ b/src/main/java/org/opensearch/security/support/HeaderHelper.java @@ -44,6 +44,12 @@ public static boolean isDirectRequest(final ThreadContext context) { || context.getTransient(ConfigConstants.OPENDISTRO_SECURITY_CHANNEL_TYPE) == null; } + public static boolean isSameNodeRequest(final ThreadContext context) { + + return context.getTransient(ConfigConstants.OPENDISTRO_SECURITY_SAME_NODE_REQUEST) != null + && (boolean) context.getTransient(ConfigConstants.OPENDISTRO_SECURITY_SAME_NODE_REQUEST); + } + // CS-SUPPRESS-SINGLE: RegexpSingleline Java Cryptography Extension is unrelated to OpenSearch extensions public static boolean isExtensionRequest(final ThreadContext context) { return context.getTransient(ConfigConstants.OPENDISTRO_SECURITY_SSL_TRANSPORT_EXTENSION_REQUEST) == Boolean.TRUE; diff --git a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java index 66f4140d3e..858833dea9 100644 --- a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java +++ b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java @@ -263,6 +263,10 @@ && getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_ORIGIN_HEADE getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_ORIGIN_HEADER, origin); } + if (getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_SAME_NODE_REQUEST) == null) { + getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_SAME_NODE_REQUEST, isSameNodeRequest); + } + if (origin == null && getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_ORIGIN_HEADER) == null) { getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_ORIGIN_HEADER, Origin.LOCAL.toString()); } diff --git a/src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java b/src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java index fc9c37c59f..2e1eeadc2d 100644 --- a/src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java +++ b/src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java @@ -165,9 +165,43 @@ protected void messageReceivedDecorate( } putInitialActionClassHeader(initialActionClassValue, resolvedActionClass); + } else { + final String userHeader = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER); + final String injectedRolesHeader = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES_HEADER); + final String injectedUserHeader = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_USER_HEADER); + + if (Strings.isNullOrEmpty(userHeader)) { + // Keeping role injection with higher priority as plugins under OpenSearch will be using this + // on transport layer + if (!Strings.isNullOrEmpty(injectedRolesHeader)) { + getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES, injectedRolesHeader); + } else if (!Strings.isNullOrEmpty(injectedUserHeader)) { + getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_USER, injectedUserHeader); + } + } else { + getThreadContext().putTransient( + ConfigConstants.OPENDISTRO_SECURITY_USER, + Objects.requireNonNull((User) Base64Helper.deserializeObject(userHeader)) + ); + } - super.messageReceivedDecorate(request, handler, transportChannel, task); - return; + String originalRemoteAddress = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS_HEADER); + + if (!Strings.isNullOrEmpty(originalRemoteAddress)) { + getThreadContext().putTransient( + ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS, + new TransportAddress((InetSocketAddress) Base64Helper.deserializeObject(originalRemoteAddress)) + ); + } else { + getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS, request.remoteAddress()); + } + + final String rolesValidation = getThreadContext().getHeader( + ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES_VALIDATION_HEADER + ); + if (!Strings.isNullOrEmpty(rolesValidation)) { + getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES_VALIDATION, rolesValidation); + } } boolean skipSecurityIfDualMode = getThreadContext().getTransient( @@ -190,8 +224,6 @@ protected void messageReceivedDecorate( ); } - super.messageReceivedDecorate(request, handler, transportChannel, task); - return; } // if the incoming request is an internal:* or a shard request allow only if request was sent by a server node @@ -202,6 +234,7 @@ protected void messageReceivedDecorate( if (!HeaderHelper.isInterClusterRequest(getThreadContext()) && !HeaderHelper.isTrustedClusterRequest(getThreadContext()) && !HeaderHelper.isExtensionRequest(getThreadContext()) + && !HeaderHelper.isDirectRequest(getThreadContext()) && !task.getAction().equals("internal:transport/handshake") && (task.getAction().startsWith("internal:") || task.getAction().contains("["))) { // CS-ENFORCE-SINGLE @@ -224,7 +257,8 @@ protected void messageReceivedDecorate( String principal = null; - if ((principal = getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_SSL_TRANSPORT_PRINCIPAL)) == null) { + if ((principal = getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_SSL_TRANSPORT_PRINCIPAL)) == null + && !HeaderHelper.isDirectRequest(getThreadContext())) { Exception ex = new OpenSearchSecurityException( "No SSL client certificates found for transport type " + transportChannel.getChannelType() @@ -245,58 +279,11 @@ protected void messageReceivedDecorate( // network intercluster request or cross search cluster request // CS-SUPPRESS-SINGLE: RegexpSingleline Used to allow/disallow TLS connections to extensions - if (HeaderHelper.isInterClusterRequest(getThreadContext()) + if (!(HeaderHelper.isInterClusterRequest(getThreadContext()) || HeaderHelper.isTrustedClusterRequest(getThreadContext()) - || HeaderHelper.isExtensionRequest(getThreadContext())) { + || HeaderHelper.isExtensionRequest(getThreadContext()) + || channelType.equals("direct"))) { // CS-ENFORCE-SINGLE - - final String userHeader = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER); - final String injectedRolesHeader = getThreadContext().getHeader( - ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES_HEADER - ); - final String injectedUserHeader = getThreadContext().getHeader( - ConfigConstants.OPENDISTRO_SECURITY_INJECTED_USER_HEADER - ); - - if (Strings.isNullOrEmpty(userHeader)) { - // Keeping role injection with higher priority as plugins under OpenSearch will be using this - // on transport layer - if (!Strings.isNullOrEmpty(injectedRolesHeader)) { - getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES, injectedRolesHeader); - } else if (!Strings.isNullOrEmpty(injectedUserHeader)) { - getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_USER, injectedUserHeader); - } - } else { - getThreadContext().putTransient( - ConfigConstants.OPENDISTRO_SECURITY_USER, - Objects.requireNonNull((User) Base64Helper.deserializeObject(userHeader)) - ); - } - - String originalRemoteAddress = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS_HEADER); - - if (!Strings.isNullOrEmpty(originalRemoteAddress)) { - getThreadContext().putTransient( - ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS, - new TransportAddress((InetSocketAddress) Base64Helper.deserializeObject(originalRemoteAddress)) - ); - } else { - getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS, request.remoteAddress()); - } - - final String rolesValidation = getThreadContext().getHeader( - ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES_VALIDATION_HEADER - ); - if (!Strings.isNullOrEmpty(rolesValidation)) { - getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES_VALIDATION, rolesValidation); - } - - } else { - // this is a netty request from a non-server node (maybe also be internal: or a shard request) - // and therefore issued by a transport client - - // since OS 2.0 we do not support this any longer because transport client no longer available - final OpenSearchException exception = ExceptionUtils.createTransportClientNoLongerSupportedException(); log.error(exception.toString()); transportChannel.sendResponse(exception); @@ -319,9 +306,8 @@ protected void messageReceivedDecorate( } putInitialActionClassHeader(initialActionClassValue, resolvedActionClass); - - super.messageReceivedDecorate(request, handler, transportChannel, task); } + super.messageReceivedDecorate(request, handler, transportChannel, task); } finally { if (isActionTraceEnabled()) { From 35de56db9623aa2c0d07c68126d19a2eb8a64926 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 30 Jun 2023 15:51:50 -0400 Subject: [PATCH 18/19] Remove add tc header Signed-off-by: Craig Perkins --- .../org/opensearch/security/support/ConfigConstants.java | 2 -- .../java/org/opensearch/security/support/HeaderHelper.java | 6 ------ .../opensearch/security/transport/SecurityInterceptor.java | 4 ---- 3 files changed, 12 deletions(-) diff --git a/src/main/java/org/opensearch/security/support/ConfigConstants.java b/src/main/java/org/opensearch/security/support/ConfigConstants.java index d83dc56e7e..ee04ff62f3 100644 --- a/src/main/java/org/opensearch/security/support/ConfigConstants.java +++ b/src/main/java/org/opensearch/security/support/ConfigConstants.java @@ -47,8 +47,6 @@ public class ConfigConstants { public static final String OPENDISTRO_SECURITY_ORIGIN = OPENDISTRO_SECURITY_CONFIG_PREFIX + "origin"; public static final String OPENDISTRO_SECURITY_ORIGIN_HEADER = OPENDISTRO_SECURITY_CONFIG_PREFIX + "origin_header"; - public static final String OPENDISTRO_SECURITY_SAME_NODE_REQUEST = OPENDISTRO_SECURITY_CONFIG_PREFIX + "is_same_node_request"; - public static final String OPENDISTRO_SECURITY_DLS_QUERY_HEADER = OPENDISTRO_SECURITY_CONFIG_PREFIX + "dls_query"; public static final String OPENDISTRO_SECURITY_DLS_FILTER_LEVEL_QUERY_HEADER = OPENDISTRO_SECURITY_CONFIG_PREFIX diff --git a/src/main/java/org/opensearch/security/support/HeaderHelper.java b/src/main/java/org/opensearch/security/support/HeaderHelper.java index c7489b8ff6..e8d50346a8 100644 --- a/src/main/java/org/opensearch/security/support/HeaderHelper.java +++ b/src/main/java/org/opensearch/security/support/HeaderHelper.java @@ -44,12 +44,6 @@ public static boolean isDirectRequest(final ThreadContext context) { || context.getTransient(ConfigConstants.OPENDISTRO_SECURITY_CHANNEL_TYPE) == null; } - public static boolean isSameNodeRequest(final ThreadContext context) { - - return context.getTransient(ConfigConstants.OPENDISTRO_SECURITY_SAME_NODE_REQUEST) != null - && (boolean) context.getTransient(ConfigConstants.OPENDISTRO_SECURITY_SAME_NODE_REQUEST); - } - // CS-SUPPRESS-SINGLE: RegexpSingleline Java Cryptography Extension is unrelated to OpenSearch extensions public static boolean isExtensionRequest(final ThreadContext context) { return context.getTransient(ConfigConstants.OPENDISTRO_SECURITY_SSL_TRANSPORT_EXTENSION_REQUEST) == Boolean.TRUE; diff --git a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java index 858833dea9..66f4140d3e 100644 --- a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java +++ b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java @@ -263,10 +263,6 @@ && getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_ORIGIN_HEADE getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_ORIGIN_HEADER, origin); } - if (getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_SAME_NODE_REQUEST) == null) { - getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_SAME_NODE_REQUEST, isSameNodeRequest); - } - if (origin == null && getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_ORIGIN_HEADER) == null) { getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_ORIGIN_HEADER, Origin.LOCAL.toString()); } From 219b0b329efe7d736ce5a492df87350bc3932795 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 30 Jun 2023 16:35:19 -0400 Subject: [PATCH 19/19] Handle skipSecurityInDualMode Signed-off-by: Craig Perkins --- .../security/transport/SecurityRequestHandler.java | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java b/src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java index 2e1eeadc2d..8ea82c9d9d 100644 --- a/src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java +++ b/src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java @@ -204,6 +204,11 @@ protected void messageReceivedDecorate( } } + if (channelType.equals("direct")) { + super.messageReceivedDecorate(request, handler, transportChannel, task); + return; + } + boolean skipSecurityIfDualMode = getThreadContext().getTransient( ConfigConstants.SECURITY_SSL_DUAL_MODE_SKIP_SECURITY ) == Boolean.TRUE; @@ -224,6 +229,8 @@ protected void messageReceivedDecorate( ); } + super.messageReceivedDecorate(request, handler, transportChannel, task); + return; } // if the incoming request is an internal:* or a shard request allow only if request was sent by a server node @@ -234,7 +241,6 @@ protected void messageReceivedDecorate( if (!HeaderHelper.isInterClusterRequest(getThreadContext()) && !HeaderHelper.isTrustedClusterRequest(getThreadContext()) && !HeaderHelper.isExtensionRequest(getThreadContext()) - && !HeaderHelper.isDirectRequest(getThreadContext()) && !task.getAction().equals("internal:transport/handshake") && (task.getAction().startsWith("internal:") || task.getAction().contains("["))) { // CS-ENFORCE-SINGLE @@ -257,8 +263,7 @@ protected void messageReceivedDecorate( String principal = null; - if ((principal = getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_SSL_TRANSPORT_PRINCIPAL)) == null - && !HeaderHelper.isDirectRequest(getThreadContext())) { + if ((principal = getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_SSL_TRANSPORT_PRINCIPAL)) == null) { Exception ex = new OpenSearchSecurityException( "No SSL client certificates found for transport type " + transportChannel.getChannelType() @@ -281,8 +286,7 @@ protected void messageReceivedDecorate( // CS-SUPPRESS-SINGLE: RegexpSingleline Used to allow/disallow TLS connections to extensions if (!(HeaderHelper.isInterClusterRequest(getThreadContext()) || HeaderHelper.isTrustedClusterRequest(getThreadContext()) - || HeaderHelper.isExtensionRequest(getThreadContext()) - || channelType.equals("direct"))) { + || HeaderHelper.isExtensionRequest(getThreadContext()))) { // CS-ENFORCE-SINGLE final OpenSearchException exception = ExceptionUtils.createTransportClientNoLongerSupportedException(); log.error(exception.toString());