I may have found the technical reason. It would be logical if the customizable reconfigurable extension point extends the Reconfigurable interface. That's what the first iteration of KIP-383 proposed. It lost its vote because the voters wanted to preserve the reconfiguration checks in SslFactory. The next thing to try is to push those checks in the creator which is the channel builder. A Reconfigurable instance is mutable by definition. It can either be in its current configuration, or the reconfigured configuration but not in between. The checks require to partially build the new configuration before accepting it and therefore this approach does not work.
We could move the checks in a helper validator class, then we would rely on the Reconfigurable instance to call that code. This would reuse the code but the checks would not be mandatory. The conclusion is the checks are mandatory or the interface extends the Reconfigurable interface but not both. Is that reasoning what you were saying? Do we make the checks mandatory or not? Do we support all the use cases we want? -----Original Message----- From: Pellerin, Clement Sent: Friday, September 20, 2019 5:24 PM To: dev@kafka.apache.org Subject: RE: [DISCUSS] KIP-519: Make SSL context/engine configuration extensible The KIP now says: We believe that making SSLEngine creation pluggable is worth to allow SSL experts to write their own implementation having the SSL domain knowledge and keep them free of knowing much about Kafka's reconfigurability. The other custom reconfigurable extension points don't seem to have a problem with that. You may have a point though, so I need to look at the reconfiguration code you mention. Is your latest prototype available so I can study it? -----Original Message----- From: Maulin Vasavada [mailto:maulin.vasav...@gmail.com] Sent: Friday, September 20, 2019 4:40 PM To: dev@kafka.apache.org Subject: Re: [DISCUSS] KIP-519: Make SSL context/engine configuration extensible Thanks Clement for your thoughts. According to my current experience rewriting the code twice I would say I did what you suggest in the last point - " We must make an attempt, if only to explain why it fails in the Rejected Alternatives section of the KIP." In the rejected alternatives I already mention why not merge SslFactory and SslEngineFactory and make SslFactory reconfigurable. @Colin can you please express your thoughts on this discussion so far? Since you refactored the SslFactory's code to have SslEngineBuilder I feel you would have more insights into future changes. Thanks Maulin On Fri, Sep 20, 2019 at 7:29 AM Pellerin, Clement <clement_pelle...@ibi.com> wrote: > There will be chaperon code in the base class of the channel builders. > The arguments you gave me are emotional not technical. > The SSL extension point is reconfigurable hence it should extend > Reconfigurable. > We will encounter issues when we try to prototype it that way. > We will solve the issues or backtrack to another solution. > We must make an attempt, if only to explain why it fails in the Rejected > Alternatives section of the KIP. > > -----Original Message----- > From: Maulin Vasavada [mailto:maulin.vasav...@gmail.com] > Sent: Friday, September 20, 2019 2:40 AM > To: dev@kafka.apache.org > Subject: Re: [DISCUSS] KIP-519: Make SSL context/engine configuration > extensible > > Overall my thinking is - When somebody wants to customize creation of > SSLEngine, most likely they are more expert in dealing with SSL domain > related stuff than "Kafka's reconfigurability" aspect. As a custom > implementation it makes more sense to me to say - Hey I'll control how I > initialize my SSL context/engine and btw if Kafka can give me a way to just > get re-created whenever some set of config keys changes, it is great! This > is similar to my thinking on KIP-486 which is- as a Kafka operator I am > just trying to be compliant to my company's security policies to load > keys/certs in certain way. For that, I should not be penalized by Kafka to > know all about Java security Providers and how to really create SSLContext > object etc given Java already provides a way to feed in KeyStore object > regardless of how I load it. > > On Thu, Sep 19, 2019 at 10:57 PM Maulin Vasavada < > maulin.vasav...@gmail.com> > wrote: > > > Hi Clement > > > > There will be good amount of state in the SslEngineFactory's default > > implementation. Hence I feel we might anyway have a chaperon class to > > provide reconfigurable functionality and will have one more class to host > > the state/behavior of actual SSLContext/SSLEngine creation. While doing > the > > internal rewrite (so far two times) both of the times I reached to the > same > > conclusion.I feel if we leave the reconfigurations to the implementation > - > > it will repeat the same pattern of having two classes to manage it - > since > > most likely they will also have similar state information. Instead keep > > that reconfigurations in SslFactory as is today and just allow "plugin of > > creation of SSLEngine". > > > > One note I would like to make is: You are comparing this to > MetricReporter > > but we have to keep in mind that SSL configuration is inherently more > > complex than a MetricReporters functionality. There are no JSSE > equivalent > > documents needed to be written for MetricReporter for example. So what > > works best for simpler solutions may not work equally well for more > complex > > scenarios. > > > > > > Thanks > > Maulin > > > > > > On Thu, Sep 19, 2019 at 8:36 PM Pellerin, Clement < > > clement_pelle...@ibi.com> wrote: > > > >> We will get there eventually but I need to address another point first. > >> > >> My goal is to do exactly what the "other extension points with > >> reconfigurable custom configs" are doing unless there is a good reason > not > >> to. They provide a ready-made solution that will let us reuse code, > avoid > >> pitfalls and show consistency. > >> > >> So far the roadblocks are > >> - the need to enforce mandatory compatibility checks for the keystores > >> and SSL handshake > >> - SslFactory is used in two channel builders. > >> > >> Both of these roadblocks can be overcome by moving the checks to a new > >> common base class of SslChannelBuilder and SaslChannelBuilder. This is > easy > >> since both classes extend Object directly. The new base class is not a > >> public API so any implementation will do. The chaperon class SslFactory > >> disappears and the interface extends Reconfigurable. > >> > >> Does this proposal address all the reasons you had not to do exactly > what > >> other extension points are doing? > >> > >> -----Original Message----- > >> From: Maulin Vasavada [mailto:maulin.vasav...@gmail.com] > >> Sent: Thursday, September 19, 2019 10:21 PM > >> To: dev@kafka.apache.org > >> Subject: Re: [DISCUSS] KIP-519: Make SSL context/engine configuration > >> extensible > >> > >> Hi Clement > >> > >> So assuming there are two classes - SslFactory and SslEngineFactory > like I > >> suggested in my detailed post before this, we can use > >> config.getConfiguredInstance() in SslFactory for SslEngineFactory class > >> configuration and then followed by init() method. I don't see a > challenge > >> there. Can you please provide your input on my detailed post along with > >> this recent point I am making? > >> > >> Thanks > >> Maulin > >> > >> On Thu, Sep 19, 2019 at 5:04 PM Maulin Vasavada < > >> maulin.vasav...@gmail.com> > >> wrote: > >> > >> > Hi Clement, > >> > > >> > Thanks for pointing to AbstractConfig. Now I understand what you were > >> > saying. I'll respond by tonight with more thoughts. > >> > > >> > Thanks > >> > Maulin > >> > > >> > On Thu, Sep 19, 2019 at 5:46 AM Pellerin, Clement < > >> > clement_pelle...@ibi.com> wrote: > >> > > >> >> I appreciate the effort you put into this. > >> >> > >> >> Lets do this in steps. You had a question on getConfiguredInstance(). > >> >> > >> >> The method getConfiguredInstance(key, Class) implemented in > >> >> AbstractConfig is how the MetricsReporter and other extension points > >> are > >> >> intantiated. Creating the extension point this way calls the default > >> >> constructor which is good. Since the (Re)Configurable interface > >> dictates > >> >> the signature of the configure() method, that forces the addition of > a > >> new > >> >> init(...) method to pass the other constructor arguments. > >> >> > >> >> Do we agree on that before we move on to other issues? > >> >> > >> >> -----Original Message----- > >> >> From: Maulin Vasavada [mailto:maulin.vasav...@gmail.com] > >> >> Sent: Wednesday, September 18, 2019 4:37 PM > >> >> To: dev@kafka.apache.org > >> >> Subject: Re: [DISCUSS] KIP-519: Make SSL context/engine configuration > >> >> extensible > >> >> > >> >> Hi Clement > >> >> > >> >> Here are my thoughts based on my latest re-write attempt and > learnings, > >> >> > >> >> 1. I think that it will be a great value to keep both classes > separate > >> - > >> >> SslFactory and SslEngineFactory and having method > >> reconfigurableConfigs() > >> >> in the SslEngineFactory. Here is the reasoning, > >> >> > >> >> a. It is kind of a Decorator pattern to me - even without named like > >> one > >> >> SslFactory is acting as a decorator/surrogate to the SslEngineFactory > >> and > >> >> helping it get created and re-created as needed based on the > >> >> terms/conditions specified by SslEngineFactory (via > >> >> reconfigurableConfigs() > >> >> method) > >> >> > >> >> b. SslEngineFactory will be pluggable class. By keeping the > SslFactory > >> >> reconfigurable with delegation of reconfigurableConfigs() to > >> >> SslEngineFactory it allows the implementation of SslEngineFactory to > be > >> >> worry free of - How Kafka manages reconfigurations. The contract is - > >> >> Kafka's SslFactory will ask the implementation to provide which > >> >> configurations it is ready to be reconfigured for. Rest of the logic > >> for > >> >> triggering and reconfiguring and validation is in SslFactory. > >> >> > >> >> c. The current validation in SslFactory about inter-broker-ssl > >> handshake > >> >> AND verifying that certificate chain doesn't change via dynamic > config > >> >> changes is rightly owned by SslFactory. We should not give > flexibility > >> to > >> >> SslEngineFactory to decide if they want that validation or not. > >> >> > >> >> d. If SslEngineFactory fails to be re-created with new dynamic config > >> >> changes the constructor will throw some exception and the SslFactory > >> will > >> >> fail the validateReconfiguration() call resulting in no-change. Hence > >> the > >> >> validation if the new config is right is still controlled by the > >> >> SslEngineFactory without necessarily having explicit validate method > >> >> (assuming if you had a point about - we should keep validation of > >> changed > >> >> configs in the pluggable class) > >> >> > >> >> > >> >> 2. About the keystore validation in SslFactory - as I mentioned in > >> above > >> >> points, > >> >> > >> >> a. I feel it is Kafka's policy that it wants to mandate that > validation > >> >> regardless of the SslEngineFactory's implementation. I feel that > >> >> regardless > >> >> of customized implementation it is doing a 'logical' enforcement. I > >> don't > >> >> see many cases where you will end up changing certificate chain (I > >> can't > >> >> say the same about SANs entries though. see my below points). Hence > >> that > >> >> validation is reasonable to be generally enforced for dynamic config > >> >> changes. If you change something violating that validation, you can > >> avoid > >> >> making such changes via dynamic configuration and do a rolling > >> restarts of > >> >> the boxes. > >> >> > >> >> b. If the implementation doesn't use keystore then automatically no > >> >> validation will happen. Hence I don't see any issue with > >> >> SslEngineFactory's > >> >> implementations not having requirement to use keystores. > >> >> > >> >> c. There could be an argument however about - what it validates > >> currently > >> >> and is there a scope of change. Example: It validates SANs entries > and > >> >> that > >> >> to me is a challenge because I have had scenarios where I kept adding > >> more > >> >> VIPs in my certs SANs entries without really changing any certificate > >> >> chain. The existing validation will fail that setup unnecessarily. > >> Given > >> >> that - there could be change in SslFactory but that doesn't still > make > >> >> that > >> >> validation eligible to go to SslEngineFactory implementations. > >> >> > >> >> > >> >> 3. I am still in two minds about your point on - not using existing > SSL > >> >> Reconfigurable configs to be used by SslFactory on top of > >> >> SslEngineFactory's reconfigurable configs. The reason for that is- > >> >> > >> >> a. I agree with you on that we should not worry about existing SSL > >> >> reconfigurable configs in new changed code for SslFactory. Why depend > >> on > >> >> something you really don't need. However, Rajini's point is- if we > >> decide > >> >> to add more configs in the SSL reconfigurable configs which may be > >> common > >> >> across SslEngineFactory's implementations, it will make it easier. > >> Again, > >> >> just to make it easier we should not do it upfront. So now you see > why > >> I > >> >> am > >> >> double minded on it while more leaning toward your suggestion. > >> >> > >> >> 4. I think I totally miss what you refer by > >> >> config.getConfiguredInstance(key, Class). Which Kafka existing class > >> you > >> >> are referring to when you do that? Do we have that in KafkaConfig? If > >> you > >> >> can clarify me on that I can think more about your input on it. > >> >> > >> >> 5. Now above all means- > >> >> > >> >> a. We will have createEngine(), reconfigurableConfigs(), keystore(), > >> >> truststore() methods in the SslEngineFactory interface. However the > >> return > >> >> type for keystore and truststore method can't be existing > >> SecurityStore. > >> >> For that I already thought of the replacement with KeystoreHolder > class > >> >> which only contains references to java's KeyStore object and Kafka's > >> >> Password object making it feasible for us to return > non-implementation > >> >> specific return type. > >> >> > >> >> b. We didn't talk about shouldBeRebuilt() so far at all given other > >> >> important conflicts to resolve. We will get to it once we can hash > out > >> >> other stuff. > >> >> > >> >> 6. On Rajini's point on 'push notifications' for client side code > when > >> the > >> >> key/trust store changes, > >> >> > >> >> " - For client-side, custom SslEngineFactory implementations could > >> >> reconfigure themselves, we don't really need SslFactory to be > >> involved > >> >> at all." > >> >> > >> >> I think I am missing something. If we just have SslEngineFactory > >> >> reconfigure itself it will generate new SSLContext and in-turn new > >> >> SSLEgnine but how will it communicate that to the ChannelBuilders? > >> Don't > >> >> they have to refresh the reference to the SslEngineFactory via > >> >> SslFactory's > >> >> reconfigure() method in order to pick up that change? > >> >> > >> >> > >> >> Thanks > >> >> Maulin > >> >> > >> >> > >> >> > >> >> > >> >> > >> >> > >> >> > >> >> > >> >> > >> >> > >> >> > >> >> On Tue, Sep 17, 2019 at 4:49 AM Pellerin, Clement < > >> >> clement_pelle...@ibi.com> > >> >> wrote: > >> >> > >> >> > Good point about the two callers of SslFactory. We can move the > >> >> SslEngine > >> >> > validation to a separate class and call it in both places. That > >> >> SslEngine > >> >> > validation class would not be part of the public API and therefore > we > >> >> don't > >> >> > need to fuss about its API. > >> >> > > >> >> > -----Original Message----- > >> >> > From: Maulin Vasavada [mailto:maulin.vasav...@gmail.com] > >> >> > Sent: Tuesday, September 17, 2019 2:28 AM > >> >> > To: dev@kafka.apache.org > >> >> > Subject: Re: [DISCUSS] KIP-519: Make SSL context/engine > configuration > >> >> > extensible > >> >> > > >> >> > Hi Clement/Rajini > >> >> > > >> >> > When I read your responses - I swing between both of your > suggestions > >> >> :) I > >> >> > see both of your points. Let me ponder little bit more and give me > >> take > >> >> in > >> >> > a day or so. > >> >> > > >> >> > I tend to agree with Clement in a sense that we need to define > clear > >> >> > responsibilities of classes. Right now I feel it's not clear. > Also, I > >> >> tend > >> >> > to agree to both of you about keystore/truststore validation - the > >> >> conflict > >> >> > I've to propose a clean agreeable solution to. > >> >> > > >> >> > One clarification to Clement is - there are two classes using > >> SslFactory > >> >> > today - SslChannelBuilder and SaslChannelBuilder so we have to keep > >> >> that in > >> >> > mind. However, once we have clear responsibilities of classes, that > >> >> should > >> >> > automatically clear what goes where. > >> >> > > >> >> > Thanks > >> >> > Maulin > >> >> > > >> >> > >> > > >> > > >
