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