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" <cmcc...@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:
> > >> >
> > >> > >
> > >> > >
> > >> >
> > >>
> > >
> >
>
