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 > >> > > >> > > >
