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