SEC-1473 Allow no Truststore password#206
SEC-1473 Allow no Truststore password#206Aneel Nazareth (WanderingStar) wants to merge 3 commits into5.5.xfrom
Conversation
Aneel Nazareth (WanderingStar)
requested review from
Ryan P (rnpridgeon) and
Yang Lei (yanglei99)
|
Confluent Inc. (@confluentinc) It looks like Aneel Nazareth (@WanderingStar) just signed our Contributor License Agreement. 👍 Always at your service, clabot |
Aneel Nazareth (WanderingStar)
requested a review
from Milo Simpson (milosimpson)
| */ | ||
| 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.
| .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.
Aneel Nazareth (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? |
3 participants
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).