Skip to content

KAFKA-13983: Fail the creation with "/" in resource name in zk ACL#12359

Merged
omkreddy merged 5 commits intoapache:trunkfrom
singhnama:KAFKA-13983
Jul 8, 2022
Merged

KAFKA-13983: Fail the creation with "/" in resource name in zk ACL#12359
omkreddy merged 5 commits intoapache:trunkfrom
singhnama:KAFKA-13983

Conversation

@singhnama
Copy link
Copy Markdown
Contributor

@singhnama singhnama commented Jun 29, 2022 •
edited
Loading

Problem
Sanitizing the resource name have a compatibility issue with already existing ACLs, and also there are very few special characters which are not allowed by the zookeeper which can be found here so we decided to fail the creation with / in the resource name since this is allowed by zookeeper but it's creating the problem in acl operation.

Not allowed characters by zookeeper:

The null character (\u0000) cannot be part of a path name. (This causes problems with the C binding.)

The following characters can't be used because they don't display well, or render in confusing ways: \u0001 - \u0019 and \u007F - \u009F.

The following characters are not allowed: \ud800 -uF8FFF, \uFFF0-uFFFF, \uXFFFE - \uXFFFF (where X is a digit 1 - E), \uF0000 - \uFFFFF.

The "." character can be used as part of another name, but "." and ".." cannot alone be used to indicate a node along a path, because ZooKeeper doesn't use relative paths. The following would be invalid: "/a/b/./c" or "/a/b/../c".

The token "zookeeper" is reserved.

hachikuji
hachikuji reviewed Jul 1, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

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

Thanks for the fix. Are there any test cases we can add?

@singhnama singhnama marked this pull request as ready for review July 6, 2022 10:17
@singhnama
Copy link
Copy Markdown
Contributor Author

singhnama commented Jul 6, 2022 •
edited
Loading

Sanitizing the resource name have a compatibility issue with already existing ACLs, and also there are very few special characters which are not allowed by the zookeeper which can be found here https://zookeeper.apache.org/doc/r3.1.2/zookeeperProgrammers.html so we decided to fail the creation with / in the resource name since this is allowed by zookeeper but it's creating the problem.

Not allowed characters:

The null character (\u0000) cannot be part of a path name. (This causes problems with the C binding.)

The following characters can't be used because they don't display well, or render in confusing ways: \u0001 - \u0019 and \u007F - \u009F.

The following characters are not allowed: \ud800 -uF8FFF, \uFFF0-uFFFF, \uXFFFE - \uXFFFF (where X is a digit 1 - E), \uF0000 - \uFFFFF.

The "." character can be used as part of another name, but "." and ".." cannot alone be used to indicate a node along a path, because ZooKeeper doesn't use relative paths. The following would be invalid: "/a/b/./c" or "/a/b/../c".

The token "zookeeper" is reserved.

@singhnama singhnama changed the title KAFKA-13983: Support special character in Resource name in ACLs operation by sanitizing KAFKA-13983: Fail the creation with "/" in resource name in zk ACL Jul 6, 2022
omkreddy
omkreddy reviewed Jul 7, 2022
View reviewed changes
Comment thread core/src/main/scala/kafka/security/authorizer/AclAuthorizer.scala Outdated
throw new UnsupportedVersionException(s"Adding ACLs on prefixed resource patterns requires " +
s"${KafkaConfig.InterBrokerProtocolVersionProp} of $IBP_2_0_IV1 or greater")
}
if (inValidAclBindingResourceName(aclBinding.pattern().name())) {
Copy link
Copy Markdown
Contributor

@omkreddy omkreddy Jul 7, 2022

Choose a reason for hiding this comment

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

Can we this to validateAclBinding method?

Copy link
Copy Markdown
Contributor Author

@singhnama singhnama Jul 7, 2022

Choose a reason for hiding this comment

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

updated

omkreddy
omkreddy approved these changes Jul 8, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@omkreddy omkreddy left a comment

Choose a reason for hiding this comment

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

@singhnama Thanks for the PR. LGTM.

@omkreddy omkreddy merged commit dc6f555 into apache:trunk Jul 8, 2022
ijuma added a commit to confluentinc/kafka that referenced this pull request Aug 3, 2022
@ijuma
Merge up to the Apache Kafka 3.3 branching point
affe8cd 
> $ git merge-base apache-github/3.3 apache-github/trunk
> 23c92ce

> $ git show 23c92ce
> commit 23c92ce
> Author: SC <pch838811@gmail.com>
> Date:   Mon Jul 11 11:36:56 2022 +0900
>
>    MINOR: Use String#format for niceMemoryUnits result (apache#12389)
>
>    Reviewers: Luke Chen <showuon@gmail.com>, Divij Vaidya <diviv@amazon.com>

* commit '23c92ce79366e86ca719e5e51c550c27324acd83':
  MINOR: Use String#format for niceMemoryUnits result (apache#12389)
  KAFKA-14055; Txn markers should not be removed by matching records in the offset map (apache#12390)
  KAFKA-13474: Allow reconfiguration of SSL certs for broker to controller connection (apache#12381)
  KAFKA-13996: log.cleaner.io.max.bytes.per.second can be changed dynamically (apache#12296)
  KAFKA-13983: Fail the creation with "/" in resource name in zk ACL (apache#12359)
  KAFKA-12943: update aggregating documentation (apache#12091)
  KAFKA-13846: Follow up PR to address review comments (apache#12297)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hachikuji hachikuji hachikuji left review comments

@omkreddy omkreddy omkreddy approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@singhnama @omkreddy @hachikuji