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
