[DISCUSS] KIP-252: Extend ACLs to allow filtering based on ip ranges and subnets

[Sönke Liebau <so...@opencore.com.INVALID>]

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

Re: [DISCUSS] KIP-252: Extend ACLs to allow filtering based on ip ranges and subnets

[Colin McCabe <cm...@apache.org>]

On Mon, Apr 8, 2019, at 06:38, Sönke Liebau wrote:
> Hi Colin,
> 
> quick summary up front: I totally agree, and always have! I think we
> misunderstood each other a little :)
> I was never really opposed the idea of restricting which ACL features can
> be used, I was just opposed to doing it specifically for this change, while
> not introducing real versioning. But when I suggested a few things around
> how to implement versioning back in January [1] and got no replies on those
> suggestions I figured no one was really interested in that, so I fell back
> to the minimal idea of not doing anything about it. I am absolutely happy
> to pick this up.
> 
> I've done some more digging through the code and feel plenty stupid at the
> moment, because there actually already is an ACL version [2]. Then again,
> good news, we can just use this. Currently loading ACLs from Zookeeper
> fails if the hard-coded version in the authorizer differs from what is
> stored in Zookeeper [3].

Hi Sonke,

Thanks for doing the research!  I remember that we went back and forth on whether we should move this over to JSON when implementing the KIP-290 work.  I didn't remember where we landed.  I am certainly glad we did land on JSON in the end!  Now the last thing to confirm is that all ACLs are created with the "extended" format, even the non-prefix ones.  I remember this being a point of discussion, don't remember where we landed on it.

> Following your idea of using the inter-broker protocol, we probably need to
> create a mapping of IBP -> ACL version somewhere, so we would have
> something like ACL_2 -> KAFKA_2_3 which defines that this version requires
> the IBP to be at least the stated version. When we make a change that
> requires a new ACL version, we simply add a new mapping to the appropriate
> API version and change the hard-coded version value in the authorizer.
> One potential issue that I see with this is that I am unsure if the
> Authorizer will actually have access to the configured IBP version when it
> is running from the kafka-acls.sh tool. I'll look for a way, but I think
> this might actually not be possible, in "legacy" mode, when writing ACLs
> directly to Zookeeper.

Yeah, I don't think it's possible.  In any case, there are no compatibility guarantees when writing directly to ZK.  This is one of the reasons why we're phasing out direct ZK access (KIP-4 has many more)  kafka-acls.sh is kind of unusual in still providing direct ZK access as an option.  We will have to phase that out eventually-- perhaps in the next major release.

> 
> The biggest take away that I have from this is, that we will probably need
> to change the zk path after all. If we do not do this, older brokers using
> the ACLAuthorizer will simply fail to start if someone creates an ACL with
> a newer version. And I am not sure we can actually prohibit them from doing
> that, as mentioned above.

I think the current behavior is OK.  In general, once you bump the IBP, downgrade is not possible any more.  A tool to downgrade clusters is an interesting idea... which doesn't currently exist.  It would be pretty tricky to write because you would need to do things like rewrite logs written in newer message formats, and so on.  Or maybe just delete that data?

best,
Colin

> 
> Maybe an alternative would be to keep the current logic as is, i.e. fail on
> presence of an ACL version != current version and add a "migrate" command
> to the acl commandline tool - at the cost of not being able to downgrade
> again, unless we make the migrate tool bi-directional..
> 
> I am unsure of the best way to be honest, will do some more looking through
> the code and thinking. But thought I'd share this in case you have some
> thoughts.
> 
> Best regards,
> Sönke
> 
> [1]
> https://lists.apache.org/thread.html/4936d5205852d0db19653d17aa9821d4ba30f077aee528bb90955ce4@%3Cdev.kafka.apache.org%3E
> [2]
> https://github.com/apache/kafka/blob/b4fa87cc51f0a7d640dc6ae2cc8f89f006aae652/core/src/main/scala/kafka/security/auth/Acl.scala#L37
> [3]
> https://github.com/apache/kafka/blob/b4fa87cc51f0a7d640dc6ae2cc8f89f006aae652/core/src/main/scala/kafka/security/auth/Acl.scala#L66
> 
> 
> On Fri, Apr 5, 2019 at 8:09 AM Colin McCabe <cm...@apache.org> wrote:
> 
> > On Thu, Apr 4, 2019, at 01:52, Sönke Liebau wrote:
> > > Hi Colin,
> > >
> > > I agree, we need to have a way of failing incorrect ranges server-side,
> > > I'll amend the KIP and look into that. I think INVALID_REQUEST should fit
> > > fine, afaik we can send a message along with that code, so that could
> > > explain the actual reason.
> >
> > Hi Sonke,
> >
> > Sounds good.
> >
> > >
> > > Regarding prohibiting these ACLs from being created before the
> > inter-broker
> > > protocol is updated, I am a bit hesitant about this for two reasons.
> > >
> > > 1. I can't help but feel that we are mixing things in with each other
> > here
> > > that don't really belong together. The broker protocol and ACL versions
> > and
> > > potential ACL versioning are to my mind only loosely linked, because
> > broker
> > > and authorizer are usually updated at the same time. But if we look at
> > > Sentry, Ranger or the Confluent LDAP authorizer I am not sure that this
> > > will hold true forever.
> >
> > I strongly doubt that anyone actually wants to update the authorizer
> > plugins on a separate schedule than the brokers. I worked on Hadoop for a
> > while, so I have some perspective on this. Even in an upgrade scenario, you
> > have to bounce the broker process at some point, and at the point, it's not
> > clear that you gain anything except a headache by bringing it up again with
> > the old authorizer code and the new broker code.
> >
> > > Also, this doesn't address the issue of potential future ACL changes that
> > > might actually be incompatible with existing ACLs. If we go down this
> > road
> > > I think we should go all the way and come up with a full ACL
> > > versioning-scheme. I've written down a few thoughts/suggestions/questions
> > > on that in this thread.
> >
> > I think an ACL versioning scheme is a good idea, but mainly just so that
> > we can provide clean error messages when someone with old software tries to
> > access ACLs in the new format. Right now, we have no way to evolve the
> > format of the data in ZK at all, which is why we used the hack of creating
> > a new path under ZK to implement prefix ACLs.
> >
> > I think it makes sense to "gate" using the new ACL versions on having a
> > specific value of the inter-broker-protocol configured. The thing to keep
> > in mind is that the IBP should not be bumped until all brokers have been
> > upgraded. Once all brokers have been upgraded, using the new ACL version
> > should be OK.
> >
> > > 2. I agree that not preventing this creates a security risk, however in a
> > > very limited scenario.
> > > The same scenario applied when we introduced prefixed-ACLs in version 2
> > and
> > > back then it was not addressed as far as I can tell. The only reference I
> > > can find is one sentence in the KIP that states the risk.
> > > For people using the Confluent LDAP authorizer (or any other authorizer
> > > that is based on the SimpleACLAuthorizer) there is not much we can do, as
> > > once the cluster is updated these ACLs could be created, but the
> > > Authorizers would only honor them after they themselves are upgraded
> > (which
> > > will probably be a rolling restart as well, hence have the same issue).
> >
> > That is a fair point-- in both scenarios, DENY ACLs could cause problems.
> >
> > However, it seems much more likely that people will set up DENY ACLs on a
> > range of IPs than on a topic prefix. Prefix-based security is mainly used
> > to grant permissions, not to revoke them. For example, you might grant
> > someone permission to create topics under "foo_" so that their KSQL jobs
> > can run smoothly, etc. On the other hand, it is natural to think in terms
> > of excluding certain IPs -- iptables is often used this way.
> >
> > Prefix ACLs also exist under a separate ZK path, which at least avoids
> > brokers crashing or getting exceptions if someone creates a prefix ACL and
> > the cluster is partially upgraded.
> >
> > Finally, there was a lot of discussion during the prefix ACLs KIP about
> > the need to fix the versioning issue.
> >
> > >
> > > I am really not trying to duck the work here, but just addressing this
> > > specific change feels like it misses the larger issue to me.
> >
> > I think you make some reasonable points. But we really should not keep
> > kicking the can down the road.
> >
> > Let's just create a JSON structure for ACLs in ZK, similar to the other
> > JSON structures we have there. Put it under a new ZK root if we must. Call
> > what exists today version 0, and make it so that you can't use version 1
> > (the first JSON version) until upgrading the IBP. What do you think?
> >
> > best,
> > Colin
> >
> > >
> > > Best regards,
> > > Sönke
> > >
> > > On Wed, Apr 3, 2019 at 4:36 PM Colin McCabe <cm...@apache.org> wrote:
> > >
> > > > Hi Sönke,
> > > >
> > > > Maybe a reasonable design here would be to not allow creating ACLs
> > based
> > > > on ip ranges and subnets unless the inter-broker protocol setting has
> > been
> > > > upgraded. If an upgrade is done correctly, the IBP should not be
> > upgraded
> > > > until all the brokers have been upgraded, so there shouldn't be older
> > > > brokers in the cluster erroneously giving access to things they
> > shouldn't.
> > > > In that case, perhaps we can hold off on introducing an ACL versioning
> > > > scheme for now.
> > > >
> > > > Another thing that is important here is having some way of rejecting
> > > > malformed ip address ranges in the CreateAcls call. This is probably
> > not
> > > > too difficult, but it should be spelled out. We could use
> > INVALID_REQUEST
> > > > as the error code for this situation, or maybe create a new one to be
> > more
> > > > specific.
> > > >
> > > > best,
> > > > Colin
> > > >
> > > >
> > > > On Wed, Apr 3, 2019, at 04:58, Sönke Liebau wrote:
> > > > > 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.liebau@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.liebau@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 <cm...@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.liebau@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
> > > > > >> > > > <so...@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 <
> > > > colin@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.liebau@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.reddy@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.liebau@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.reddy@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.liebau@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
> > > > >
> > > >
> > >
> > >
> > > --
> > > 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
>

Re: [DISCUSS] KIP-252: Extend ACLs to allow filtering based on ip ranges and subnets

[Sönke Liebau <so...@opencore.com.INVALID>]

Hi Colin,

quick summary up front: I totally agree, and always have! I think we
misunderstood each other a little :)
I was never really opposed the idea of restricting which ACL features can
be used, I was just opposed to doing it specifically for this change, while
not introducing real versioning. But when I suggested a few things around
how to implement versioning back in January [1] and got no replies on those
suggestions I figured no one was really interested in that, so I fell back
to the minimal idea of not doing anything about it. I am absolutely happy
to pick this up.

I've done some more digging through the code and feel plenty stupid at the
moment, because there actually already is an ACL version [2]. Then again,
good news, we can just use this. Currently loading ACLs from Zookeeper
fails if the hard-coded version in the authorizer differs from what is
stored in Zookeeper [3].
Following your idea of using the inter-broker protocol, we probably need to
create a mapping of IBP -> ACL version somewhere, so we would have
something like ACL_2 -> KAFKA_2_3 which defines that this version requires
the IBP to be at least the stated version. When we make a change that
requires a new ACL version, we simply add a new mapping to the appropriate
API version and change the hard-coded version value in the authorizer.
One potential issue that I see with this is that I am unsure if the
Authorizer will actually have access to the configured IBP version when it
is running from the kafka-acls.sh tool. I'll look for a way, but I think
this might actually not be possible, in "legacy" mode, when writing ACLs
directly to Zookeeper.

The biggest take away that I have from this is, that we will probably need
to change the zk path after all. If we do not do this, older brokers using
the ACLAuthorizer will simply fail to start if someone creates an ACL with
a newer version. And I am not sure we can actually prohibit them from doing
that, as mentioned above.

Maybe an alternative would be to keep the current logic as is, i.e. fail on
presence of an ACL version != current version and add a "migrate" command
to the acl commandline tool - at the cost of not being able to downgrade
again, unless we make the migrate tool bi-directional..

I am unsure of the best way to be honest, will do some more looking through
the code and thinking. But thought I'd share this in case you have some
thoughts.

Best regards,
Sönke

[1]
https://lists.apache.org/thread.html/4936d5205852d0db19653d17aa9821d4ba30f077aee528bb90955ce4@%3Cdev.kafka.apache.org%3E
[2]
https://github.com/apache/kafka/blob/b4fa87cc51f0a7d640dc6ae2cc8f89f006aae652/core/src/main/scala/kafka/security/auth/Acl.scala#L37
[3]
https://github.com/apache/kafka/blob/b4fa87cc51f0a7d640dc6ae2cc8f89f006aae652/core/src/main/scala/kafka/security/auth/Acl.scala#L66


On Fri, Apr 5, 2019 at 8:09 AM Colin McCabe <cm...@apache.org> wrote:

> On Thu, Apr 4, 2019, at 01:52, Sönke Liebau wrote:
> > Hi Colin,
> >
> > I agree, we need to have a way of failing incorrect ranges server-side,
> > I'll amend the KIP and look into that. I think INVALID_REQUEST should fit
> > fine, afaik we can send a message along with that code, so that could
> > explain the actual reason.
>
> Hi Sonke,
>
> Sounds good.
>
> >
> > Regarding prohibiting these ACLs from being created before the
> inter-broker
> > protocol is updated, I am a bit hesitant about this for two reasons.
> >
> > 1. I can't help but feel that we are mixing things in with each other
> here
> > that don't really belong together. The broker protocol and ACL versions
> and
> > potential ACL versioning are to my mind only loosely linked, because
> broker
> > and authorizer are usually updated at the same time. But if we look at
> > Sentry, Ranger or the Confluent LDAP authorizer I am not sure that this
> > will hold true forever.
>
> I strongly doubt that anyone actually wants to update the authorizer
> plugins on a separate schedule than the brokers. I worked on Hadoop for a
> while, so I have some perspective on this. Even in an upgrade scenario, you
> have to bounce the broker process at some point, and at the point, it's not
> clear that you gain anything except a headache by bringing it up again with
> the old authorizer code and the new broker code.
>
> > Also, this doesn't address the issue of potential future ACL changes that
> > might actually be incompatible with existing ACLs. If we go down this
> road
> > I think we should go all the way and come up with a full ACL
> > versioning-scheme. I've written down a few thoughts/suggestions/questions
> > on that in this thread.
>
> I think an ACL versioning scheme is a good idea, but mainly just so that
> we can provide clean error messages when someone with old software tries to
> access ACLs in the new format. Right now, we have no way to evolve the
> format of the data in ZK at all, which is why we used the hack of creating
> a new path under ZK to implement prefix ACLs.
>
> I think it makes sense to "gate" using the new ACL versions on having a
> specific value of the inter-broker-protocol configured. The thing to keep
> in mind is that the IBP should not be bumped until all brokers have been
> upgraded. Once all brokers have been upgraded, using the new ACL version
> should be OK.
>
> > 2. I agree that not preventing this creates a security risk, however in a
> > very limited scenario.
> > The same scenario applied when we introduced prefixed-ACLs in version 2
> and
> > back then it was not addressed as far as I can tell. The only reference I
> > can find is one sentence in the KIP that states the risk.
> > For people using the Confluent LDAP authorizer (or any other authorizer
> > that is based on the SimpleACLAuthorizer) there is not much we can do, as
> > once the cluster is updated these ACLs could be created, but the
> > Authorizers would only honor them after they themselves are upgraded
> (which
> > will probably be a rolling restart as well, hence have the same issue).
>
> That is a fair point-- in both scenarios, DENY ACLs could cause problems.
>
> However, it seems much more likely that people will set up DENY ACLs on a
> range of IPs than on a topic prefix. Prefix-based security is mainly used
> to grant permissions, not to revoke them. For example, you might grant
> someone permission to create topics under "foo_" so that their KSQL jobs
> can run smoothly, etc. On the other hand, it is natural to think in terms
> of excluding certain IPs -- iptables is often used this way.
>
> Prefix ACLs also exist under a separate ZK path, which at least avoids
> brokers crashing or getting exceptions if someone creates a prefix ACL and
> the cluster is partially upgraded.
>
> Finally, there was a lot of discussion during the prefix ACLs KIP about
> the need to fix the versioning issue.
>
> >
> > I am really not trying to duck the work here, but just addressing this
> > specific change feels like it misses the larger issue to me.
>
> I think you make some reasonable points. But we really should not keep
> kicking the can down the road.
>
> Let's just create a JSON structure for ACLs in ZK, similar to the other
> JSON structures we have there. Put it under a new ZK root if we must. Call
> what exists today version 0, and make it so that you can't use version 1
> (the first JSON version) until upgrading the IBP. What do you think?
>
> best,
> Colin
>
> >
> > Best regards,
> > Sönke
> >
> > On Wed, Apr 3, 2019 at 4:36 PM Colin McCabe <cm...@apache.org> wrote:
> >
> > > Hi Sönke,
> > >
> > > Maybe a reasonable design here would be to not allow creating ACLs
> based
> > > on ip ranges and subnets unless the inter-broker protocol setting has
> been
> > > upgraded. If an upgrade is done correctly, the IBP should not be
> upgraded
> > > until all the brokers have been upgraded, so there shouldn't be older
> > > brokers in the cluster erroneously giving access to things they
> shouldn't.
> > > In that case, perhaps we can hold off on introducing an ACL versioning
> > > scheme for now.
> > >
> > > Another thing that is important here is having some way of rejecting
> > > malformed ip address ranges in the CreateAcls call. This is probably
> not
> > > too difficult, but it should be spelled out. We could use
> INVALID_REQUEST
> > > as the error code for this situation, or maybe create a new one to be
> more
> > > specific.
> > >
> > > best,
> > > Colin
> > >
> > >
> > > On Wed, Apr 3, 2019, at 04:58, Sönke Liebau wrote:
> > > > 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.liebau@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.liebau@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 <cm...@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.liebau@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
> > > > >> > > > <so...@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 <
> > > colin@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.liebau@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.reddy@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.liebau@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.reddy@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.liebau@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
> > > >
> > >
> >
> >
> > --
> > 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

Re: [DISCUSS] KIP-252: Extend ACLs to allow filtering based on ip ranges and subnets

[Colin McCabe <cm...@apache.org>]

On Thu, Apr 4, 2019, at 01:52, Sönke Liebau wrote:
> Hi Colin,
> 
> I agree, we need to have a way of failing incorrect ranges server-side,
> I'll amend the KIP and look into that. I think INVALID_REQUEST should fit
> fine, afaik we can send a message along with that code, so that could
> explain the actual reason.

Hi Sonke,

Sounds good.

> 
> Regarding prohibiting these ACLs from being created before the inter-broker
> protocol is updated, I am a bit hesitant about this for two reasons.
> 
> 1. I can't help but feel that we are mixing things in with each other here
> that don't really belong together. The broker protocol and ACL versions and
> potential ACL versioning are to my mind only loosely linked, because broker
> and authorizer are usually updated at the same time. But if we look at
> Sentry, Ranger or the Confluent LDAP authorizer I am not sure that this
> will hold true forever.

I strongly doubt that anyone actually wants to update the authorizer plugins on a separate schedule than the brokers. I worked on Hadoop for a while, so I have some perspective on this. Even in an upgrade scenario, you have to bounce the broker process at some point, and at the point, it's not clear that you gain anything except a headache by bringing it up again with the old authorizer code and the new broker code.

> Also, this doesn't address the issue of potential future ACL changes that
> might actually be incompatible with existing ACLs. If we go down this road
> I think we should go all the way and come up with a full ACL
> versioning-scheme. I've written down a few thoughts/suggestions/questions
> on that in this thread.

I think an ACL versioning scheme is a good idea, but mainly just so that we can provide clean error messages when someone with old software tries to access ACLs in the new format. Right now, we have no way to evolve the format of the data in ZK at all, which is why we used the hack of creating a new path under ZK to implement prefix ACLs.

I think it makes sense to "gate" using the new ACL versions on having a specific value of the inter-broker-protocol configured. The thing to keep in mind is that the IBP should not be bumped until all brokers have been upgraded. Once all brokers have been upgraded, using the new ACL version should be OK.

> 2. I agree that not preventing this creates a security risk, however in a
> very limited scenario.
> The same scenario applied when we introduced prefixed-ACLs in version 2 and
> back then it was not addressed as far as I can tell. The only reference I
> can find is one sentence in the KIP that states the risk.
> For people using the Confluent LDAP authorizer (or any other authorizer
> that is based on the SimpleACLAuthorizer) there is not much we can do, as
> once the cluster is updated these ACLs could be created, but the
> Authorizers would only honor them after they themselves are upgraded (which
> will probably be a rolling restart as well, hence have the same issue).

That is a fair point-- in both scenarios, DENY ACLs could cause problems.

However, it seems much more likely that people will set up DENY ACLs on a range of IPs than on a topic prefix. Prefix-based security is mainly used to grant permissions, not to revoke them. For example, you might grant someone permission to create topics under "foo_" so that their KSQL jobs can run smoothly, etc. On the other hand, it is natural to think in terms of excluding certain IPs -- iptables is often used this way.

Prefix ACLs also exist under a separate ZK path, which at least avoids brokers crashing or getting exceptions if someone creates a prefix ACL and the cluster is partially upgraded.

Finally, there was a lot of discussion during the prefix ACLs KIP about the need to fix the versioning issue.

> 
> I am really not trying to duck the work here, but just addressing this
> specific change feels like it misses the larger issue to me.

I think you make some reasonable points. But we really should not keep kicking the can down the road.

Let's just create a JSON structure for ACLs in ZK, similar to the other JSON structures we have there. Put it under a new ZK root if we must. Call what exists today version 0, and make it so that you can't use version 1 (the first JSON version) until upgrading the IBP. What do you think?

best,
Colin

> 
> Best regards,
> Sönke
> 
> On Wed, Apr 3, 2019 at 4:36 PM Colin McCabe <cm...@apache.org> wrote:
> 
> > Hi Sönke,
> >
> > Maybe a reasonable design here would be to not allow creating ACLs based
> > on ip ranges and subnets unless the inter-broker protocol setting has been
> > upgraded. If an upgrade is done correctly, the IBP should not be upgraded
> > until all the brokers have been upgraded, so there shouldn't be older
> > brokers in the cluster erroneously giving access to things they shouldn't.
> > In that case, perhaps we can hold off on introducing an ACL versioning
> > scheme for now.
> >
> > Another thing that is important here is having some way of rejecting
> > malformed ip address ranges in the CreateAcls call. This is probably not
> > too difficult, but it should be spelled out. We could use INVALID_REQUEST
> > as the error code for this situation, or maybe create a new one to be more
> > specific.
> >
> > best,
> > Colin
> >
> >
> > On Wed, Apr 3, 2019, at 04:58, Sönke Liebau wrote:
> > > 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.liebau@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.liebau@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 <cm...@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.liebau@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
> > > >> > > > <so...@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 <
> > colin@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.liebau@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.reddy@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.liebau@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.reddy@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.liebau@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
> > >
> >
> 
> 
> -- 
> Sönke Liebau
> Partner
> Tel. +49 179 7940878
> OpenCore GmbH & Co. KG - Thomas-Mann-Straße 8 - 22880 Wedel - Germany
>

Re: [DISCUSS] KIP-252: Extend ACLs to allow filtering based on ip ranges and subnets

[Sönke Liebau <so...@opencore.com.INVALID>]

Hi Colin,

I agree, we need to have a way of failing incorrect ranges server-side,
I'll amend the KIP and look into that. I think INVALID_REQUEST should fit
fine, afaik we can send a message along with that code, so that could
explain the actual reason.

Regarding prohibiting these ACLs from being created before the inter-broker
protocol is updated, I am a bit hesitant about this for two reasons.

1. I can't help but feel that we are mixing things in with each other here
that don't really belong together. The broker protocol and ACL versions and
potential ACL versioning are to my mind only loosely linked, because broker
and authorizer are usually updated at the same time. But if we look at
Sentry, Ranger or the Confluent LDAP authorizer I am not sure that this
will hold true forever.
Also, this doesn't address the issue of potential future ACL changes that
might actually be incompatible with existing ACLs.  If we go down this road
I think we should go all the way and come up with a full ACL
versioning-scheme. I've written down a few thoughts/suggestions/questions
on that in this thread.

2. I agree that not preventing this creates a security risk, however in a
very limited scenario.
The same scenario applied when we introduced prefixed-ACLs in version 2 and
back then it was not addressed as far as I can tell. The only reference I
can find is one sentence in the KIP that states the risk.
For people using the Confluent LDAP authorizer (or any other authorizer
that is based on the SimpleACLAuthorizer) there is not much we can do, as
once the cluster is updated these ACLs could be created, but the
Authorizers would only honor them after they themselves are upgraded (which
will probably be a rolling restart as well, hence have the same issue).

I am really not trying to duck the work here, but just addressing this
specific change feels like it misses the larger issue to me.

Best regards,
Sönke

On Wed, Apr 3, 2019 at 4:36 PM Colin McCabe <cm...@apache.org> wrote:

> Hi Sönke,
>
> Maybe a reasonable design here would be to not allow creating ACLs based
> on ip ranges and subnets unless the inter-broker protocol setting has been
> upgraded.  If an upgrade is done correctly, the IBP should not be upgraded
> until all the brokers have been upgraded, so there shouldn't be older
> brokers in the cluster erroneously giving access to things they shouldn't.
> In that case, perhaps we can hold off on introducing an ACL versioning
> scheme for now.
>
> Another thing that is important here is having some way of rejecting
> malformed ip address ranges in the CreateAcls call.  This is probably not
> too difficult, but it should be spelled out.  We could use INVALID_REQUEST
> as the error code for this situation, or maybe create a new one to be more
> specific.
>
> best,
> Colin
>
>
> On Wed, Apr 3, 2019, at 04:58, Sönke Liebau wrote:
> > 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.liebau@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.liebau@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 <cm...@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.liebau@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
> > >> > > > <so...@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 <
> colin@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.liebau@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.reddy@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.liebau@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.reddy@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.liebau@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
> >
>


-- 
Sönke Liebau
Partner
Tel. +49 179 7940878
OpenCore GmbH & Co. KG - Thomas-Mann-Straße 8 - 22880 Wedel - Germany

Re: [DISCUSS] KIP-252: Extend ACLs to allow filtering based on ip ranges and subnets

[Colin McCabe <cm...@apache.org>]

Hi Sönke,

Maybe a reasonable design here would be to not allow creating ACLs based on ip ranges and subnets unless the inter-broker protocol setting has been upgraded.  If an upgrade is done correctly, the IBP should not be upgraded until all the brokers have been upgraded, so there shouldn't be older brokers in the cluster erroneously giving access to things they shouldn't.  In that case, perhaps we can hold off on introducing an ACL versioning scheme for now.

Another thing that is important here is having some way of rejecting malformed ip address ranges in the CreateAcls call.  This is probably not too difficult, but it should be spelled out.  We could use INVALID_REQUEST as the error code for this situation, or maybe create a new one to be more specific.

best,
Colin


On Wed, Apr 3, 2019, at 04:58, Sönke Liebau wrote:
> 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 <so...@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 <so...@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 <cm...@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.liebau@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
> >> > > > <so...@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.liebau@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.reddy@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.liebau@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.reddy@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.liebau@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
>

Re: [DISCUSS] KIP-252: Extend ACLs to allow filtering based on ip ranges and subnets

[Sönke Liebau <so...@opencore.com.INVALID>]

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 <so...@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 <so...@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 <cm...@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.liebau@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
>> > > > <so...@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.liebau@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.reddy@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.liebau@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.reddy@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.liebau@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

Re: [DISCUSS] KIP-252: Extend ACLs to allow filtering based on ip ranges and subnets

[Sönke Liebau <so...@opencore.com.INVALID>]

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 <so...@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 <cm...@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.liebau@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
> > > > <so...@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.liebau@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.reddy@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.liebau@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.reddy@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.liebau@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

Re: [DISCUSS] KIP-252: Extend ACLs to allow filtering based on ip ranges and subnets

[Sönke Liebau <so...@opencore.com.INVALID>]

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 <cm...@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 <so...@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
> > > <so...@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.liebau@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" <ma...@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.liebau@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.reddy@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.liebau@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

Re: [DISCUSS] KIP-252: Extend ACLs to allow filtering based on ip ranges and subnets

[Colin McCabe <cm...@apache.org>]

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 <so...@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
> > <so...@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.liebau@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" <ma...@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.liebau@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.reddy@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.liebau@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

Re: [DISCUSS] KIP-252: Extend ACLs to allow filtering based on ip ranges and subnets

[Sönke Liebau <so...@opencore.com.INVALID>]

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 <so...@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
> <so...@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.liebau@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" <ma...@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.liebau@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.reddy@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.liebau@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

Re: [DISCUSS] KIP-252: Extend ACLs to allow filtering based on ip ranges and subnets

[Sönke Liebau <so...@opencore.com.INVALID>]

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
<so...@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 <so...@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" <ma...@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.liebau@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 <ma...@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.liebau@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

Re: [DISCUSS] KIP-252: Extend ACLs to allow filtering based on ip ranges and subnets

[Sönke Liebau <so...@opencore.com.INVALID>]

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 <so...@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" <ma...@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.liebau@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 <ma...@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.liebau@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

Re: [DISCUSS] KIP-252: Extend ACLs to allow filtering based on ip ranges and subnets

[Colin McCabe <co...@cmccabe.xyz>]

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 <so...@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" <ma...@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.liebau@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 <ma...@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.liebau@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

Re: [DISCUSS] KIP-252: Extend ACLs to allow filtering based on ip ranges and subnets

[Sönke Liebau <so...@opencore.com.INVALID>]

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 <so...@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" <ma...@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.liebau@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 <ma...@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.liebau@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

Re: [DISCUSS] KIP-252: Extend ACLs to allow filtering based on ip ranges and subnets

[Sönke Liebau <so...@opencore.com.INVALID>]

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" <ma...@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.liebau@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 <ma...@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.liebau@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
>

Re: [DISCUSS] KIP-252: Extend ACLs to allow filtering based on ip ranges and subnets

[Manikumar <ma...@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.liebau@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 <ma...@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.liebau@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
>

Re: [DISCUSS] KIP-252: Extend ACLs to allow filtering based on ip ranges and subnets

[Sönke Liebau <so...@opencore.com.INVALID>]

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 <ma...@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.liebau@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

Re: [DISCUSS] KIP-252: Extend ACLs to allow filtering based on ip ranges and subnets

[Manikumar <ma...@gmail.com>]

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.liebau@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
>