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