All,
as this thread has now been dormant for about three months again I'll am
willing to consider the attempt at looking at a larger versioning scheme
for ACLs as failed.
I am away for a long weekend tomorrow and will start a [VOTE] thread on
implementing this as is on Monday, as I personally consider the security
implications of these ACLs in a mixed version cluster quite minimal and
addressable via the release notes.
Best,
Sönke
On Sat, Mar 16, 2019 at 1:32 PM Sönke Liebau <soenke.lie...@opencore.com>
wrote:
> Just a quick bump, as this has been quiet for a while again.
>
> On Tue, Jan 8, 2019 at 12:44 PM Sönke Liebau <soenke.lie...@opencore.com>
> wrote:
>
>> 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
>>
>
>
> --
> 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
