Skip to content

KAFKA-12866: Avoid root access to Zookeeper#10795

Merged
omkreddy merged 1 commit into
apache:trunkfrom
soarez:zk-acls
Jun 1, 2021
Merged

KAFKA-12866: Avoid root access to Zookeeper#10795
omkreddy merged 1 commit into
apache:trunkfrom
soarez:zk-acls

Conversation

@soarez

@soarez soarez commented May 30, 2021

Copy link
Copy Markdown
Member

https://issues.apache.org/jira/browse/KAFKA-12866

The broker shouldn't assume create access to the chroot. There are
deployement scenarios where the chroot is already created is the only
znode which the broker can access.

To test this, we can use a ZK integration test, and configure zookeeper in the same way the issue is reproduced.

  1. Create the chroot
  2. Set free access to the chroot
  3. Lock down access to the root znode
  4. Try to connect the KafkaZkClient

It should be a separate ZooKeeperTestHarness to avoid leaving the ACL changes made to ZK root visible to other tests.

Rejected alternatives

  • Expect NoAuth in KafkaZkClient.createRecursive and assume NoAuth as success.
  • Create new configuration to set createChrootIfNecessary = false instead of the current non configurable default value of true.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

The broker shouldn't assume create access to the chroot. There are
deployement scenarios where the chroot is already created is the only
znode which the broker can access.
@soarez

soarez commented May 31, 2021

Copy link
Copy Markdown
Member Author

@omkreddy can you have a look at this one?

@omkreddy omkreddy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@soarez Thanks for the PR. LGTM

@omkreddy

Copy link
Copy Markdown
Contributor

cc @rondagostino

@rondagostino

Copy link
Copy Markdown
Contributor

Good catch @soarez, and thanks for the PR! Test failed without the fix and passed with it. Thanks again.

@omkreddy omkreddy merged commit a842e0e into apache:trunk Jun 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants