You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Colin McCabe <cm...@apache.org> on 2019/03/01 19:06:41 UTC
Re: [DISCUSS] KIP-433: Provide client API version to authorizer
On Wed, Feb 27, 2019, at 15:53, Harsha wrote:
> 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 .
+1, sounds like a good idea.
Colin
>
> 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:
> > > > > > >> >
> > > > > > >> > >
> > > > > > >> > >
> > > > > > >> >
> > > > > > >>
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: [DISCUSS] KIP-433: Provide client API version to authorizer
Posted by Ying Zheng <yi...@uber.com.INVALID>.
@Colin, Thank you for the feedback!
I have updated the KIP, added the explanation of why we use API version
rather than Kafka version.
I will start a vote for this KIP
On Fri, Mar 29, 2019 at 9:47 AM Colin McCabe <cm...@apache.org> wrote:
> Hi Ying,
>
> That's a fair point. Maybe using API keys directly is reasonable here.
>
> One thing that's probably worth calling out is that if we make the name
> part of the configuration, we can't rename APIs in the future. That's
> probably OK as long as it's documented.
>
> best,
> Colin
>
> On Thu, Mar 28, 2019, at 17:36, Ying Zheng wrote:
> > @Colin McCabe <cm...@confluent.io>
> >
> > I did think about that option. Yes, for most users, it's much easier to
> > understand Kafka version, rather than API version. However, the existing
> > Kafka clients only report API versions in the Kafka requests. So, the
> > brokers can only block clients by API versions rather than the real Kafka
> > versions. This can be very confusing for the users, if an API did not
> > change for a specified Kafka version.
> >
> > For example, a user sets the min Kafka version of produce request to
> Kafka
> > 1.1. She would expect the broker will reject Kafka 1.0 producers.
> However,
> > both Kafka 1.1 and Kafka 1.0 are using api version 5. The broker can't
> > distinguish the 2 version. So, Kafka 1.0 producers are still allowed.
> >
> > I think we can say this configuration is only for "advanced users". The
> > user has to know the concept of "api version", and know the function of
> > each API, to be able to use this feature. (Similarly, it's easier for the
> > users to say: "I want to block old consumers, or old admin clients.". But
> > there is no clear definition of which set of APIs are "consumer APIs".
> So,
> > we still have to use API names, rather than "client roles")
> >
> >
> >
> > On Wed, Mar 27, 2019 at 5:32 PM Colin McCabe <cm...@apache.org> wrote:
> >
> > > Thanks, Ying Zheng. Looks good overall.
> > >
> > > One question is, should the version be specified as a Kafka version
> rather
> > > than as a RPC API version? I don't think most users are aware of RPC
> > > versions, but something like "min kafka version" would be easier to
> > > understand. That is how we handle the minimum inter-broker protocol
> > > version and the minimum on-disk format version, after all.
> > >
> > > best,
> > > Colin
> > >
> > > On Tue, Mar 26, 2019, at 17:52, Ying Zheng wrote:
> > > > I have rewritten the KIP. The new proposal is adding a new
> configuration
> > > > min.api.version in Kafka broker.
> > > >
> > > > Please review the new KIP. Thank you!
> > > >
> > > > On Fri, Mar 1, 2019 at 11:06 AM Colin McCabe <cm...@apache.org>
> wrote:
> > > >
> > > > > On Wed, Feb 27, 2019, at 15:53, Harsha wrote:
> > > > > > 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 .
> > > > >
> > > > > +1, sounds like a good idea.
> > > > >
> > > > > Colin
> > > > >
> > > > >
> > > > > >
> > > > > > 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 <
> yingz@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 <
> kafka@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:
> > > > > > > > > > > >> >
> > > > > > > > > > > >> > >
> > > > > > > > > > > >> > >
> > > > > > > > > > > >> >
> > > > > > > > > > > >>
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: [DISCUSS] KIP-433: Provide client API version to authorizer
Posted by Colin McCabe <cm...@apache.org>.
Hi Ying,
That's a fair point. Maybe using API keys directly is reasonable here.
One thing that's probably worth calling out is that if we make the name part of the configuration, we can't rename APIs in the future. That's probably OK as long as it's documented.
best,
Colin
On Thu, Mar 28, 2019, at 17:36, Ying Zheng wrote:
> @Colin McCabe <cm...@confluent.io>
>
> I did think about that option. Yes, for most users, it's much easier to
> understand Kafka version, rather than API version. However, the existing
> Kafka clients only report API versions in the Kafka requests. So, the
> brokers can only block clients by API versions rather than the real Kafka
> versions. This can be very confusing for the users, if an API did not
> change for a specified Kafka version.
>
> For example, a user sets the min Kafka version of produce request to Kafka
> 1.1. She would expect the broker will reject Kafka 1.0 producers. However,
> both Kafka 1.1 and Kafka 1.0 are using api version 5. The broker can't
> distinguish the 2 version. So, Kafka 1.0 producers are still allowed.
>
> I think we can say this configuration is only for "advanced users". The
> user has to know the concept of "api version", and know the function of
> each API, to be able to use this feature. (Similarly, it's easier for the
> users to say: "I want to block old consumers, or old admin clients.". But
> there is no clear definition of which set of APIs are "consumer APIs". So,
> we still have to use API names, rather than "client roles")
>
>
>
> On Wed, Mar 27, 2019 at 5:32 PM Colin McCabe <cm...@apache.org> wrote:
>
> > Thanks, Ying Zheng. Looks good overall.
> >
> > One question is, should the version be specified as a Kafka version rather
> > than as a RPC API version? I don't think most users are aware of RPC
> > versions, but something like "min kafka version" would be easier to
> > understand. That is how we handle the minimum inter-broker protocol
> > version and the minimum on-disk format version, after all.
> >
> > best,
> > Colin
> >
> > On Tue, Mar 26, 2019, at 17:52, Ying Zheng wrote:
> > > I have rewritten the KIP. The new proposal is adding a new configuration
> > > min.api.version in Kafka broker.
> > >
> > > Please review the new KIP. Thank you!
> > >
> > > On Fri, Mar 1, 2019 at 11:06 AM Colin McCabe <cm...@apache.org> wrote:
> > >
> > > > On Wed, Feb 27, 2019, at 15:53, Harsha wrote:
> > > > > 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 .
> > > >
> > > > +1, sounds like a good idea.
> > > >
> > > > Colin
> > > >
> > > >
> > > > >
> > > > > 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 <yingz@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 <kafka@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:
> > > > > > > > > > >> >
> > > > > > > > > > >> > >
> > > > > > > > > > >> > >
> > > > > > > > > > >> >
> > > > > > > > > > >>
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: [DISCUSS] KIP-433: Provide client API version to authorizer
Posted by Ying Zheng <yi...@uber.com.INVALID>.
@Colin McCabe <cm...@confluent.io>
I did think about that option. Yes, for most users, it's much easier to
understand Kafka version, rather than API version. However, the existing
Kafka clients only report API versions in the Kafka requests. So, the
brokers can only block clients by API versions rather than the real Kafka
versions. This can be very confusing for the users, if an API did not
change for a specified Kafka version.
For example, a user sets the min Kafka version of produce request to Kafka
1.1. She would expect the broker will reject Kafka 1.0 producers. However,
both Kafka 1.1 and Kafka 1.0 are using api version 5. The broker can't
distinguish the 2 version. So, Kafka 1.0 producers are still allowed.
I think we can say this configuration is only for "advanced users". The
user has to know the concept of "api version", and know the function of
each API, to be able to use this feature. (Similarly, it's easier for the
users to say: "I want to block old consumers, or old admin clients.". But
there is no clear definition of which set of APIs are "consumer APIs". So,
we still have to use API names, rather than "client roles")
On Wed, Mar 27, 2019 at 5:32 PM Colin McCabe <cm...@apache.org> wrote:
> Thanks, Ying Zheng. Looks good overall.
>
> One question is, should the version be specified as a Kafka version rather
> than as a RPC API version? I don't think most users are aware of RPC
> versions, but something like "min kafka version" would be easier to
> understand. That is how we handle the minimum inter-broker protocol
> version and the minimum on-disk format version, after all.
>
> best,
> Colin
>
> On Tue, Mar 26, 2019, at 17:52, Ying Zheng wrote:
> > I have rewritten the KIP. The new proposal is adding a new configuration
> > min.api.version in Kafka broker.
> >
> > Please review the new KIP. Thank you!
> >
> > On Fri, Mar 1, 2019 at 11:06 AM Colin McCabe <cm...@apache.org> wrote:
> >
> > > On Wed, Feb 27, 2019, at 15:53, Harsha wrote:
> > > > 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 .
> > >
> > > +1, sounds like a good idea.
> > >
> > > Colin
> > >
> > >
> > > >
> > > > 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 <yingz@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 <kafka@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:
> > > > > > > > > >> >
> > > > > > > > > >> > >
> > > > > > > > > >> > >
> > > > > > > > > >> >
> > > > > > > > > >>
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: [DISCUSS] KIP-433: Provide client API version to authorizer
Posted by Colin McCabe <cm...@apache.org>.
Thanks, Ying Zheng. Looks good overall.
One question is, should the version be specified as a Kafka version rather than as a RPC API version? I don't think most users are aware of RPC versions, but something like "min kafka version" would be easier to understand. That is how we handle the minimum inter-broker protocol version and the minimum on-disk format version, after all.
best,
Colin
On Tue, Mar 26, 2019, at 17:52, Ying Zheng wrote:
> I have rewritten the KIP. The new proposal is adding a new configuration
> min.api.version in Kafka broker.
>
> Please review the new KIP. Thank you!
>
> On Fri, Mar 1, 2019 at 11:06 AM Colin McCabe <cm...@apache.org> wrote:
>
> > On Wed, Feb 27, 2019, at 15:53, Harsha wrote:
> > > 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 .
> >
> > +1, sounds like a good idea.
> >
> > Colin
> >
> >
> > >
> > > 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:
> > > > > > > > >> >
> > > > > > > > >> > >
> > > > > > > > >> > >
> > > > > > > > >> >
> > > > > > > > >>
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: [DISCUSS] KIP-433: Provide client API version to authorizer
Posted by Ying Zheng <yi...@uber.com.INVALID>.
I have rewritten the KIP. The new proposal is adding a new configuration
min.api.version in Kafka broker.
Please review the new KIP. Thank you!
On Fri, Mar 1, 2019 at 11:06 AM Colin McCabe <cm...@apache.org> wrote:
> On Wed, Feb 27, 2019, at 15:53, Harsha wrote:
> > 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 .
>
> +1, sounds like a good idea.
>
> Colin
>
>
> >
> > 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:
> > > > > > > >> >
> > > > > > > >> > >
> > > > > > > >> > >
> > > > > > > >> >
> > > > > > > >>
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>