HI Colin,
Overlooked the IDEMPOTENT_WRITE ACL. This along with
client.min.version should solve the cases proposed in the KIP.
Can we turn this KIP into adding min.client.version config to broker and it
could be part of the dynamic config .
Thanks,
Harsha
On Wed, Feb 27, 2019, at 12:17 PM, Colin McCabe wrote:
> 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:
> > > > > >> >
> > > > > >> > >
> > > > > >> > >
> > > > > >> >
> > > > > >>
> > > > > >
> > > > >
> > > >
> > >
> >
>
