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