Hi Rajini, I think it would be good to rename KafkaRequestContext to something like AuthorizableRequestContext, and put it in the org.apache.kafka.server.authorizer namespace. If we put it in the org.apache.kafka.common namespace, then it's not really clear that it's part of the Authorizer API. Since this class is purely an interface, with no concrete implementation of anything, there's nothing common to really reuse in any case. We definitely don't want someone to accidentally add or remove methods because they think this is just another internal class used for requests.
The BrokerInfo class is a nice improvement. It looks like it will be useful for passing in information about the context we're running in. It would be nice to call this class ServerInfo rather than BrokerInfo, since we will want to run the authorizer on controllers as well as on brokers, and the controller may run as a separate process post KIP-500. I also think that this class should be in the org.apache.kafka.server.authorizer namespace. Again, it is an interface, not a concrete implementation, and it's an interface that is very specifically for the authorizer. I agree that we probably don't have enough information preserved for requests currently to always know what entity made them. So we can leave that out for now (except in the special case of Fetch). Perhaps we can add this later if it's needed. I understand the intention behind AuthorizationMode (which is now called AuditFlag in the latest revision). But it still feels complex. What if we just had two booleans in Action: logSuccesses and logFailures? That seems to cover all the cases here. MANDATORY_AUTHORIZE = true, true. OPTIONAL_AUTHORIZE = true, false. FILTER = true, false. LIST_AUTHORIZED = false, false. Would there be anything lost versus having the enum? best, Colin On Wed, Aug 14, 2019, at 06:29, Mickael Maison wrote: > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
