Hi Colin,
thanks for your response!
in theory we could get away without any additional path changes I
think.. I am still somewhat unsure about the best way of addressing
this. I'll outline my current idea and concerns that I still have,
maybe you have some thoughts on it.
ACLs are currently stored in two places in ZK: /kafka-acl and
/kafka-acl-extended based on whether they make use of prefixes or not.
The reasoning[1] for this is not fundamentally changed by anything we
are discussing here, so I think that split will need to remain.
ACLs are then stored in the form of a json array:
[zk: 127.0.0.1:2181(CONNECTED) 9] get /kafka-acl/Topic/*
{"version":1,"acls":[{"principal":"User:sliebau","permissionType":"Allow","operation":"Read","host":"*"},{"principal":"User:sliebau","permissionType":"Allow","operation":"Describe","host":"*"},{"principal":"User:sliebau2","permissionType":"Allow","operation":"Describe","host":"*"},{"principal":"User:sliebau2","permissionType":"Allow","operation":"Read","host":"*"}]}
What we could do is add a version property to the individual ACL
elements like so:
[
{
"principal": "User:sliebau",
"permissionType": "Allow",
"operation": "Read",
"host": "*",
"acl_version": "1"
}
]
We define the current state of ACLs as version 0 and the Authorizer
will default a missing "acl_version" element to this value for
backwards compatibility. So there should hopefully be no need to
migrate existing ACLs (concerns notwithstanding, see later).
Additionally the authorizer will get a max_supported_acl_version
setting which will cause it to ignore any ACLs larger than what is set
here, hence allowing for controlled upgrading similar to the process
using inter broker protocol version. If this happens we should
probably log a warning in case this was unintentional. Maybe even have
a setting that controls whether startup is even possible when not all
ACLs are in effect.
As I mentioned I have a few concerns, question marks still outstanding on this:
- This approach would necessitate being backwards compatible with all
earlier versions of ACLs unless we also add a min_acl_version setting
- which would put the topic of ACL migrations back on the agenda.
- Do we need to touch the wire protocol for the admin client for this?
In theory I think not, as the authorizer would write ACLs in the most
current (unless forced down by max_acl_version) version it knows, but
this takes any control over this away from the user.
- This adds json parsing logic to the Authorizer, as it would have to
check the version first, look up the proper ACL schema for that
version and then re-parse the ACL string with that schema - should not
be a real issue if the initial parsing is robust, but strictly
speaking we are parsing something that we don't know the schema for
which might create issues with updates down the line.
Beyond the practical concerns outlined above there are also some
broader things maybe worth thinking about. The long term goal is to
move away from Zookeeper and other data like consumer group offsets
has already been moved into Kafka topics - is that something that we'd
want to consider for ACLs as well? With the current storage model we'd
need more than one topic for this to cleanly separate resources and
prefixed ACLs - if we consider pursuing this option it might be a
chance for a "larger" change to the format which introduces versioning
and allows storing everything in one compacted topic.
Any thoughts on this?
Best regards,
Sönke
[1]
https://cwiki.apache.org/confluence/display/KAFKA/KIP-290%3A+Support+for+Prefixed+ACLs
On Sat, Dec 22, 2018 at 5:51 AM Colin McCabe <cmcc...@apache.org> wrote:
>
> Hi Sönke,
>
> One path forward would be to forbid the new ACL types from being created
> until the inter-broker protocol had been upgraded. We'd also have to figure
> out how the new ACLs were stored in ZooKeeper. There are a bunch of
> proposals in this thread that could work for that-- I really hope we don't
> keep changing the ZK path each time there is a version bump.
>
> best,
> Colin
>
>
> On Thu, Nov 29, 2018, at 14:25, Sönke Liebau wrote:
> > This has been dormant for a while now, can I interest anybody in chiming in
> > here?
> >
> > I think we need to come up with an idea of how to handle changes to ACLs
> > going forward, i.e. some sort of versioning scheme. Not necessarily what I
> > proposed in my previous mail, but something.
> > Currently this fairly simple change is stuck due to this being unsolved.
> >
> > I am happy to move forward without addressing the larger issue (I think the
> > issue raised by Colin is valid but could be mitigated in the release
> > notes), but that would mean that the next KIP to touch ACLs would inherit
> > the issue, which somehow doesn't seem right.
> >
> > Looking forward to your input :)
> >
> > Best regards,
> > Sönke
> >
> > On Tue, Jun 19, 2018 at 5:32 PM Sönke Liebau <soenke.lie...@opencore.com>
> > wrote:
> >
> > > Picking this back up, now that KIP-290 has been merged..
> > >
> > > As Colin mentioned in an earlier mail this change could create a
> > > potential security issue if not all brokers are upgraded and a DENY
> > > Acl based on an IP range is created, as old brokers won't match this
> > > rule and still allow requests. As I stated earlier I am not sure
> > > whether for this specific change this couldn't be handled via the
> > > release notes (see also this comment [1] from Jun Rao on a similar
> > > topic), but in principle I think some sort of versioning system around
> > > ACLs would be useful. As seen in KIP-290 there were a few
> > > complications around where to store ACLs. To avoid adding ever new
> > > Zookeeper paths for future ACL changes a versioning system is probably
> > > useful.
> > >
> > > @Andy: I've copied you directly in this mail, since you did a bulk of
> > > the work around KIP-290 and mentioned potentially picking up the
> > > follow up work, so I think your input would be very valuable here. Not
> > > trying to shove extra work your way, I'm happy to contribute, but we'd
> > > be touching a lot of the same areas I think.
> > >
> > > If we want to implement a versioning system for ACLs I see the
> > > following todos (probably incomplete & missing something at the same
> > > time):
> > > 1. ensure that the current Authorizer doesn't pick up newer ACLs
> > > 2. add a version marker to new ACLs
> > > 3. change SimpleACLAuthorizer to know what version of ACLs it is
> > > compatible with and only load ACLs of this / smaller version
> > > 4. Decide how to handle if incompatible (newer version) ACLs are
> > > present: log warning, fail broker startup, ...
> > >
> > >
> > > Post-KIP-290 ACLs are stored in two places in Zookeeper:
> > > /kafka-acl-extended - for ACLs with wildcards in the resource
> > > /kafka-acl - for literal ACLs without wildcards (i.e. * means * not
> > > any character)
> > >
> > > To ensure 1 we probably need to move to a new directory once more,
> > > call it /kafka-acl-extended-new for arguments sake. Any ACL stored
> > > here would get a version number stored with it, and only
> > > SimpleAuthorizers that actually know to look here would find these
> > > ACLs and also know to check for a version number. I think Andy
> > > mentioned moving the resource definition in the new ACL format to JSON
> > > instead of simple string in a follow up PR, maybe these pieces of work
> > > are best tackled together - and if a new znode can be avoided even
> > > better.
> > >
> > > This would allow us to recognize situations where ACLs are defined
> > > that not all Authorizers can understand, as those Authorizers would
> > > notice that there are ACLs with a larger version than the one they
> > > support (not applicable to legacy ACLs up until now). How we want to
> > > treat this scenario is up for discussion, I think make it
> > > configurable, as customers have different requirements around
> > > security. Some would probably want to fail a broker that encounters
> > > unknown ACLs so as to not create potential security risks t others
> > > might be happy with just a warning in the logs. This should never
> > > happen, if users fully upgrade their clusters before creating new ACLs
> > > - but to counteract the situation that Colin described it would be
> > > useful.
> > >
> > > Looking forward, a migration option might be added to the kafka-acl
> > > tool to migrate all legacy ACLs once into the new structure once the
> > > user is certain that no old brokers will come online again.
> > >
> > > If you think this sounds like a convoluted way to go about things ...
> > > I agree :) But I couldn't come up with a better way yet.
> > >
> > > Any thoughts?
> > >
> > > Best regards,
> > > Sönke
> > >
> > > [1] https://github.com/apache/kafka/pull/5079#pullrequestreview-124512689
> > >
> > > On Thu, May 3, 2018 at 10:57 PM, Sönke Liebau
> > > <soenke.lie...@opencore.com> wrote:
> > > > Technically I absolutely agree with you, this would indeed create
> > > > issues. If we were just talking about this KIP I think I'd argue that
> > > > it is not too harsh of a requirement for users to refrain from using
> > > > new features until they have fully upgraded their entire cluster. I
> > > > think in that case it could have been solved in the release notes -
> > > > similarly to the way a binary protocol change is handled.
> > > > However looking at the discussion on KIP-290 and thinking ahead to
> > > > potential other changes on ACLs it would really just mean putting off
> > > > a proper solution which is a versioning system for ACLs makes sense.
> > > >
> > > > At least from the point of view of this KIP versioning should be a
> > > > separate KIP as otherwise we don't solve the issue you mentioned above
> > > > - not sure about 290..
> > > >
> > > > I thought about this for a little while, would something like the
> > > > following make sense?
> > > >
> > > > ACLs are either stored in a separate Zookeeper node or get a version
> > > > stored with them (separate node is probably easier). So current ACLs
> > > > would default to v0 and post-KIP252 would be an explicit v1 for
> > > > example.
> > > > Authorizers declare which versions they are compatible with (though
> > > > I'd say i backwards compatibility is what we shoud shoot for) and
> > > > load ACLs of those versions.
> > > > Introduce a new parameter authorizer.acl.maxversion which controls
> > > > which ACLs are loaded by the authorizer - nothing with a version
> > > > higher than specified here gets loaded, even if the Authorizer would
> > > > be able to.
> > > >
> > > > So the process for a cluster update would be similar to a binary
> > > > protocol change, set authorizer.acl.maxversion to new_version - 1.
> > > > Upgrade brokers one by one. Once you are done, change/remove parameter
> > > > and restart cluster.
> > > >
> > > > I'm sure I missed something, but sound good in principle?
> > > >
> > > > Best regards,
> > > > Sönke
> > > >
> > > >
> > > > On Thu, May 3, 2018 at 8:15 PM, Colin McCabe <co...@cmccabe.xyz> wrote:
> > > >> There are still some problems with compatibility here, right?
> > > >>
> > > >> One example is if we construct a DENY ACL with an IP range and then
> > > install it. If all of our brokers have been upgraded, it will work. But
> > > if there are some that still haven't been upgraded, they will not honor
> > > the
> > > DENY ACL, possibly causing a security issue.
> > > >>
> > > >> In general, it seems like we need some kind of versioning system in
> > > ACLs to handle these cases.
> > > >>
> > > >> best,
> > > >> Colin
> > > >>
> > > >> On Thu, May 3, 2018, at 08:11, Sönke Liebau wrote:
> > > >>> Hi all,
> > > >>>
> > > >>> I'd like to readopt this KIP, I got a bit sidetracked by other stuff
> > > >>> after posting the initial version and discussion, sorry for that.
> > > >>>
> > > >>> I've added IPv6 to the KIP, but decided to forego the other scope
> > > >>> extensions that I mentioned in my previous mail, as there are other
> > > >>> efforts underway in KIP-290 that cover most of the suggestions
> > > >>> already.
> > > >>>
> > > >>> Does anybody have any other objections to starting a vote on this KIP?
> > > >>>
> > > >>> Regards,
> > > >>> Sönke
> > > >>>
> > > >>> On Fri, Feb 2, 2018 at 5:11 PM, Sönke Liebau <
> > > soenke.lie...@opencore.com> wrote:
> > > >>> > Hi Manikumar,
> > > >>> >
> > > >>> > you are right, 5713 is a bit ambiguous about which fields are
> > > considered in
> > > >>> > scope, but I agree that wildcards for Ips are not necessary when we
> > > have
> > > >>> > ranges.
> > > >>> >
> > > >>> > I am wondering though, if we might want to extend the scope of this
> > > KIP a
> > > >>> > bit while we are changing acl and authorizer classes anyway.
> > > >>> >
> > > >>> > After considering this a bit on a flihht with no wifi yesterday I
> > > came up
> > > >>> > with the following:
> > > >>> >
> > > >>> > * wildcards or regular expressions for principals, groups and topics
> > > >>> > * extend the KafkaPrincipal object to allow adding custom key-value
> > > pairs in
> > > >>> > principalbuilder implementations
> > > >>> > * extend SimpleAclAuthorizer and the ACL tools to authorize on these
> > > >>> > key/value pairs
> > > >>> >
> > > >>> > The second and third bullet points would allow easy creation of for
> > > example
> > > >>> > a principalbuilder that adds groups the user belongs to in the
> > > >>> > active
> > > >>> > directory to its principal, without requiring the user to also
> > > extend the
> > > >>> > authorizer and create custom ACL storage. This would significantly
> > > lower the
> > > >>> > technical debt incurred by custom authorizer mechanisms I think.
> > > >>> >
> > > >>> > There are a few issues to hash out of course, but I'd think in
> > > general this
> > > >>> > should work work nicely and be a step towards meeting corporate
> > > >>> > authorization requirements.
> > > >>> >
> > > >>> > Best regards,
> > > >>> > Sönke
> > > >>> >
> > > >>> > Am 01.02.2018 18:46 schrieb "Manikumar" <manikumar.re...@gmail.com>:
> > > >>> >
> > > >>> > Hi,
> > > >>> >
> > > >>> > They are few deployments using IPv6. It is good to support IPv6
> > > also.
> > > >>> >
> > > >>> > I think KAFKA-5713 is about adding regular expression support to
> > > resource
> > > >>> > names (topic. consumer etc..).
> > > >>> > Yes, wildcards (*) in hostname doesn't makes sense. Range and subnet
> > > >>> > support will give us the flexibility.
> > > >>> >
> > > >>> > On Thu, Feb 1, 2018 at 5:56 PM, Sönke Liebau <
> > > >>> > soenke.lie...@opencore.com.invalid> wrote:
> > > >>> >
> > > >>> >> Hi Manikumar,
> > > >>> >>
> > > >>> >> the current proposal indeed leaves out IPv6 addresses, as I was
> > > unsure
> > > >>> >> whether Kafka fully supports that yet to be honest. But it would be
> > > >>> >> fairly easy to add these to the proposal - I'll update it over the
> > > >>> >> weekend.
> > > >>> >>
> > > >>> >> Regarding KAFKA-5713, I simply listed it as related, since it is
> > > >>> >> similar in spirit, if not exact wording. Parts of that issue
> > > >>> >> (wildcards in hosts) would be covered by this kip - just in a
> > > slightly
> > > >>> >> different way. Do we really need wildcard support in IP addresses
> > > >>> >> if
> > > >>> >> we can specify ranges and subnets? I considered it, but only came
> > > >>> >> up
> > > >>> >> with scenarios that seemed fairly academic to me, like allowing the
> > > >>> >> same host from multiple subnets (10.0.*.1) for example.
> > > >>> >>
> > > >>> >> Allowing wildcards has the potential to make the code more complex,
> > > >>> >> depending on how we decide to implement this feature, hance I
> > > decided
> > > >>> >> to leave wildcards out for now.
> > > >>> >>
> > > >>> >> What do you think?
> > > >>> >>
> > > >>> >> Best regards,
> > > >>> >> Sönke
> > > >>> >>
> > > >>> >> On Thu, Feb 1, 2018 at 10:14 AM, Manikumar <
> > > manikumar.re...@gmail.com>
> > > >>> >> wrote:
> > > >>> >> > Hi,
> > > >>> >> >
> > > >>> >> > 1. Do we support IPv6 CIDR/ranges?
> > > >>> >> >
> > > >>> >> > 2. KAFKA-5713 is mentioned in Related JIRAs section. But there is
> > > no
> > > >>> >> > mention of wildcard support in the KIP.
> > > >>> >> >
> > > >>> >> >
> > > >>> >> > Thanks,
> > > >>> >> >
> > > >>> >> > On Thu, Feb 1, 2018 at 4:05 AM, Sönke Liebau <
> > > >>> >> > soenke.lie...@opencore.com.invalid> wrote:
> > > >>> >> >
> > > >>> >> >> Hey everybody,
> > > >>> >> >>
> > > >>> >> >> following a brief inital discussion a couple of days ago on this
> > > list
> > > >>> >> >> I'd like to get a discussion going on KIP-252 which would allow
> > > >>> >> >> specifying ip ranges and subnets for the -allow-host and
> > > --deny-host
> > > >>> >> >> parameters of the acl tool.
> > > >>> >> >>
> > > >>> >> >> The KIP can be found at
> > > >>> >> >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > >>> >> >>
> > > 252+-+Extend+ACLs+to+allow+filtering+based+on+ip+ranges+and+subnets
> > > >>> >> >>
> > > >>> >> >> Best regards,
> > > >>> >> >> Sönke
> > > >>> >> >>
> > > >>> >>
> > > >>> >>
> > > >>> >>
> > > >>> >> --
> > > >>> >> Sönke Liebau
> > > >>> >> Partner
> > > >>> >> Tel. +49 179 7940878
> > > >>> >> OpenCore GmbH & Co. KG - Thomas-Mann-Straße 8 - 22880 Wedel -
> > > Germany
> > > >>> >>
> > > >>> >
> > > >>> >
> > > >>>
> > > >>>
> > > >>>
> > > >>> --
> > > >>> Sönke Liebau
> > > >>> Partner
> > > >>> Tel. +49 179 7940878
> > > >>> OpenCore GmbH & Co. KG - Thomas-Mann-Straße 8 - 22880 Wedel - Germany
> > > >
> > > >
> > > >
> > > > --
> > > > Sönke Liebau
> > > > Partner
> > > > Tel. +49 179 7940878
> > > > OpenCore GmbH & Co. KG - Thomas-Mann-Straße 8 - 22880 Wedel - Germany
> > >
> > >
> > >
> > > --
> > > Sönke Liebau
> > > Partner
> > > Tel. +49 179 7940878
> > > OpenCore GmbH & Co. KG - Thomas-Mann-Straße 8 - 22880 Wedel - Germany
> > >
> >
> >
> > --
> > Sönke Liebau
> > Partner
> > Tel. +49 179 7940878
> > OpenCore GmbH & Co. KG - Thomas-Mann-Straße 8 - 22880 Wedel - Germany
--
Sönke Liebau
Partner
Tel. +49 179 7940878
OpenCore GmbH & Co. KG - Thomas-Mann-Straße 8 - 22880 Wedel - Germany
