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