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