Makes sense. I'll update the KIP. Does anyone have any other comments? :)
Thanks Piyush Vijay On Thu, May 3, 2018 at 11:55 AM, Colin McCabe <[email protected]> wrote: > Yeah, I guess that's a good point. It probably makes sense to support the > prefix scheme for consumer groups and transactional IDs as well as topics. > > I agree that the current situation where anything goes in consumer group > names and transactional ID names is not ideal. I wish we could rewind the > clock and impose restrictions on the names. However, it doesn't seem > practical at the moment. Adding new restrictions would break a lot of > existing users after an upgrade. It would be a really bad upgrade > experience. > > However, I think we can support this in a compatible way. From the > perspective of AdminClient, we just have to add a new field to > ResourceFilter. Currently, it has two fields, resourceType and name: > > > /** > > * A filter which matches Resource objects. > > * > > * The API for this class is still evolving and we may break > compatibility in minor releases, if necessary. > > */ > > @InterfaceStability.Evolving > > public class ResourceFilter { > > private final ResourceType resourceType; > > private final String name; > > We can add a third field, pattern. > > So the API will basically be, if I create a ResourceFilter(resourceType=GROUP, > name=foo*, pattern=null), it applies only to the consumer group named > "foo*". If I create a ResourceFilter(resourceType=GROUP, name=null, > pattern=foo*), it applies to any consumer group starting in "foo". name > and pattern cannot be both set at the same time. This preserves > compatibility at the AdminClient level. > > It's possible that we will want to add more types of pattern in the > future. So we should reserve "special characters" such as +, /, &, %, #, > $, etc. These characters should be treated as special unless they are > prefixed with a backslash to escape them. This will allow us to add > support for using these characters in the future without breaking > compatibility. > > At the protocol level, we need a new API version for CreateAclsRequest / > DeleteAclsRequest. The new API version will send all special characters > over the wire escaped rather than directly. (So there is no need for the > equivalent of both "name" and "pattern"-- we translate name into a validly > escaped pattern that matches only one thing, by adding escape characters as > appropriate.) The broker will validate the new API version and reject > malformed of unsupported patterns. > > At the ZK level, we can introduce a protocol version to the data in ZK-- > or store it under a different root. > > best, > Colin > > > On Wed, May 2, 2018, at 18:09, Piyush Vijay 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> > 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 < > > > > > > [email protected]> 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, <[email protected]> > 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 < > [email protected]> > > > > > 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 <[email protected]> > 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 <[email protected] > > > > > > 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 < > > > [email protected]> > > > > > > > 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 < > > > > > > > > [email protected]> > > > > > > > > > >> > 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 > > > > > > > >
