[FEATURE] Improve built-in secure transports support (#4179)

Signed-off-by: Andriy Redko <[email protected]>
opensearch-project · Mar 28, 2024 · f375765 · f375765
1 parent a870b40
commit f375765
Show file tree

Hide file tree

Showing 9 changed files with 223 additions and 199 deletions.
diff --git a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java
@@ -107,6 +107,7 @@
 import org.opensearch.extensions.ExtensionsManager;
 import org.opensearch.http.HttpServerTransport;
 import org.opensearch.http.HttpServerTransport.Dispatcher;
+import org.opensearch.http.netty4.ssl.SecureNetty4HttpServerTransport;
 import org.opensearch.identity.Subject;
 import org.opensearch.identity.noop.NoopSubject;
 import org.opensearch.index.IndexModule;
@@ -117,6 +118,7 @@
 import org.opensearch.plugins.ExtensionAwarePlugin;
 import org.opensearch.plugins.IdentityPlugin;
 import org.opensearch.plugins.MapperPlugin;
+import org.opensearch.plugins.SecureHttpTransportSettingsProvider;
 import org.opensearch.plugins.SecureSettingsFactory;
 import org.opensearch.plugins.SecureTransportSettingsProvider;
 import org.opensearch.repositories.RepositoriesService;
@@ -156,7 +158,6 @@
 import org.opensearch.security.filter.SecurityFilter;
 import org.opensearch.security.filter.SecurityRestFilter;
 import org.opensearch.security.http.NonSslHttpServerTransport;
-import org.opensearch.security.http.SecureHttpServerTransport;
 import org.opensearch.security.http.XFFResolver;
 import org.opensearch.security.identity.SecurityTokenManager;
 import org.opensearch.security.privileges.PrivilegesEvaluator;
@@ -239,7 +240,6 @@ public final class OpenSearchSecurityPlugin extends OpenSearchSecuritySSLPlugin
     private volatile PrivilegesEvaluator evaluator;
     private volatile UserService userService;
     private volatile RestLayerPrivilegesEvaluator restLayerEvaluator;
-    private volatile ThreadPool threadPool;
     private volatile ConfigurationRepository cr;
     private volatile AdminDNs adminDns;
     private volatile ClusterService cs;
@@ -927,7 +927,7 @@ public Map<String, Supplier<HttpServerTransport>> getSecureHttpTransports(
         NetworkService networkService,
         Dispatcher dispatcher,
         ClusterSettings clusterSettings,
-        SecureTransportSettingsProvider secureTransportSettingsProvider,
+        SecureHttpTransportSettingsProvider secureHttpTransportSettingsProvider,
         Tracer tracer
     ) {
 
@@ -942,7 +942,7 @@ public Map<String, Supplier<HttpServerTransport>> getSecureHttpTransports(
                 networkService,
                 dispatcher,
                 clusterSettings,
-                secureTransportSettingsProvider,
+                secureHttpTransportSettingsProvider,
                 tracer
             );
         }
@@ -958,7 +958,7 @@ public Map<String, Supplier<HttpServerTransport>> getSecureHttpTransports(
                     evaluateSslExceptionHandler()
                 );
                 // TODO close odshst
-                final SecureHttpServerTransport odshst = new SecureHttpServerTransport(
+                final SecureNetty4HttpServerTransport odshst = new SecureNetty4HttpServerTransport(
                     migrateSettings(settings),
                     networkService,
                     bigArrays,
@@ -967,9 +967,8 @@ public Map<String, Supplier<HttpServerTransport>> getSecureHttpTransports(
                     validatingDispatcher,
                     clusterSettings,
                     sharedGroupFactory,
-                    secureTransportSettingsProvider,
-                    tracer,
-                    securityRestHandler
+                    secureHttpTransportSettingsProvider,
+                    tracer
                 );
 
                 return Collections.singletonMap("org.opensearch.security.http.SecurityHttpServerTransport", () -> odshst);
@@ -985,9 +984,8 @@ public Map<String, Supplier<HttpServerTransport>> getSecureHttpTransports(
                         dispatcher,
                         clusterSettings,
                         sharedGroupFactory,
-                        secureTransportSettingsProvider,
-                        tracer,
-                        securityRestHandler
+                        secureHttpTransportSettingsProvider,
+                        tracer
                     )
                 );
             }
@@ -2032,7 +2030,7 @@ public SecurityTokenManager getTokenManager() {
 
     @Override
     public Optional<SecureSettingsFactory> getSecureSettingFactory(Settings settings) {
-        return Optional.of(new OpenSearchSecureSettingsFactory(settings, sks, sslExceptionHandler));
+        return Optional.of(new OpenSearchSecureSettingsFactory(threadPool, sks, sslExceptionHandler, securityRestHandler));
     }
 
     public static class GuiceHolder implements LifecycleComponent {

diff --git a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java
@@ -55,6 +55,7 @@
 import org.opensearch.security.privileges.RestLayerPrivilegesEvaluator;
 import org.opensearch.security.securityconf.impl.AllowlistingSettings;
 import org.opensearch.security.securityconf.impl.WhitelistingSettings;
+import org.opensearch.security.ssl.http.netty.Netty4HttpRequestHeaderVerifier;
 import org.opensearch.security.ssl.transport.PrincipalExtractor;
 import org.opensearch.security.ssl.util.ExceptionUtils;
 import org.opensearch.security.ssl.util.SSLRequestHelper;
@@ -69,10 +70,6 @@
 
 import static org.opensearch.security.OpenSearchSecurityPlugin.LEGACY_OPENDISTRO_PREFIX;
 import static org.opensearch.security.OpenSearchSecurityPlugin.PLUGINS_PREFIX;
-import static org.opensearch.security.http.SecureHttpServerTransport.CONTEXT_TO_RESTORE;
-import static org.opensearch.security.http.SecureHttpServerTransport.EARLY_RESPONSE;
-import static org.opensearch.security.http.SecureHttpServerTransport.IS_AUTHENTICATED;
-import static org.opensearch.security.http.SecureHttpServerTransport.UNCONSUMED_PARAMS;
 
 public class SecurityRestFilter {
 
@@ -128,15 +125,18 @@ public AuthczRestHandler(RestHandler original, AdminDNs adminDNs) {
 
         @Override
         public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception {
-            final Optional<SecurityResponse> maybeSavedResponse = NettyAttribute.popFrom(request, EARLY_RESPONSE);
+            final Optional<SecurityResponse> maybeSavedResponse = NettyAttribute.popFrom(
+                request,
+                Netty4HttpRequestHeaderVerifier.EARLY_RESPONSE
+            );
             if (maybeSavedResponse.isPresent()) {
-                NettyAttribute.clearAttribute(request, CONTEXT_TO_RESTORE);
-                NettyAttribute.clearAttribute(request, IS_AUTHENTICATED);
+                NettyAttribute.clearAttribute(request, Netty4HttpRequestHeaderVerifier.CONTEXT_TO_RESTORE);
+                NettyAttribute.clearAttribute(request, Netty4HttpRequestHeaderVerifier.IS_AUTHENTICATED);
                 channel.sendResponse(maybeSavedResponse.get().asRestResponse());
                 return;
             }
 
-            NettyAttribute.popFrom(request, CONTEXT_TO_RESTORE).ifPresent(storedContext -> {
+            NettyAttribute.popFrom(request, Netty4HttpRequestHeaderVerifier.CONTEXT_TO_RESTORE).ifPresent(storedContext -> {
                 // X_OPAQUE_ID will be overritten on restore - save to apply after restoring the saved context
                 final String xOpaqueId = threadContext.getHeader(Task.X_OPAQUE_ID);
                 storedContext.restore();
@@ -145,7 +145,7 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c
                 }
             });
 
-            NettyAttribute.popFrom(request, UNCONSUMED_PARAMS).ifPresent(unconsumedParams -> {
+            NettyAttribute.popFrom(request, Netty4HttpRequestHeaderVerifier.UNCONSUMED_PARAMS).ifPresent(unconsumedParams -> {
                 for (String unconsumedParam : unconsumedParams) {
                     // Consume the parameter on the RestRequest
                     request.param(unconsumedParam);
@@ -155,7 +155,7 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c
             final SecurityRequestChannel requestChannel = SecurityRequestFactory.from(request, channel);
 
             // Authenticate request
-            if (!NettyAttribute.popFrom(request, IS_AUTHENTICATED).orElse(false)) {
+            if (!NettyAttribute.popFrom(request, Netty4HttpRequestHeaderVerifier.IS_AUTHENTICATED).orElse(false)) {
                 // we aren't authenticated so we should skip this step
                 checkAndAuthenticateRequest(requestChannel);
             }

diff --git a/src/main/java/org/opensearch/security/http/NonSslHttpServerTransport.java b/src/main/java/org/opensearch/security/http/NonSslHttpServerTransport.java
@@ -34,22 +34,15 @@
 import org.opensearch.http.HttpHandlingSettings;
 import org.opensearch.http.netty4.Netty4HttpServerTransport;
 import org.opensearch.http.netty4.ssl.SecureNetty4HttpServerTransport;
-import org.opensearch.plugins.SecureTransportSettingsProvider;
-import org.opensearch.security.filter.SecurityRestFilter;
-import org.opensearch.security.ssl.http.netty.Netty4ConditionalDecompressor;
-import org.opensearch.security.ssl.http.netty.Netty4HttpRequestHeaderVerifier;
+import org.opensearch.plugins.SecureHttpTransportSettingsProvider;
 import org.opensearch.telemetry.tracing.Tracer;
 import org.opensearch.threadpool.ThreadPool;
 import org.opensearch.transport.SharedGroupFactory;
 
 import io.netty.channel.Channel;
 import io.netty.channel.ChannelHandler;
-import io.netty.channel.ChannelInboundHandlerAdapter;
 
 public class NonSslHttpServerTransport extends SecureNetty4HttpServerTransport {
-
-    private final ChannelInboundHandlerAdapter headerVerifier;
-
     public NonSslHttpServerTransport(
         final Settings settings,
         final NetworkService networkService,
@@ -59,9 +52,8 @@ public NonSslHttpServerTransport(
         final Dispatcher dispatcher,
         final ClusterSettings clusterSettings,
         final SharedGroupFactory sharedGroupFactory,
-        final SecureTransportSettingsProvider secureTransportSettingsProvider,
-        final Tracer tracer,
-        final SecurityRestFilter restFilter
+        final SecureHttpTransportSettingsProvider secureHttpTransportSettingsProvider,
+        final Tracer tracer
     ) {
         super(
             settings,
@@ -72,10 +64,9 @@ public NonSslHttpServerTransport(
             dispatcher,
             clusterSettings,
             sharedGroupFactory,
-            secureTransportSettingsProvider,
+            secureHttpTransportSettingsProvider,
             tracer
         );
-        headerVerifier = new Netty4HttpRequestHeaderVerifier(restFilter, threadPool, settings);
     }
 
     @Override
@@ -94,14 +85,4 @@ protected void initChannel(Channel ch) throws Exception {
             super.initChannel(ch);
         }
     }
-
-    @Override
-    protected ChannelInboundHandlerAdapter createHeaderVerifier() {
-        return headerVerifier;
-    }
-
-    @Override
-    protected ChannelInboundHandlerAdapter createDecompressor() {
-        return new Netty4ConditionalDecompressor();
-    }
 }
diff --git a/src/main/java/org/opensearch/security/http/SecureHttpServerTransport.java b/src/main/java/org/opensearch/security/http/SecureHttpServerTransport.java