Merge commit 'cwperks/improve-compressed-handling' into improve-compr…

…essed-handling Signed-off-by: Peter Nied <[email protected]>
cwperks · Oct 6, 2023 · 52f7844 · 52f7844
2 parents b891098 + cce6722
commit 52f7844
Show file tree

Hide file tree

Showing 4 changed files with 33 additions and 27 deletions.
diff --git a/src/main/java/org/opensearch/security/filter/NettyRequest.java b/src/main/java/org/opensearch/security/filter/NettyRequest.java
@@ -12,8 +12,6 @@
 package org.opensearch.security.filter;
 
 import java.net.InetSocketAddress;
-import java.net.MalformedURLException;
-import java.net.URL;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
@@ -64,11 +62,8 @@ public SSLEngine getSSLEngine() {
 
     @Override
     public String path() {
-        try {
-            return new URL(underlyingRequest.uri()).getPath();
-        } catch (final MalformedURLException e) {
-            return "";
-        }
+        String rawPath = SecurityRestUtils.path(underlyingRequest.uri());
+        return RestUtils.decodeComponent(rawPath);
     }
 
     @Override

diff --git a/src/main/java/org/opensearch/security/filter/SecurityRestUtils.java b/src/main/java/org/opensearch/security/filter/SecurityRestUtils.java
@@ -0,0 +1,12 @@
+package org.opensearch.security.filter;
+
+public class SecurityRestUtils {
+    public static String path(final String uri) {
+        final int index = uri.indexOf('?');
+        if (index >= 0) {
+            return uri.substring(0, index);
+        } else {
+            return uri;
+        }
+    }
+}
diff --git a/src/main/java/org/opensearch/security/http/XFFResolver.java b/src/main/java/org/opensearch/security/http/XFFResolver.java
@@ -34,11 +34,8 @@
 
 import org.opensearch.OpenSearchSecurityException;
 import org.opensearch.core.common.transport.TransportAddress;
-import org.opensearch.http.netty4.Netty4HttpChannel;
-import org.opensearch.rest.RestRequest;
 import org.opensearch.common.util.concurrent.ThreadContext;
 import org.opensearch.security.filter.SecurityRequest;
-import org.opensearch.security.filter.OpenSearchRequest;
 import org.opensearch.security.securityconf.DynamicConfigModel;
 import org.opensearch.security.support.ConfigConstants;
 import org.opensearch.threadpool.ThreadPool;
@@ -61,15 +58,7 @@ public TransportAddress resolve(final SecurityRequest request) throws OpenSearch
             log.trace("resolve {}", request.getRemoteAddress().orElse(null));
         }
 
-        boolean requestFromNetty = false;
-        if (request instanceof OpenSearchRequest) {
-            final OpenSearchRequest securityRequestChannel = (OpenSearchRequest) request;
-            final RestRequest restRequest = securityRequestChannel.breakEncapsulationForRequest();
-
-            requestFromNetty = restRequest.getHttpChannel() instanceof Netty4HttpChannel;
-        }
-
-        if (enabled && request.getRemoteAddress().isPresent() && requestFromNetty) {
+        if (enabled && request.getRemoteAddress().isPresent()) {
             final InetSocketAddress remoteAddress = request.getRemoteAddress().get();
             final InetSocketAddress isa = new InetSocketAddress(detector.detect(request, threadContext), remoteAddress.getPort());
 

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
@@ -20,11 +20,13 @@
 import io.netty.channel.ChannelHandler.Sharable;
 import io.netty.channel.ChannelHandlerContext;
 import org.opensearch.http.netty4.Netty4HttpChannel;
+import org.opensearch.rest.RestUtils;
 import org.opensearch.security.filter.SecurityRequestChannel;
 import org.opensearch.security.filter.SecurityRequestChannelUnsupported;
 import org.opensearch.security.filter.SecurityRequestFactory;
 import org.opensearch.security.filter.SecurityResponse;
 import org.opensearch.security.filter.SecurityRestFilter;
+import org.opensearch.security.filter.SecurityRestUtils;
 import org.opensearch.security.ssl.transport.SSLConfig;
 import org.opensearch.threadpool.ThreadPool;
 import org.opensearch.security.support.ConfigConstants;
@@ -83,7 +85,9 @@ public void channelRead0(ChannelHandlerContext ctx, DefaultHttpRequest msg) thro
         // TODO: GET PROPER MAVEN BUILD
         // final Netty4HttpChannel httpChannel = ctx.channel().attr(Netty4HttpServerTransport.HTTP_CHANNEL_KEY).get();
         final Netty4HttpChannel httpChannel = ctx.channel().attr(AttributeKey.<Netty4HttpChannel>valueOf("opensearch-http-channel")).get();
-        Matcher matcher = PATTERN_PATH_PREFIX.matcher(msg.uri());
+        String rawPath = SecurityRestUtils.path(msg.uri());
+        String path = RestUtils.decodeComponent(rawPath);
+        Matcher matcher = PATTERN_PATH_PREFIX.matcher(path);
         final String suffix = matcher.matches() ? matcher.group(2) : null;
         if (API_AUTHTOKEN_SUFFIX.equals(suffix)) {
             // TODO: I think this is going to create problems - we should have a sensible size limit, not prevention of
@@ -96,20 +100,26 @@ public void channelRead0(ChannelHandlerContext ctx, DefaultHttpRequest msg) thro
         ThreadContext threadContext = threadPool.getThreadContext();
         try (ThreadContext.StoredContext ignore = threadPool.getThreadContext().stashContext()) {
             injectUser(msg, threadContext);
-            // If request channel is completed and a response is sent, then there was a failure during authentication
-            restFilter.checkAndAuthenticateRequest(requestChannel);
+
+            boolean shouldSkipAuthentication = HttpMethod.OPTIONS.equals(msg.method())
+                || HEALTH_SUFFIX.equals(suffix)
+                || WHO_AM_I_SUFFIX.equals(suffix);
+
+            if (!shouldSkipAuthentication) {
+                // If request channel is completed and a response is sent, then there was a failure during authentication
+                restFilter.checkAndAuthenticateRequest(requestChannel);
+            }
 
             ThreadContext.StoredContext contextToRestore = threadPool.getThreadContext().newStoredContext(false);
             ctx.channel().attr(CONTEXT_TO_RESTORE).set(contextToRestore);
 
             requestChannel.getQueuedResponse().ifPresent(response -> ctx.channel().attr(EARLY_RESPONSE).set(response));
 
-            if (requestChannel.getQueuedResponse().isEmpty()
-                && !HttpMethod.OPTIONS.equals(msg.method())
-                && !HEALTH_SUFFIX.equals(suffix)
-                && !WHO_AM_I_SUFFIX.equals(suffix)) {
+            boolean shouldDecompress = !shouldSkipAuthentication && requestChannel.getQueuedResponse().isEmpty();
+
+            if (requestChannel.getQueuedResponse().isEmpty() || shouldSkipAuthentication) {
                 // Only allow decompression on authenticated requests that also aren't one of those ^
-                ctx.channel().attr(SHOULD_DECOMPRESS).set(Boolean.TRUE);
+                ctx.channel().attr(SHOULD_DECOMPRESS).set(Boolean.valueOf(shouldDecompress));
                 ctx.channel().attr(IS_AUTHENTICATED).set(Boolean.TRUE);
             }
         } catch (final OpenSearchSecurityException e) {