You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Maulin Vasavada <ma...@gmail.com> on 2020/03/05 00:03:06 UTC
Re: [DISCUSS] KIP-519: Make SSL context/engine configuration extensible
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 <ma...@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 <Cl...@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:rajinisivaram@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 <ra...@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.
>>
>
Re: [DISCUSS] KIP-519: Make SSL context/engine configuration extensible
Posted by Maulin Vasavada <ma...@gmail.com>.
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 <ma...@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 <ma...@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.vasavada@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 <ra...@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.vasavada@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.vasavada@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_Pellerin@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:rajinisivaram@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 <
>>>> rajinisivaram@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.
>>>> > >>
>>>> > >
>>>> >
>>>>
>>>
Re: [DISCUSS] KIP-519: Make SSL context/engine configuration extensible
Posted by Maulin Vasavada <ma...@gmail.com>.
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 <ma...@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.vasavada@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 <ra...@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.vasavada@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.vasavada@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_Pellerin@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:rajinisivaram@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 <
>>> rajinisivaram@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.
>>> > >>
>>> > >
>>> >
>>>
>>
Re: [DISCUSS] KIP-519: Make SSL context/engine configuration extensible
Posted by Maulin Vasavada <ma...@gmail.com>.
still working on the pull request. hopefully will be done soon.
On Wed, Mar 11, 2020 at 11:48 AM Maulin Vasavada <ma...@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 <ra...@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.vasavada@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.vasavada@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_Pellerin@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:rajinisivaram@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 <
>> rajinisivaram@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.
>> > >>
>> > >
>> >
>>
>
Re: [DISCUSS] KIP-519: Make SSL context/engine configuration extensible
Posted by Maulin Vasavada <ma...@gmail.com>.
Thanks Rajini. Sounds good. I'll make changes and update this thread.
On Wed, Mar 11, 2020 at 6:41 AM Rajini Sivaram <ra...@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.vasavada@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.vasavada@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_Pellerin@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:rajinisivaram@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 <
> rajinisivaram@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.
> > >>
> > >
> >
>
Re: [DISCUSS] KIP-519: Make SSL context/engine configuration extensible
Posted by Rajini Sivaram <ra...@gmail.com>.
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 <ma...@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 <ma...@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_Pellerin@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:rajinisivaram@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 <ra...@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.
> >>
> >
>