On Tue, Feb 26, 2019, at 16:33, Harsha wrote: > Hi Colin, > > "> I think Ismael and Gwen here bring up a good point. The version of the > > request is a technical detail that isn't really related to > > authorization. There are a lot of other technical details like this > > like the size of the request, the protocol it came in on, etc. None of > > them are passed to the authorizer-- they all have configuration knobs > > to control how we handle them. If we add this technical detail, > > logically we'll have to start adding all the others, and the authorizer > > API will get really bloated. It's better to keep it focused on > > authorization, I think." > > probably my previous email is not clear but I am agreeing with Gwen's point. > I am not in favor of extending authorizer to support this. > > > "> Another thing to consider is that if we add a new broker configuration > > that lets us set a minimum client version which is allowed, that could > > be useful to other users as well. On the other hand, most users are > > not likely to write a custom authorizer to try to take advantage of > > version information being passed to the authorizer. So, I think using> a > > configuration is clearly the better way to go here. Perhaps it can > > be a KIP-226 dynamic configuration to make this easier to deploy?" > > Although minimum client version might help to a certain extent there > are other cases where we want users to not start using transactions for > example. My proposal in the previous thread was to introduce another > module/interface, let's say > "SupportedAPIs" which will take in dynamic configuration to check which > APIs are allowed. > It can throw UnsupportedException just like we are throwing > Authorization Exception.
Hi Harsha, We can already prevent people from using transactions using ACLs, right? That's what the IDEMPOTENT_WRITE ACL was added for. In general, I think users should be able to think of ACLs in terms of "what can I do" rather than "how is it implemented." For example, maybe some day we will replace FetchRequest with GetStuffRequest. But users who have READ permission on a topic shouldn't have to change anything. So I think the Authorizer interface should not be aware of individual RPC types or message versions. best, Colin > > > Thanks, > Harsha > > > n Tue, Feb 26, 2019, at 10:04 AM, Colin McCabe wrote: > > Hi Harsha, > > > > I think Ismael and Gwen here bring up a good point. The version of the > > request is a technical detail that isn't really related to > > authorization. There are a lot of other technical details like this > > like the size of the request, the protocol it came in on, etc. None of > > them are passed to the authorizer-- they all have configuration knobs > > to control how we handle them. If we add this technical detail, > > logically we'll have to start adding all the others, and the authorizer > > API will get really bloated. It's better to keep it focused on > > authorization, I think. > > > > Another thing to consider is that if we add a new broker configuration > > that lets us set a minimum client version which is allowed, that could > > be useful to other users as well. On the other hand, most users are > > not likely to write a custom authorizer to try to take advantage of > > version information being passed to the authorizer. So, I think using > > a configuration is clearly the better way to go here. Perhaps it can > > be a KIP-226 dynamic configuration to make this easier to deploy? > > > > cheers, > > Colin > > > > > > On Mon, Feb 25, 2019, at 15:43, Harsha wrote: > > > Hi Ying, > > > I think the question is can we add a module in the core which > > > can take up the dynamic config and does a block certain APIs. This > > > module will be called in each of the APIs like the authorizer does > > > today to check if the API is supported for the client. > > > Instead of throwing AuthorizationException like the authorizer does > > > today it can throw UnsupportedException. > > > Benefits are, we are keeping the authorizer interface as is and adding > > > the flexibility based on dynamic configs without the need for > > > categorizing broker APIs and it will be easy to extend to do additional > > > options, like turning off certain features which might be in interest > > > to the service providers. > > > One drawback, It will introduce another call to check instead of > > > centralizing everything around Authorizer. > > > > > > Thanks, > > > Harsha > > > > > > On Mon, Feb 25, 2019, at 2:43 PM, Ying Zheng wrote: > > > > If you guys don't like the extension of authorizer interface, I will > > > > just > > > > propose a single broker dynamic configuration: client.min.api.version, > > > > to > > > > keep things simple. > > > > > > > > What do you think? > > > > > > > > On Mon, Feb 25, 2019 at 2:23 PM Ying Zheng <yi...@uber.com> wrote: > > > > > > > > > @Viktor Somogyi-Vass, @Harsha, It seems the biggest concern is the > > > > > backward-compatibility to the existing authorizers. We can put the new > > > > > method into a new trait / interface: > > > > > trait AuthorizerEx extends Authorizer { > > > > > def authorize(session: Session, operation: Operation, resource: > > > > > Resource, > > > > > apiVersion: Short): Boolean > > > > > } > > > > > > > > > > When loading an authorizer class, broker will check if the class > > > > > implemented AuthorizerEx interface. If not, broker will wrapper the > > > > > Authorizer object with an Adapter class, in which authorizer(... > > > > > apiVersion) call is translated to the old authorizer() call. So that, > > > > > both > > > > > old and new Authorizer is supported and can be treated as > > > > > AuthorizerEx in > > > > > the new broker code. > > > > > > > > > > As for the broker dynamic configuration approach, I'm not sure how to > > > > > correctly categorize the 40+ broker APIs into a few categories. > > > > > For example, describe is used by producer, consumer, and admin. > > > > > Should it > > > > > be controlled by producer.min.api.version or consumer.min.api.version? > > > > > Should producer.min.api.version apply to transaction operations? > > > > > > > > > > > > > > > On Mon, Feb 25, 2019 at 10:33 AM Harsha <ka...@harsha.io> wrote: > > > > > > > > > >> I think the motivation of the KIP is to configure which API we want > > > > >> to > > > > >> allow for a broker. > > > > >> This is challenging for a hosted service where you have customers > > > > >> with > > > > >> different versions of clients. > > > > >> It's not just about down conversion but for example transactions, > > > > >> there > > > > >> is a case where we do not want to allow users to start using > > > > >> transactions > > > > >> and there is no way to disable to this right now and as specified in > > > > >> the > > > > >> KIP, having a lock on which client versions we support. > > > > >> Authorizer's original purpose is to allow policies to be enforced for > > > > >> each of the Kafka APIs, specifically in the context of security. > > > > >> Extending this to a general purpose gatekeeper might not be suitable > > > > >> and > > > > >> as mentioned in the thread every implementation of authorizer needs > > > > >> to > > > > >> re-implement to provide the same set of functionality. > > > > >> I think it's better to add an implementation which will use a > > > > >> broker's > > > > >> dynamic config as mentioned in approach 1. > > > > >> > > > > >> Thanks, > > > > >> Harsha > > > > >> > > > > >> On Sat, Feb 23, 2019, at 6:21 AM, Ismael Juma wrote: > > > > >> > Thanks for the KIP. Have we considered the existing topic config > > > > >> > that > > > > >> makes > > > > >> > it possible to disallow down conversions? That's the biggest > > > > >> > downside in > > > > >> > allowing older clients. > > > > >> > > > > > >> > Ismael > > > > >> > > > > > >> > On Fri, Feb 22, 2019, 2:11 PM Ying Zheng <yi...@uber.com.invalid> > > > > >> wrote: > > > > >> > > > > > >> > > > > > > >> > > > > > > >> > > > > > >> > > > > > > > > > > > > > > >