Hi Rajini,

Thanks for the KIP!
I really like that authorize() will be able to take a batch of
requests, this will speed up many implementations!

On Tue, Aug 13, 2019 at 5:57 PM Rajini Sivaram <rajinisiva...@gmail.com> wrote:
>
> Thanks David! I have fixed the typo.
>
> Also made a couple of changes to make the context interfaces more generic.
> KafkaRequestContext now returns the 16-bit API key as Colin suggested as
> well as the friendly name used in metrics which are useful in audit logs.
> `Authorizer#start` is now provided a generic `BrokerInfo` interface that
> gives cluster id, broker id and endpoint information. The generic interface
> can potentially be used in other broker plugins in future and provides
> dynamically generated configs like broker id and ports which are currently
> not available to plugins unless these configs are statically configured.
> Please let me know if there are any concerns.
>
> Regards,
>
> Rajini
>
> On Tue, Aug 13, 2019 at 4:30 PM David Jacot <dja...@confluent.io> wrote:
>
> > Hi Rajini,
> >
> > Thank you for the update! It looks good to me. There is a typo in the
> > `AuditFlag` enum: `MANDATORY_AUTHOEIZE` -> `MANDATORY_AUTHORIZE`.
> >
> > Regards,
> > David
> >
> > On Mon, Aug 12, 2019 at 2:54 PM Rajini Sivaram <rajinisiva...@gmail.com>
> > wrote:
> >
> > > Hi David,
> > >
> > > Thanks for reviewing the KIP! Since questions about `authorization mode`
> > > and `count` have come up multiple times, I have renamed both.
> > >
> > > 1) Renamed `count` to `resourceReferenceCount`. It is the number of times
> > > the resource being authorized is referenced within the request.
> > >
> > > 2) Renamed `AuthorizationMode` to `AuditFlag`. It is provided to improve
> > > audit logging in the authorizer. The enum values have javadoc which
> > > indicate how the authorization result is used in each of the modes to
> > > enable authorizers to log audit messages at the right severity level.
> > >
> > > Regards,
> > >
> > > Rajini
> > >
> > > On Mon, Aug 12, 2019 at 5:54 PM David Jacot <dja...@confluent.io> wrote:
> > >
> > > > Hi Rajini,
> > > >
> > > > Thank you for the KIP. Overall, it looks good to me. I have few
> > > > questions/suggestions:
> > > >
> > > > 1. It is hard to grasp what `Action#count` is for. I guess I understand
> > > > where you want to go with it but it took me a while to figure it out.
> > > > Perhaps, we could come up with a better name than `count`?
> > > >
> > > > 2. I had a hard time trying to understand the `AuthorizationMode`
> > > concept,
> > > > especially wrt. the OPTIONAL one. My understanding is that an ACL is
> > > either
> > > > defined or not. Could you elaborate a bit more on that?
> > > >
> > > > Thanks,
> > > > David
> > > >
> > > > On Fri, Aug 9, 2019 at 10:26 PM Don Bosco Durai <bo...@apache.org>
> > > wrote:
> > > >
> > > > > Hi Rajini
> > > > >
> > > > > 3.2 - This makes sense. Thanks for clarifying.
> > > > >
> > > > > Rest looks fine. Once the implementations are done, it will be more
> > > clear
> > > > > on the different values RequestType and Mode.
> > > > >
> > > > > Thanks
> > > > >
> > > > > Bosco
> > > > >
> > > > >
> > > > > On 8/9/19, 5:08 AM, "Rajini Sivaram" <rajinisiva...@gmail.com>
> > wrote:
> > > > >
> > > > >     Hi Don,
> > > > >
> > > > >     Thanks for the suggestions. A few responses below:
> > > > >
> > > > >     3.1 Can rename and improve docs if we keep this. Let's finish the
> > > > >     discussion on Colin's suggestions regarding this first.
> > > > >     3.2 No, I was thinking of some requests that have an old way of
> > > > > authorizing
> > > > >     and a new way where we have retained the old way for backward
> > > > >     compatibility. One example is Cluster:Create permission to create
> > > > > topics.
> > > > >     We have replaced this with fine-grained topic create access using
> > > > > Topic:Create
> > > > >     for topic patterns. But we still check if user has Cluster:Create
> > > > > first. If
> > > > >     Denied, the deny is ignored and we check Topic:Create. We dont
> > want
> > > > to
> > > > > log
> > > > >     DENY for Cluster:Create at INFO level for this, since this is
> > not a
> > > > >     mandatory ACL for creating topics. We will get appropriate logs
> > > from
> > > > > the
> > > > >     subsequent Topic:Create in this case.
> > > > >     3.3 They are not quite the same. FILTER implies that user
> > actually
> > > > >     requested access to and performed some operation on the filtered
> > > > > resources.
> > > > >     LIST_AUTHORZED did not result in any actual access. User simply
> > > > wanted
> > > > > to
> > > > >     know what they are allowed to access.
> > > > >     3.4 Request types are Produce, JoinGroup, OffsetCommit etc. So
> > that
> > > > is
> > > > >     different from authorization mode, operation etc.
> > > > >
> > > > >
> > > > >     On Thu, Aug 8, 2019 at 11:36 PM Don Bosco Durai <
> > bo...@apache.org>
> > > > > wrote:
> > > > >
> > > > >     > Hi Rajini
> > > > >     >
> > > > >     > Thanks for clarifying. This is very helpful...
> > > > >     >
> > > > >     > #1 - On the Ranger side, we should be able to handle multiple
> > > > > requests at
> > > > >     > the same time. I was just not sure how much processing overhead
> > > > will
> > > > > be
> > > > >     > there on the Broker side to split and then consolidate the
> > > results.
> > > > > If it
> > > > >     > is negligible,  then this is the better way. It will make it
> > > future
> > > > > proof.
> > > > >     > #2 -  I agree, having a single "start" call makes it cleaner.
> > The
> > > > >     > Authorization implementation will only have to do the
> > > > initialization
> > > > > only
> > > > >     > once.
> > > > >     > #3.1 - Thanks for the clarification. I think it makes sense to
> > > have
> > > > > this.
> > > > >     > The term "Mode" might not be explicit enough. Essentially it
> > > seems
> > > > > you want
> > > > >     > the Authorizer to know the Intent/Purpose of the authorize call
> > > and
> > > > > let the
> > > > >     > Authorizer decide what to log as Audit event. Changing the
> > > > > class/field name
> > > > >     > or giving more documentation will do.
> > > > >     > #3.2 - Regarding the option "OPTIONAL". Are you thinking from
> > > > > chaining
> > > > >     > multiple Authorizer? If so,  I am not sure whether the Broker
> > > would
> > > > > have
> > > > >     > enough information to make that decision. I feel the Authorizer
> > > > will
> > > > > be the
> > > > >     > one who would have that knowledge. E.g. in Ranger we have
> > > explicit
> > > > > deny,
> > > > >     > which means no matter what, the request should be denied for
> > the
> > > > > user/group
> > > > >     > or condition. So if you are thinking of chaining Authorizers,
> > > then
> > > > >     > responses should have the third state, e.g. "DENIED_FINAL", in
> > > > which
> > > > > case
> > > > >     > if there is an Authorization chain, it will be stop and the
> > > request
> > > > > will be
> > > > >     > denied and if it is just denied, then you might fall back to
> > next
> > > > >     > authorizer. If we don't have chaining of Authorizing in mind,
> > > then
> > > > we
> > > > >     > should reconsider OPTIONAL for now. Or clarify under which
> > > scenario
> > > > >     > OPTIONAL will be called.
> > > > >     > #3.3 Regarding, FILTER v/s LIST_AUTHORIZED, isn't
> > > LIST_AUTHORIZED a
> > > > >     > special case for "FILTER"?
> > > > >     > #3.4 KafkaRequestContext. requestType() v/s Action.
> > > > > authorizationMode. I
> > > > >     > am not sure about the overlap or ambiguity.
> > > > >     > #4 - Cool. This is good, it will be less stress on the
> > > Authorizer.
> > > > > Ranger
> > > > >     > already supports the "count" concept and also has batching
> > > > > capability to
> > > > >     > aggregate similar requests to reduce the number of audit logs
> > to
> > > > > write. We
> > > > >     > should be able to pass this through.
> > > > >     > #5 - Assuming if the object instance is going out of scope, we
> > > > > should be
> > > > >     > fine. Not a super important ask. Ranger is already catching
> > KILL
> > > > > signal for
> > > > >     > clean up.
> > > > >     >
> > > > >     > Thanks again. These are good enhancements. Appreciate your
> > > efforts
> > > > > here.
> > > > >     >
> > > > >     > Bosco
> > > > >     >
> > > > >     >
> > > > >     >
> > > > >     > On 8/8/19, 2:03 AM, "Rajini Sivaram" <rajinisiva...@gmail.com
> > >
> > > > > wrote:
> > > > >     >
> > > > >     >     Hi Don,
> > > > >     >
> > > > >     >     Thanks for reviewing the KIP.
> > > > >     >
> > > > >     >     1. I had this originally as a single Action, but thought it
> > > may
> > > > > be
> > > > >     > useful
> > > > >     >     to support batched authorize calls as well and keep it
> > > > > consistent with
> > > > >     >     other methods. Single requests can contain multiple topics.
> > > For
> > > > >     > example a
> > > > >     >     produce request can contain records for several partitions
> > of
> > > > > different
> > > > >     >     topics. Broker could potentially authorize these together.
> > > For
> > > > >     >     SimpleAclAuthorizer, batched authorize methods don't
> > provide
> > > > any
> > > > >     >     optimisation since lookup is based on resources followed by
> > > the
> > > > >     > matching
> > > > >     >     logic. But some authorizers may manage ACLs by user
> > principal
> > > > > rather
> > > > >     > than
> > > > >     >     resource and may be able to optimize batched requests. I am
> > > ok
> > > > > with
> > > > >     > using
> > > > >     >     single Action if this is likely to cause issues.
> > > > >     >     2. If you have two listeners, one for inter-broker traffic
> > > and
> > > > > another
> > > > >     > for
> > > > >     >     external clients, start method is invoked twice, once for
> > > each
> > > > >     > listener. On
> > > > >     >     second thought, that may be confusing and a single start()
> > > > > invocation
> > > > >     > that
> > > > >     >     provides all listener information and returns multiple
> > > futures
> > > > > would be
> > > > >     >     better. Will update the KIP.
> > > > >     >     3. A typical example is a consumer subscribing to a regex
> > > > > pattern. We
> > > > >     >     request all topic metadata from the broker in order to
> > decide
> > > > > whether
> > > > >     > the
> > > > >     >     pattern matches, expecting to receive a list of authorised
> > > > > topics. The
> > > > >     > user
> > > > >     >     is not asking to subscribe to an unauthorized topic. If
> > there
> > > > > are 10000
> > > > >     >     topics in the cluster and the user has access to 100 of
> > them,
> > > > at
> > > > > the
> > > > >     > moment
> > > > >     >     we log 9900 DENIED log entries at INFO level in
> > > > > SimpleAclAuthorizer.
> > > > >     > The
> > > > >     >     proposal is to authorize this request with
> > > > > AuthorizationMode.FILTER, so
> > > > >     >     that authorizers can log resources that are filtered out at
> > > > > lower level
> > > > >     >     like DEBUG since this is not an attempt to access
> > > unauthorized
> > > > >     > resources.
> > > > >     >     Brokers already handle these differently since no
> > > authorization
> > > > > error
> > > > >     > is
> > > > >     >     returned to the client in these cases. Providing
> > > authorization
> > > > > mode to
> > > > >     >     authorizers enables authorizer implementations to generate
> > > > > better audit
> > > > >     >     logs.
> > > > >     >     4. Each request may contain multiple instances of the same
> > > > > authorizable
> > > > >     >     resource. For example a produce request may contain records
> > > for
> > > > > 10
> > > > >     >     partitions of the same topic. At the moment, we invoke
> > > > authorize
> > > > >     > method 10
> > > > >     >     times. The proposal is to invoke it once with count=10. The
> > > > > count is
> > > > >     >     provided to authorizer just for audit logging purposes.
> > > > >     >     5. Authorizer implements Closeable, so you could use
> > close()
> > > to
> > > > > flush
> > > > >     >     audits?
> > > > >     >
> > > > >     >     On Thu, Aug 8, 2019 at 7:01 AM Don Bosco Durai <
> > > > bo...@apache.org
> > > > > >
> > > > >     > wrote:
> > > > >     >
> > > > >     >     > Rajini
> > > > >     >     >
> > > > >     >     > Thanks for putting this together. It is looking good. I
> > > have
> > > > > few
> > > > >     >     > questions...
> > > > >     >     >
> > > > >     >     > 1. List<AuthorizationResult> authorize(..., List<Action>
> > > > > actions).
> > > > >     > Do you
> > > > >     >     > see a scenario where the broker will call authorize for
> > > > > multiple
> > > > >     > topics at
> > > > >     >     > the same time? I can understand that during
> > > creating/deleting
> > > > > ACLS,
> > > > >     >     > multiple permissions for multiple resources might be
> > done.
> > > > For
> > > > >     > authorize
> > > > >     >     > call, would this be a case? And does the Authorize
> > > > > implementation
> > > > >     > will be
> > > > >     >     > able to do performance optimization because of this? Or
> > > > should
> > > > > we
> > > > >     > just keep
> > > > >     >     > it simple? I don't see it as an issue from Apache Ranger
> > > > side,
> > > > > but
> > > > >     > just
> > > > >     >     > checking to see whether we need to be aware of something.
> > > > >     >     > 2. Should I assume that the SecurityProtocol passed
> > during
> > > > > start and
> > > > >     > the
> > > > >     >     > one return by KafkaRequestContext.securityProtocol() will
> > > be
> > > > > the
> > > > >     > same?
> > > > >     >     > CompletableFuture<Void> start(String listenerName,
> > > > > SecurityProtocol
> > > > >     >     > securityProtocol);
> > > > >     >     > KafkaRequestContext.securityProtocol()
> > > > >     >     > 3. What is the purpose of AuthorizationMode? How does the
> > > > > broker
> > > > >     > decide
> > > > >     >     > what mode to use when the authorize() method is called?
> > > > >     >     > 4. Can we clarify "count" in Action a bit more? How is it
> > > > used?
> > > > >     >     > 5. Do you feel having "stop" along with "start" be
> > helpful?
> > > > > E.g. In
> > > > >     > Ranger
> > > > >     >     > we try to optimize the Audit writing by caching the logs
> > > for
> > > > a
> > > > > fixed
> > > > >     >     > interval. But when the Broker terminates, we do a forced
> > > > flush.
> > > > >     > Having an
> > > > >     >     > explicit "stop" might give us a formal way to flush our
> > > > audits.
> > > > >     >     >
> > > > >     >     > Thanks
> > > > >     >     >
> > > > >     >     > Bosco
> > > > >     >     >
> > > > >     >     > On 8/7/19, 3:59 PM, "Rajini Sivaram" <
> > > > rajinisiva...@gmail.com
> > > > > >
> > > > >     > wrote:
> > > > >     >     >
> > > > >     >     >     Hi Ron/Harsha/Satish,
> > > > >     >     >
> > > > >     >     >     Thanks for reviewing the KIP!
> > > > >     >     >
> > > > >     >     >     We should perhaps have a wider discussion outside
> > this
> > > > KIP
> > > > > for
> > > > >     >     > refactoring
> > > > >     >     >     clients so that others who are not following this KIP
> > > > also
> > > > >     > notice the
> > > > >     >     >     discussion. Satish, would you like to start a
> > > discussion
> > > > > thread
> > > > >     > on dev?
> > > > >     >     >
> > > > >     >     >     Regards,
> > > > >     >     >
> > > > >     >     >     Rajini
> > > > >     >     >
> > > > >     >     >
> > > > >     >     >     On Wed, Aug 7, 2019 at 6:21 PM Satish Duggana <
> > > > >     >     > satish.dugg...@gmail.com>
> > > > >     >     >     wrote:
> > > > >     >     >
> > > > >     >     >     > I felt the same need when we want to add a
> > pluggable
> > > > API
> > > > > for
> > > > >     > core
> > > > >     >     >     > server functionality. This does not need to be part
> > > of
> > > > > this
> > > > >     > KIP, it
> > > > >     >     >     > can be a separate KIP. I can contribute those
> > > > refactoring
> > > > >     > changes if
> > > > >     >     >     > others are OK with that.
> > > > >     >     >     >
> > > > >     >     >     > It is better to have a structure like below.
> > > > >     >     >     >
> > > > >     >     >     > kafka-common:
> > > > >     >     >     > common classes which can be used in any of the
> > other
> > > > > modules
> > > > >     > in Kafka
> > > > >     >     >     > like client, Kafka-server-common and server etc.
> > > > >     >     >     >
> > > > >     >     >     > kafka-client-common:
> > > > >     >     >     > common classes which can be used in the client
> > > module.
> > > > > This
> > > > >     > can be
> > > > >     >     >     > part of client module itself.
> > > > >     >     >     >
> > > > >     >     >     > kafka-server-common:
> > > > >     >     >     > classes required only for kafka-server.
> > > > >     >     >     >
> > > > >     >     >     > Thanks.
> > > > >     >     >     > Satish.
> > > > >     >     >     >
> > > > >     >     >     > On Wed, Aug 7, 2019 at 9:28 PM Harsha Chintalapani
> > <
> > > > >     > ka...@harsha.io>
> > > > >     >     >     > wrote:
> > > > >     >     >     > >
> > > > >     >     >     > > Thanks for the KIP Rajini.
> > > > >     >     >     > > Quick thought, it would be good to have a common
> > > > module
> > > > >     > outside of
> > > > >     >     >     > clients
> > > > >     >     >     > > that only applies to server side interfaces &
> > > > changes.
> > > > > It
> > > > >     > looks
> > > > >     >     > like we
> > > > >     >     >     > are
> > > > >     >     >     > > increasingly in favor of using Java interface for
> > > > > pluggable
> > > > >     >     > modules  on
> > > > >     >     >     > the
> > > > >     >     >     > > broker side.
> > > > >     >     >     > >
> > > > >     >     >     > > Thanks,
> > > > >     >     >     > > Harsha
> > > > >     >     >     > >
> > > > >     >     >     > >
> > > > >     >     >     > > On Tue, Aug 06, 2019 at 2:31 PM, Rajini Sivaram <
> > > > >     >     > rajinisiva...@gmail.com
> > > > >     >     >     > >
> > > > >     >     >     > > wrote:
> > > > >     >     >     > >
> > > > >     >     >     > > > Hi all,
> > > > >     >     >     > > >
> > > > >     >     >     > > > I have created a KIP to replace the Scala
> > > > Authorizer
> > > > > API
> > > > >     > with a
> > > > >     >     > new
> > > > >     >     >     > Java
> > > > >     >     >     > > > API:
> > > > >     >     >     > > >
> > > > >     >     >     > > > -
> > > > >     >     >     > > >
> > > https://cwiki.apache.org/confluence/display/KAFKA/
> > > > >     >     >     > > > KIP-504+-+Add+new+Java+Authorizer+Interface
> > > > >     >     >     > > >
> > > > >     >     >     > > > This is replacement for KIP-50 which was
> > accepted
> > > > > but never
> > > > >     >     > merged.
> > > > >     >     >     > Apart
> > > > >     >     >     > > > from moving to a Java API consistent with other
> > > > > pluggable
> > > > >     >     > interfaces
> > > > >     >     >     > in the
> > > > >     >     >     > > > broker, KIP-504 also attempts to address known
> > > > > limitations
> > > > >     > in the
> > > > >     >     >     > > > authorizer. If you have come across other
> > > > > limitations that
> > > > >     > you
> > > > >     >     > would
> > > > >     >     >     > like
> > > > >     >     >     > > > to see addressed in the new API, please raise
> > > these
> > > > > on the
> > > > >     >     > discussion
> > > > >     >     >     > > > thread so that we can consider those too. All
> > > > > suggestions
> > > > >     > and
> > > > >     >     > feedback
> > > > >     >     >     > are
> > > > >     >     >     > > > welcome.
> > > > >     >     >     > > >
> > > > >     >     >     > > > Thank you,
> > > > >     >     >     > > >
> > > > >     >     >     > > > Rajini
> > > > >     >     >     > > >
> > > > >     >     >     >
> > > > >     >     >
> > > > >     >     >
> > > > >     >     >
> > > > >     >     >
> > > > >     >
> > > > >     >
> > > > >     >
> > > > >     >
> > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > >
> >

Reply via email to