Yes the checks should be mandatory and as I mentioned before we should keep them outside of the pluggable implementation - the way currently it is in SslFactory.
I think we can wait for comments from @Colin McCabe <cmcc...@apache.org> and @Rajini Sivaram <rajinisiva...@gmail.com> as soon as they can get some time out of 2.4 release. May be later this week or early next week? Thanks Maulin On Sat, Sep 21, 2019 at 5:24 AM Pellerin, Clement <clement_pelle...@ibi.com> wrote: > 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 > > >> >> > > > >> >> > > >> > > > >> > > > > > >
