Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SEC-1473 Allow no Truststore password #206

Open
wants to merge 3 commits into
base: 5.5.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions core/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -142,5 +142,10 @@
<version>${guava.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.awaitility</groupId>
<artifactId>awaitility</artifactId>
<scope>test</scope>
</dependency>
</dependencies>
</project>
4 changes: 3 additions & 1 deletion core/src/main/java/io/confluent/rest/Application.java
Original file line number Diff line number Diff line change
Expand Up @@ -545,7 +545,9 @@ public void join() throws InterruptedException {
* @throws Exception If the application fails to stop
*/
public void stop() throws Exception {
server.stop();
if (server != null) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be the case if there was an exception thrown during start() and we stop() in a finally block... as we do in the tests.

server.stop();
}
}

final void doShutdown() {
Expand Down
18 changes: 14 additions & 4 deletions core/src/main/java/io/confluent/rest/ApplicationServer.java
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ public final class ApplicationServer<T extends RestConfig> extends Server {
private final T config;
private final ApplicationGroup applications;
private final SslContextFactory sslContextFactory;
private FileWatcher sslKeystoreFileWatcher;

private List<NetworkTrafficServerConnector> connectors = new ArrayList<>();

Expand Down Expand Up @@ -177,6 +178,9 @@ private void finalizeHandlerCollection(HandlerCollection handlers, HandlerCollec
protected void doStop() throws Exception {
super.doStop();
applications.doStop();
if (sslKeystoreFileWatcher != null) {
sslKeystoreFileWatcher.shutdown();
}
}

protected final void doStart() throws Exception {
Expand Down Expand Up @@ -243,7 +247,9 @@ private Path getWatchLocation(RestConfig config) {
return keystorePath;
}

// CHECKSTYLE_RULES.OFF: CyclomaticComplexity|NPathComplexity
private SslContextFactory createSslContextFactory(RestConfig config) {
// CHECKSTYLE_RULES.ON: CyclomaticComplexity|NPathComplexity
SslContextFactory sslContextFactory = new SslContextFactory.Server();
if (!config.getString(RestConfig.SSL_KEYSTORE_LOCATION_CONFIG).isEmpty()) {
sslContextFactory.setKeyStorePath(
Expand All @@ -267,7 +273,9 @@ private SslContextFactory createSslContextFactory(RestConfig config) {
if (config.getBoolean(RestConfig.SSL_KEYSTORE_RELOAD_CONFIG)) {
Path watchLocation = getWatchLocation(config);
try {
FileWatcher.onFileChange(watchLocation, () -> {
// create and shutdown a sslKeystoreFileWatcher for each Application, so that
// all Applications in the same JVM don't use the same shared threadpool
sslKeystoreFileWatcher = FileWatcher.onFileChange(watchLocation, () -> {
// Need to reset the key store path for symbolic link case
sslContextFactory.setKeyStorePath(
config.getString(RestConfig.SSL_KEYSTORE_LOCATION_CONFIG)
Expand Down Expand Up @@ -301,9 +309,11 @@ private SslContextFactory createSslContextFactory(RestConfig config) {
sslContextFactory.setTrustStorePath(
config.getString(RestConfig.SSL_TRUSTSTORE_LOCATION_CONFIG)
);
sslContextFactory.setTrustStorePassword(
config.getPassword(RestConfig.SSL_TRUSTSTORE_PASSWORD_CONFIG).value()
);
if (config.getPassword(RestConfig.SSL_TRUSTSTORE_PASSWORD_CONFIG) != null) {
sslContextFactory.setTrustStorePassword(
config.getPassword(RestConfig.SSL_TRUSTSTORE_PASSWORD_CONFIG).value()
);
}
sslContextFactory.setTrustStoreType(
config.getString(RestConfig.SSL_TRUSTSTORE_TYPE_CONFIG)
);
Expand Down
9 changes: 6 additions & 3 deletions core/src/main/java/io/confluent/rest/FileWatcher.java
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,9 @@
// reference https://gist.github.com/danielflower/f54c2fe42d32356301c68860a4ab21ed
public class FileWatcher implements Runnable {
private static final Logger log = LoggerFactory.getLogger(FileWatcher.class);
private static final ExecutorService executor = Executors.newFixedThreadPool(1,

// don't have static shared threadpool for all FileWatchers in the JVM
private final ExecutorService executor = Executors.newFixedThreadPool(1,
new ThreadFactory() {
public Thread newThread(Runnable r) {
Thread t = Executors.defaultThreadFactory().newThread(r);
Expand Down Expand Up @@ -67,10 +69,11 @@ public FileWatcher(Path file, Callback callback) throws IOException {
* Starts watching a file calls the callback when it is changed.
* A shutdown hook is registered to stop watching.
*/
public static void onFileChange(Path file, Callback callback) throws IOException {
public static FileWatcher onFileChange(Path file, Callback callback) throws IOException {
log.info("Configure watch file change: " + file);
FileWatcher fileWatcher = new FileWatcher(file, callback);
executor.submit(fileWatcher);
fileWatcher.executor.submit(fileWatcher);
return fileWatcher;
}

public void run() {
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/io/confluent/rest/RestConfig.java
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ public class RestConfig extends AbstractConfig {
public static final String SSL_TRUSTSTORE_PASSWORD_CONFIG = "ssl.truststore.password";
protected static final String SSL_TRUSTSTORE_PASSWORD_DOC =
"The store password for the trust store file.";
protected static final String SSL_TRUSTSTORE_PASSWORD_DEFAULT = "";
protected static final String SSL_TRUSTSTORE_PASSWORD_DEFAULT = null;
public static final String SSL_TRUSTSTORE_TYPE_CONFIG = "ssl.truststore.type";
protected static final String SSL_TRUSTSTORE_TYPE_DOC =
"The type of trust store file.";
Expand Down
15 changes: 11 additions & 4 deletions core/src/test/java/io/confluent/rest/ApiHeadersTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;

public class ApiHeadersTest {

Expand All @@ -56,11 +57,16 @@ public class ApiHeadersTest {
private static String clientKeystoreLocation;
private static TestApplication app;

// Use a temporary folder so that .jks files created by this test are isolated
// and deleted when the test is done
public static TemporaryFolder tempFolder = new TemporaryFolder();

@BeforeClass
public static void setUp() throws Exception {
final File trustStore = File.createTempFile("ApiHeadersTest-truststore", ".jks");
final File clientKeystore = File.createTempFile("ApiHeadersTest-client-keystore", ".jks");
final File serverKeystore = File.createTempFile("ApiHeadersTest-server-keystore", ".jks");
tempFolder.create();
final File trustStore = File.createTempFile("ApiHeadersTest-truststore", ".jks", tempFolder.getRoot());
final File clientKeystore = File.createTempFile("ApiHeadersTest-client-keystore", ".jks", tempFolder.getRoot());
final File serverKeystore = File.createTempFile("ApiHeadersTest-server-keystore", ".jks", tempFolder.getRoot());

clientKeystoreLocation = clientKeystore.getAbsolutePath();

Expand All @@ -84,6 +90,7 @@ public static void teardown() throws Exception {
if (app != null) {
app.stop();
}
tempFolder.delete();
}

@Test
Expand Down Expand Up @@ -130,7 +137,7 @@ private static void createKeystoreWithCert(File file, String alias, Map<String,
final X509Certificate cert = new CertificateBuilder(30, "SHA1withRSA")
.sanDnsName("localhost").generate("CN=mymachine.local, O=A client", keypair);

TestSslUtils.createKeyStore(file.getPath(), new Password(SSL_PASSWORD), alias,
TestSslUtils.createKeyStore(file.getPath(), new Password(SSL_PASSWORD), new Password(SSL_PASSWORD), alias,
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This confuses me. It seems like createKeyStore's signature has been stable since 2015, but there have been changes to these tests more recently than that. How could the tests have passed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, although this method has been stable, the previously called method now no longer exists, hence the compile failure. This just bit MDS as well.

keypair.getPrivate(), cert);
certs.put(alias, cert);
}
Expand Down
Loading