still working on the pull request. hopefully will be done soon.

On Wed, Mar 11, 2020 at 11:48 AM Maulin Vasavada <maulin.vasav...@gmail.com>
wrote:

> Thanks Rajini. Sounds good. I'll make changes and update this thread.
>
> On Wed, Mar 11, 2020 at 6:41 AM Rajini Sivaram <rajinisiva...@gmail.com>
> wrote:
>
>> 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