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
> > > > >
> > >
> >
> >
> >
> >
>
>
>
>
