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 >
