Hi Jason, Thanks for the replies.
I think it would be better to discuss with an example that we were trying to address with KIP-111 and see if the current mentioned solution would address it. Let's consider a third party library called authz_lib that is provided by some Security team at some company. - When we call authz_lib.createPrincipal(X509_cert), it would return an AuthzPrincipal that implements Java.Security.Principal. - The authz_lib also provides an checkAccess(....) call that takes in 3 parameters : - authz_principal - operation type ("Read", "Write"...) - resource (for simplicity lets consider it as a TopicName) - The AuthzPrincipal looks like this : class AuthzPrincipal implements java.security.Principal { String name; String field1; Object field2; Object field3; .....//Some third party logic...... } - In PrincipalBuilder.buildPrincipal() would return AuthzPrincipal as follows : public Principal buildPrincipal(...) { ...... X509Certificate x509Cert = session.getCert(..); return authz_lib.createPrincipal(x509Cert); } - The custom Authorizer (lets call it CustomAuthzAuthorizer), we would use the checkAccess() function provided by the authz_lib as follows : public class CustomAuthzAuthorizer implements Authorizer { ......... public boolean authorize(.....) { AuthzPrincipal authz_principal = (AuthzPrincipal) session.getPrincipal(); return authz_lib.checkAccess(authz_principal, "Read", "topicX"); } .......... } - The issue with current implementation is that in processCompletedReceives() in SocketServer we create a KafkaPrincipal that just extracts the name from AuthzPrincipal as follows : session = RequestChannel.Session(new KafkaPrincipal(KafkaPrincipal.USER_TYPE, *openOrClosingChannel.principal.getName*), openOrClosingChannel.socketAddress) So the "AuthzPrincipal authz_principal = (AuthzPrincipal) session.getPrincipal()" call in the CustomAuthzAuthorizer would error out because we are trying to cast a KafkaPrincipal to AuthzPrincipal. In your reply when you said that : The KIP says that a user can have a class that extends KafkaPrincipal. Would this extended class be used when constructing the Session object in the SocketServer instead of constructing a new KafkaPrincipal? Yes, that's correct. We want to allow the authorizer to be able to leverage > additional information from the authentication layer. Would it make sense to make this extended class pluggable and when constructing the Session object in SocketServer check if a plugin is defined and use it and if not use the default KafkaPrincipal something like : if (getConfig("principal.pluggedIn.class").isDefined()) //"principal.pluggedIn.class" is just an example name for the config { session = RequestChannel.Session(*Extended_Plugged_In_Class*, openOrClosingChannel.socketAddress) } else { session = RequestChannel.Session(new KafkaPrincipal(KafkaPrincipal.USER_TYPE, *openOrClosingChannel.principal.getName*), openOrClosingChannel.socketAddress) } This would solve the issue above as follows : We can have something like : public class Extended_Plugged_In_Class extends KafkaPrincipal { AuthzPrincipal authzPrincipal; public Extended_Plugged_In_Class(....., AuthzPrincipal principal) { super(...); authzPrincipal = principal; } ...... public AuthzPrincipal getAuthzPrincipal() { return authzPrincipal; } } In the CustomAuthzAuthorizer we could do something like : public class CustomAuthzAuthorizer implements Authorizer { ......... public boolean authorize(.....) { Extended_Plugged_In_Class extended_Kafka_Principal = (Extended_Plugged_In_Class) session.getPrincipal(); AuthzPrincipal authz_principal = extended_Kafka_Principal.getAuthzPrincipal(); return authz_lib.checkAccess(authz_principal, "Read", "topicX"); } .......... } Thanks, Mayuresh On Fri, Aug 25, 2017 at 11:53 AM, Jason Gustafson <ja...@confluent.io> wrote: > Hey Mayuresh, > > Thanks for the comments. > > - The KIP says that a user can have a class that extends KafkaPrincipal. > > Would this extended class be used when constructing the Session object > > in > > the SocketServer instead of constructing a new KafkaPrincipal? > > > Yes, that's correct. We want to allow the authorizer to be able to leverage > additional information from the authentication layer. > > - The KIP says "A principal is always identifiable by a principal type > > and a name. Nothing else should ever be required." This might not be > > true > > always, right? For example, we might have a custom third party ACL > > library > > that creates a custom Principal from the passed in cert (this is done > in > > PrincipalBuilder/KafkaPrincipalBuilder) and the custom Authorizer > might > > use this third party library to authorize using this custom Principal > > object. The developer who is implementing the Kafka Authorizer should > > not be caring about what the custom Principal would look like and its > > details, since it will just pass it to the third party library in > Kafka > > Authorizer's authorize() call. > > > I'm not sure I understand this. Are you saying that the authorizer and > principal builder are implemented by separate individuals? If the > authorizer doesn't understand how to identify the principal, then it > wouldn't work, right? Maybe I'm missing something? > > Let me explain how I see this. The simple ACL authorizer that Kafka ships > with understands user principals as consisting of a type and a name. Any > principal builder that follows this assumption will work with the > SimpleAclAuthorizer. In some cases, the principal builder may provide > additional metadata in a KafkaPrincipal extension such as user groups or > roles. This information is not needed to identify the user principal, so > the builder is still compatible with the SimpleAclAuthorizer. It would also > be compatible with a RoleBasedAuthorizer which understood how to use the > role metadata provided by the KafkaPrincipal extension. Basically what we > would have is a user principal which is related to one or more role > principals through the KafkaPrincipal extension. Both user and role > principals are identifiable with a type and a name, so the ACL command tool > can then be used (perhaps with a custom authorizer) to define permissions > in either case. > > On the other hand, if a user principal is identified by more than just its > name, then it is not compatible with the SimpleAclAuthorizer. This doesn't > necessarily rule out this use case. As long as the authorizer and the > principal builder both agree on how user principals are identified, then > they can still be used together. But I am explicitly leaving out support in > the ACL command tool for this use case in this KIP. This is mostly about > clarifying what is compatible with the authorization system that Kafka > ships with. Of course we can always reconsider it in the future. > > Thanks, > Jason > > On Fri, Aug 25, 2017 at 10:48 AM, Mayuresh Gharat < > gharatmayures...@gmail.com> wrote: > > > Hi Jason, > > > > Thanks a lot for the KIP and sorry for the delayed response. > > > > I had a few questions : > > > > > > - The KIP says that a user can have a class that extends > KafkaPrincipal. > > Would this extended class be used when constructing the Session object > > in > > the SocketServer instead of constructing a new KafkaPrincipal? > > > > > > - The KIP says "A principal is always identifiable by a principal type > > and a name. Nothing else should ever be required." This might not be > > true > > always, right? For example, we might have a custom third party ACL > > library > > that creates a custom Principal from the passed in cert (this is done > in > > PrincipalBuilder/KafkaPrincipalBuilder) and the custom Authorizer > might > > use this third party library to authorize using this custom Principal > > object. The developer who is implementing the Kafka Authorizer should > > not be caring about what the custom Principal would look like and its > > details, since it will just pass it to the third party library in > Kafka > > Authorizer's authorize() call. > > > > > > Thanks, > > > > Mayuresh > > > > > > On Thu, Aug 24, 2017 at 10:21 AM, Mayuresh Gharat < > > gharatmayures...@gmail.com> wrote: > > > > > Sure. > > > > > > Thanks, > > > > > > Mayuresh > > > > > > On Wed, Aug 23, 2017 at 5:07 PM, Jun Rao <j...@confluent.io> wrote: > > > > > >> Hi, Mayuresh, > > >> > > >> Since this KIP covers the requirement in KIP-111, could you review it > > too? > > >> > > >> Thanks, > > >> > > >> Jun > > >> > > >> > > >> On Tue, Aug 22, 2017 at 3:04 PM, Jason Gustafson <ja...@confluent.io> > > >> wrote: > > >> > > >>> Bump. I'll open a vote in a few days if there are no comments. > > >>> > > >>> Thanks, > > >>> Jason > > >>> > > >>> On Sat, Aug 19, 2017 at 12:28 AM, Ismael Juma <ism...@juma.me.uk> > > wrote: > > >>> > > >>> > Thanks for the KIP Jason. It seems reasonable and cleans up some > > >>> > inconsistencies in that area. It would be great to get some > feedback > > >>> from > > >>> > Mayuresh and others who worked on KIP-111. > > >>> > > > >>> > Ismael > > >>> > > > >>> > On Thu, Aug 17, 2017 at 1:21 AM, Jason Gustafson < > ja...@confluent.io > > > > > >>> > wrote: > > >>> > > > >>> > > Hi All, > > >>> > > > > >>> > > I've added a new KIP to improve and extend the principal building > > API > > >>> > that > > >>> > > Kafka exposes: > > >>> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > >>> > > 189%3A+Improve+principal+builder+interface+and+add+ > > support+for+SASL > > >>> > > . > > >>> > > > > >>> > > As always, feedback is appreciated. > > >>> > > > > >>> > > Thanks, > > >>> > > Jason > > >>> > > > > >>> > > > >>> > > >> > > >> > > > > > > > > > -- > > > -Regards, > > > Mayuresh R. Gharat > > > (862) 250-7125 > > > > > > > > > > > -- > > -Regards, > > Mayuresh R. Gharat > > (862) 250-7125 > > > -- -Regards, Mayuresh R. Gharat (862) 250-7125