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 > > >