From 5dc47baedaba8e94be28aa8162160a466d473ba4 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Wed, 14 Feb 2024 05:52:10 -0600 Subject: [PATCH 1/2] Alternative solution for tracking unconsumed parameters Signed-off-by: Peter Nied --- .../auth/http/jwt/HTTPJwtAuthenticator.java | 4 +- .../security/filter/NettyRequest.java | 46 ++++++++++++++----- .../security/filter/OpenSearchRequest.java | 8 +++- .../security/filter/SecurityRequest.java | 5 +- .../security/filter/SecurityRestFilter.java | 8 ++-- .../http/SecurityHttpServerTransport.java | 2 +- .../Netty4HttpRequestHeaderVerifier.java | 6 +-- 7 files changed, 53 insertions(+), 26 deletions(-) diff --git a/src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java b/src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java index d0d68c74bc..9bf22bf7f3 100644 --- a/src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java +++ b/src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java @@ -130,10 +130,10 @@ private AuthCredentials extractCredentials0(final SecurityRequest request) { } if ((jwtToken == null || jwtToken.isEmpty()) && jwtUrlParameter != null) { - jwtToken = request.param(jwtUrlParameter); + jwtToken = request.params().get(jwtUrlParameter); } else { // just consume to avoid "contains unrecognized parameter" - request.param(jwtUrlParameter); + request.params().get(jwtUrlParameter); } if (jwtToken == null || jwtToken.length() == 0) { diff --git a/src/main/java/org/opensearch/security/filter/NettyRequest.java b/src/main/java/org/opensearch/security/filter/NettyRequest.java index 5890521e2c..d5cd07dc7b 100644 --- a/src/main/java/org/opensearch/security/filter/NettyRequest.java +++ b/src/main/java/org/opensearch/security/filter/NettyRequest.java @@ -20,10 +20,19 @@ import java.util.Optional; import java.util.Set; import java.util.TreeMap; +import java.util.stream.Collector; +import java.util.stream.Collectors; + import javax.net.ssl.SSLEngine; +import org.apache.commons.lang3.concurrent.ConcurrentException; +import org.apache.commons.lang3.concurrent.LazyInitializer; import org.opensearch.http.netty4.Netty4HttpChannel; import org.opensearch.rest.RestRequest.Method; + +import com.google.common.base.Supplier; +import com.google.common.base.Suppliers; + import org.opensearch.rest.RestUtils; import io.netty.handler.codec.http.HttpRequest; @@ -36,8 +45,7 @@ public class NettyRequest implements SecurityRequest { protected final HttpRequest underlyingRequest; protected final Netty4HttpChannel underlyingChannel; - - private final Set consumedParams = new HashSet<>(); + protected final Supplier parameters = Suppliers.memoize(() -> new CheckedAccessMap(params(uri()))); NettyRequest(final HttpRequest request, final Netty4HttpChannel channel) { this.underlyingRequest = request; @@ -86,18 +94,12 @@ public String uri() { @Override public Map params() { - return params(underlyingRequest.uri()); + return parameters.get(); } @Override - public String param(String key) { - Map urlParams = params(); - consumedParams.add(key); - return urlParams != null ? params().get(key) : null; - } - - public Set getConsumedParams() { - return consumedParams; + public Set getUnconsumedParams() { + return parameters.get().accessedKeys(); } private static Map params(String uri) { @@ -115,4 +117,26 @@ private static Map params(String uri) { return params; } + + /** Records access of any keys if explicetly requested from this map */ + private static class CheckedAccessMap extends HashMap { + private final Set accessedKeys = new HashSet<>(); + + public CheckedAccessMap(final Map map) { + super(map); + } + + @Override + public String get(final Object key) { + // Never noticed this about java's map interface the getter is not generic + if (key instanceof String) { + accessedKeys.add((String)key); + } + return super.get(key); + } + + public Set accessedKeys() { + return accessedKeys; + } + } } diff --git a/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java b/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java index 3827c3a8ab..244be6a568 100644 --- a/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java +++ b/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java @@ -16,6 +16,8 @@ import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.Set; + import javax.net.ssl.SSLEngine; import org.opensearch.rest.RestRequest; @@ -79,10 +81,12 @@ public String get(Object key) { } @Override - public String param(String key) { - return underlyingRequest.param(key); + public Set getUnconsumedParams() { + // params() Map consumes explict parameter access + return Set.of(); } + /** Gets access to the underlying request object */ public RestRequest breakEncapsulationForRequest() { return underlyingRequest; diff --git a/src/main/java/org/opensearch/security/filter/SecurityRequest.java b/src/main/java/org/opensearch/security/filter/SecurityRequest.java index 195c7d51a5..d3741585ac 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityRequest.java +++ b/src/main/java/org/opensearch/security/filter/SecurityRequest.java @@ -15,6 +15,7 @@ import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.Set; import java.util.stream.Stream; import javax.net.ssl.SSLEngine; @@ -50,6 +51,6 @@ default String header(final String headerName) { /** The parameters associated with this request */ Map params(); - /** Gets the parameter for the given key */ - String param(String key); + /** The list of parameters that have been accessed but not recorded as being consumed */ + Set getUnconsumedParams(); } diff --git a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java index a15813eec7..0b721e42f9 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java +++ b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java @@ -69,7 +69,7 @@ import static org.opensearch.security.OpenSearchSecurityPlugin.LEGACY_OPENDISTRO_PREFIX; import static org.opensearch.security.OpenSearchSecurityPlugin.PLUGINS_PREFIX; -import static org.opensearch.security.http.SecurityHttpServerTransport.CONSUMED_PARAMS; +import static org.opensearch.security.http.SecurityHttpServerTransport.UNCONSUMED_PARAMS; import static org.opensearch.security.http.SecurityHttpServerTransport.CONTEXT_TO_RESTORE; import static org.opensearch.security.http.SecurityHttpServerTransport.EARLY_RESPONSE; import static org.opensearch.security.http.SecurityHttpServerTransport.IS_AUTHENTICATED; @@ -145,10 +145,10 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c } }); - NettyAttribute.popFrom(request, CONSUMED_PARAMS).ifPresent(consumedParams -> { - for (String param : consumedParams) { + NettyAttribute.popFrom(request, UNCONSUMED_PARAMS).ifPresent(unconsumedParams -> { + for (String unconsumedParam : unconsumedParams) { // Consume the parameter on the RestRequest - request.param(param); + request.param(unconsumedParam); } }); diff --git a/src/main/java/org/opensearch/security/http/SecurityHttpServerTransport.java b/src/main/java/org/opensearch/security/http/SecurityHttpServerTransport.java index 0223632269..eb75f898f4 100644 --- a/src/main/java/org/opensearch/security/http/SecurityHttpServerTransport.java +++ b/src/main/java/org/opensearch/security/http/SecurityHttpServerTransport.java @@ -49,7 +49,7 @@ public class SecurityHttpServerTransport extends SecuritySSLNettyHttpServerTransport { public static final AttributeKey EARLY_RESPONSE = AttributeKey.newInstance("opensearch-http-early-response"); - public static final AttributeKey> CONSUMED_PARAMS = AttributeKey.newInstance("opensearch-http-request-consumed-params"); + public static final AttributeKey> UNCONSUMED_PARAMS = AttributeKey.newInstance("opensearch-http-request-consumed-params"); public static final AttributeKey CONTEXT_TO_RESTORE = AttributeKey.newInstance( "opensearch-http-request-thread-context" ); diff --git a/src/main/java/org/opensearch/security/ssl/http/netty/Netty4HttpRequestHeaderVerifier.java b/src/main/java/org/opensearch/security/ssl/http/netty/Netty4HttpRequestHeaderVerifier.java index b6ca4ca79c..da66c4c797 100644 --- a/src/main/java/org/opensearch/security/ssl/http/netty/Netty4HttpRequestHeaderVerifier.java +++ b/src/main/java/org/opensearch/security/ssl/http/netty/Netty4HttpRequestHeaderVerifier.java @@ -33,7 +33,7 @@ import io.netty.handler.codec.http.HttpRequest; import io.netty.util.ReferenceCountUtil; -import static org.opensearch.security.http.SecurityHttpServerTransport.CONSUMED_PARAMS; +import static org.opensearch.security.http.SecurityHttpServerTransport.UNCONSUMED_PARAMS; import static org.opensearch.security.http.SecurityHttpServerTransport.CONTEXT_TO_RESTORE; import static org.opensearch.security.http.SecurityHttpServerTransport.EARLY_RESPONSE; import static org.opensearch.security.http.SecurityHttpServerTransport.IS_AUTHENTICATED; @@ -86,9 +86,7 @@ public void channelRead0(ChannelHandlerContext ctx, DefaultHttpRequest msg) thro // If request channel is completed and a response is sent, then there was a failure during authentication restFilter.checkAndAuthenticateRequest(requestChannel); - if (requestChannel instanceof NettyRequest) { - ctx.channel().attr(CONSUMED_PARAMS).set(((NettyRequest) requestChannel).getConsumedParams()); - } + ctx.channel().attr(UNCONSUMED_PARAMS).set(requestChannel.getUnconsumedParams()); ThreadContext.StoredContext contextToRestore = threadPool.getThreadContext().newStoredContext(false); ctx.channel().attr(CONTEXT_TO_RESTORE).set(contextToRestore); From b0b354ceb8b070e7d9c37bbef2535ae027b55c03 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Wed, 14 Feb 2024 05:53:54 -0600 Subject: [PATCH 2/2] SpotlessApply Signed-off-by: Peter Nied --- .../org/opensearch/security/filter/NettyRequest.java | 12 +++--------- .../security/filter/OpenSearchRequest.java | 2 -- .../security/filter/SecurityRestFilter.java | 2 +- .../http/netty/Netty4HttpRequestHeaderVerifier.java | 3 +-- 4 files changed, 5 insertions(+), 14 deletions(-) diff --git a/src/main/java/org/opensearch/security/filter/NettyRequest.java b/src/main/java/org/opensearch/security/filter/NettyRequest.java index d5cd07dc7b..143d94ae9a 100644 --- a/src/main/java/org/opensearch/security/filter/NettyRequest.java +++ b/src/main/java/org/opensearch/security/filter/NettyRequest.java @@ -20,19 +20,13 @@ import java.util.Optional; import java.util.Set; import java.util.TreeMap; -import java.util.stream.Collector; -import java.util.stream.Collectors; - import javax.net.ssl.SSLEngine; -import org.apache.commons.lang3.concurrent.ConcurrentException; -import org.apache.commons.lang3.concurrent.LazyInitializer; -import org.opensearch.http.netty4.Netty4HttpChannel; -import org.opensearch.rest.RestRequest.Method; - import com.google.common.base.Supplier; import com.google.common.base.Suppliers; +import org.opensearch.http.netty4.Netty4HttpChannel; +import org.opensearch.rest.RestRequest.Method; import org.opensearch.rest.RestUtils; import io.netty.handler.codec.http.HttpRequest; @@ -130,7 +124,7 @@ public CheckedAccessMap(final Map map) { public String get(final Object key) { // Never noticed this about java's map interface the getter is not generic if (key instanceof String) { - accessedKeys.add((String)key); + accessedKeys.add((String) key); } return super.get(key); } diff --git a/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java b/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java index 244be6a568..7cca8111c9 100644 --- a/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java +++ b/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java @@ -17,7 +17,6 @@ import java.util.Map; import java.util.Optional; import java.util.Set; - import javax.net.ssl.SSLEngine; import org.opensearch.rest.RestRequest; @@ -86,7 +85,6 @@ public Set getUnconsumedParams() { return Set.of(); } - /** Gets access to the underlying request object */ public RestRequest breakEncapsulationForRequest() { return underlyingRequest; diff --git a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java index 0b721e42f9..b649b8d71f 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java +++ b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java @@ -69,10 +69,10 @@ import static org.opensearch.security.OpenSearchSecurityPlugin.LEGACY_OPENDISTRO_PREFIX; import static org.opensearch.security.OpenSearchSecurityPlugin.PLUGINS_PREFIX; -import static org.opensearch.security.http.SecurityHttpServerTransport.UNCONSUMED_PARAMS; import static org.opensearch.security.http.SecurityHttpServerTransport.CONTEXT_TO_RESTORE; import static org.opensearch.security.http.SecurityHttpServerTransport.EARLY_RESPONSE; import static org.opensearch.security.http.SecurityHttpServerTransport.IS_AUTHENTICATED; +import static org.opensearch.security.http.SecurityHttpServerTransport.UNCONSUMED_PARAMS; public class SecurityRestFilter { diff --git a/src/main/java/org/opensearch/security/ssl/http/netty/Netty4HttpRequestHeaderVerifier.java b/src/main/java/org/opensearch/security/ssl/http/netty/Netty4HttpRequestHeaderVerifier.java index da66c4c797..e6dec0c213 100644 --- a/src/main/java/org/opensearch/security/ssl/http/netty/Netty4HttpRequestHeaderVerifier.java +++ b/src/main/java/org/opensearch/security/ssl/http/netty/Netty4HttpRequestHeaderVerifier.java @@ -14,7 +14,6 @@ import org.opensearch.common.util.concurrent.ThreadContext; import org.opensearch.http.netty4.Netty4HttpChannel; import org.opensearch.http.netty4.Netty4HttpServerTransport; -import org.opensearch.security.filter.NettyRequest; import org.opensearch.security.filter.SecurityRequestChannel; import org.opensearch.security.filter.SecurityRequestChannelUnsupported; import org.opensearch.security.filter.SecurityRequestFactory; @@ -33,11 +32,11 @@ import io.netty.handler.codec.http.HttpRequest; import io.netty.util.ReferenceCountUtil; -import static org.opensearch.security.http.SecurityHttpServerTransport.UNCONSUMED_PARAMS; import static org.opensearch.security.http.SecurityHttpServerTransport.CONTEXT_TO_RESTORE; import static org.opensearch.security.http.SecurityHttpServerTransport.EARLY_RESPONSE; import static org.opensearch.security.http.SecurityHttpServerTransport.IS_AUTHENTICATED; import static org.opensearch.security.http.SecurityHttpServerTransport.SHOULD_DECOMPRESS; +import static org.opensearch.security.http.SecurityHttpServerTransport.UNCONSUMED_PARAMS; @Sharable public class Netty4HttpRequestHeaderVerifier extends SimpleChannelInboundHandler {