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. >> > >> >> > > >> > >> >
