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.
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.
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
> >
>
--
-Regards,
Mayuresh R. Gharat
(862) 250-7125
