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