kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Maulin Vasavada <maulin.vasav...@gmail.com>
Subject Re: [DISCUSS] KIP-519: Make SSL context/engine configuration extensible
Date Fri, 20 Sep 2019 20:39:30 GMT
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_Pellerin@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.vasavada@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.vasavada@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_Pellerin@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.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?
> >>
> >> Thanks
> >> Maulin
> >>
> >> On Thu, Sep 19, 2019 at 5:04 PM Maulin Vasavada <
> >> maulin.vasavada@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_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
> >> >> >
> >> >>
> >> >
> >>
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message