Thanks, Jason. +1 (non-binding)
Colin
On Tue, Aug 29, 2017, at 09:12, Jason Gustafson wrote:
> Hi Colin,
>
> Thanks for the comments. Seems reasonable to provide a safer `equals` for
> extensions. I don't think this needs to be part of the KIP, but I can add
> it to my patch.
>
> Moving `fromString` makes sense also. This method is technically already
> part of the public API, which means we should probably deprecate it
> instead
> of removing it from `KafkaPrincipal`. I'll mention this in the KIP.
>
> Thanks,
> Jason
>
> On Mon, Aug 28, 2017 at 5:50 PM, Ted Yu <yuzhih...@gmail.com> wrote:
>
> > bq. change the check in equals() to be this.getClass().equals(other.
> > getClass())
> >
> > I happened to have Effective Java on hand.
> > Please take a look at related discussion on page 39.
> >
> > Josh later on mentioned Liskov substitution principle and a workaround
> > (favoring composition).
> >
> > FYI
> >
> >
> >
> > On Mon, Aug 28, 2017 at 4:48 PM, Colin McCabe <cmcc...@apache.org> wrote:
> >
> > > Thanks, Jason, this is a great improvement!
> > >
> > > One minor nit. The current KafkaPrincipal#equals looks like this:
> > >
> > > > @Override
> > > > public boolean equals(Object o) {
> > > > if (this == o) return true;
> > > > if (!(o instanceof KafkaPrincipal)) return false;
> > > >
> > > > KafkaPrincipal that = (KafkaPrincipal) o;
> > > >
> > > > if (!principalType.equals(that.principalType)) return false;
> > > > return name.equals(that.name);
> > > > }
> > >
> > > So if I implement MyKafkaPrincipalWithGroup that has an extra groupName
> > > field, I can have this situation:
> > >
> > > > KafkaPrincipal oldPrincipal = new KafkaPrincipal("User", "foo");
> > > > MyKafkaPrincipalWithGroup newPrincipal =
> > > > new MyKafkaPrincipalWithGroup("User", "foo", "mygroup")
> > > > System.out.println("" + oldPrincipal == newPrincipal) // true
> > > > System.out.println("" + newPrincipal == oldPrincipal) // false
> > >
> > > This is clearly bad, because it makes equality non-transitive. The
> > > reason for this problem is because KafkaPrincipal#equals checks if
> > > MyKafkaPrincipalWithGroup is an instance of KafkaPrincipal-- and it is.
> > > It then proceeds to check if the user is the same-- and it is. So it
> > > returns true. It does not check the groups field, because it doesn't
> > > know about it. On the other hand, MyKafkaPrincipalWithGroup#equals will
> > > check to see KafkaPrincipal is an instance of
> > > MyKafkaPrincipalWithGroup-- and it isn't. So it returns false.
> > >
> > > In the KafkaPrincipal base class, it would be better to change the check
> > > in equals() to be this.getClass().equals(other.getClass()). In other
> > > words, check for exact class equality, not instanceof.
> > >
> > > Alternately, we could implement a final equals method in the base class
> > > that compares by the toString method, under the assumption that any
> > > difference in KafkaPrincipal objects should be reflected in their
> > > serialized form.
> > >
> > > > @Override
> > > > public final boolean equals(Object o) {
> > > > if (this == o) return true;
> > > > if (!(o instanceof KafkaPrincipal)) return false;
> > > > return toString().equals(o.toString());
> > > > }
> > >
> > > Another question related to subclassing KafkaPrincipal: should we move
> > > KafkaPrincipal#fromString into an internal, non-public class? It seems
> > > like people might expect
> > > KafkaPrincipal.fromString(myCustomPrincipal.toString()) to work, but it
> > > will not for subclasses.
> > >
> > > best,
> > > Colin
> > >
> > >
> > > On Mon, Aug 28, 2017, at 15:51, Jason Gustafson wrote:
> > > > Thanks all for the discussion. I'll begin a vote in the next couple
> > days.
> > > >
> > > > -Jason
> > > >
> > > > On Fri, Aug 25, 2017 at 5:01 PM, Don Bosco Durai <bo...@apache.org>
> > > > wrote:
> > > >
> > > > > Jason, thanks for the clarification.
> > > > >
> > > > > Bosco
> > > > >
> > > > >
> > > > > On 8/25/17, 4:59 PM, "Jason Gustafson" <ja...@confluent.io> wrote:
> > > > >
> > > > > Hey Don,
> > > > >
> > > > > That is not actually part of the KIP. It was a (somewhat
> > pedantic)
> > > > > example
> > > > > used to illustrate how the kafka principal semantics could be
> > > applied
> > > > > to
> > > > > authorizers which understood group-level ACLs. The key point is
> > > this:
> > > > > although a principal is identified only by its type and name, the
> > > > > KafkaPrincipal can be used to represent relations to other
> > > principals.
> > > > > In
> > > > > this case, we have a user principal which is related to a group
> > > > > principal
> > > > > through the UserPrincipalAndGroup object. A GroupAuthorizer could
> > > then
> > > > > leverage this relation. As you suggest, a true implementation
> > would
> > > > > allow
> > > > > multiple groups.
> > > > >
> > > > > I will add a note to the KIP to emphasize that this is just an
> > > example.
> > > > >
> > > > > Thanks,
> > > > > Jason
> > > > >
> > > > > On Fri, Aug 25, 2017 at 4:37 PM, Don Bosco Durai <
> > bo...@apache.org
> > > >
> > > > > wrote:
> > > > >
> > > > > > Jason, thanks for confirming that. Since there are existing
> > > custom
> > > > > > plugins, we might have to give enough time for them to start
> > > using
> > > > > the
> > > > > > newer interface.
> > > > > >
> > > > > > I quickly glanced over the KIP, it looks good. Here is one
> > > comment:
> > > > > >
> > > > > > -------
> > > > > > In the future, we may add support for groups to Kafka. This was
> > > > > brought up
> > > > > > in the KIP-111 discussion. To support this, we can provide a
> > > > > groupId()
> > > > > > method in KafkaPrincipal which defaults to a null value or an
> > > empty
> > > > > string.
> > > > > > Extensions can override this just as before. Also note that it
> > is
> > > > > still
> > > > > > possible for the Authorizer implementation to derive its own
> > > group
> > > > > > information for enforcement.
> > > > > > class UserPrincipalAndGroup extends KafkaPrincipal {
> > > > > > private final String userId;
> > > > > > private final String groupId;
> > > > > > -------
> > > > > >
> > > > > > We should assume that users might belong to multiple groups.
> > > > > >
> > > > > > Also, not sure what the below method is really doing?
> > > > > > ---
> > > > > > class UserPrincipalAndGroup extends KafkaPrincipal {
> > > > > >
> > > > > > public KafkaPrincipal group() {
> > > > > > return new KafkaPrincipal(KafkaPrincipal.GROUP_TYPE,
> > > groupId);
> > > > > > }
> > > > > > ---
> > > > > > Thanks
> > > > > >
> > > > > > Bosco
> > > > > >
> > > > > >
> > > > > >
> > > > > > On 8/25/17, 4:11 PM, "Jason Gustafson" <ja...@confluent.io>
> > > wrote:
> > > > > >
> > > > > > Hi Don,
> > > > > >
> > > > > > I don't think so. We are not making any changes to the
> > > Authorizer
> > > > > > interface
> > > > > > itself. The KafkaPrincipal object does not change either,
> > > though
> > > > > we now
> > > > > > explicitly allow it to be extended. That means you have to
> > > > > exercise a
> > > > > > little caution when combining a custom PrincipalBuilder
> > with
> > > a
> > > > > custom
> > > > > > Authorizer. For the default principal builder shipped with
> > > > > Kafka, it
> > > > > > will
> > > > > > work the same as it currently does. Old implementations of
> > > > > > PrincipalBuilder
> > > > > > will also continue to work exactly as they do now, but
> > please
> > > > > note
> > > > > > that I
> > > > > > am proposing to deprecate this interface. It will still be
> > > > > supported in
> > > > > > 1.0.0, but we may remove it in a future major release.
> > > > > >
> > > > > > -Jason
> > > > > >
> > > > > > On Fri, Aug 25, 2017 at 3:51 PM, Don Bosco Durai <
> > > > > bo...@apache.org>
> > > > > > wrote:
> > > > > >
> > > > > > > Jason
> > > > > > >
> > > > > > > Do you anticipate any backward compatibility issues with
> > > > > existing
> > > > > > custom
> > > > > > > implementation of the authorization interface/plugins?
> > > > > > >
> > > > > > > Thanks
> > > > > > >
> > > > > > > Bosco
> > > > > > >
> > > > > > >
> > > > > > > On 8/25/17, 3:22 PM, "Jason Gustafson" <
> > ja...@confluent.io
> > > >
> > > > > wrote:
> > > > > > >
> > > > > > > No problem. I'll add a note to the KIP to emphasize
> > > that
> > > > > we will
> > > > > > use
> > > > > > > the
> > > > > > > same object built by the KafkaPrincipalBuilder in the
> > > > > Session
> > > > > > object
> > > > > > > passed
> > > > > > > to the authorizer.
> > > > > > >
> > > > > > > -Jason
> > > > > > >
> > > > > > > On Fri, Aug 25, 2017 at 3:17 PM, Mayuresh Gharat <
> > > > > > > gharatmayures...@gmail.com
> > > > > > > > wrote:
> > > > > > >
> > > > > > > > Perfect.
> > > > > > > > As long as there is a way we can access the
> > > originally
> > > > > created
> > > > > > > Principal in
> > > > > > > > the Authorizer, it would solve the KIP-111 issue.
> > > > > > > >
> > > > > > > > This is really helpful, thanks again.
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > > >
> > > > > > > > Mayuresh
> > > > > > > >
> > > > > > > > On Fri, Aug 25, 2017 at 3:13 PM, Jason Gustafson <
> > > > > > ja...@confluent.io
> > > > > > > >
> > > > > > > > wrote:
> > > > > > > >
> > > > > > > > > Hi Mayuresh,
> > > > > > > > >
> > > > > > > > > To clarify, the intention is to use the
> > > KafkaPrincipal
> > > > > object
> > > > > > > built by
> > > > > > > > the
> > > > > > > > > KafkaPrincipalBuilder inside the Session. So we
> > > would
> > > > > remove
> > > > > > the
> > > > > > > logic to
> > > > > > > > > construct a new KafkaPrincipal using only the
> > name
> > > > > from the
> > > > > > > Principal.
> > > > > > > > Then
> > > > > > > > > it should be possible to pass the
> > `AuthzPrincipal`
> > > to
> > > > > the
> > > > > > > underlying
> > > > > > > > > library through the `Extended_Plugged_In_Class`
> > as
> > > > > you've
> > > > > > suggested
> > > > > > > > above.
> > > > > > > > > Is that reasonable for this use case?
> > > > > > > > >
> > > > > > > > > Thanks,
> > > > > > > > > Jason
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > On Fri, Aug 25, 2017 at 2:44 PM, Mayuresh Gharat
> > <
> > > > > > > > > gharatmayures...@gmail.com
> > > > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > 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
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > --
> > > > > > > > -Regards,
> > > > > > > > Mayuresh R. Gharat
> > > > > > > > (862) 250-7125
> > > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > >
> >
