From 8cef4cd5a561769a470cedff3bda030532a645b0 Mon Sep 17 00:00:00 2001 From: Chris Egerton Date: Wed, 3 Feb 2021 16:51:03 -0500 Subject: [PATCH] KAFKA-10895: Gracefully handle invalid JAAS configs (follow up fix) (#9987) Fixes a regression introduced by https://github.com/apache/kafka/pull/9806 in the original fix for KAFKA-10895 It was discovered that if an invalid JAAS config is present on the worker, invoking Configuration::getConfiguration throws an exception. The changes from #9806 cause that exception to be thrown during plugin scanning, which causes the worker to fail even if it is not configured to use the basic auth extension at all. This follow-up handles invalid JAAS configurations more gracefully, and only throws them if the worker is actually configured to use the basic auth extension, at the time that the extension is instantiated and configured. Two unit tests are added. Reviewers: Greg Harris , Konstantine Karantasis --- .../BasicAuthSecurityRestExtension.java | 33 +++++++++++-- .../BasicAuthSecurityRestExtensionTest.java | 49 +++++++++++++++++++ 2 files changed, 79 insertions(+), 3 deletions(-) diff --git a/connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/BasicAuthSecurityRestExtension.java b/connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/BasicAuthSecurityRestExtension.java index 6ef77f0fb7447..8c41762d76b66 100644 --- a/connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/BasicAuthSecurityRestExtension.java +++ b/connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/BasicAuthSecurityRestExtension.java @@ -18,6 +18,7 @@ package org.apache.kafka.connect.rest.basic.auth.extension; import org.apache.kafka.common.utils.AppInfoParser; +import org.apache.kafka.connect.errors.ConnectException; import org.apache.kafka.connect.rest.ConnectRestExtension; import org.apache.kafka.connect.rest.ConnectRestExtensionContext; import org.slf4j.Logger; @@ -26,6 +27,7 @@ import javax.security.auth.login.Configuration; import java.io.IOException; import java.util.Map; +import java.util.function.Supplier; /** * Provides the ability to authenticate incoming BasicAuth credentials using the configured JAAS {@link @@ -62,14 +64,38 @@ public class BasicAuthSecurityRestExtension implements ConnectRestExtension { private static final Logger log = LoggerFactory.getLogger(BasicAuthSecurityRestExtension.class); + private static final Supplier CONFIGURATION = initializeConfiguration(Configuration::getConfiguration); + // Capture the JVM's global JAAS configuration as soon as possible, as it may be altered later // by connectors, converters, other REST extensions, etc. - private static final Configuration CONFIGURATION = Configuration.getConfiguration(); + static Supplier initializeConfiguration(Supplier configurationSupplier) { + try { + Configuration configuration = configurationSupplier.get(); + return () -> configuration; + } catch (Exception e) { + // We have to be careful not to throw anything here as this static block gets executed during plugin scanning and any exceptions will + // cause the worker to fail during startup, even if it's not configured to use the basic auth extension. + return () -> { + throw new ConnectException("Failed to retrieve JAAS configuration", e); + }; + } + } + + private final Supplier configuration; + + public BasicAuthSecurityRestExtension() { + this(CONFIGURATION); + } + + // For testing + BasicAuthSecurityRestExtension(Supplier configuration) { + this.configuration = configuration; + } @Override public void register(ConnectRestExtensionContext restPluginContext) { log.trace("Registering JAAS basic auth filter"); - restPluginContext.configurable().register(new JaasBasicAuthFilter(CONFIGURATION)); + restPluginContext.configurable().register(new JaasBasicAuthFilter(configuration.get())); log.trace("Finished registering JAAS basic auth filter"); } @@ -80,7 +106,8 @@ public void close() throws IOException { @Override public void configure(Map configs) { - + // If we failed to retrieve a JAAS configuration during startup, throw that exception now + configuration.get(); } @Override diff --git a/connect/basic-auth-extension/src/test/java/org/apache/kafka/connect/rest/basic/auth/extension/BasicAuthSecurityRestExtensionTest.java b/connect/basic-auth-extension/src/test/java/org/apache/kafka/connect/rest/basic/auth/extension/BasicAuthSecurityRestExtensionTest.java index aeffa0a5d5f40..3de6f31039d75 100644 --- a/connect/basic-auth-extension/src/test/java/org/apache/kafka/connect/rest/basic/auth/extension/BasicAuthSecurityRestExtensionTest.java +++ b/connect/basic-auth-extension/src/test/java/org/apache/kafka/connect/rest/basic/auth/extension/BasicAuthSecurityRestExtensionTest.java @@ -17,6 +17,7 @@ package org.apache.kafka.connect.rest.basic.auth.extension; +import org.apache.kafka.connect.errors.ConnectException; import org.apache.kafka.connect.rest.ConnectRestExtensionContext; import org.easymock.Capture; import org.easymock.EasyMock; @@ -27,7 +28,15 @@ import javax.security.auth.login.Configuration; import javax.ws.rs.core.Configurable; +import java.io.IOException; +import java.util.Collections; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Supplier; + +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; public class BasicAuthSecurityRestExtensionTest { @@ -66,4 +75,44 @@ public void testJaasConfigurationNotOverwritten() { jaasFilter.getValue().configuration ); } + + @Test + public void testBadJaasConfigInitialization() { + SecurityException jaasConfigurationException = new SecurityException(new IOException("Bad JAAS config is bad")); + Supplier configuration = BasicAuthSecurityRestExtension.initializeConfiguration(() -> { + throw jaasConfigurationException; + }); + + ConnectException thrownException = assertThrows(ConnectException.class, configuration::get); + assertEquals(jaasConfigurationException, thrownException.getCause()); + } + + @Test + public void testGoodJaasConfigInitialization() { + AtomicBoolean configurationInitializerEvaluated = new AtomicBoolean(false); + Configuration mockConfiguration = EasyMock.mock(Configuration.class); + Supplier configuration = BasicAuthSecurityRestExtension.initializeConfiguration(() -> { + configurationInitializerEvaluated.set(true); + return mockConfiguration; + }); + + assertTrue(configurationInitializerEvaluated.get()); + assertEquals(mockConfiguration, configuration.get()); + } + + @Test + public void testBadJaasConfigExtensionSetup() { + SecurityException jaasConfigurationException = new SecurityException(new IOException("Bad JAAS config is bad")); + Supplier configuration = () -> { + throw jaasConfigurationException; + }; + + BasicAuthSecurityRestExtension extension = new BasicAuthSecurityRestExtension(configuration); + + Exception thrownException = assertThrows(Exception.class, () -> extension.configure(Collections.emptyMap())); + assertEquals(jaasConfigurationException, thrownException); + + thrownException = assertThrows(Exception.class, () -> extension.register(EasyMock.mock(ConnectRestExtensionContext.class))); + assertEquals(jaasConfigurationException, thrownException); + } }