kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Pellerin, Clement" <Clement_Pelle...@ibi.com>
Subject RE: [DISCUSS] KIP-519: Make SSL context/engine configuration extensible
Date Fri, 20 Sep 2019 03:36:25 GMT
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.vasavada@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?


On Thu, Sep 19, 2019 at 5:04 PM Maulin Vasavada <maulin.vasavada@gmail.com>

> 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_Pellerin@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.vasavada@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_Pellerin@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.vasavada@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
>> >
View raw message