JAVA-1792: Add AuthProvider callback to handle missing challenge from server

olim7t · olim7t · commit a2bc0bab15f3 · 2018-04-24T16:51:37.000-07:00
diff --git a/changelog/README.md b/changelog/README.md
@@ -4,6 +4,7 @@
 
 ### 4.0.0-alpha4 (in progress)
 
+- [improvement] JAVA-1792: Add AuthProvider callback to handle missing challenge from server
 - [improvement] JAVA-1775: Assume default packages for built-in policies
 - [improvement] JAVA-1774: Standardize policy locations
 - [improvement] JAVA-1798: Allow passing the default LBP filter as a session builder argument
diff --git a/core/src/main/java/com/datastax/oss/driver/api/core/auth/AuthProvider.java b/core/src/main/java/com/datastax/oss/driver/api/core/auth/AuthProvider.java
@@ -15,6 +15,7 @@
  */
 package com.datastax.oss.driver.api.core.auth;
 
+import com.datastax.oss.driver.api.core.connection.ReconnectionPolicy;
 import com.datastax.oss.driver.internal.core.auth.PlainTextAuthProvider;
 import java.net.SocketAddress;
 
@@ -35,4 +36,22 @@ public interface AuthProvider {
    */
   Authenticator newAuthenticator(SocketAddress host, String serverAuthenticator)
       throws AuthenticationException;
+
+  /**
+   * What to do if the server does not send back an authentication challenge (in other words, lets
+   * the client connect without any form of authentication).
+   *
+   * <p>This is suspicious because having authentication enabled on the client but not on the server
+   * is probably a configuration mistake.
+   *
+   * <p>Provider implementations are free to handle this however they want; typical approaches are:
+   *
+   * <ul>
+   *   <li>ignoring;
+   *   <li>logging a warning;
+   *   <li>throwing an {@link AuthenticationException} to abort the connection (but note that it
+   *       will be retried according to the {@link ReconnectionPolicy}).
+   * </ul>
+   */
+  void onMissingChallenge(SocketAddress host) throws AuthenticationException;
 }
diff --git a/core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java b/core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java
@@ -109,7 +109,6 @@ public enum DefaultDriverOption implements DriverOption {
   AUTH_PROVIDER_CLASS("protocol.auth-provider.class", false),
   AUTH_PROVIDER_USER_NAME("protocol.auth-provider.username", false),
   AUTH_PROVIDER_PASSWORD("protocol.auth-provider.password", false),
-  AUTH_PROVIDER_WARN_IF_NO_SERVER_AUTH("protocol.auth-provider.warn-if-no-server-auth", false),
 
   SSL_ENGINE_FACTORY_CLASS("ssl-engine-factory.class", false),
   SSL_CIPHER_SUITES("ssl-engine-factory.cipher-suites", false),
diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/auth/PlainTextAuthProvider.java b/core/src/main/java/com/datastax/oss/driver/internal/core/auth/PlainTextAuthProvider.java
@@ -25,6 +25,8 @@
 import java.net.SocketAddress;
 import java.nio.ByteBuffer;
 import net.jcip.annotations.ThreadSafe;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /**
  * A simple authentication provider that supports SASL authentication using the PLAIN mechanism for
@@ -48,10 +50,14 @@
 @ThreadSafe
 public class PlainTextAuthProvider implements AuthProvider {
 
+  private static final Logger LOG = LoggerFactory.getLogger(PlainTextAuthProvider.class);
+
+  private final String logPrefix;
   private final DriverConfigProfile config;
 
   /** Builds a new instance. */
   public PlainTextAuthProvider(DriverContext context) {
+    this.logPrefix = context.sessionName();
     this.config = context.config().getDefaultProfile();
   }
 
@@ -62,6 +68,15 @@ public Authenticator newAuthenticator(SocketAddress host, String serverAuthentic
     return new PlainTextAuthenticator(username, password);
   }
 
+  @Override
+  public void onMissingChallenge(SocketAddress host) {
+    LOG.warn(
+        "[{}] {} did not send an authentication challenge; "
+            + "This is suspicious because the driver expects authentication",
+        logPrefix,
+        host);
+  }
+
   private static class PlainTextAuthenticator implements SyncAuthenticator {
 
     private final ByteBuffer initialToken;
diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandler.java b/core/src/main/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandler.java
@@ -61,7 +61,6 @@ class ProtocolInitHandler extends ConnectInitHandler {
 
   private final InternalDriverContext context;
   private final long timeoutMillis;
-  private final boolean warnIfNoServerAuth;
   private final ProtocolVersion initialProtocolVersion;
   private final DriverChannelOptions options;
   // might be null if this is the first channel to this cluster
@@ -83,8 +82,6 @@ class ProtocolInitHandler extends ConnectInitHandler {
 
     this.timeoutMillis =
         defaultConfig.getDuration(DefaultDriverOption.CONNECTION_INIT_QUERY_TIMEOUT).toMillis();
-    this.warnIfNoServerAuth =
-        defaultConfig.getBoolean(DefaultDriverOption.AUTH_PROVIDER_WARN_IF_NO_SERVER_AUTH);
     this.initialProtocolVersion = protocolVersion;
     this.expectedClusterName = expectedClusterName;
     this.options = options;
@@ -168,15 +165,9 @@ void onResponse(Message response) {
           ProtocolUtils.opcodeString(response.opcode));
       try {
         if (step == Step.STARTUP && response instanceof Ready) {
-          if (warnIfNoServerAuth && context.authProvider().isPresent()) {
-            LOG.warn(
-                "[{}] {} did not send an authentication challenge; "
-                    + "This is suspicious because the driver expects authentication "
-                    + "(configured auth provider = {})",
-                logPrefix,
-                channel.remoteAddress(),
-                context.authProvider().get().getClass().getName());
-          }
+          context
+              .authProvider()
+              .ifPresent(provider -> provider.onMissingChallenge(channel.remoteAddress()));
           step = Step.GET_CLUSTER_NAME;
           send();
         } else if (step == Step.STARTUP && response instanceof Authenticate) {
diff --git a/core/src/main/resources/reference.conf b/core/src/main/resources/reference.conf
@@ -74,17 +74,6 @@ datastax-java-driver {
       # Sample configuration for the plain-text provider:
       // username = cassandra
       // password = cassandra
-
-      # Whether to log a warning if an authentication provider is configured on the driver side, but
-      # the server does not issue an authentication challenge (i.e. lets the driver connect without
-      # any form of authentication).
-      #
-      # This is intended as a help to detect server configuration issues. The warning will be logged
-      # for every faulty node, and each time a new connection is created.
-      #
-      # This option can be changed at runtime, the new value will be used for new connections created
-      # after the change.
-      warn-if-no-server-auth = true
     }
 
     # The compressor to use for protocol frames. If it is not qualified, the driver assumes that it
diff --git a/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandlerTest.java b/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandlerTest.java
@@ -16,12 +16,7 @@
 package com.datastax.oss.driver.internal.core.channel;
 
 import static com.datastax.oss.driver.Assertions.assertThat;
-import static org.mockito.Mockito.atLeast;
 
-import ch.qos.logback.classic.Level;
-import ch.qos.logback.classic.Logger;
-import ch.qos.logback.classic.spi.ILoggingEvent;
-import ch.qos.logback.core.Appender;
 import com.datastax.oss.driver.api.core.CqlIdentifier;
 import com.datastax.oss.driver.api.core.DefaultProtocolVersion;
 import com.datastax.oss.driver.api.core.InvalidKeyspaceException;
@@ -56,16 +51,11 @@
 import java.util.List;
 import java.util.Optional;
 import java.util.concurrent.TimeUnit;
-import org.assertj.core.api.filter.Filters;
-import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
-import org.mockito.ArgumentCaptor;
-import org.mockito.Captor;
 import org.mockito.Mock;
 import org.mockito.Mockito;
 import org.mockito.MockitoAnnotations;
-import org.slf4j.LoggerFactory;
 
 public class ProtocolInitHandlerTest extends ChannelHandlerTestBase {
 
@@ -75,13 +65,10 @@ public class ProtocolInitHandlerTest extends ChannelHandlerTestBase {
   @Mock private DriverConfig driverConfig;
   @Mock private DriverConfigProfile defaultConfigProfile;
   @Mock private Compressor<ByteBuf> compressor;
-  @Mock private Appender<ILoggingEvent> appender;
-  @Captor private ArgumentCaptor<ILoggingEvent> loggingEventCaptor;
 
   private ProtocolVersionRegistry protocolVersionRegistry =
       new CassandraProtocolVersionRegistry("test");
   private HeartbeatHandler heartbeatHandler;
-  private Logger logger;
 
   @Before
   @Override
@@ -96,10 +83,6 @@ public void setup() {
     Mockito.when(
             defaultConfigProfile.getDuration(DefaultDriverOption.CONNECTION_HEARTBEAT_INTERVAL))
         .thenReturn(Duration.ofSeconds(30));
-    Mockito.when(
-            defaultConfigProfile.getBoolean(
-                DefaultDriverOption.AUTH_PROVIDER_WARN_IF_NO_SERVER_AUTH))
-        .thenReturn(true);
     Mockito.when(internalDriverContext.protocolVersionRegistry())
         .thenReturn(protocolVersionRegistry);
     Mockito.when(internalDriverContext.compressor()).thenReturn(compressor);
@@ -119,14 +102,6 @@ public void setup() {
                 "test"));
 
     heartbeatHandler = new HeartbeatHandler(defaultConfigProfile);
-
-    logger = (Logger) LoggerFactory.getLogger(ProtocolInitHandler.class);
-    logger.addAppender(appender);
-  }
-
-  @After
-  public void teardown() {
-    logger.detachAppender(appender);
   }
 
   @Test
@@ -321,7 +296,7 @@ public void should_initialize_with_authentication() {
   }
 
   @Test
-  public void should_warn_if_auth_configured_but_server_does_not_send_challenge() {
+  public void should_invoke_auth_provider_when_server_does_not_send_challenge() {
     channel
         .pipeline()
         .addLast(
@@ -341,16 +316,11 @@ public void should_warn_if_auth_configured_but_server_does_not_send_challenge()
     Frame requestFrame = readOutboundFrame();
     assertThat(requestFrame.message).isInstanceOf(Startup.class);
 
-    // Simulate a READY response, a warning should be logged
+    // Simulate a READY response, the provider should be notified
     writeInboundFrame(buildInboundFrame(requestFrame, new Ready()));
-    Mockito.verify(appender, atLeast(1)).doAppend(loggingEventCaptor.capture());
-    Iterable<ILoggingEvent> warnLogs =
-        Filters.filter(loggingEventCaptor.getAllValues()).with("level", Level.WARN).get();
-    assertThat(warnLogs).hasSize(1);
-    assertThat(warnLogs.iterator().next().getFormattedMessage())
-        .contains("did not send an authentication challenge");
-
-    // Apart from the warning, init should proceed normally
+    Mockito.verify(authProvider).onMissingChallenge(channel.remoteAddress());
+
+    // Since our mock does nothing, init should proceed normally
     requestFrame = readOutboundFrame();
     assertThat(requestFrame.message).isInstanceOf(Query.class);
     writeInboundFrame(requestFrame, TestResponses.clusterNameResponse("someClusterName"));
