Hi Maulin,

I have reviewed the PR and left some comments, can you turn it into a PR
for https://github.com/apache/kafka? It looks good overall.

We pass all configs to other plugins, so we can do the same here. That
would be safer than assuming that all custom SSL-related configs start with
`ssl.`. You can use principal builder as an example for what we do today.

Regards,

Rajini

On Thu, Mar 5, 2020 at 12:03 AM Maulin Vasavada <maulin.vasav...@gmail.com>
wrote:

> Hi Rajini
>
> I made changes suggested by you on
> https://github.com/maulin-vasavada/kafka/pull/4. Please check.
>
> In particular I had challenge in 'SslFactory#configure()' method the first
> time to know which configs I have to add without having actual
> SslEngineFactory impl. I think it is best to just copy all configs with
> "ssl." prefix. Can you please review
>
> https://github.com/maulin-vasavada/kafka/pull/4/files#diff-1e3432211fdbb7b2e2b44b5d8838a40bR90
>  particularly?
>
> Clement, I missed to see your point about Mode in previous post but even
> after I realized what you are suggesting, my response would be the same as
> before :)
>
> Thanks
> Maulin
>
>
> On Wed, Feb 5, 2020 at 8:40 PM Maulin Vasavada <maulin.vasav...@gmail.com>
> wrote:
>
> > Hi Rajini
> >
> > Will accommodate your comments.
> >
> > Celement, while SSLContext factories are common, in this particular case,
> > we need SSLEngine object because Kafka is using SSLEngine (as mentioned
> > much before in this email thread, the SSLContext acts as factory for
> > getting SSLEngine, SSLSocketFactory or SSLServerSocketFactory and Kafka
> > chooses SSLEngine to be used for SSL Connections). The 'Mode' challenge
> > doesn't go away easily since Kafka is using the "same" classes for Client
> > side and Server side. Other stacks where you don't see this challenge is
> > because either libraries are client centric or server centric and not
> both
> > at the same time. I would suggest you do a deeper dive into the sample
> Pull
> > request, build the code to get better idea about it. I don't find it
> > strange to have 'Mode' argument in this context of Kafka. Kafka is not
> > doing anything out of norm here since ultimately it is using JSSE spec
> for
> > SSL Connection.
> >
> > I will try to respond to code comments in couple of weeks since I am out
> > for few weeks. Will keep you guys posted.
> >
> > Thanks
> > Maulin
> >
> >
> >
> >
> >
> >
> >
> >
> > On Wed, Feb 5, 2020 at 9:50 PM Pellerin, Clement <
> clement_pelle...@ibi.com>
> > wrote:
> >
> >> Many of these points came up before.
> >>
> >> I had great hope when Maulin suggested the custom factory could
> >> return an SSLContext instead of SSLEngine.  SSLContext factories are
> >> common,
> >> whereas I have never seen an SSLEngine factory being used before.
> >> He must have hit the same problem I had with the Mode.
> >>
> >> If the Mode can be removed, can we find a way to return an SSLContext
> now?
> >> What is so special about Kafka that it needs to hardcode the Mode when
> >> everyone
> >> else works with the SSLContext and ignores the other mode they don't
> use.
> >>
> >> -----Original Message-----
> >> From: Rajini Sivaram [mailto:rajinisiva...@gmail.com]
> >> Sent: Wednesday, February 5, 2020 10:03 AM
> >> To: dev
> >> Subject: Re: [DISCUSS] KIP-519: Make SSL context/engine configuration
> >> extensible
> >>
> >> One more point:
> >> 5) We should also add a method to SslEngineFactory that returns
> >> `Set<String>
> >> reconfigurableConfigs()`
> >>
> >> On Wed, Feb 5, 2020 at 1:50 PM Rajini Sivaram <rajinisiva...@gmail.com>
> >> wrote:
> >>
> >> > Hi Maulin,
> >> >
> >> > Thanks for the updates. A few comments below:
> >> >
> >> > 1) SslEngineFactory is currently in the internal package
> >> > org.apache.kafka.common.security.ssl. We should move it to the public
> >> > package org.apache.kafka.common.security.auth.
> >> > 2) SslEngineFactory should extend Configurable and Closeable. We
> should
> >> > expect the implementation class to have a default constructor and
> >> invoke configure()
> >> > to be consistent with otSslher pluggable classes.
> >> > 3) SslEngineFactory.createSslEngine uses the internal enum `Mode`. It
> >> > would be better to add two methods instead:
> >> >
> >> >
> >> >    - SSLEngine createClientSslEngine(String peerHost, int peerPort,
> >> String endpointIdentification);
> >> >    - SSLEngine createServerSslEngine(String peerHost, int peerPort);
> >> >
> >> > 4) Don't think we need a method on SslEngineFactory to return configs.
> >> It seems like an odd thing to do in a pubic Configurable API and is
> >> inconsistent with other APIs. We can track configs in the internal
> >> SslFactory class instead.
> >>
> >
>

Reply via email to