Colin/Piyush, I was under the impression that we are keeping both Resource and ResourceFilter since they are used in different contexts. And hence we would retain both AclBinding and AclBindingFilter. Is that not the case? Anyway, an updated KIP would be useful since it has got to a stage where it is hard to work out exactly where we have got to.
Piyush, Since there is a lot of interest in this KIP, it would be great to get it into 2.0.0. It will be good to start the vote by tomorrow if there is general agreement on an updated KIP today. Thanks, Rajini On Tue, May 15, 2018 at 7:18 PM, Rajini Sivaram <rajinisiva...@gmail.com> wrote: > Hi Piyush, > > It is possible to configure PrincipalBuilder for SASL ( > https://cwiki.apache.org/confluence/display/KAFKA/KIP- > 189%3A+Improve+principal+builder+interface+and+add+support+for+SASL). If > that satisfies your requirements, perhaps we can move wildcarded principals > out of this KIP and focus on wildcarded resources? > > > On Tue, May 15, 2018 at 7:15 PM, Piyush Vijay <piyushvij...@gmail.com> > wrote: > >> Hi Colin, >> >> Escaping at this level is making sense to me but let me think more and get >> back to you. >> >> But should we not just get rid of one of AclBinding or AclBindingFilter >> then? Is there a reason to keep both given that AclBindingFilter and >> AclBinding look exact copy of each other after this change? This will be a >> one-time breaking change in APIs marked as "Evolving", but makes sense in >> the long term? Am I missing something here? >> >> >> >> Piyush Vijay >> >> On Tue, May 15, 2018 at 9:01 AM, Colin McCabe <cmcc...@apache.org> wrote: >> >> > Hi Piyush, >> > >> > I think AclBinding should operate the same way as AclBindingFilter. >> > >> > So you should be able to do something like this: >> > > AclBindingFilter filter = new AclBindingFiler(new >> > ResourceFilter(ResourceType.GROUP, "foo*")) >> > > AclBinding binding = new AclBinding(new Resource(ResourceType.GROUP, >> > "foo*")) >> > > assertTrue(filter.matches(binding)); >> > >> > Thinking about this more, it's starting to feel really messy to create >> new >> > "pattern" constructors for Resource and ResourceFilter. I don't think >> > people will be able to figure this out. Maybe we should just have a >> > limited compatibility break here, where it is now required to escape >> weird >> > consumer group names when creating ACLs for them. >> > >> > To future-proof this, we should reserve a bunch of characters at once, >> > like *, @, $, %, ^, &, +, [, ], etc. If these characters appear in a >> > resource name, it should be an error, unless they are escaped with a >> > backslash. That way, we can use them in the future. We should create a >> > Resource.escapeName function which adds the correct escape characters to >> > resource names (so it would translate foo* into foo\*, foo+bar into >> > foo\+bar, etc. etc. >> > >> > best, >> > Colin >> > >> > >> > On Mon, May 14, 2018, at 17:08, Piyush Vijay wrote: >> > > Colin, >> > > >> > > createAcls take a AclBinding, however, instead of AclBindingFilter. >> What >> > > are your thoughts here? >> > > >> > > public abstract DescribeAclsResult describeAcls(AclBindingFilter >> > > filter, DescribeAclsOptions options); >> > > >> > > public abstract CreateAclsResult createAcls(Collection<AclBinding> >> > > acls, CreateAclsOptions options); >> > > >> > > public abstract DeleteAclsResult >> > > deleteAcls(Collection<AclBindingFilter> filters, DeleteAclsOptions >> > > options); >> > > >> > > >> > > Thanks >> > > >> > > Piyush Vijay >> > > >> > > On Mon, May 14, 2018 at 9:26 AM, Andy Coates <a...@confluent.io> >> wrote: >> > > >> > > > +1 >> > > > >> > > > On 11 May 2018 at 17:14, Colin McCabe <cmcc...@apache.org> wrote: >> > > > >> > > > > Hi Andy, >> > > > > >> > > > > I see what you mean. I guess my thought here is that if the >> fields >> > are >> > > > > private, we can change it later if we need to. >> > > > > >> > > > > I definitely agree that we should use the scheme you describe for >> > sending >> > > > > ACLs over the wire (just the string + version number) >> > > > > >> > > > > cheers, >> > > > > Colin >> > > > > >> > > > > >> > > > > On Fri, May 11, 2018, at 09:39, Andy Coates wrote: >> > > > > > i think I'm agreeing with you. I was merely suggesting that >> having >> > an >> > > > > > additional field that controls how the current field is >> > interpreted is >> > > > > more >> > > > > > flexible / extensible in the future than using a 'union' style >> > > > approach, >> > > > > > where only one of several possible fields should be populated. >> But >> > > > it's a >> > > > > > minor thing. >> > > > > > >> > > > > > >> > > > > > >> > > > > > >> > > > > > >> > > > > > >> > > > > > On 10 May 2018 at 09:29, Colin McCabe <cmcc...@apache.org> >> wrote: >> > > > > > >> > > > > > > Hi Andy, >> > > > > > > >> > > > > > > The issue that I was trying to solve here is the Java API. >> Right >> > > > now, >> > > > > > > someone can write "new ResourceFilter(ResourceType. >> > TRANSACTIONAL_ID, >> > > > > > > "foo*") and have a ResourceFilter that applies to a >> > Transactional ID >> > > > > named >> > > > > > > "foo*". This has to continue to work, or else we have broken >> > > > > compatibility. >> > > > > > > >> > > > > > > I was proposing that there would be something like a new >> function >> > > > like >> > > > > > > ResourceFilter.fromPattern(ResourceType.TRANSACTIONAL_ID, >> > "foo*") >> > > > > which >> > > > > > > would create a ResourceFilter that applied to transactional >> IDs >> > > > > starting >> > > > > > > with "foo", rather than transactional IDs named "foo*" >> > specifically. >> > > > > > > >> > > > > > > I don't think it's important whether the Java class has an >> > integer, >> > > > an >> > > > > > > enum, or two string fields. The important thing is that >> there's >> > a >> > > > new >> > > > > > > static function, or new constructor overload, etc. that works >> for >> > > > > patterns >> > > > > > > rather than literal strings. >> > > > > > > >> > > > > > > On Thu, May 10, 2018, at 03:30, Andy Coates wrote: >> > > > > > > > Rather than having name and pattern fields on the >> > ResourceFilter, >> > > > > where >> > > > > > > > it’s only valid for one to be set, and we want to restrict >> the >> > > > > character >> > > > > > > > set in case future enhancements need them, we could instead >> > add a >> > > > new >> > > > > > > > integer ‘nameType’ field, and use constants to indicate how >> the >> > > > name >> > > > > > > > field should be interpreted, e.g. 0 = literal, 1 = wildcard. >> > This >> > > > > would >> > > > > > > > be extendable, e.g we can later add 2 = regex, or what ever, >> > and >> > > > > > > > wouldn’t require any escaping. >> > > > > > > >> > > > > > > This is very user-unfriendly, though. Users don't want to >> have >> > to >> > > > > > > explicitly supply a version number when using the API, which >> is >> > what >> > > > > this >> > > > > > > would force them to do. I don't think users are going to >> want to >> > > > > memorize >> > > > > > > that version 4 supprted "+", whereas version 3 only supported >> > > > "[0-9]", >> > > > > or >> > > > > > > whatever. >> > > > > > > >> > > > > > > Just as an example, do you remember which versions of >> > FetchRequest >> > > > > added >> > > > > > > which features? I don't. I always have to look at the code >> to >> > > > > remember. >> > > > > > > >> > > > > > > Also, escaping is still required any time you overload a >> > character to >> > > > > mean >> > > > > > > two things. Escaping is required in the current proposal to >> be >> > able >> > > > to >> > > > > > > create a pattern that matches only "foo*". You have to type >> > "foo\*" >> > > > > It >> > > > > > > would be required if we forced users to specify a version, as >> > well. >> > > > > > > >> > > > > > > best, >> > > > > > > Colin >> > > > > > > >> > > > > > > > >> > > > > > > > Sent from my iPhone >> > > > > > > > >> > > > > > > > > On 7 May 2018, at 05:16, Piyush Vijay < >> > piyushvij...@gmail.com> >> > > > > wrote: >> > > > > > > > > >> > > > > > > > > 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 < >> > > > cmcc...@apache.org >> > > > > > >> > > > > > > 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=GR >> OUP, >> > > > > 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/com >> mon/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 >> > > > > > > > >>>>> >> > > > > > > > >>>> >> > > > > > > > >> >> > > > > > > >> > > > > >> > > > >> > >> > >
