KAFKA-2656: Remove hardcoded default key and truststores

[rajinisivaram]

Removed default hardcoded keystore and truststore in /tmp so that default JVM keystore/truststore may be used when keystore/truststore is not specified in Kafka server or client properties

[ijuma]

Thanks for the PR. What happens if no keystore or truststore is configured via any of the mechanisms? Will the user get a helpful message? If not, then we may want to change SSLFactory to provide a helpful message, in that case (assuming it's not hard to do).

Also, looking at the SSLFactory code, I am not sure checkKeyStoreConfigs is actually useful. Also, we don't have an equivalent checkTruststoreConfigs. Maybe worth checking that code and simplifying it at the same time.

[ijuma]

Also, if you're willing, I noticed that the following are unused in SSLConfigs:

SSL_NEED_CLIENT_AUTH_DOC
DEFAULT_SSL_NEED_CLIENT_AUTH

It would be good to remove them.

[ijuma]

Sorry, I noticed one more thing. The messages in the exceptions thrown by createKeystore and createTruststore don't match the conditions.

[rajinisivaram]

@ijuma Thank you for the review. I will remove SSL_NEED_CLIENT_AUTH properties in SSLConfig and fix the exception messages.

The checks in checkKeyStoreConfigs dont do anything useful because the keys are always present even the value is null. Since null checks are performed in createKeystore anyway, I will remove checkKeyStoreConfigs.

Java uses $JAVA_HOME/lib/security/cacerts as default keystore and truststore if the stores are not explicitly configured and no system properties are specified. We would like to use the default truststore part of this so that our customer's clients dont have to set a truststore at all.

The default keystore is of less value. Even before this PR, the code was falling back to default keystore for clients since keystore is optional unless server has client-auth turned on. So on the client side, the code is fine as is.

On the server side, we do always need a valid keystore. If one is not specified and the default is used, handshake would fail unless the default cacerts contains a valid keypair or the javax.net.ssl.keyStore property is set. And the user would need to debug the problem in the same way as mismatched client/server certificates. I dont think we should any additional checks of cacerts file itself other than what Java does anyway. The question is whether we should always mandate that keystore is explicitly specified in Kafka configs for Kafka server when SSL is enabled. Or should we simply fall back to the JVM system defaults? What do you think?

[ijuma]

I agree with your assessment @rajinisivaram.

Regarding the server side keystore, I can see pros and cons for each option. Maybe it's easier to be consistent and fallback to the JVM system defaults while doing a good job of logging what we're doing to make it easy for users to diagnose configuration problems if they occur.

It would be good to get @junrao's and @harshach's opinions too.

[junrao]

LGTM. @harshach , do you want to take a look?

[ijuma]

@rajinisivaram, thanks, your changes look good so far. One thing I noticed though is that in KafkaConfig.channelConfigs, we add items to a map without checking if they are null. This means that for items that don't have a default value (like in this example) we get inconsistent behaviour between get() == null and containsKey. I think we need to do something like the following for optional keys that don't have a default value:

if (sslKeystoreLocation == null)
  channelConfigs.put(SSLKeystoreLocationProp, sslKeystoreLocation)

Does this make sense?

[ijuma]

I'm fixing the issue with null values in channelConfigs in the SASL branch and it doesn't affect current usage, so your change LGTM.

@junrao can this be merged?

[rajinisivaram]

@ijuma Yes, null check will be good. Thank you for making the changes in the SASL branch.

[ijuma]

Jenkins is reporting an unrelated CopyCat failure, which means all the core and client tests passed.