Hi all,

I resolved the challenge with having Class Type for the
ssl.engine.factory.class configuration. Raised pull request to the trunk -
https://github.com/apache/kafka/pull/8338. Updated on the JIRA -
https://issues.apache.org/jira/browse/KAFKA-8890

Please review & Vote!

Thanks
Maulin

On Mon, Mar 23, 2020 at 6:57 PM Maulin Vasavada <maulin.vasav...@gmail.com>
wrote:

> Hi Rajini
>
> When I configure the Default value for ssl.engine.factory.class as Type
> Class, it is resulting into lot of test cases failures since in many places
> - tests and actual classes/scala code it is converting the configuration
> maps value to value.toString(). I verified that was the issues by fixing
> some of it but still evaluating how many places we need to fix if we really
> want to support Class Type key for the configuration. Password and List
> Types are also of non-String types but it seems their defaults are Null and
> they are optional vs in my case the ssl.engine.factory.class is *not*
> optional - it needs a value by default.
>
> Will keep you posted. I am thinking of reverting the config type to String
> and then load it as String and do Class loading in SslFactory.
>
> Thanks
> Maulin
>
>
> On Mon, Mar 23, 2020 at 1:38 AM Maulin Vasavada <maulin.vasav...@gmail.com>
> wrote:
>
>> 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