RE: [DISCUSS] KIP-383 Pluggable interface for SSL Factory
I updated the KIP to keep reconfiguration in SslFactory and create a new
SslEngineFactory interface
public interface SslEngineFactory {
void configure(Map configs, Mode mode);
SSLEngine createSslEngine(String peerHost, int peerPort);
}
Please read the KIP and send your comments.
-Original Message-
From: Pellerin, Clement
Sent: Wednesday, November 28, 2018 11:49 AM
To: dev@kafka.apache.org
Subject: RE: [DISCUSS] KIP-383 Pluggable interface for SSL Factory
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.
RE: [DISCUSS] KIP-383 Pluggable interface for SSL Factory
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 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 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 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?
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 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 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 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?
-Original Message-
From: Rajini Sivaram [mailto:rajinisiva...@gmail.com]
Sent: Tuesday, October 23, 2018 7:54 AM
To: dev
Subject: Re: [DISCUSS] KIP-383 Pluggable interface for SSL Factory
Thanks for the PR. A couple of comments:
1) For other public interfaces, we use parameters to configure instead of
internal config names going into the same list of configs. Kafka has public
config names and custom config names that don't conflict with public config
names. These internal config names fall into neither category.
An example of how we do this in other classes is Deserializer:
void configure(Map configs, boolean isKey);
2) I am not sure `SslFactory` as it stands today is the right class to turn
into a pluggable class. I can see why we want to have a custom class to
configure SSLEngines. But I don't think we want a custom class to decide
the logic for dynamic reconfiguration or decide whether to validate against
trust stores. It would perhaps be better to have a class to just create
SSLContext and possibly configure SSLEngine with additional options
RE: [DISCUSS] KIP-383 Pluggable interface for SSL Factory
1) I agree that signature pattern is cleaner. To support this, we have to give up inheriting Reconfigurable. That's exactly what you propose when you want to keep reconfiguration in the non-pluggable class. 2) I agree reconfiguration and the option to validate keystores against truststores do not belong in a pluggable SSL Factory. I was planning to ignore those features in my custom SslFactory implementation, but I now see there is no reason to if Kafka does it all for me. Reconfiguration creates a new SSLContext anyway, so that can be handled like the initial creation by the non-pluggable class. Back to the drawing board to keep a non-pluggable class and split a pluggable SSLContext factory out of it. Thanks for the feedback. -Original Message- From: Rajini Sivaram [mailto:rajinisiva...@gmail.com] Sent: Tuesday, October 23, 2018 7:54 AM To: dev Subject: Re: [DISCUSS] KIP-383 Pluggable interface for SSL Factory Thanks for the PR. A couple of comments: 1) For other public interfaces, we use parameters to configure instead of internal config names going into the same list of configs. Kafka has public config names and custom config names that don't conflict with public config names. These internal config names fall into neither category. An example of how we do this in other classes is Deserializer: void configure(Map configs, boolean isKey); 2) I am not sure `SslFactory` as it stands today is the right class to turn into a pluggable class. I can see why we want to have a custom class to configure SSLEngines. But I don't think we want a custom class to decide the logic for dynamic reconfiguration or decide whether to validate against trust stores. It would perhaps be better to have a class to just create SSLContext and possibly configure SSLEngine with additional options is there is a good use case for that. But the non-pluggable class in Kafka should deal with all the reconfiguration and validation logic. Thoughts? >
Re: [DISCUSS] KIP-383 Pluggable interface for SSL Factory
Thanks for the PR. A couple of comments: 1) For other public interfaces, we use parameters to configure instead of internal config names going into the same list of configs. Kafka has public config names and custom config names that don't conflict with public config names. These internal config names fall into neither category. An example of how we do this in other classes is Deserializer: void configure(Map configs, boolean isKey); 2) I am not sure `SslFactory` as it stands today is the right class to turn into a pluggable class. I can see why we want to have a custom class to configure SSLEngines. But I don't think we want a custom class to decide the logic for dynamic reconfiguration or decide whether to validate against trust stores. It would perhaps be better to have a class to just create SSLContext and possibly configure SSLEngine with additional options is there is a good use case for that. But the non-pluggable class in Kafka should deal with all the reconfiguration and validation logic. Thoughts? On Tue, Oct 23, 2018 at 12:40 AM Pellerin, Clement wrote: > If it is up to me, I'd say the KIP is now finalized. Can I call for a > [VOTE] or I need more feedback? > > -Original Message- > From: Harsha Chintalapani [mailto:ka...@harsha.io] > Sent: Monday, October 22, 2018 4:36 PM > To: dev@kafka.apache.org > Subject: RE: [DISCUSS] KIP-383 Pluggable interface for SSL Factory > > Thanks for the changes. KIP looks good to me. > > -Harsha > On Oct 22, 2018, 12:57 PM -0700, Pellerin, Clement < > clement_pelle...@ibi.com>, wrote: > > I updated the KIP. The proposed interface is now named SslFactory and > the default implementation is DefaultSslFactory. > > I also mention the existing non-default constructors will be removed. > > The constants for the new config keys are now declared in the interface > itself. > > Please give me your feedback on this new version. > > > > -Original Message- > > From: Harsha Chintalapani [mailto:ka...@harsha.io] > > Sent: Friday, October 19, 2018 6:51 PM > > To: dev@kafka.apache.org; dev@kafka.apache.org > > Subject: RE: [DISCUSS] KIP-383 Pluggable interface for SSL Factory > > > > SslFactory is not a public interface for others to use. EchoServer is > internal testing. > > We should make these as proposed in rejected alternatives to SslFactory > and DefaultSslFactory. > > I don’t see any one using a internal class as public API. > > > > -Harsha >
RE: [DISCUSS] KIP-383 Pluggable interface for SSL Factory
If it is up to me, I'd say the KIP is now finalized. Can I call for a [VOTE] or I need more feedback? -Original Message- From: Harsha Chintalapani [mailto:ka...@harsha.io] Sent: Monday, October 22, 2018 4:36 PM To: dev@kafka.apache.org Subject: RE: [DISCUSS] KIP-383 Pluggable interface for SSL Factory Thanks for the changes. KIP looks good to me. -Harsha On Oct 22, 2018, 12:57 PM -0700, Pellerin, Clement , wrote: > I updated the KIP. The proposed interface is now named SslFactory and the > default implementation is DefaultSslFactory. > I also mention the existing non-default constructors will be removed. > The constants for the new config keys are now declared in the interface > itself. > Please give me your feedback on this new version. > > -Original Message- > From: Harsha Chintalapani [mailto:ka...@harsha.io] > Sent: Friday, October 19, 2018 6:51 PM > To: dev@kafka.apache.org; dev@kafka.apache.org > Subject: RE: [DISCUSS] KIP-383 Pluggable interface for SSL Factory > > SslFactory is not a public interface for others to use. EchoServer is > internal testing. > We should make these as proposed in rejected alternatives to SslFactory and > DefaultSslFactory. > I don’t see any one using a internal class as public API. > > -Harsha
RE: [DISCUSS] KIP-383 Pluggable interface for SSL Factory
Thanks for the changes. KIP looks good to me. -Harsha On Oct 22, 2018, 12:57 PM -0700, Pellerin, Clement , wrote: > I updated the KIP. The proposed interface is now named SslFactory and the > default implementation is DefaultSslFactory. > I also mention the existing non-default constructors will be removed. > The constants for the new config keys are now declared in the interface > itself. > Please give me your feedback on this new version. > > -Original Message- > From: Harsha Chintalapani [mailto:ka...@harsha.io] > Sent: Friday, October 19, 2018 6:51 PM > To: dev@kafka.apache.org; dev@kafka.apache.org > Subject: RE: [DISCUSS] KIP-383 Pluggable interface for SSL Factory > > SslFactory is not a public interface for others to use. EchoServer is > internal testing. > We should make these as proposed in rejected alternatives to SslFactory and > DefaultSslFactory. > I don’t see any one using a internal class as public API. > > -Harsha
RE: [DISCUSS] KIP-383 Pluggable interface for SSL Factory
I updated the KIP. The proposed interface is now named SslFactory and the default implementation is DefaultSslFactory. I also mention the existing non-default constructors will be removed. The constants for the new config keys are now declared in the interface itself. Please give me your feedback on this new version. -Original Message- From: Harsha Chintalapani [mailto:ka...@harsha.io] Sent: Friday, October 19, 2018 6:51 PM To: dev@kafka.apache.org; dev@kafka.apache.org Subject: RE: [DISCUSS] KIP-383 Pluggable interface for SSL Factory SslFactory is not a public interface for others to use. EchoServer is internal testing. We should make these as proposed in rejected alternatives to SslFactory and DefaultSslFactory. I don’t see any one using a internal class as public API. -Harsha
RE: [DISCUSS] KIP-383 Pluggable interface for SSL Factory
SslFactory is not a public interface for others to use. EchoServer is internal testing. We should make these as proposed in rejected alternatives to SslFactory and DefaultSslFactory. I don’t see any one using a internal class as public API. -Harsha On Oct 19, 2018, 3:47 PM -0700, Pellerin, Clement , wrote: > > > Can you explain why calling SslFactory and DefaultSslFactory cause any > > > issues. > > When you say "calling", I guess you mean "naming". > > Renaming SslFactory will only cause backwards compatibility issues for > applications that refer to it directly. EchoServer is an example, but maybe > that is just an artificial test. > You make it sound like SslFactory was never part of the public API. I cannot > make that judgement just by looking at the code. > > I don't mind using the better names if the Kafka community thinks no Kafka > application calls SslFactory directly. I would like more opinions on this > though. > > > -Original Message- > From: Harsha Chintalapani [mailto:ka...@harsha.io] > Sent: Friday, October 19, 2018 5:55 PM > To: dev@kafka.apache.org > Subject: RE: [DISCUSS] KIP-383 Pluggable interface for SSL Factory > > Hi, > Overall the KIP looks good to me. > > "Ideally, the interface should be called SslFactory and the built-in > implementation should be called DefaultSslFactory. This was rejected to > improve backwards compatibility for applications that call the SslFactory > directly.” > > Can you explain why calling SslFactory and DefaultSslFactory cause any > issues. For clients the config will point to DefaultSslFactory and similarly > on broker side as well. Not sure which cases it will break the backward > compatability. > > -Harsha > On Oct 19, 2018, 1:48 PM -0700, Pellerin, Clement , > wrote: > > I have updated the KIP to use a default constructor in the pluggable SSL > > Factory implementation. > > I also changed the name of the config to ssl.sslfactory.class and fixed a > > typo in the constant names. > > I would like your feedback on this version of the KIP. > > > > -----Original Message- > > From: Pellerin, Clement > > Sent: Wednesday, October 17, 2018 3:11 PM > > To: dev@kafka.apache.org > > Subject: [DISCUSS] KIP-383 Pluggable interface for SSL Factory > > > > I would like feedback on this proposal to make it possible to replace > > SslFactory with a custom implementation. > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-383%3A++Pluggable+interface+for+SSL+Factory > >
RE: [DISCUSS] KIP-383 Pluggable interface for SSL Factory
>> Can you explain why calling SslFactory and DefaultSslFactory cause any >> issues. When you say "calling", I guess you mean "naming". Renaming SslFactory will only cause backwards compatibility issues for applications that refer to it directly. EchoServer is an example, but maybe that is just an artificial test. You make it sound like SslFactory was never part of the public API. I cannot make that judgement just by looking at the code. I don't mind using the better names if the Kafka community thinks no Kafka application calls SslFactory directly. I would like more opinions on this though. -Original Message- From: Harsha Chintalapani [mailto:ka...@harsha.io] Sent: Friday, October 19, 2018 5:55 PM To: dev@kafka.apache.org Subject: RE: [DISCUSS] KIP-383 Pluggable interface for SSL Factory Hi, Overall the KIP looks good to me. "Ideally, the interface should be called SslFactory and the built-in implementation should be called DefaultSslFactory. This was rejected to improve backwards compatibility for applications that call the SslFactory directly.” Can you explain why calling SslFactory and DefaultSslFactory cause any issues. For clients the config will point to DefaultSslFactory and similarly on broker side as well. Not sure which cases it will break the backward compatability. -Harsha On Oct 19, 2018, 1:48 PM -0700, Pellerin, Clement , wrote: > I have updated the KIP to use a default constructor in the pluggable SSL > Factory implementation. > I also changed the name of the config to ssl.sslfactory.class and fixed a > typo in the constant names. > I would like your feedback on this version of the KIP. > > -Original Message- > From: Pellerin, Clement > Sent: Wednesday, October 17, 2018 3:11 PM > To: dev@kafka.apache.org > Subject: [DISCUSS] KIP-383 Pluggable interface for SSL Factory > > I would like feedback on this proposal to make it possible to replace > SslFactory with a custom implementation. > https://cwiki.apache.org/confluence/display/KAFKA/KIP-383%3A++Pluggable+interface+for+SSL+Factory >
RE: [DISCUSS] KIP-383 Pluggable interface for SSL Factory
Hi, Overall the KIP looks good to me. "Ideally, the interface should be called SslFactory and the built-in implementation should be called DefaultSslFactory. This was rejected to improve backwards compatibility for applications that call the SslFactory directly.” Can you explain why calling SslFactory and DefaultSslFactory cause any issues. For clients the config will point to DefaultSslFactory and similarly on broker side as well. Not sure which cases it will break the backward compatability. -Harsha On Oct 19, 2018, 1:48 PM -0700, Pellerin, Clement , wrote: > I have updated the KIP to use a default constructor in the pluggable SSL > Factory implementation. > I also changed the name of the config to ssl.sslfactory.class and fixed a > typo in the constant names. > I would like your feedback on this version of the KIP. > > -Original Message- > From: Pellerin, Clement > Sent: Wednesday, October 17, 2018 3:11 PM > To: dev@kafka.apache.org > Subject: [DISCUSS] KIP-383 Pluggable interface for SSL Factory > > I would like feedback on this proposal to make it possible to replace > SslFactory with a custom implementation. > https://cwiki.apache.org/confluence/display/KAFKA/KIP-383%3A++Pluggable+interface+for+SSL+Factory >
RE: [DISCUSS] KIP-383 Pluggable interface for SSL Factory
I have updated the KIP to use a default constructor in the pluggable SSL Factory implementation. I also changed the name of the config to ssl.sslfactory.class and fixed a typo in the constant names. I would like your feedback on this version of the KIP. -Original Message- From: Pellerin, Clement Sent: Wednesday, October 17, 2018 3:11 PM To: dev@kafka.apache.org Subject: [DISCUSS] KIP-383 Pluggable interface for SSL Factory I would like feedback on this proposal to make it possible to replace SslFactory with a custom implementation. https://cwiki.apache.org/confluence/display/KAFKA/KIP-383%3A++Pluggable+interface+for+SSL+Factory
RE: [DISCUSS] KIP-383 Pluggable interface for SSL Factory
I guess it could but unless I'm mistaken, you have the same problem as before since it is not pluggable. The idea is you should be able to replace the implementation through configuration, without rebuilding a custom Kafka distribution or resorting to classpath tricks to shadow Kafka classes. I will make that goal explicit in the KIP. If I could convince Kafka to create my subclass of SslChannelBuilder or SaslChannelBuilder when building a channel, that would get me pretty far for what I'm trying to do. I would simply need to move the call to the SslFactory constructor to its own method so I could override that method to create my own subclass of SslFactory. Please enlighten me if that's already possible while respecting the goal above. I concentrated the solution towards SslFactory since that's what the Jira was asking for. I like the smaller scope of the proposed solution because it solves a problem reported by many people. I am not sure people are clamoring to have a pluggable ChannelBuilder interface. Finally, the proposed solution gets reused twice in SslChannelBuilder and SaslChannelBuilder for that part of the protocol they share. -Original Message- From: Harsha [mailto:ka...@harsha.io] Sent: Thursday, October 18, 2018 10:58 AM To: dev@kafka.apache.org Subject: Re: [DISCUSS] KIP-383 Pluggable interface for SSL Factory Hi, Thanks for the KIP. Curious to understand why the ChannelBuilder interface doesn't solve the stated reasons in Motiviation section. Thanks, Harsha On Wed, Oct 17, 2018, at 12:10 PM, Pellerin, Clement wrote: > I would like feedback on this proposal to make it possible to replace > SslFactory with a custom implementation. > https://cwiki.apache.org/confluence/display/KAFKA/KIP-383%3A++Pluggable+interface+for+SSL+Factory >
Re: [DISCUSS] KIP-383 Pluggable interface for SSL Factory
Hi, Thanks for the KIP. Curious to understand why the ChannelBuilder interface doesn't solve the stated reasons in Motiviation section. Thanks, Harsha On Wed, Oct 17, 2018, at 12:10 PM, Pellerin, Clement wrote: > I would like feedback on this proposal to make it possible to replace > SslFactory with a custom implementation. > https://cwiki.apache.org/confluence/display/KAFKA/KIP-383%3A++Pluggable+interface+for+SSL+Factory >
[DISCUSS] KIP-383 Pluggable interface for SSL Factory
I would like feedback on this proposal to make it possible to replace SslFactory with a custom implementation. https://cwiki.apache.org/confluence/display/KAFKA/KIP-383%3A++Pluggable+interface+for+SSL+Factory
