You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Ying Zheng <yi...@uber.com.INVALID> on 2019/02/22 22:11:17 UTC
[DISCUSS] KIP-433: Provide client API version to authorizer
Re: [DISCUSS] KIP-433: Provide client API version to authorizer
Posted by Don Bosco Durai <bo...@apache.org>.
In general, making authorization decision based on client version might not be a good idea. It will just provide a loop hole for someone to downgrade their client version to exploit or workaround any restrictions.
> > propose a single broker dynamic configuration: client.min.api.version, to
> 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.
These could be Authorizer implementation detail. If needed, native implementation could use this property to make the decision, while the custom authorizer can use this property or come up with their own set of properties to support or not support certain checks or features. Regardless, it would be good to have checks consistent across the version.
> 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.
I am not sure whether the client version is available during authorization API call. It might be good if we can send these information in a generic name/value list in session context. In that way, the authorization implementation can use it the way the way want to or ignore it.
Bosco
On 2/26/19, 10:04 AM, "Colin McCabe" <cm...@apache.org> 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:
> > > > > > > >> >
> > > > > > > >> > >
> > > > > > > >> > >
> > > > > > > >> >
> > > > > > > >>
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: [DISCUSS] KIP-433: Provide client API version to authorizer
Posted by Colin McCabe <cm...@apache.org>.
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 Harsha <ka...@harsha.io>.
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:
> > > > > >> >
> > > > > >> > >
> > > > > >> > >
> > > > > >> >
> > > > > >>
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: [DISCUSS] KIP-433: Provide client API version to authorizer
Posted by Colin McCabe <cm...@apache.org>.
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 Harsha <ka...@harsha.io>.
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.
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 Colin McCabe <cm...@apache.org>.
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 Harsha <ka...@harsha.io>.
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>.
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>.
@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 Harsha <ka...@harsha.io>.
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 Ismael Juma <is...@gmail.com>.
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 Viktor Somogyi-Vass <vi...@gmail.com>.
Hi Ying,
I was also thinking about your KIP yesterday I second Gwen on this. There
are several other authorization components that are compatible with Kafka
and putting this in there means that each and every component has to
implement this functionality.
At the first read I thought that this functionality could be a good
extension of the API_VERSIONS protocol where based on a dynamic config
(producer.min.api.version or consumer.min.api.version) the brokers would
reject certain clients that are too old. And based on the motivation, to
avoid certain bugs with certain producer/consumer version, we might not
need finer grained control over this which I think also plays for the
config approach.
Cheers,
Viktor
On Sat, Feb 23, 2019 at 12:06 AM Ying Zheng <yi...@uber.com.invalid> wrote:
> Hi Gwen,
>
> Thank you for the quick feedback!
>
> It's a good point that broker configuration can be dynamic and is more
> convenient. Technically, anything inside the authorizer can also be
> dynamic. For example, the SimpleAclAuthorizer in Kafka stores ACLs in
> Zookeeper, which can be dynamically changed with CLI.
>
>
>
>
>
> On Fri, Feb 22, 2019 at 2:41 PM Gwen Shapira <gw...@confluent.io> wrote:
>
> > Link, for convenience:
> >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-433%3A+Provide+client+API+version+to+authorizer
> >
> > I actually prefer the first rejected alternative (add a
> > configuration). While you are right that configuration is inherently
> > less flexible, putting the logic in the authorizer means that an admin
> > that wants to limit the allowed client API versions has to implement
> > an authorizer. This is more challenging than changing a config (and
> > AFAIK, can't be done dynamically - configs can be dynamic and the
> > admin can avoid a restart).
> >
> > Would be interested to hear what others think.
> >
> > Gwen
> >
> > On Fri, Feb 22, 2019 at 2:11 PM Ying Zheng <yi...@uber.com.invalid>
> wrote:
> > >
> > >
> >
> >
> > --
> > Gwen Shapira
> > Product Manager | Confluent
> > 650.450.2760 | @gwenshap
> > Follow us: Twitter | blog
> >
>
Re: [DISCUSS] KIP-433: Provide client API version to authorizer
Posted by Ying Zheng <yi...@uber.com.INVALID>.
Hi Gwen,
Thank you for the quick feedback!
It's a good point that broker configuration can be dynamic and is more
convenient. Technically, anything inside the authorizer can also be
dynamic. For example, the SimpleAclAuthorizer in Kafka stores ACLs in
Zookeeper, which can be dynamically changed with CLI.
On Fri, Feb 22, 2019 at 2:41 PM Gwen Shapira <gw...@confluent.io> wrote:
> Link, for convenience:
>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-433%3A+Provide+client+API+version+to+authorizer
>
> I actually prefer the first rejected alternative (add a
> configuration). While you are right that configuration is inherently
> less flexible, putting the logic in the authorizer means that an admin
> that wants to limit the allowed client API versions has to implement
> an authorizer. This is more challenging than changing a config (and
> AFAIK, can't be done dynamically - configs can be dynamic and the
> admin can avoid a restart).
>
> Would be interested to hear what others think.
>
> Gwen
>
> On Fri, Feb 22, 2019 at 2:11 PM Ying Zheng <yi...@uber.com.invalid> wrote:
> >
> >
>
>
> --
> Gwen Shapira
> Product Manager | Confluent
> 650.450.2760 | @gwenshap
> Follow us: Twitter | blog
>
Re: [DISCUSS] KIP-433: Provide client API version to authorizer
Posted by Gwen Shapira <gw...@confluent.io>.
Link, for convenience:
https://cwiki.apache.org/confluence/display/KAFKA/KIP-433%3A+Provide+client+API+version+to+authorizer
I actually prefer the first rejected alternative (add a
configuration). While you are right that configuration is inherently
less flexible, putting the logic in the authorizer means that an admin
that wants to limit the allowed client API versions has to implement
an authorizer. This is more challenging than changing a config (and
AFAIK, can't be done dynamically - configs can be dynamic and the
admin can avoid a restart).
Would be interested to hear what others think.
Gwen
On Fri, Feb 22, 2019 at 2:11 PM Ying Zheng <yi...@uber.com.invalid> wrote:
>
>
--
Gwen Shapira
Product Manager | Confluent
650.450.2760 | @gwenshap
Follow us: Twitter | blog