Mayuresh & Ismael,
Agree on not breaking interfaces on public API.
+1 on option 2.
Thanks,
Harsha
On Mon, May 23, 2016, at 10:30 AM, Mayuresh Gharat wrote:
> Hi Harsha and Ismael,
>
> Option 2 sounds like a good idea if we want to make this quick fix I
> think.
> Option 4 might require a KIP as its public interface change. I can
> resubmit
> a patch for option 2 or create a KIP if necessary for option 4.
>
> From the previous conversation here, I think Ismael prefers option 2.
> I don't have a strong opinion here since I understand its not easy to
> make
> public API changes but IMO, would go with option 4.
>
> Harsha what do you think on this?
>
>
> Thanks,
>
> Mayuresh
>
> On Mon, May 23, 2016 at 5:45 AM, Ismael Juma <ism...@juma.me.uk> wrote:
>
> > Hi Mayuresh and Harsha,
> >
> > If we were doing this from scratch, I would prefer option 4 too. However,
> > users have their own custom principal builders now and option 2 with a
> > suitably updated javadoc is the way to go in my opinion.
> >
> > Ismael
> >
> > On Sat, May 21, 2016 at 2:28 AM, Harsha <ka...@harsha.io> wrote:
> >
> > > Mayuresh,
> > > Thanks for the write up. With principal builder,
> > > the idea is to reuse a single principal builder
> > > across all the security protocols where its
> > > applicable and given that principal builder has
> > > access to transportLayer and authenticator it
> > > should be able to figure out what type of
> > > transportLayer it is and it should be able
> > > construct the principal based on that and it should
> > > handle all the security protocols that we support.
> > > In your options 1,2 & 4 seems to be doing the same
> > > thing i.e checking what security protocol that a
> > > given transportLayer is and building a principal ,
> > > correct me if I am wrong here. I like going with 4
> > > as others stated on PR . As passing
> > > security_protocol makes it more specific to the
> > > method that its need to be handled . In the interest
> > > of having less config I think option 4 seems to be
> > > better even though it breaks the interface.
> > >
> > > Thanks,
> > > Harsha
> > > On Fri, May 20, 2016, at 05:00 PM, Mayuresh Gharat wrote:
> > > > Hi All,
> > > >
> > > > I came across an issue with plugging in a custom PrincipalBuilder class
> > > > using the config "principal.builder.class" along with a custom
> > Authorizer
> > > > class using the config "authorizer.class.name".
> > > >
> > > > Consider the following scenario :
> > > >
> > > > For PlainText we don't supply any PrincipalBuilder. For SSL we want to
> > > > supply a PrincipalBuilder using the property "principal.builder.class".
> > > >
> > > > a) Now consider we have a broker running on these 2 ports and supply
> > that
> > > > custom principalBuilder class using that config.
> > > >
> > > > b) The interbroker communication is using PlainText. I am using a
> > single
> > > > broker cluster for testing.
> > > >
> > > > c) Now we issue a produce request on the SSL port of the broker.
> > > >
> > > > d) The controller tries to build a channel for plaintext with this
> > broker
> > > > for the new topic instructions.
> > > >
> > > > e) PlainText tries to use the principal builder specified in the
> > > > "principal.builder.class" config which was meant only for SSL port
> > since
> > > > the code path is same
> > "ChannelBuilders.createPrincipalBuilder(configs)".
> > > >
> > > > f) In the custom principal Builder if we are trying to do some cert
> > > > checks
> > > > or down conversion of transportLayer to SSLTransportLayer so that we
> > can
> > > > use its functionality we get error/exception at runtime.
> > > >
> > > > The basic idea is the PlainText channel should not be using the
> > > > PrincipalBuilder meant for other types of channels.
> > > >
> > > > Now there are few options/workarounds to avoid this :
> > > >
> > > > 1) Do instanceOf check in Authorizer.authorize() on TransportLayer
> > > > instance
> > > > passed in and do the correct handling. This is not intuitive and
> > imposes
> > > > a
> > > > strict coding rule on the programmer.
> > > >
> > > > 2) TransportLayer should expose an API for telling the security
> > protocol
> > > > type. This is not too intuitive either.
> > > >
> > > > 3) Add extra configs for Authorizer and PrincipalBuilder for each
> > channel
> > > > type. This gives us a flexibility for the PrincipalBuilder and
> > Authorizer
> > > > handle requests on different types of ports in a different way.
> > > >
> > > > 4) PrincipalBuilder.buildPrincipal() should take in extra parameter for
> > > > the
> > > > type of protocol and we should document this in javadoc to use it to
> > > > handle
> > > > the type of request. This is little better than 1) and 2) but again
> > > > imposes
> > > > a strict coding rule on the programmer.
> > > >
> > > > Just wanted to know what the community thinks about this and get any
> > > > suggestions/feedback . There's some discussion about this here :
> > > > https://github.com/apache/kafka/pull/1403
> > > >
> > > > Thanks,
> > > >
> > > > Mayuresh
> > >
> >
>
>
>
> --
> -Regards,
> Mayuresh R. Gharat
> (862) 250-7125
