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
