absurdfarce commented on code in PR #1907:
URL:
https://github.com/apache/cassandra-java-driver/pull/1907#discussion_r1470651460
##########
core/src/main/resources/reference.conf:
##########
@@ -790,6 +790,13 @@ datastax-java-driver {
// truststore-password = password123
// keystore-path = /path/to/client.keystore
// keystore-password = password123
+
+ # The duration between attempts to reload the keystore from the contents
of the file specified
+ # by `keystore-path`. This is mainly relevant in environments where
certificates have short
+ # lifetimes and applications are restarted infrequently, since an expired
client certificate
+ # will prevent new connections from being established until the
application is restarted. If
+ # not set, defaults to not reload the keystore.
+ // keystore-reload-interval = 30 minutes
Review Comment:
As discussed in the DefaultSslEngineFactory code above this would no longer
be commented out and would have a value of 0 minutes.
##########
core/src/main/java/com/datastax/oss/driver/internal/core/ssl/DefaultSslEngineFactory.java:
##########
@@ -54,6 +55,7 @@
* truststore-password = password123
* keystore-path = /path/to/client.keystore
* keystore-password = password123
+ * keystore-reload-interval = 30 minutes
Review Comment:
Following on from the conversation below it might be worth a comment here
saying something like "We enable keystore reloading by specifying a non-zero
interval here" or something similar.
##########
core/src/main/java/com/datastax/oss/driver/internal/core/ssl/DefaultSslEngineFactory.java:
##########
@@ -159,8 +150,22 @@ protected SSLContext buildContext(DriverExecutionProfile
config) throws Exceptio
}
}
+ private ReloadingKeyManagerFactory
buildReloadingKeyManagerFactory(DriverExecutionProfile config)
+ throws Exception {
+ Path keystorePath =
Paths.get(config.getString(DefaultDriverOption.SSL_KEYSTORE_PATH));
+ String password =
+ config.isDefined(DefaultDriverOption.SSL_KEYSTORE_PASSWORD)
+ ? config.getString(DefaultDriverOption.SSL_KEYSTORE_PASSWORD)
+ : null;
Review Comment:
DriverExecutionProfile includes getters with default values so
config.getString(DefaultDriverOption.SSL_KEYSTORE_PASSWORD, null) is probably a
cleaner way to express this.
##########
core/src/main/java/com/datastax/oss/driver/internal/core/ssl/ReloadingKeyManagerFactory.java:
##########
@@ -0,0 +1,253 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.datastax.oss.driver.internal.core.ssl;
+
+import
com.datastax.oss.driver.shaded.guava.common.annotations.VisibleForTesting;
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.net.Socket;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
+import java.security.Principal;
+import java.security.PrivateKey;
+import java.security.Provider;
+import java.security.UnrecoverableKeyException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import java.time.Duration;
+import java.util.Arrays;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicReference;
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.KeyManagerFactorySpi;
+import javax.net.ssl.ManagerFactoryParameters;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.X509ExtendedKeyManager;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ReloadingKeyManagerFactory extends KeyManagerFactory implements
AutoCloseable {
+ private static final Logger logger =
LoggerFactory.getLogger(ReloadingKeyManagerFactory.class);
+ private static final String KEYSTORE_TYPE = "JKS";
+ private Path keystorePath;
+ private String keystorePassword;
+ private ScheduledExecutorService executor;
+ private final Spi spi;
+
+ // We're using a single thread executor so this shouldn't need to be
volatile, since all updates
+ // to lastDigest should come from the same thread
+ private volatile byte[] lastDigest;
+
+ /**
+ * Create a new {@link ReloadingKeyManagerFactory} with the given keystore
file and password,
+ * reloading from the file's content at the given interval. This function
will do an initial
+ * reload before returning, to confirm that the file exists and is readable.
+ *
+ * @param keystorePath the keystore file to reload
+ * @param keystorePassword the keystore password
+ * @param reloadInterval the duration between reload attempts. Set to {@link
+ * java.time.Duration#ZERO} to disable scheduled reloading.
+ * @return
+ */
+ public static ReloadingKeyManagerFactory create(
+ Path keystorePath, String keystorePassword, Duration reloadInterval)
+ throws UnrecoverableKeyException, KeyStoreException,
NoSuchAlgorithmException,
+ CertificateException, IOException {
+ KeyManagerFactory kmf =
KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm());
+
+ KeyStore ks;
+ try (InputStream ksf = Files.newInputStream(keystorePath)) {
+ ks = KeyStore.getInstance(KEYSTORE_TYPE);
+ ks.load(ksf, keystorePassword.toCharArray());
+ }
+ kmf.init(ks, keystorePassword.toCharArray());
+
+ ReloadingKeyManagerFactory reloadingKeyManagerFactory = new
ReloadingKeyManagerFactory(kmf);
+ reloadingKeyManagerFactory.start(keystorePath, keystorePassword,
reloadInterval);
+ return reloadingKeyManagerFactory;
+ }
+
+ @VisibleForTesting
+ protected ReloadingKeyManagerFactory(KeyManagerFactory initial) {
+ this(
+ new Spi((X509ExtendedKeyManager) initial.getKeyManagers()[0]),
+ initial.getProvider(),
+ initial.getAlgorithm());
+ }
+
+ private ReloadingKeyManagerFactory(Spi spi, Provider provider, String
algorithm) {
+ super(spi, provider, algorithm);
+ this.spi = spi;
+ }
+
+ private void start(Path keystorePath, String keystorePassword, Duration
reloadInterval) {
+ this.keystorePath = keystorePath;
+ this.keystorePassword = keystorePassword;
+
+ // Ensure that reload is called once synchronously, to make sure the file
exists etc.
+ reload();
+
+ if (!reloadInterval.isZero()) {
+ this.executor =
+ Executors.newScheduledThreadPool(
+ 1,
+ runnable -> {
+ Thread t =
Executors.defaultThreadFactory().newThread(runnable);
+ t.setName(String.format("%s-%%d",
this.getClass().getSimpleName()));
+ t.setDaemon(true);
+ return t;
+ });
+ this.executor.scheduleWithFixedDelay(
+ this::reload,
+ reloadInterval.toMillis(),
+ reloadInterval.toMillis(),
+ TimeUnit.MILLISECONDS);
+ }
+ }
+
+ @VisibleForTesting
+ void reload() {
+ try {
+ reload0();
+ } catch (Exception e) {
+ String msg =
+ "Failed to reload KeyStore. If this continues to happen, your client
may use stale identity"
+ + "certificates and fail to re-establish connections to
Cassandra hosts.";
Review Comment:
Nit: this should be " certificates" in order to make the spacing of the full
sentence work out.
##########
core/src/main/java/com/datastax/oss/driver/internal/core/ssl/ReloadingKeyManagerFactory.java:
##########
@@ -0,0 +1,253 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.datastax.oss.driver.internal.core.ssl;
+
+import
com.datastax.oss.driver.shaded.guava.common.annotations.VisibleForTesting;
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.net.Socket;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
+import java.security.Principal;
+import java.security.PrivateKey;
+import java.security.Provider;
+import java.security.UnrecoverableKeyException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import java.time.Duration;
+import java.util.Arrays;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicReference;
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.KeyManagerFactorySpi;
+import javax.net.ssl.ManagerFactoryParameters;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.X509ExtendedKeyManager;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ReloadingKeyManagerFactory extends KeyManagerFactory implements
AutoCloseable {
+ private static final Logger logger =
LoggerFactory.getLogger(ReloadingKeyManagerFactory.class);
+ private static final String KEYSTORE_TYPE = "JKS";
+ private Path keystorePath;
+ private String keystorePassword;
+ private ScheduledExecutorService executor;
+ private final Spi spi;
+
+ // We're using a single thread executor so this shouldn't need to be
volatile, since all updates
+ // to lastDigest should come from the same thread
+ private volatile byte[] lastDigest;
+
+ /**
+ * Create a new {@link ReloadingKeyManagerFactory} with the given keystore
file and password,
+ * reloading from the file's content at the given interval. This function
will do an initial
+ * reload before returning, to confirm that the file exists and is readable.
+ *
+ * @param keystorePath the keystore file to reload
+ * @param keystorePassword the keystore password
+ * @param reloadInterval the duration between reload attempts. Set to {@link
+ * java.time.Duration#ZERO} to disable scheduled reloading.
+ * @return
+ */
+ public static ReloadingKeyManagerFactory create(
+ Path keystorePath, String keystorePassword, Duration reloadInterval)
+ throws UnrecoverableKeyException, KeyStoreException,
NoSuchAlgorithmException,
+ CertificateException, IOException {
+ KeyManagerFactory kmf =
KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm());
+
+ KeyStore ks;
+ try (InputStream ksf = Files.newInputStream(keystorePath)) {
+ ks = KeyStore.getInstance(KEYSTORE_TYPE);
+ ks.load(ksf, keystorePassword.toCharArray());
+ }
+ kmf.init(ks, keystorePassword.toCharArray());
+
+ ReloadingKeyManagerFactory reloadingKeyManagerFactory = new
ReloadingKeyManagerFactory(kmf);
+ reloadingKeyManagerFactory.start(keystorePath, keystorePassword,
reloadInterval);
+ return reloadingKeyManagerFactory;
+ }
+
+ @VisibleForTesting
+ protected ReloadingKeyManagerFactory(KeyManagerFactory initial) {
+ this(
+ new Spi((X509ExtendedKeyManager) initial.getKeyManagers()[0]),
+ initial.getProvider(),
+ initial.getAlgorithm());
+ }
+
+ private ReloadingKeyManagerFactory(Spi spi, Provider provider, String
algorithm) {
+ super(spi, provider, algorithm);
+ this.spi = spi;
+ }
+
+ private void start(Path keystorePath, String keystorePassword, Duration
reloadInterval) {
+ this.keystorePath = keystorePath;
+ this.keystorePassword = keystorePassword;
+
+ // Ensure that reload is called once synchronously, to make sure the file
exists etc.
+ reload();
+
+ if (!reloadInterval.isZero()) {
+ this.executor =
+ Executors.newScheduledThreadPool(
+ 1,
+ runnable -> {
+ Thread t =
Executors.defaultThreadFactory().newThread(runnable);
+ t.setName(String.format("%s-%%d",
this.getClass().getSimpleName()));
+ t.setDaemon(true);
+ return t;
+ });
+ this.executor.scheduleWithFixedDelay(
+ this::reload,
+ reloadInterval.toMillis(),
+ reloadInterval.toMillis(),
+ TimeUnit.MILLISECONDS);
+ }
+ }
+
+ @VisibleForTesting
+ void reload() {
+ try {
+ reload0();
+ } catch (Exception e) {
+ String msg =
+ "Failed to reload KeyStore. If this continues to happen, your client
may use stale identity"
+ + "certificates and fail to re-establish connections to
Cassandra hosts.";
+ logger.warn(msg, e);
+ }
+ }
+
+ private synchronized void reload0()
+ throws NoSuchAlgorithmException, IOException, KeyStoreException,
CertificateException,
+ UnrecoverableKeyException {
+ logger.debug("Checking KeyStore file {} for updates", keystorePath);
+
+ final byte[] keyStoreBytes = Files.readAllBytes(keystorePath);
+ final byte[] newDigest = digest(keyStoreBytes);
+ if (lastDigest != null && Arrays.equals(lastDigest,
digest(keyStoreBytes))) {
+ logger.debug("KeyStore file content has not changed; skipping update");
+ return;
+ }
+
+ final KeyStore keyStore = KeyStore.getInstance(KEYSTORE_TYPE);
+ try (InputStream inputStream = new ByteArrayInputStream(keyStoreBytes)) {
+ keyStore.load(inputStream, keystorePassword.toCharArray());
+ }
+ KeyManagerFactory kmf =
KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm());
+ kmf.init(keyStore, keystorePassword.toCharArray());
+ logger.info("Detected updates to KeyStore file {}", keystorePath);
+
+ this.spi.keyManager.set((X509ExtendedKeyManager) kmf.getKeyManagers()[0]);
+ this.lastDigest = newDigest;
+ }
Review Comment:
Seems like this operation should generalize nicely to a CAS op which would
allow you to avoid the synchronized usage here.
##########
core/src/main/java/com/datastax/oss/driver/internal/core/ssl/DefaultSslEngineFactory.java:
##########
@@ -159,8 +150,22 @@ protected SSLContext buildContext(DriverExecutionProfile
config) throws Exceptio
}
}
+ private ReloadingKeyManagerFactory
buildReloadingKeyManagerFactory(DriverExecutionProfile config)
+ throws Exception {
+ Path keystorePath =
Paths.get(config.getString(DefaultDriverOption.SSL_KEYSTORE_PATH));
+ String password =
+ config.isDefined(DefaultDriverOption.SSL_KEYSTORE_PASSWORD)
+ ? config.getString(DefaultDriverOption.SSL_KEYSTORE_PASSWORD)
+ : null;
+ Duration reloadInterval =
+ config.isDefined(DefaultDriverOption.SSL_KEYSTORE_RELOAD_INTERVAL)
+ ?
config.getDuration(DefaultDriverOption.SSL_KEYSTORE_RELOAD_INTERVAL)
+ : Duration.ZERO;
Review Comment:
The convention in the driver config is for configs that have a legit default
value to be included in reference.conf; the commented-out configs are intended
to provide examples of what the user could do. In this case I'd argue the
keystore-reload-interval should be included directly in reference.conf (without
being commented out) with a value of 0 minutes. That's actually the default we
want here... it's on the user to override that with their app config if they
wish to do so.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
