Hi, Mayuresh, Thanks for the updated KIP. A couple of more comments.
1. Do we convert java.security.Principal to KafkaPrincipal for authorization check in SimpleAclAuthorizer? If so, it would be useful to mention that in the wiki so that people can understand how this change doesn't affect the default authorizer implementation. 2. Currently, we log the principal name in the request log in RequestChannel, which has the format of "principalType + SEPARATOR + name;". It would be good if we can keep the same convention after this KIP. One way to do that is to convert java.security.Principal to KafkaPrincipal for logging the requests. Jun On Fri, Feb 17, 2017 at 5:35 PM, Mayuresh Gharat <gharatmayures...@gmail.com > wrote: > Hi Jun, > > I have updated the KIP. Would you mind taking another look? > > Thanks, > > Mayuresh > > On Fri, Feb 17, 2017 at 4:42 PM, Mayuresh Gharat < > gharatmayures...@gmail.com > > wrote: > > > Hi Jun, > > > > Sure sounds good to me. > > > > Thanks, > > > > Mayuresh > > > > On Fri, Feb 17, 2017 at 1:54 PM, Jun Rao <j...@confluent.io> wrote: > > > >> Hi, Mani, > >> > >> Good point on using PrincipalBuilder for SASL. It seems that > >> PrincipalBuilder already has access to Authenticator. So, we could just > >> enable that in SaslChannelBuilder. We probably could do that in a > separate > >> KIP? > >> > >> Hi, Mayuresh, > >> > >> If you don't think there is a concrete use case for using > >> PrincipalBuilder in > >> kafka-acls.sh, perhaps we could do the simpler approach for now? > >> > >> Thanks, > >> > >> Jun > >> > >> > >> > >> On Fri, Feb 17, 2017 at 12:23 PM, Mayuresh Gharat < > >> gharatmayures...@gmail.com> wrote: > >> > >> > @Manikumar, > >> > > >> > Can you give an example how you are planning to use PrincipalBuilder? > >> > > >> > @Jun > >> > Yes, that is right. To give a brief overview, we just extract the cert > >> and > >> > hand it over to a third party library for creating a Principal. So we > >> > cannot create a Principal from just a string. > >> > The main motive behind adding the PrincipalBuilder for kafk-acls.sh > was > >> > that someone else (who can generate a Principal from map of propertie, > >> > <String, String> for example) can use it. > >> > As I said, Linkedin is fine with not making any changes to > Kafka-acls.sh > >> > for now. But we thought that it would be a good improvement to the > tool > >> and > >> > it makes it more flexible and usable. > >> > > >> > Let us know your thoughts, if you would like us to make kafka-acls.sh > >> more > >> > flexible and usable and not limited to Authorizer coming out of the > box. > >> > > >> > Thanks, > >> > > >> > Mayuresh > >> > > >> > > >> > On Thu, Feb 16, 2017 at 10:18 PM, Manikumar < > manikumar.re...@gmail.com> > >> > wrote: > >> > > >> > > Hi Jun, > >> > > > >> > > yes, we can just customize rules to send full principal name. I was > >> > > just thinking to > >> > > use PrinciplaBuilder interface for implementing SASL rules also. So > >> that > >> > > the interface > >> > > will be consistent across protocols. > >> > > > >> > > Thanks > >> > > > >> > > On Fri, Feb 17, 2017 at 1:07 AM, Jun Rao <j...@confluent.io> wrote: > >> > > > >> > > > Hi, Radai, Mayuresh, > >> > > > > >> > > > Thanks for the explanation. Good point on a pluggable authorizer > can > >> > > > customize how acls are added. However, earlier, Mayuresh was > saying > >> > that > >> > > in > >> > > > LinkedIn's customized authorizer, it's not possible to create a > >> > principal > >> > > > from string. If that's the case, will adding the principal builder > >> in > >> > > > kafka-acl.sh help? If the principal can be constructed from a > >> string, > >> > > > wouldn't it be simpler to just let kafka-acl.sh do authorization > >> based > >> > on > >> > > > that string name and not be aware of the principal builder? If you > >> > still > >> > > > think there is a need, perhaps you can add a more concrete use > case > >> > that > >> > > > can't be done otherwise? > >> > > > > >> > > > > >> > > > Hi, Mani, > >> > > > > >> > > > For SASL, if the authorizer needs the full kerberos principal > name, > >> > > > currently, the user can just customize " > sasl.kerberos.principal.to. > >> > > > local.rules" > >> > > > to return the full principal name as the name for authorization, > >> right? > >> > > > > >> > > > Thanks, > >> > > > > >> > > > Jun > >> > > > > >> > > > On Wed, Feb 15, 2017 at 10:25 AM, Mayuresh Gharat < > >> > > > gharatmayures...@gmail.com> wrote: > >> > > > > >> > > > > @Jun thanks for the comments.Please see the replies inline. > >> > > > > > >> > > > > Currently kafka-acl.sh just creates an ACL path in ZK with the > >> > > principal > >> > > > > name string. > >> > > > > ----> Yes, the kafka-acl.sh calls the addAcl() on the inbuilt > >> > > > > SimpleAclAuthorizer which in turn creates an ACL in ZK with the > >> > > Principal > >> > > > > name string. This is because we supply the SimpleAclAuthorizer > as > >> a > >> > > > > commandline argument in the Kafka-acls.sh command. > >> > > > > > >> > > > > The authorizer module in the broker reads the principal name > >> > > > > string from the acl path in ZK and creates the expected > >> > KafkaPrincipal > >> > > > for > >> > > > > matching. As you can see, the expected principal is created on > the > >> > > broker > >> > > > > side, not by the kafka-acl.sh tool. > >> > > > > ----> This is considering the fact that the user is using the > >> > > > > SimpleAclAuthorizer on the broker side and not his own custom > >> > > Authorizer. > >> > > > > The SimpleAclAuthorizer will take the Principal it gets from the > >> > > Session > >> > > > > class . Currently the Principal is KafkaPrincipal. This > >> > KafkaPrincipal > >> > > is > >> > > > > generated from the name of the actual channel Principal, in > >> > > SocketServer > >> > > > > class when processing completed receives. > >> > > > > With this KIP, this will no longer be the case as the Session > >> class > >> > > will > >> > > > > store a java.security.Principal instead of specific > >> KafkaPrincipal. > >> > So > >> > > > the > >> > > > > SimpleAclAuthorizer will construct the KafkaPrincipal from the > >> > channel > >> > > > > Principal it gets from the Session class. > >> > > > > User might not want to use the SimpleAclAuthorizer but use > his/her > >> > own > >> > > > > custom Authorizer. > >> > > > > > >> > > > > The broker already has the ability to > >> > > > > configure PrincipalBuilder. That's why I am not sure if there > is a > >> > need > >> > > > for > >> > > > > kafka-acl.sh to customize PrincipalBuilder. > >> > > > > ----> This is exactly the reason why we want to propose a > >> > > > PrincipalBuilder > >> > > > > in kafka-acls.sh so that the Principal generated by the > >> > > PrincipalBuilder > >> > > > on > >> > > > > broker is consistent with that generated while creating ACLs > using > >> > the > >> > > > > kafka-acls.sh command line tool. > >> > > > > > >> > > > > > >> > > > > *To summarize the above discussions :* > >> > > > > What if we only make the following changes: pass the java > >> principal > >> > in > >> > > > > session and in > >> > > > > SimpleAuthorizer, construct KafkaPrincipal from java principal > >> name. > >> > > Will > >> > > > > that work for LinkedIn? > >> > > > > ------> Yes, this works for Linkedin as we are not using the > >> > > > kafka-acls.sh > >> > > > > tool to create/update/add ACLs, for now. > >> > > > > > >> > > > > Do you think there is a use case for a customized authorizer and > >> > > > kafka-acl > >> > > > > at the > >> > > > > same time? If not, it's better not to complicate the kafka-acl > >> api. > >> > > > > -----> At Linkedin, we don't use this tool for now. So we are > fine > >> > with > >> > > > the > >> > > > > minimal change for now. > >> > > > > > >> > > > > Initially, our change was minimal, just getting the Kafka to > >> preserve > >> > > the > >> > > > > channel principal. Since there was a discussion how > kafka-acls.sh > >> > would > >> > > > > work with this change, on the ticket, we designed a detailed > >> solution > >> > > to > >> > > > > make this tool generally usable with all sorts of combinations > of > >> > > > > Authorizers and PrincipalBuilders and give more flexibility to > the > >> > end > >> > > > > users. > >> > > > > Without the changes proposed for kafka-acls.sh in this KIP, it > >> cannot > >> > > be > >> > > > > used with a custom Authorizer/PrinipalBuilder but will only work > >> with > >> > > > > SimpleAclAuthorizer. > >> > > > > > >> > > > > Although, I would actually like it to work for general scenario, > >> I am > >> > > > fine > >> > > > > with separating it under a separate KIP and limit the scope of > >> this > >> > > KIP. > >> > > > > I will update the KIP accordingly and put this under rejected > >> > > > alternatives > >> > > > > and create a new KIP for the Kafka-acls.sh changes. > >> > > > > > >> > > > > @Manikumar > >> > > > > Since we are limiting the scope of this KIP by not making any > >> changes > >> > > to > >> > > > > kafka-acls.sh, I will cover your concern in a separate KIP that > I > >> > will > >> > > > put > >> > > > > up for kafka-acls.sh. Does that work? > >> > > > > > >> > > > > Thanks, > >> > > > > > >> > > > > Mayuresh > >> > > > > > >> > > > > > >> > > > > On Wed, Feb 15, 2017 at 9:18 AM, radai < > >> radai.rosenbl...@gmail.com> > >> > > > wrote: > >> > > > > > >> > > > > > @jun: > >> > > > > > "Currently kafka-acl.sh just creates an ACL path in ZK with > the > >> > > > principal > >> > > > > > name string" - yes, but not directly. all it actually does it > >> > spin-up > >> > > > the > >> > > > > > Authorizer and call Authorizer.addAcl() on it. > >> > > > > > the vanilla Authorizer goes to ZK. > >> > > > > > but generally speaking, users can plug in their own > Authorizers > >> > (that > >> > > > can > >> > > > > > store/load ACLs to/from wherever). > >> > > > > > > >> > > > > > it would be nice if users who customize Authorizers (and > >> > > > > PrincipalBuilders) > >> > > > > > did not immediately lose the ability to use kafka-acl.sh with > >> their > >> > > new > >> > > > > > Authorizers. > >> > > > > > > >> > > > > > On Wed, Feb 15, 2017 at 5:50 AM, Manikumar < > >> > > manikumar.re...@gmail.com> > >> > > > > > wrote: > >> > > > > > > >> > > > > > > Sorry, I am late to this discussion. > >> > > > > > > > >> > > > > > > PrincipalBuilder is only used for SSL Protocol. > >> > > > > > > For SASL, we use "sasl.kerberos.principal.to.local.rules" > >> config > >> > > to > >> > > > > map > >> > > > > > > SASL principal names to short names. To make it consistent, > >> > > > > > > Do we also need to pass the SASL full principal name to > >> > authorizer > >> > > ? > >> > > > > > > We may need to use PrincipalBuilder for mapping SASL names. > >> > > > > > > > >> > > > > > > Related JIRA is here: > >> > > > > > > https://issues.apache.org/jira/browse/KAFKA-2854 > >> > > > > > > > >> > > > > > > > >> > > > > > > On Wed, Feb 15, 2017 at 7:47 AM, Jun Rao <j...@confluent.io> > >> > wrote: > >> > > > > > > > >> > > > > > > > Hi, Radai, > >> > > > > > > > > >> > > > > > > > Currently kafka-acl.sh just creates an ACL path in ZK with > >> the > >> > > > > > principal > >> > > > > > > > name string. The authorizer module in the broker reads the > >> > > > principal > >> > > > > > name > >> > > > > > > > string from the acl path in ZK and creates the expected > >> > > > > KafkaPrincipal > >> > > > > > > for > >> > > > > > > > matching. As you can see, the expected principal is > created > >> on > >> > > the > >> > > > > > broker > >> > > > > > > > side, not by the kafka-acl.sh tool. The broker already has > >> the > >> > > > > ability > >> > > > > > to > >> > > > > > > > configure PrincipalBuilder. That's why I am not sure if > >> there > >> > is > >> > > a > >> > > > > need > >> > > > > > > for > >> > > > > > > > kafka-acl.sh to customize PrincipalBuilder. > >> > > > > > > > > >> > > > > > > > Thanks, > >> > > > > > > > > >> > > > > > > > Jun > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > On Mon, Feb 13, 2017 at 7:01 PM, radai < > >> > > radai.rosenbl...@gmail.com > >> > > > > > >> > > > > > > wrote: > >> > > > > > > > > >> > > > > > > > > if i understand correctly, kafka-acls.sh spins up an > >> instance > >> > > of > >> > > > > (the > >> > > > > > > > > custom, in our case) Authorizer, and calls things like > >> > > > > addAcls(acls: > >> > > > > > > > > Set[Acl], resource: Resource) on it, which are defined > in > >> the > >> > > > > > > interface, > >> > > > > > > > > hence expected to be "extensible". > >> > > > > > > > > > >> > > > > > > > > (side note: if Authorizer and PrincipalBuilder are > >> defined as > >> > > > > > > extensible > >> > > > > > > > > interfaces, why doesnt class Acl, which is in the > >> signature > >> > for > >> > > > > > > > Authorizer > >> > > > > > > > > calls, use java.security.Principal?) > >> > > > > > > > > > >> > > > > > > > > we would like to be able to use the standard kafka-acl > >> > command > >> > > > line > >> > > > > > for > >> > > > > > > > > defining ACLs even when replacing the vanilla Authorizer > >> and > >> > > > > > > > > PrincipalBuilder (even though we have a management UI > for > >> > these > >> > > > > > > > operations > >> > > > > > > > > within linkedin) - simply because thats the correct > thing > >> to > >> > do > >> > > > > from > >> > > > > > an > >> > > > > > > > > extensibility point of view. > >> > > > > > > > > > >> > > > > > > > > On Mon, Feb 13, 2017 at 1:39 PM, Jun Rao < > >> j...@confluent.io> > >> > > > wrote: > >> > > > > > > > > > >> > > > > > > > > > Hi, Mayuresh, > >> > > > > > > > > > > >> > > > > > > > > > I seems to me that there are two common use cases of > >> > > > authorizer. > >> > > > > > (1) > >> > > > > > > > Use > >> > > > > > > > > > the default SimpleAuthorizer and the kafka-acl to do > >> > > > > authorization. > >> > > > > > > (2) > >> > > > > > > > > Use > >> > > > > > > > > > a customized authorizer and an external tool for > >> > > authorization. > >> > > > > Do > >> > > > > > > you > >> > > > > > > > > > think there is a use case for a customized authorizer > >> and > >> > > > > kafka-acl > >> > > > > > > at > >> > > > > > > > > the > >> > > > > > > > > > same time? If not, it's better not to complicate the > >> > > kafka-acl > >> > > > > api. > >> > > > > > > > > > > >> > > > > > > > > > Thanks, > >> > > > > > > > > > > >> > > > > > > > > > Jun > >> > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > On Mon, Feb 13, 2017 at 10:35 AM, Mayuresh Gharat < > >> > > > > > > > > > gharatmayures...@gmail.com> wrote: > >> > > > > > > > > > > >> > > > > > > > > > > Hi Jun, > >> > > > > > > > > > > > >> > > > > > > > > > > Thanks for the review and comments. Please find the > >> > replies > >> > > > > > inline > >> > > > > > > : > >> > > > > > > > > > > > >> > > > > > > > > > > This is so that in the future, we can extend to > types > >> > like > >> > > > > group. > >> > > > > > > > > > > ---> Yep, I did think the same. But since the > >> > SocketServer > >> > > > was > >> > > > > > > always > >> > > > > > > > > > > creating User type, it wasn't actually used. If we > go > >> > ahead > >> > > > > with > >> > > > > > > > > changes > >> > > > > > > > > > in > >> > > > > > > > > > > this KIP, we will give this power of creating > >> different > >> > > > > Principal > >> > > > > > > > types > >> > > > > > > > > > to > >> > > > > > > > > > > the PrincipalBuilder (which users can define there > >> own). > >> > In > >> > > > > that > >> > > > > > > way > >> > > > > > > > > > Kafka > >> > > > > > > > > > > will not have to deal with handling this. So the > >> > Principal > >> > > > > > building > >> > > > > > > > and > >> > > > > > > > > > > Authorization will be opaque to Kafka which seems > >> like an > >> > > > > > expected > >> > > > > > > > > > > behavior. > >> > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > Hmm, normally, the configurations you specify for > >> > plug-ins > >> > > > > refer > >> > > > > > to > >> > > > > > > > > those > >> > > > > > > > > > > needed to construct the plug-in object. So, it's > kind > >> of > >> > > > weird > >> > > > > to > >> > > > > > > use > >> > > > > > > > > > that > >> > > > > > > > > > > to call a method. For example, why can't > >> > > > > > > > principalBuilderService.rest. > >> > > > > > > > > > url > >> > > > > > > > > > > be passed in through the configure() method and the > >> > > > > > implementation > >> > > > > > > > can > >> > > > > > > > > > use > >> > > > > > > > > > > that to build principal. This way, there is only a > >> single > >> > > > > method > >> > > > > > to > >> > > > > > > > > > compute > >> > > > > > > > > > > the principal in a consistent way in the broker and > in > >> > the > >> > > > > > > kafka-acl > >> > > > > > > > > > tool. > >> > > > > > > > > > > ----> We can do that as well. But since the rest url > >> is > >> > not > >> > > > > > related > >> > > > > > > > to > >> > > > > > > > > > the > >> > > > > > > > > > > Principal, it seems out of place to me to pass it > >> every > >> > > time > >> > > > we > >> > > > > > > have > >> > > > > > > > to > >> > > > > > > > > > > create a Principal. I should replace > >> "principalConfigs" > >> > > with > >> > > > > > > > > > > "principalProperties". > >> > > > > > > > > > > I was trying to differentiate the configs/properties > >> that > >> > > are > >> > > > > > used > >> > > > > > > to > >> > > > > > > > > > > create the PrincipalBuilder class and the > >> > > > Principal/Principals > >> > > > > > > > itself. > >> > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > For LinkedIn's use case, do you actually use the > >> > kafka-acl > >> > > > > tool? > >> > > > > > My > >> > > > > > > > > > > understanding is that LinkedIn does authorization > >> through > >> > > an > >> > > > > > > external > >> > > > > > > > > > tool. > >> > > > > > > > > > > ----> For Linkedin's use case we don't actually use > >> the > >> > > > > kafka-acl > >> > > > > > > > tool > >> > > > > > > > > > > right now. As per the discussion that we had on > >> > > > > > > > > > > https://issues.apache.org/jira/browse/KAFKA-4454, > we > >> > > thought > >> > > > > > that > >> > > > > > > it > >> > > > > > > > > > would > >> > > > > > > > > > > be good to make kafka-acl tool changes, to make it > >> > flexible > >> > > > and > >> > > > > > we > >> > > > > > > > > might > >> > > > > > > > > > be > >> > > > > > > > > > > even able to use it in future. > >> > > > > > > > > > > > >> > > > > > > > > > > It seems it's simpler if kafka-acl doesn't to need > to > >> > > > > understand > >> > > > > > > the > >> > > > > > > > > > > principal builder. The tool does authorization based > >> on a > >> > > > > string > >> > > > > > > > name, > >> > > > > > > > > > > which is expected to match the principal name. So, I > >> am > >> > > > > wondering > >> > > > > > > why > >> > > > > > > > > the > >> > > > > > > > > > > tool needs to know the principal builder. > >> > > > > > > > > > > ----> If we don't make this change, I am not sure > how > >> > > > > clients/end > >> > > > > > > > users > >> > > > > > > > > > > will be able to use this tool if they have there own > >> > > > Authorizer > >> > > > > > > that > >> > > > > > > > > does > >> > > > > > > > > > > Authorization based on Principal, that has more > >> > information > >> > > > > apart > >> > > > > > > > from > >> > > > > > > > > > name > >> > > > > > > > > > > and type. > >> > > > > > > > > > > > >> > > > > > > > > > > What if we only make the following changes: pass the > >> java > >> > > > > > principal > >> > > > > > > > in > >> > > > > > > > > > > session and in > >> > > > > > > > > > > SimpleAuthorizer, construct KafkaPrincipal from java > >> > > > principal > >> > > > > > > name. > >> > > > > > > > > Will > >> > > > > > > > > > > that work for LinkedIn? > >> > > > > > > > > > > ----> This can work for Linkedin but as explained > >> above, > >> > it > >> > > > > does > >> > > > > > > not > >> > > > > > > > > seem > >> > > > > > > > > > > like a complete design from open source point of > view. > >> > > > > > > > > > > > >> > > > > > > > > > > Thanks, > >> > > > > > > > > > > > >> > > > > > > > > > > Mayuresh > >> > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > On Thu, Feb 9, 2017 at 11:29 AM, Jun Rao < > >> > j...@confluent.io > >> > > > > >> > > > > > wrote: > >> > > > > > > > > > > > >> > > > > > > > > > > > Hi, Mayuresh, > >> > > > > > > > > > > > > >> > > > > > > > > > > > Thanks for the reply. A few more comments below. > >> > > > > > > > > > > > > >> > > > > > > > > > > > On Wed, Feb 8, 2017 at 9:14 PM, Mayuresh Gharat < > >> > > > > > > > > > > > gharatmayures...@gmail.com> > >> > > > > > > > > > > > wrote: > >> > > > > > > > > > > > > >> > > > > > > > > > > > > Hi Jun, > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > Thanks for the review. Please find the responses > >> > > inline. > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > 1. It seems the problem that you are trying to > >> > address > >> > > is > >> > > > > > that > >> > > > > > > > java > >> > > > > > > > > > > > > principal returned from KafkaChannel may have > >> > > additional > >> > > > > > fields > >> > > > > > > > > than > >> > > > > > > > > > > name > >> > > > > > > > > > > > > that are needed during authorization. Have you > >> > > > considered a > >> > > > > > > > > > customized > >> > > > > > > > > > > > > PrincipleBuilder that extracts all needed fields > >> from > >> > > > java > >> > > > > > > > > principal > >> > > > > > > > > > > and > >> > > > > > > > > > > > > squeezes them as a json in the name of the > >> returned > >> > > > > > principal? > >> > > > > > > > > Then, > >> > > > > > > > > > > the > >> > > > > > > > > > > > > authorizer can just parse the json and extract > >> needed > >> > > > > fields. > >> > > > > > > > > > > > > ---> Yes we had thought about this. We use a > third > >> > > party > >> > > > > > > library > >> > > > > > > > > that > >> > > > > > > > > > > > takes > >> > > > > > > > > > > > > in the passed in cert and creates the Principal. > >> This > >> > > > > > Principal > >> > > > > > > > is > >> > > > > > > > > > then > >> > > > > > > > > > > > > used by the library to make the decision > >> (ALLOW/DENY) > >> > > > when > >> > > > > we > >> > > > > > > > call > >> > > > > > > > > it > >> > > > > > > > > > > in > >> > > > > > > > > > > > > the Authorizer. It does not have an API to > create > >> the > >> > > > > > Principal > >> > > > > > > > > from > >> > > > > > > > > > a > >> > > > > > > > > > > > > String. If it did support, still we would have > to > >> be > >> > > > aware > >> > > > > of > >> > > > > > > the > >> > > > > > > > > > > > internal > >> > > > > > > > > > > > > details of the library, like the field values it > >> > > creates > >> > > > > from > >> > > > > > > the > >> > > > > > > > > > > certs, > >> > > > > > > > > > > > > defaults and so on. > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > 2. Could you explain how the default authorizer > >> works > >> > > > now? > >> > > > > > > > > Currently, > >> > > > > > > > > > > the > >> > > > > > > > > > > > > code just compares the two principal objects. > Are > >> we > >> > > > > > converting > >> > > > > > > > the > >> > > > > > > > > > > java > >> > > > > > > > > > > > > principal to a KafkaPrincipal there? > >> > > > > > > > > > > > > ---> The SimpleAclAuthorizer currently expects > >> that, > >> > > the > >> > > > > > > > Principal > >> > > > > > > > > it > >> > > > > > > > > > > > > fetches from the Session object is an instance > of > >> > > > > > > KafkaPrincipal. > >> > > > > > > > > It > >> > > > > > > > > > > then > >> > > > > > > > > > > > > uses it compare with the KafkaPrincipal > extracted > >> > from > >> > > > the > >> > > > > > > stored > >> > > > > > > > > > ACLs. > >> > > > > > > > > > > > In > >> > > > > > > > > > > > > this case, we can construct the KafkaPrincipal > >> object > >> > > on > >> > > > > the > >> > > > > > > fly > >> > > > > > > > by > >> > > > > > > > > > > using > >> > > > > > > > > > > > > the name of the Principal as follows : > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > *val principal = session.principal* > >> > > > > > > > > > > > > *val kafkaPrincipal = new > >> > > KafkaPrincipal(KafkaPrincipal. > >> > > > > > > > USER_TYPE, > >> > > > > > > > > > > > > principal.getName)* > >> > > > > > > > > > > > > I was also planning to get rid of the > >> principalType > >> > > field > >> > > > > in > >> > > > > > > > > > > > > KafkaPrincipal as > >> > > > > > > > > > > > > it is always set to *"*User*"* in the > SocketServer > >> > > > > currently. > >> > > > > > > > After > >> > > > > > > > > > > this > >> > > > > > > > > > > > > KIP, it will no longer be used in SocketServer. > >> But > >> > to > >> > > > > > maintain > >> > > > > > > > > > > backwards > >> > > > > > > > > > > > > compatibility of kafka-acls.sh, I preserved it. > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > > This is so that in the future, we can extend to > >> types > >> > > like > >> > > > > > group. > >> > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > > 3. Do we need to add the following method in > >> > > > > > PrincipalBuilder? > >> > > > > > > > The > >> > > > > > > > > > > > configs > >> > > > > > > > > > > > > are already passed in through configure() and an > >> > > > > > implementation > >> > > > > > > > can > >> > > > > > > > > > > cache > >> > > > > > > > > > > > > it and use it in buildPrincipal(). It's also not > >> > clear > >> > > to > >> > > > > me > >> > > > > > > > where > >> > > > > > > > > we > >> > > > > > > > > > > > call > >> > > > > > > > > > > > > the new and the old method, and whether both > will > >> be > >> > > > called > >> > > > > > or > >> > > > > > > > one > >> > > > > > > > > of > >> > > > > > > > > > > > them > >> > > > > > > > > > > > > will be called. > >> > > > > > > > > > > > > Principal buildPrincipal(Map<String, ?> > >> > > > principalConfigs); > >> > > > > > > > > > > > > ---> My thought was that the configure() method > >> will > >> > be > >> > > > > used > >> > > > > > to > >> > > > > > > > > build > >> > > > > > > > > > > the > >> > > > > > > > > > > > > PrincipalBuilder class object itself. It follows > >> the > >> > > same > >> > > > > way > >> > > > > > > as > >> > > > > > > > > > > > Authorizer > >> > > > > > > > > > > > > gets configured. The buildPrincipal(Map<String, > ?> > >> > > > > > > > > principalConfigs) > >> > > > > > > > > > > will > >> > > > > > > > > > > > > be used to build individual principals. > >> > > > > > > > > > > > > Let me give an example, with the kafka-acls.sh : > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > - bin/kafka-acls.sh --principalBuilder > >> > > > > > > > > > > > > userDefinedPackage.kafka. > >> > security.PrincipalBuilder > >> > > > > > > > > > > > > --principalBuilder-properties > >> > > > > > > > > > > > > principalBuilderService.rest.url=URL > >> > --authorizer > >> > > > > > > > > > > > > kafka.security.auth.SimpleAclAuthorizer > >> > > > > > > > --authorizer-properties > >> > > > > > > > > > > > > zookeeper.connect=localhost:2181 --add > >> > > > > --allow-principal > >> > > > > > > > > name=bob > >> > > > > > > > > > > > > type=USER_PRINCIPAL --allow-principal > >> > > > > > > name=ALPHA-GAMMA-SERVICE > >> > > > > > > > > > > > > type=SERVICE_PRINCIPAL --allow-hosts > >> Host1,Host2 > >> > > > > > > --operations > >> > > > > > > > > > > > Read,Write > >> > > > > > > > > > > > > --topic Test-topic > >> > > > > > > > > > > > > 1. *userDefinedPackage.kafka. > >> > > > > > security.PrincipalBuilder* > >> > > > > > > is > >> > > > > > > > > the > >> > > > > > > > > > > > user > >> > > > > > > > > > > > > defined PrincipalBuilder class. > >> > > > > > > > > > > > > 2. *principalBuilderService.rest.url=URL* > >> can > >> > > be a > >> > > > > > > remote > >> > > > > > > > > > > service > >> > > > > > > > > > > > > that provides you an HTTP endpoint which > >> takes > >> > > in a > >> > > > > set > >> > > > > > > of > >> > > > > > > > > > > > > parameters and > >> > > > > > > > > > > > > provides you with the Principal. > >> > > > > > > > > > > > > 3. *name=bob type=USER_PRINCIPAL* can be > >> used > >> > by > >> > > > > > > > > > PrincipalBuilder > >> > > > > > > > > > > > to > >> > > > > > > > > > > > > create UserPrincipal with name as bob > >> > > > > > > > > > > > > 4. *name=ALPHA-GAMMA-SERVICE > >> > > type=SERVICE_PRINCIPAL > >> > > > > > *can > >> > > > > > > be > >> > > > > > > > > > used > >> > > > > > > > > > > by > >> > > > > > > > > > > > > PrincipalBuilder to create a > >> ServicePrincipal > >> > > with > >> > > > > name > >> > > > > > > as > >> > > > > > > > > > > > > ALPHA-GAMMA-SERVICE. > >> > > > > > > > > > > > > - This seems more flexible and intuitive to > me > >> > from > >> > > > end > >> > > > > > > user's > >> > > > > > > > > > > > > perspective. > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > > Hmm, normally, the configurations you specify for > >> > > plug-ins > >> > > > > > refer > >> > > > > > > to > >> > > > > > > > > > those > >> > > > > > > > > > > > needed to construct the plug-in object. So, it's > >> kind > >> > of > >> > > > > weird > >> > > > > > to > >> > > > > > > > use > >> > > > > > > > > > > that > >> > > > > > > > > > > > to call a method. For example, why can't > >> > > > > > > > > principalBuilderService.rest. > >> > > > > > > > > > > url > >> > > > > > > > > > > > be passed in through the configure() method and > the > >> > > > > > > implementation > >> > > > > > > > > can > >> > > > > > > > > > > use > >> > > > > > > > > > > > that to build principal. This way, there is only a > >> > single > >> > > > > > method > >> > > > > > > to > >> > > > > > > > > > > compute > >> > > > > > > > > > > > the principal in a consistent way in the broker > and > >> in > >> > > the > >> > > > > > > > kafka-acl > >> > > > > > > > > > > tool. > >> > > > > > > > > > > > > >> > > > > > > > > > > > For LinkedIn's use case, do you actually use the > >> > > kafka-acl > >> > > > > > tool? > >> > > > > > > My > >> > > > > > > > > > > > understanding is that LinkedIn does authorization > >> > through > >> > > > an > >> > > > > > > > external > >> > > > > > > > > > > tool. > >> > > > > > > > > > > > It seems it's simpler if kafka-acl doesn't to need > >> to > >> > > > > > understand > >> > > > > > > > the > >> > > > > > > > > > > > principal builder. The tool does authorization > based > >> > on a > >> > > > > > string > >> > > > > > > > > name, > >> > > > > > > > > > > > which is expected to match the principal name. So, > >> I am > >> > > > > > wondering > >> > > > > > > > why > >> > > > > > > > > > the > >> > > > > > > > > > > > tool needs to know the principal builder. What if > we > >> > only > >> > > > > make > >> > > > > > > the > >> > > > > > > > > > > > following changes: pass the java principal in > >> session > >> > and > >> > > > in > >> > > > > > > > > > > > SimpleAuthorizer, construct KafkaPrincipal from > java > >> > > > > principal > >> > > > > > > > name. > >> > > > > > > > > > Will > >> > > > > > > > > > > > that work for LinkedIn? > >> > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > > Principal buildPrincipal(Map<String, ?> > >> > > principalConfigs) > >> > > > > > will > >> > > > > > > be > >> > > > > > > > > > > called > >> > > > > > > > > > > > > from the commandline client kafka-acls.sh while > >> the > >> > > other > >> > > > > API > >> > > > > > > can > >> > > > > > > > > be > >> > > > > > > > > > > > called > >> > > > > > > > > > > > > at runtime when Kafka receives a client request > >> over > >> > > > > request > >> > > > > > > > > channel. > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > 4. The KIP has "If users use there custom > >> > > > PrincipalBuilder, > >> > > > > > > they > >> > > > > > > > > will > >> > > > > > > > > > > > have > >> > > > > > > > > > > > > to implement there custom Authorizer as the out > of > >> > box > >> > > > > > > Authorizer > >> > > > > > > > > > that > >> > > > > > > > > > > > > Kafka provides uses KafkaPrincipal." This is not > >> > ideal > >> > > > for > >> > > > > > > > existing > >> > > > > > > > > > > > users. > >> > > > > > > > > > > > > Could we avoid that? > >> > > > > > > > > > > > > ---> Yes, this is possible to avoid if we do > >> point 2. > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > Thanks, > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > Mayuresh > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > On Wed, Feb 8, 2017 at 3:31 PM, Jun Rao < > >> > > > j...@confluent.io> > >> > > > > > > > wrote: > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > > Hi, Mayuresh, > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > > Thanks for the KIP. A few comments below. > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > > 1. It seems the problem that you are trying to > >> > > address > >> > > > is > >> > > > > > > that > >> > > > > > > > > java > >> > > > > > > > > > > > > > principal returned from KafkaChannel may have > >> > > > additional > >> > > > > > > fields > >> > > > > > > > > > than > >> > > > > > > > > > > > name > >> > > > > > > > > > > > > > that are needed during authorization. Have you > >> > > > > considered a > >> > > > > > > > > > > customized > >> > > > > > > > > > > > > > PrincipleBuilder that extracts all needed > fields > >> > from > >> > > > > java > >> > > > > > > > > > principal > >> > > > > > > > > > > > and > >> > > > > > > > > > > > > > squeezes them as a json in the name of the > >> returned > >> > > > > > > principal? > >> > > > > > > > > > Then, > >> > > > > > > > > > > > the > >> > > > > > > > > > > > > > authorizer can just parse the json and extract > >> > needed > >> > > > > > fields. > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > > 2. Could you explain how the default > authorizer > >> > works > >> > > > > now? > >> > > > > > > > > > Currently, > >> > > > > > > > > > > > the > >> > > > > > > > > > > > > > code just compares the two principal objects. > >> Are > >> > we > >> > > > > > > converting > >> > > > > > > > > the > >> > > > > > > > > > > > java > >> > > > > > > > > > > > > > principal to a KafkaPrincipal there? > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > > 3. Do we need to add the following method in > >> > > > > > > PrincipalBuilder? > >> > > > > > > > > The > >> > > > > > > > > > > > > configs > >> > > > > > > > > > > > > > are already passed in through configure() and > an > >> > > > > > > implementation > >> > > > > > > > > can > >> > > > > > > > > > > > cache > >> > > > > > > > > > > > > > it and use it in buildPrincipal(). It's also > not > >> > > clear > >> > > > to > >> > > > > > me > >> > > > > > > > > where > >> > > > > > > > > > we > >> > > > > > > > > > > > > call > >> > > > > > > > > > > > > > the new and the old method, and whether both > >> will > >> > be > >> > > > > called > >> > > > > > > or > >> > > > > > > > > one > >> > > > > > > > > > of > >> > > > > > > > > > > > > them > >> > > > > > > > > > > > > > will be called. > >> > > > > > > > > > > > > > Principal buildPrincipal(Map<String, ?> > >> > > > > principalConfigs); > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > > 4. The KIP has "If users use there custom > >> > > > > PrincipalBuilder, > >> > > > > > > > they > >> > > > > > > > > > will > >> > > > > > > > > > > > > have > >> > > > > > > > > > > > > > to implement there custom Authorizer as the > out > >> of > >> > > box > >> > > > > > > > Authorizer > >> > > > > > > > > > > that > >> > > > > > > > > > > > > > Kafka provides uses KafkaPrincipal." This is > not > >> > > ideal > >> > > > > for > >> > > > > > > > > existing > >> > > > > > > > > > > > > users. > >> > > > > > > > > > > > > > Could we avoid that? > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > > Thanks, > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > > Jun > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > > On Fri, Feb 3, 2017 at 11:25 AM, Mayuresh > >> Gharat < > >> > > > > > > > > > > > > > gharatmayures...@gmail.com > >> > > > > > > > > > > > > > > wrote: > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > Hi All, > >> > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > It seems that there is no further concern > with > >> > the > >> > > > > > KIP-111. > >> > > > > > > > At > >> > > > > > > > > > this > >> > > > > > > > > > > > > point > >> > > > > > > > > > > > > > > we would like to start the voting process. > The > >> > KIP > >> > > > can > >> > > > > be > >> > > > > > > > found > >> > > > > > > > > > at > >> > > > > > > > > > > > > > > https://cwiki.apache.org/ > >> > confluence/pages/viewpage > >> > > . > >> > > > > > > > > > > > > > action?pageId=67638388 > >> > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > Thanks, > >> > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > Mayuresh > >> > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > Thanks, > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > -- > >> > > > > > > > > > > > > -Regards, > >> > > > > > > > > > > > > Mayuresh R. Gharat > >> > > > > > > > > > > > > (862) 250-7125 > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > Thanks, > >> > > > > > > > > > > > > >> > > > > > > > > > > > Jun > >> > > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > -- > >> > > > > > > > > > > -Regards, > >> > > > > > > > > > > Mayuresh R. Gharat > >> > > > > > > > > > > (862) 250-7125 > >> > > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > > >> > > > > > >> > > > > -- > >> > > > > -Regards, > >> > > > > Mayuresh R. Gharat > >> > > > > (862) 250-7125 > >> > > > > > >> > > > > >> > > > >> > > >> > > >> > > >> > -- > >> > -Regards, > >> > Mayuresh R. Gharat > >> > (862) 250-7125 > >> > > >> > > > > > > > > -- > > -Regards, > > Mayuresh R. Gharat > > (862) 250-7125 > > > > > > -- > -Regards, > Mayuresh R. Gharat > (862) 250-7125 >
