If no one objects, I'll rewrite the KIP like this and resubmit for discussion:
Ideal interface, ignore the atomicity issue, only keystore and truststore is
reconfigurable.
I was not planning to log a Jira for the CertificateEntries comparison bug
because my fix will correct it anyway.
-----Original Message-----
From: Pellerin, Clement
Sent: Wednesday, November 21, 2018 10:42 AM
To: dev@kafka.apache.org
Subject: RE: [DISCUSS] KIP-383 Pluggable interface for SSL Factory
This turned out to be trickier than I thought and I don't have a firm solution
yet.
Since SslFactory will handle reconfiguration, the idea is to make the
configuration immutable in the pluggable factory.
SslFactory would create a new pluggable factory every time the configuration
changes.
The pluggable factory creates its SSLContext when it is configured and never
change it.
It turns out SslFactory does not really need the SSLContext, so it can use the
new pluggable factory as an SSLEngine factory.
The ideal interface looks like:
public interface SslEngineFactory {
void configure(Map<String, ?> configs);
SSLEngine createSslEngine(String peerHost, int peerPort);
}
This assumes SslFactory copies the configs and overwrites ssl.client.auth when
there is a clientAuthConfigOverride.
I am told the right way to do this in Kafka is with extra parameters in
configure().
public interface SslEngineFactory {
void configure(Map<String, ?> configs, String
clientAuthConfigOverride);
SSLEngine createSslEngine(String peerHost, int peerPort);
}
With this interface, SslEngineFactory must load its keystore and truststore.
Meanwhile, SslFactory must also load the keystore and truststore to detect a
future change in configuration.
This slight inefficiency is not important, what truly matters is the lack of
atomicity.
The keystore and truststore might change between the time SslFactory inspects
them and the time SslEngineFactory loads them again.
There are multiple ways to address this issue:
1. We can ignore the issue. If we always make sure SslFactory inspects the
files before SslEngineFactory is configured,
SslEngineFactory might load a newer version of the files, and SslFactory
might unnecessarily recreate the SslEngineFactory
the next time a reconfiguration is called, but we will never miss an update.
2. We can load the keystore and truststore in SslFactory and pass them to
SslEngineFactory.configure().
The configs for the keystore and truststore are ignored by SslEngineFactory.
public interface SslEngineFactory {
void configure(Map<String, ?> configs, String
clientAuthConfigOverride, KeyStore keystore, KeyStore truststore);
SSLEngine createSslEngine(String peerHost, int peerPort);
}
Notice we pass the KeyStore and not the SecurityStore since
SslEngineFactory does not need the metadata.
I find this signature messy.
3. We could query the SslEngineFactory for the keystore and truststore it
loaded.
This is ugly because SslEngineFactory would need to keep metadata for these
files,
or we would have to compare the actual KeyStore entries.
4. We could create a new SslEngineFactory for every reconfiguration assuming
the configuration always changed.
This would create a new SSLContext every time and therefore lose the SSL
Session cache every time.
Maybe solution 1 is sufficient. Notice we need to load the actual keystore
early in SslFactory
since we need to fix the lastModified time and the CertificateEntries.
There is a bug currently in the comparison of the CertificateEntries because
the KeyStore is reloaded
at the time of the comparison, which might compare the new contents against the
same new contents.
Solution 4 brings the issue of what is a reconfiguration.
I was wondering why Kafka supports only reconfiguration of the keystore and
truststore?
Why not any of the other config settings?
To support reconfiguration of the keystore and truststore and nothing else,
SslFactory would have to pass the original configs to the new SslEngineFactory.
SslFactory would have to keep a copy of the original configs and if we are
using solution 1,
it must overwrite the settings for the keystore and truststore.
If we do that, we might as well overwrite the clientAuth also and use the ideal
interface instead.
Thoughts?
