SEC-1473 Allow no Truststore password #206
Conversation
|
@confluentinc It looks like @WanderingStar just signed our Contributor License Agreement. 👍 Always at your service, clabot |
| @@ -602,7 +602,9 @@ public void join() throws InterruptedException { | |||
| * @throws Exception If the application fails to stop | |||
| */ | |||
| public void stop() throws Exception { | |||
| server.stop(); | |||
| if (server != null) { | |||
There was a problem hiding this comment.
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.
| @@ -130,7 +130,7 @@ private static void createKeystoreWithCert(File file, String alias, Map<String, | |||
| final X509Certificate cert = new CertificateBuilder(30, "SHA1withRSA") | |||
| .sanDnsNames("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, | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
WanderingStar
force-pushed
the
SEC-1473
branch
from
379b4d1 to
bd14613
Compare
|
Given that this issue was identified in an MDS deployment, there should be a test in MDS repo for |
|
We should verify that this fix actually fixes the observed problem, but is it reasonable to test your dependencies on an ongoing basis? |
- 1 don't have a single static thread pool that all FileWatchers use - 2 update tests to cleanup temp/test keystore files Also, introduced awaitility test library
Yeah, I was just pointing out there "royal we" should also verify/add a test for this in MDS repo before ticket is closed, yes? |
If the Keystore is loaded without a password (with the password argument
null), the integrity check is skipped. (see: https://docs.oracle.com/javase/8/docs/api/java/security/KeyStore.html#load-java.io.InputStream-char:A-). Kafka handles this by giving the corresponding parameter anulldefault: https://github.com/confluentinc/ce-kafka/blob/master/clients/src/main/java/org/apache/kafka/common/config/SslConfigs.java#L163 But rest-utils was using""for the default, which doesn't skip the integrity check.This also fixes what seems like a long-existing error in the tests where the wrong number of arguments was supplied (see first commit).