JAVA-2850: Ignore credentials in secure connect bundle [DataStax Astra] (#1481)

tomekl007 · web-flow · commit 571fcfca73a6 · 2020-07-24T09:06:07.000+02:00
diff --git a/changelog/README.md b/changelog/README.md
@@ -4,6 +4,7 @@
 
 ### 4.8.0 (in progress)
 
+- [improvement] JAVA-2850: Ignore credentials in secure connect bundle [DataStax Astra]
 - [improvement] JAVA-2813: Don't fail when secure bundle is specified together with other options
 - [bug] JAVA-2800: Exclude SLF4J from mapper-processor dependencies
 - [new feature] JAVA-2819: Add DriverConfigLoader.fromString
diff --git a/core/src/main/java/com/datastax/oss/driver/api/core/session/SessionBuilder.java b/core/src/main/java/com/datastax/oss/driver/api/core/session/SessionBuilder.java
@@ -704,9 +704,6 @@ protected final CompletionStage<CqlSession> buildDefaultSessionAsync() {
         withLocalDatacenter(cloudConfig.getLocalDatacenter());
         withSslEngineFactory(cloudConfig.getSslEngineFactory());
         withCloudProxyAddress(cloudConfig.getProxyAddress());
-        if (cloudConfig.getAuthProvider().isPresent()) {
-          withAuthProvider(cloudConfig.getAuthProvider().get());
-        }
         programmaticArguments = programmaticArgumentsBuilder.build();
       }
 
diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/config/cloud/CloudConfig.java b/core/src/main/java/com/datastax/oss/driver/internal/core/config/cloud/CloudConfig.java
@@ -15,15 +15,12 @@
  */
 package com.datastax.oss.driver.internal.core.config.cloud;
 
-import com.datastax.oss.driver.api.core.auth.AuthProvider;
 import com.datastax.oss.driver.api.core.metadata.EndPoint;
 import com.datastax.oss.driver.api.core.ssl.SslEngineFactory;
 import com.datastax.oss.driver.shaded.guava.common.collect.ImmutableList;
 import edu.umd.cs.findbugs.annotations.NonNull;
-import edu.umd.cs.findbugs.annotations.Nullable;
 import java.net.InetSocketAddress;
 import java.util.List;
-import java.util.Optional;
 import net.jcip.annotations.ThreadSafe;
 
 @ThreadSafe
@@ -33,19 +30,16 @@ public class CloudConfig {
   private final List<EndPoint> endPoints;
   private final String localDatacenter;
   private final SslEngineFactory sslEngineFactory;
-  @Nullable private final AuthProvider authProvider;
 
   CloudConfig(
       @NonNull InetSocketAddress proxyAddress,
       @NonNull List<EndPoint> endPoints,
       @NonNull String localDatacenter,
-      @NonNull SslEngineFactory sslEngineFactory,
-      @Nullable AuthProvider authProvider) {
+      @NonNull SslEngineFactory sslEngineFactory) {
     this.proxyAddress = proxyAddress;
     this.endPoints = ImmutableList.copyOf(endPoints);
     this.localDatacenter = localDatacenter;
     this.sslEngineFactory = sslEngineFactory;
-    this.authProvider = authProvider;
   }
 
   @NonNull
@@ -67,9 +61,4 @@ public String getLocalDatacenter() {
   public SslEngineFactory getSslEngineFactory() {
     return sslEngineFactory;
   }
-
-  @NonNull
-  public Optional<AuthProvider> getAuthProvider() {
-    return Optional.ofNullable(authProvider);
-  }
 }
diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/config/cloud/CloudConfigFactory.java b/core/src/main/java/com/datastax/oss/driver/internal/core/config/cloud/CloudConfigFactory.java
@@ -15,9 +15,7 @@
  */
 package com.datastax.oss.driver.internal.core.config.cloud;
 
-import com.datastax.oss.driver.api.core.auth.AuthProvider;
 import com.datastax.oss.driver.api.core.metadata.EndPoint;
-import com.datastax.oss.driver.internal.core.auth.ProgrammaticPlainTextAuthProvider;
 import com.datastax.oss.driver.internal.core.metadata.SniEndPoint;
 import com.datastax.oss.driver.internal.core.ssl.SniSslEngineFactory;
 import com.datastax.oss.driver.shaded.guava.common.io.ByteStreams;
@@ -26,7 +24,6 @@
 import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import edu.umd.cs.findbugs.annotations.NonNull;
-import edu.umd.cs.findbugs.annotations.Nullable;
 import java.io.BufferedReader;
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
@@ -53,10 +50,12 @@
 import javax.net.ssl.SSLContext;
 import javax.net.ssl.TrustManagerFactory;
 import net.jcip.annotations.ThreadSafe;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 @ThreadSafe
 public class CloudConfigFactory {
-
+  private static final Logger LOG = LoggerFactory.getLogger(CloudConfigFactory.class);
   /**
    * Creates a {@link CloudConfig} with information fetched from the specified Cloud configuration
    * URL.
@@ -138,9 +137,8 @@ public CloudConfig createCloudConfig(@NonNull InputStream cloudConfig)
     List<EndPoint> endPoints = getEndPoints(proxyMetadataJson, sniProxyAddress);
     String localDatacenter = getLocalDatacenter(proxyMetadataJson);
     SniSslEngineFactory sslEngineFactory = new SniSslEngineFactory(sslContext);
-    AuthProvider authProvider = getAuthProvider(configJson);
-    return new CloudConfig(
-        sniProxyAddress, endPoints, localDatacenter, sslEngineFactory, authProvider);
+    validateIfBundleContainsUsernamePassword(configJson);
+    return new CloudConfig(sniProxyAddress, endPoints, localDatacenter, sslEngineFactory);
   }
 
   @NonNull
@@ -176,16 +174,11 @@ protected URL getMetadataServiceUrl(JsonNode configFile) throws MalformedURLExce
     }
   }
 
-  @Nullable
-  protected AuthProvider getAuthProvider(JsonNode configFile) {
-    if (configFile.has("username")) {
-      String username = configFile.get("username").asText();
-      if (configFile.has("password")) {
-        String password = configFile.get("password").asText();
-        return new ProgrammaticPlainTextAuthProvider(username, password);
-      }
+  protected void validateIfBundleContainsUsernamePassword(JsonNode configFile) {
+    if (configFile.has("username") || configFile.has("password")) {
+      LOG.info(
+          "The bundle contains config.json with username and/or password. Providing it in the bundle is deprecated and ignored.");
     }
-    return null;
   }
 
   @NonNull
diff --git a/integration-tests/src/test/java/com/datastax/oss/driver/api/core/cloud/CloudIT.java b/integration-tests/src/test/java/com/datastax/oss/driver/api/core/cloud/CloudIT.java
@@ -27,13 +27,16 @@
 
 import ch.qos.logback.classic.Level;
 import ch.qos.logback.classic.spi.ILoggingEvent;
+import com.datastax.oss.driver.api.core.AllNodesFailedException;
 import com.datastax.oss.driver.api.core.CqlSession;
+import com.datastax.oss.driver.api.core.auth.AuthenticationException;
 import com.datastax.oss.driver.api.core.config.DefaultDriverOption;
 import com.datastax.oss.driver.api.core.config.DriverConfigLoader;
 import com.datastax.oss.driver.api.core.cql.ResultSet;
 import com.datastax.oss.driver.api.core.session.SessionBuilder;
 import com.datastax.oss.driver.api.testinfra.session.SessionUtils;
 import com.datastax.oss.driver.categories.IsolatedTests;
+import com.datastax.oss.driver.internal.core.config.cloud.CloudConfigFactory;
 import com.datastax.oss.driver.internal.core.ssl.DefaultSslEngineFactory;
 import com.datastax.oss.driver.internal.core.util.LoggerTest;
 import com.github.tomakehurst.wiremock.junit.WireMockRule;
@@ -45,6 +48,7 @@
 import java.nio.file.Path;
 import java.security.NoSuchAlgorithmException;
 import java.util.Collections;
+import java.util.List;
 import javax.net.ssl.SSLContext;
 import org.junit.ClassRule;
 import org.junit.Rule;
@@ -67,12 +71,57 @@ public class CloudIT {
   public void should_connect_to_proxy_using_path() {
     ResultSet set;
     Path bundle = proxyRule.getProxy().getDefaultBundlePath();
-    try (CqlSession session = CqlSession.builder().withCloudSecureConnectBundle(bundle).build()) {
+    try (CqlSession session =
+        CqlSession.builder()
+            .withAuthCredentials("cassandra", "cassandra")
+            .withCloudSecureConnectBundle(bundle)
+            .build()) {
       set = session.execute("select * from system.local");
     }
     assertThat(set).isNotNull();
   }
 
+  @Test
+  public void should_connect_and_log_info_that_config_json_with_username_password_was_provided() {
+    ResultSet set;
+    Path bundle = proxyRule.getProxy().getDefaultBundlePath();
+    LoggerTest.LoggerSetup logger = setupTestLogger(CloudConfigFactory.class, Level.INFO);
+
+    try (CqlSession session =
+        CqlSession.builder()
+            .withAuthCredentials("cassandra", "cassandra")
+            .withCloudSecureConnectBundle(bundle)
+            .build()) {
+      set = session.execute("select * from system.local");
+      verify(logger.appender, timeout(500).atLeast(1))
+          .doAppend(logger.loggingEventCaptor.capture());
+      assertThat(
+              logger.loggingEventCaptor.getAllValues().stream()
+                  .map(ILoggingEvent::getFormattedMessage))
+          .contains(
+              "The bundle contains config.json with username and/or password. Providing it in the bundle is deprecated and ignored.");
+    }
+    assertThat(set).isNotNull();
+  }
+
+  @Test
+  public void
+      should_fail_with_auth_error_when_connecting_using_bundle_with_username_password_in_config_json() {
+    Path bundle = proxyRule.getProxy().getDefaultBundlePath();
+
+    // fails with auth error because username/password from config.json is ignored
+    AllNodesFailedException exception = null;
+    try {
+      CqlSession.builder().withCloudSecureConnectBundle(bundle).build();
+    } catch (AllNodesFailedException ex) {
+      exception = ex;
+    }
+    assertThat(exception).isNotNull();
+    List<Throwable> errors = exception.getAllErrors().values().iterator().next();
+    Throwable firstError = errors.get(0);
+    assertThat(firstError).isInstanceOf(AuthenticationException.class);
+  }
+
   @Test
   public void should_connect_to_proxy_without_credentials() {
     ResultSet set;
@@ -91,7 +140,11 @@ public void should_connect_to_proxy_without_credentials() {
   public void should_connect_to_proxy_using_non_normalized_path() {
     Path bundle = proxyRule.getProxy().getBundlesRootPath().resolve("../bundles/creds-v1.zip");
     ResultSet set;
-    try (CqlSession session = CqlSession.builder().withCloudSecureConnectBundle(bundle).build()) {
+    try (CqlSession session =
+        CqlSession.builder()
+            .withAuthCredentials("cassandra", "cassandra")
+            .withCloudSecureConnectBundle(bundle)
+            .build()) {
       set = session.execute("select * from system.local");
     }
     assertThat(set).isNotNull();
@@ -101,7 +154,11 @@ public void should_connect_to_proxy_using_non_normalized_path() {
   public void should_connect_to_proxy_using_input_stream() throws IOException {
     InputStream bundle = Files.newInputStream(proxyRule.getProxy().getDefaultBundlePath());
     ResultSet set;
-    try (CqlSession session = CqlSession.builder().withCloudSecureConnectBundle(bundle).build()) {
+    try (CqlSession session =
+        CqlSession.builder()
+            .withAuthCredentials("cassandra", "cassandra")
+            .withCloudSecureConnectBundle(bundle)
+            .build()) {
       set = session.execute("select * from system.local");
     }
     assertThat(set).isNotNull();
@@ -124,7 +181,10 @@ public void should_connect_to_proxy_using_URL() throws IOException {
     // when
     ResultSet set;
     try (CqlSession session =
-        CqlSession.builder().withCloudSecureConnectBundle(bundleUrl).build()) {
+        CqlSession.builder()
+            .withAuthCredentials("cassandra", "cassandra")
+            .withCloudSecureConnectBundle(bundleUrl)
+            .build()) {
 
       // then
       set = session.execute("select * from system.local");
@@ -142,7 +202,11 @@ public void should_connect_to_proxy_using_absolute_path_provided_in_the_session_
             .build();
     // when
     ResultSet set;
-    try (CqlSession session = CqlSession.builder().withConfigLoader(loader).build()) {
+    try (CqlSession session =
+        CqlSession.builder()
+            .withAuthCredentials("cassandra", "cassandra")
+            .withConfigLoader(loader)
+            .build()) {
 
       // then
       set = session.execute("select * from system.local");
@@ -161,7 +225,11 @@ public void should_connect_to_proxy_using_non_normalized_path_provided_in_the_se
             .build();
     // when
     ResultSet set;
-    try (CqlSession session = CqlSession.builder().withConfigLoader(loader).build()) {
+    try (CqlSession session =
+        CqlSession.builder()
+            .withAuthCredentials("cassandra", "cassandra")
+            .withConfigLoader(loader)
+            .build()) {
 
       // then
       set = session.execute("select * from system.local");
@@ -180,7 +248,11 @@ public void should_connect_to_proxy_using_non_normalized_path_provided_in_the_se
             .build();
     // when
     ResultSet set;
-    try (CqlSession session = CqlSession.builder().withConfigLoader(loader).build()) {
+    try (CqlSession session =
+        CqlSession.builder()
+            .withAuthCredentials("cassandra", "cassandra")
+            .withConfigLoader(loader)
+            .build()) {
 
       // then
       set = session.execute("select * from system.local");
@@ -207,7 +279,11 @@ public void should_connect_to_proxy_using_url_with_http_protocol_provided_in_the
             .build();
     // when
     ResultSet set;
-    try (CqlSession session = CqlSession.builder().withConfigLoader(loader).build()) {
+    try (CqlSession session =
+        CqlSession.builder()
+            .withAuthCredentials("cassandra", "cassandra")
+            .withConfigLoader(loader)
+            .build()) {
 
       // then
       set = session.execute("select * from system.local");

Original file line number	Diff line number	Diff line change
`@@ -704,9 +704,6 @@ protected final CompletionStage<CqlSession> buildDefaultSessionAsync() {`
`704`	`704`	`withLocalDatacenter(cloudConfig.getLocalDatacenter());`
`705`	`705`	`withSslEngineFactory(cloudConfig.getSslEngineFactory());`
`706`	`706`	`withCloudProxyAddress(cloudConfig.getProxyAddress());`
`707`		`- if (cloudConfig.getAuthProvider().isPresent()) {`
`708`		`- withAuthProvider(cloudConfig.getAuthProvider().get());`
`709`		`- }`
`710`	`707`	`programmaticArguments = programmaticArgumentsBuilder.build();`
`711`	`708`	`}`
`712`	`709`