Hi everyone. Might it help resolve the issue posed by strange resource
names if the wildcard ACLs were stored in a different place in Zookeeper?
For example, the KIP proposes this:
*$ get /kafka-acl/Topic/teamA**
*{"version":1,"acls":[{"principal":"User:clientA*","permissionType":"Allow","operation":"Read","host":"*"}]}*
But what if instead it proposed this:
*$ get /kafka-wildcard-acl/Topic/teamA**
*{"version":1,"acls":[{"principal":"User:clientA*","permissionType":"Allow","operation":"Read","host":"*"}]}*
Keeping the two types of ACLs (existing literal/non-wildcard ACLs and
proposed wildcard ACLs) separate I think might make things very clean --
even in downgrade scenarios -- and also allow the issue of a lack of
resource naming constraints to be addressed separately from this KIP.
Ron
On Wed, May 2, 2018 at 9:09 PM, Piyush Vijay <piyushvij...@gmail.com> wrote:
> Thank you everyone for the interest and, prompt and valuable feedback. I
> really appreciate the quick turnaround. I’ve tried to organize the comments
> into common headings. See my replies below:
>
>
>
> *Case of ‘*’ might already be present in consumer groups and transactional
> ids*
>
>
>
> - We definitely need wildcard ACLs support for resources like consumer
> groups and transactional ids for the reason Andy mentioned. A big win of
> this feature is that service providers don’t have to track and keep
> up-to-date all the consumer groups their customers are using.
> - I agree with Andy’s thoughts on the two possible ways.
> - My vote would be to do the breaking change because we should restrict
> the format of consumer groups and transactional ids sooner than later.
> - Consumer groups and transactional ids are basic Kafka concepts.
> There is a lot of value in having a defined naming convention on
> these
> concepts.
> - This will help us not run into more issues down the line.
> - I’m not sure if people actually use ‘*’ in their consumer group
> names anyway.
> - Escaping ‘*’ isn’t trivial because ‘\’ is an allowed character too.
>
>
> *Why introduce two new APIs?*
>
>
>
> - It’s possible to make this change without introducing new APIs but new
> APIs are required for inspection.
> - For example: If I want to fetch all ACLs that match ’topicA*’, it’s
> not possible without introducing new APIs AND maintaining backwards
> compatibility.
>
>
> *Matching multiple hosts*
>
>
>
> - Rajini is right that wildcard ACLs aren’t the correct fit for
> specifying range of hosts.
> - We will rely on KIP-252 for the proper functionality (
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> 252+-+Extend+ACLs+to+allow+filtering+based+on+ip+ranges+and+subnets
> )
>
>
> *Implementation of matching algorithm and performance concerns*
>
>
>
> - Updated the KIP with an implementation.
> - Andy, you’re right. The length doesn’t play a part. The request will
> be authorized *iff* there is at least one matching ALLOW and no matching
> DENY irrespective of the prefix length. Included this detail in the KIP.
> - Since everything is stored in memory, the performance hit isn’t really
> significantly worse than the current behavior.
> - Stephane’s performance improvement suggestion is a great idea but
> orthogonal.
>
>
> *Why extend this for principal?*
>
>
>
> - Thanks for the amazing points Rajini.
> - There is a use case where ALL principals from an org might want to
> access fix set of topics.
> - User group functionality is currently missing.
> - IIRC SASL doesn’t use custom principal builder.
> - However, prefixing is not the right choice in all cases. Agreed.
> - Thoughts?
>
>
> *Changes to AdminClient to support wildcard ACLs*
>
>
>
> - Thanks Colin for the implementation. It’s good to have you and others
> here for the expert opinions.
> - The current implementation uses two classes: AclBinding and
> AclBindingFilter. (
> https://github.com/apache/kafka/blob/trunk/clients/src/
> main/java/org/apache/kafka/common/acl/AclBinding.java
> and
> https://github.com/apache/kafka/blob/trunk/clients/src/
> main/java/org/apache/kafka/common/acl/AclBindingFilter.java
> )
> - AclBinding is definition of an Acl. It’s used to create ACLs.
> - AclBindingFilter is used to fetch or delete “matching’ ACLs.
> - In the context of wildcard suffixed ACLs, a stored ACL may have ‘*’ in
> it. *It really removes the distinction between these two classes.*
> - The current implementation uses ‘null’ to define wildcard ACL (‘*’). I
> think it’s not a good pattern and we should use ‘*’ for the wildcard
> ACL (
> https://github.com/apache/kafka/blob/trunk/clients/src/
> main/java/org/apache/kafka/common/acl/AclBindingFilter.java#L39
> and
> https://github.com/apache/kafka/blob/trunk/clients/src/
> main/java/org/apache/kafka/common/resource/ResourceFilter.java#L37
> ).
> - However, the above two changes are breaking change but it should be
> acceptable because the API is marked with @InterfaceStability.Evolving.
> - If everyone agrees to the above two changes (merging the two classes
> and using non-null values for blanket access), the only other change is
> using the matching algorithm from the KIP to match ACLs.
>
>
> Other comments:
>
>
>
> - > It may be worth excluding delegation token ACLs from using prefixed
> wildcards since it doesn't make much sense.
>
> I want to ask for clarification on what delegation token ACLs are before
> commenting. Wildcard suffixed ACLs are supported only for resource and
> principal names.
>
>
>
> - A quick read makes me believe that I’ve fixed the formatting issues
> reported by Ted. Let me know if something is still wrong and I would be
> happy to fix it.
> - I’ve fixed the mismatch in signature reported by Ron.
> - Andy, I’ve updated the KIP with the security hole related to DENY
> wildcard ACLs ‘*’ on the downgrade path.
> - Wrt naming, wildcard suffixed ACLs sound reasonable to me until
> someone raise a major objection.
>
>
>
> Let me know your thoughts. Looking forward to the next iteration.
>
>
> Best,
>
> Piyush
>
>
>
>
>
> Piyush Vijay
>
> On Wed, May 2, 2018 at 1:33 PM, Andy Coates <a...@confluent.io> wrote:
>
> > > Perhaps there is a simpler way. It seems like the resource that people
> > really want to use prefixed ACLs with is topic names. Because topic
> names
> > can't contain "*", there is no ambiguity there, either. I think maybe we
> > should restrict the prefix handling to topic resources.
> >
> > The KIP should cover consumer groups for sure, otherwise we're back to
> the
> > situation users need to know, up front, the set of consumer groups an
> > KStreams topology is going to use.
> >
> > I'm assuming transactional producer Ids would be the same.
> >
> > The cleanest solution would be to restrict the characters in group and
> > transaction producer ids, but that's a breaking change that might affect
> a
> > lot of people.
> >
> > Another solution might be to add a protocol version to the `Acl` type,
> (not
> > the current `version` field used for optimistic concurrency control),
> > defaulting it to version 1 if it is not present, and releasing this
> change
> > as version 2. This would at least allow us to leave the version 1 ACLs
> > as-is, (which makes for a cleaner storey if a cluster is downgraded).
> There
> > is then potential to escape '*' when writing version 2 ACLs. (And
> introduce
> > code to check for supported ACL versions going forward). Though... how do
> > we escape? If the consumer group is free form, then any escape sequence
> is
> > also valid. Aren't there metrics that use the group name? If there
> are,
> > I'd of thought we'd need to restrict the char set anyway.
> >
> > On 2 May 2018 at 18:57, charly molter <charly.mol...@gmail.com> wrote:
> >
> > > The fact that consumer groups and transactionalProducerId don't have a
> > > strict format is problematic (we have problems with people with empty
> > > spaces at the end of their consumer group for example).
> > > With 2.0 around the corner and the possibility to fix these errors from
> > the
> > > past should we create a KIP to restrict these names like we do for
> > topics?
> > >
> > > Thanks!
> > > Charly
> > >
> > > On Wed, May 2, 2018 at 6:22 PM Colin McCabe <cmcc...@apache.org>
> wrote:
> > >
> > > > Hi Piyush,
> > > >
> > > > Thanks for the KIP! It seems like it will be really useful.
> > > >
> > > > As Rajini commented, the names for some resources (such as consumer
> > > > groups) can include stars. So your consumer group might be named
> > "foo*".
> > > > We need a way of explicitly referring to that consumer group name,
> > rather
> > > > than to the foo prefix. A simple way would be to escape the star
> with
> > a
> > > > backslash: "foo\*" During the software upgrade process, we also need
> > to
> > > > translate all ACLs that refer to "foo*" into "foo\*". Otherwise, the
> > > > upgrade could create a security hole by granting more access than the
> > > > administrator intended.
> > > >
> > > > Perhaps there is a simpler way. It seems like the resource that
> people
> > > > really want to use prefixed ACLs with is topic names. Because topic
> > > names
> > > > can't contain "*", there is no ambiguity there, either. I think
> maybe
> > we
> > > > should restrict the prefix handling to topic resources.
> > > >
> > > > Are the new "getMatchingAcls" methods needed? It seems like they
> break
> > > > encapsulation. All that the calling code really needs to do is call
> > > > Authorizer#authorize(session, operation, resource). The authorizer
> > knows
> > > > that if it has an ACL allowing access to topics starting with "foo"
> and
> > > > someone calls authorize(foobar), it should allow access. It's not
> > > > necessary for the calling code to know exactly what the rules are for
> > > > authorization. The Authorizer#getAcls, APIs are only needed when the
> > > > AdminClient wants to list ACLs.
> > > >
> > > > best,
> > > > Colin
> > > >
> > > >
> > > > On Wed, May 2, 2018, at 03:31, Rajini Sivaram wrote:
> > > > > Hi Piyush,
> > > > >
> > > > > Thanks for the KIP for this widely requested feature. A few
> > > > > comments/questions:
> > > > >
> > > > > 1. Some resource names can contain '*'. For example, consumer
> groups
> > or
> > > > > transactional ids. I am wondering whether we need to restrict
> > > characters
> > > > > for these entities or provide a way to distinguish wildcarded
> > resource
> > > > from
> > > > > a resource containing the wildcard character.
> > > > >
> > > > > 2. I am not sure we want to do wildcarded principals. It feels
> like a
> > > > > workaround in the absence of user groups. In the longer term, I
> think
> > > > > groups (without wildcards) would be a better option to configure
> ACLs
> > > for
> > > > > groups of users, rather than building user principals which have
> > common
> > > > > prefixes. In the shorter term, since a configurable
> PrincipalBuilder
> > is
> > > > > used to build the principal used for authorization, perhaps using
> the
> > > > > prefix itself as the principal would work (unless different quotas
> > are
> > > > > required for the full user name)? User principal strings take
> > different
> > > > > formats for different security protocols (eg. CN=xxx,O=org,C=UK for
> > > SSL)
> > > > > and simple prefixing isn't probably the right grouping in many
> cases.
> > > > >
> > > > > 3. I am assuming we don't want to do wildcarded hosts in this KIP
> > since
> > > > > wildcard suffix doesn't really work for hosts.
> > > > >
> > > > > 4. It may be worth excluding delegation token ACLs from using
> > prefixed
> > > > > wildcards since it doesn't make much sense.
> > > > >
> > > > > Regards,
> > > > >
> > > > > Rajini
> > > > >
> > > > >
> > > > > On Wed, May 2, 2018 at 2:05 AM, Stephane Maarek <
> > > > > steph...@simplemachines.com.au> wrote:
> > > > >
> > > > > > Hi, thanks for this badly needed feature
> > > > > >
> > > > > > 1) Why introduce two new APIs in authorizer instead of replacing
> > the
> > > > > > implementation for simple ACL authorizer with adding the wildcard
> > > > > > capability?
> > > > > >
> > > > > > 2) is there an impact to performance as now we're evaluating more
> > > > rules ? A
> > > > > > while back I had evaluated the concept of cached Acl result so
> > > swallow
> > > > the
> > > > > > cost of computing an authorisation cost once and then doing in
> > memory
> > > > > > lookups. CF: https://issues.apache.org/jira/browse/KAFKA-5261
> > > > > >
> > > > > > 3) is there any need to also extend this to consumer group
> > resources
> > > ?
> > > > > >
> > > > > > 4) create topics KIP as recently moved permissions out of Cluster
> > > into
> > > > > > Topic. Will wildcard be supported for this action too?
> > > > > >
> > > > > > Thanks a lot for this !
> > > > > >
> > > > > > On Wed., 2 May 2018, 1:37 am Ted Yu, <yuzhih...@gmail.com>
> wrote:
> > > > > >
> > > > > > > w.r.t. naming, we can keep wildcard and drop 'prefixed' (or
> > > > 'suffixed')
> > > > > > > since the use of regex would always start with non-wildcard
> > > portion.
> > > > > > >
> > > > > > > Cheers
> > > > > > >
> > > > > > > On Tue, May 1, 2018 at 12:13 PM, Andy Coates <
> a...@confluent.io>
> > > > wrote:
> > > > > > >
> > > > > > > > Hi Piyush,
> > > > > > > >
> > > > > > > > Can you also document in the Compatibility section what would
> > > > happen
> > > > > > > should
> > > > > > > > the cluster be upgraded, wildcard-suffixed ACLs are added,
> and
> > > > then the
> > > > > > > > cluster is rolled back to the previous version. On downgrade
> > the
> > > > > > partial
> > > > > > > > wildcard ACLs will be treated as literals and hence never
> match
> > > > > > anything.
> > > > > > > > This is fine for ALLOW ACLs, but some might consider this a
> > > > security
> > > > > > hole
> > > > > > > > if a DENY ACL is ignored, so this needs to be documented,
> both
> > in
> > > > the
> > > > > > KIP
> > > > > > > > and the final docs.
> > > > > > > >
> > > > > > > > For some reason I find the term 'prefixed wildcard ACLs'
> easier
> > > to
> > > > grok
> > > > > > > > than 'wildcard suffixed ACLs'. Probably because in the former
> > the
> > > > > > > > 'wildcard' term comes after the positional adjective, which
> > > > matches the
> > > > > > > > position of the wildcard char in the resource name, i.e.
> > "some*".
> > > > It's
> > > > > > > > most likely a person thing, but I thought I'd mention it as
> > > naming
> > > > is
> > > > > > > > important when it comes to making this initiative for users.
> > > > > > > >
> > > > > > > > On 1 May 2018 at 19:57, Andy Coates <a...@confluent.io>
> wrote:
> > > > > > > >
> > > > > > > > > Hi Piyush,
> > > > > > > > >
> > > > > > > > > Thanks for raising this KIP - it's very much appreciated.
> > > > > > > > >
> > > > > > > > > I've not had chance to digest it yet, but...
> > > > > > > > >
> > > > > > > > > 1. you might want to add details of how the internals of
> the
> > > > > > > > > `getMatchingAcls` is implemented. We'd want to make sure
> the
> > > > > > complexity
> > > > > > > > of
> > > > > > > > > the operation isn't adversely affected.
> > > > > > > > > 2. You might want to be more explicit that the length of a
> > > prefix
> > > > > > does
> > > > > > > > not
> > > > > > > > > play a part in the `authorize` call, e.g. given ACLs {DENY,
> > > > some.*},
> > > > > > > > {ALLOW,
> > > > > > > > > some.prefix.*}, the longer, i.e. more specific, allow ACL
> > does
> > > > _not_
> > > > > > > > > override the more general deny ACL.
> > > > > > > > >
> > > > > > > > > Thanks,
> > > > > > > > >
> > > > > > > > > Andy
> > > > > > > > >
> > > > > > > > > On 1 May 2018 at 16:59, Ron Dagostino <rndg...@gmail.com>
> > > wrote:
> > > > > > > > >
> > > > > > > > >> Hi Piyush. I appreciated your talk at Kafka Summit and
> > > > appreciate
> > > > > > the
> > > > > > > > KIP
> > > > > > > > >> -- thanks.
> > > > > > > > >>
> > > > > > > > >> Could you explain these mismatching references? Near the
> > top
> > > > of the
> > > > > > > KIP
> > > > > > > > >> you refer to these proposed new method signatures:
> > > > > > > > >>
> > > > > > > > >> def getMatchingAcls(resource: Resource): Set[Acl]
> > > > > > > > >> def getMatchingAcls(principal: KafkaPrincipal):
> > Map[Resource,
> > > > > > > Set[Acl]]
> > > > > > > > >>
> > > > > > > > >> But near the bottom of the KIP you refer to different
> method
> > > > > > > > >> signatures that don't seem to match the above ones:
> > > > > > > > >>
> > > > > > > > >> getMatchingAcls(topicRegex)
> > > > > > > > >> getMatchingAcls(principalRegex)
> > > > > > > > >>
> > > > > > > > >> Ron
> > > > > > > > >>
> > > > > > > > >>
> > > > > > > > >> On Tue, May 1, 2018 at 11:48 AM, Ted Yu <
> > yuzhih...@gmail.com>
> > > > > > wrote:
> > > > > > > > >>
> > > > > > > > >> > The KIP was well written. Minor comment on formatting:
> > > > > > > > >> >
> > > > > > > > >> > https://github.com/apache/kafka/blob/trunk/core/src/
> > > > > > > > >> > main/scala/kafka/admin/AclCommand.scala
> > > > > > > > >> > to
> > > > > > > > >> >
> > > > > > > > >> > Leave space between the URL and 'to'
> > > > > > > > >> >
> > > > > > > > >> > Can you describe changes for the AdminClient ?
> > > > > > > > >> >
> > > > > > > > >> > Thanks
> > > > > > > > >> >
> > > > > > > > >> > On Tue, May 1, 2018 at 8:12 AM, Piyush Vijay <
> > > > > > > piyushvij...@gmail.com>
> > > > > > > > >> > wrote:
> > > > > > > > >> >
> > > > > > > > >> > > Hi all,
> > > > > > > > >> > >
> > > > > > > > >> > > I just opened a KIP to add support for wildcard
> suffixed
> > > > ACLs.
> > > > > > > This
> > > > > > > > is
> > > > > > > > >> > one
> > > > > > > > >> > > of the feature I talked about in my Kafka summit talk
> > and
> > > we
> > > > > > > > promised
> > > > > > > > >> to
> > > > > > > > >> > > upstream it :)
> > > > > > > > >> > >
> > > > > > > > >> > > The details are here -
> > > > > > > > >> > > https://cwiki.apache.org/
> confluence/display/KAFKA/KIP-
> > > > > > > > >> > > 290%3A+Support+for+wildcard+suffixed+ACLs
> > > > > > > > >> > >
> > > > > > > > >> > > There is an open question about the way to add the
> > support
> > > > in
> > > > > > the
> > > > > > > > >> > > AdminClient, which I can discuss here in more detail
> > once
> > > > > > everyone
> > > > > > > > has
> > > > > > > > >> > > taken a first look at the KIP.
> > > > > > > > >> > >
> > > > > > > > >> > > Looking forward to discuss the change.
> > > > > > > > >> > >
> > > > > > > > >> > > Best,
> > > > > > > > >> > > Piyush Vijay
> > > > > > > > >> > >
> > > > > > > > >> >
> > > > > > > > >>
> > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > >
> > >
> > >
> > > --
> > > Charly Molter
> > >
> >
>
