iloveeyjafjalla opened a new issue, #11418: URL: https://github.com/apache/rocketmq/issues/11418
### Before Creating the Bug Report - [x] I found a bug, not just asking a question, which should be created in [GitHub Discussions](https://github.com/apache/rocketmq/discussions). - [x] I have searched the [GitHub Issues](https://github.com/apache/rocketmq/issues) and [GitHub Discussions](https://github.com/apache/rocketmq/discussions) of this repository and believe that this is not a duplicate. - [x] I have confirmed that this bug belongs to the current repository, not other repositories of RocketMQ. ### Runtime platform environment OS: any (broker started with the ACL 2.0 migration switch on: `migrateAuthFromV1Enabled=true`; the migration runs at startup from `BrokerController.java:500-501`) ### RocketMQ version branch: develop Git commit id: a7edeecfb ### JDK Version Compiler: Temurin 1.8.0_504 (target 1.8, same as CI) ### Describe the Bug `AuthMigrator` turns every permission string it does not recognize into an **allow-all** policy entry. ```java private Decision parseDecision(String str) { if (StringUtils.isBlank(str)) { return Decision.DENY; } return StringUtils.equals(str, AclConstants.DENY) ? Decision.DENY : Decision.ALLOW; // everything else allows } private List<Action> parseActions(String str) { List<Action> result = new ArrayList<>(); ... default: result.add(Action.ALL); // everything unrecognized gets every action break; } ``` ACL 1.0's own handling was fail-closed in both directions: `Permission.parsePermFromString` returned `Permission.DENY` for `null` and in its `default` branch for every string outside `PUB`, `SUB`, `PUB|SUB`, `SUB|PUB`, `DENY`, and its validator `checkResourcePerms` even refused such a config loudly (`Parse resource permission error for X. The expected permissions are 'SUB' or 'PUB' or 'SUB|PUB' or 'PUB|SUB'.`). The class was removed with RIP-68, but it is the implementation this migration replaces and is still visible in history (`git show ade18bb07^:acl/src/main/java/org/apache/rocketmq/acl/common/Permission.java`). So a v1 entry that denied a resource — or a v1 config that ACL 1.0 refused to load — is migrated into an entry that allows every action on that resource. That matters for values real configurations contain. `docs/en/acl/Operations_ACL.md:10-16` lists `ANY` in the permission table ("PUB or SUB permission"), so `topicPerms: - topicA=ANY` is a plausible v1 entry — ACL 1.0 parsed it as `DENY` for `topicA` (it is not one of the five accepted strings), while the migration produces `Topic:topicA` with actions `[ALL]` and decision `ALLOW`. The same happens for a typo (`topicA=PUBB`) or any other unrecognized token. ### Steps to Reproduce 1. Keep an ACL 1.0 `plain_acl.yml` with an entry outside the documented set, e.g. ```yaml accounts: - accessKey: ak secretKey: sk topicPerms: - topicA=ANY ``` 2. Start the broker with `migrateAuthFromV1Enabled=true` (and the ACL 2.0 metadata provider configured), so `new AuthMigrator(authConfig).migrate()` runs (`BrokerController.java:500-501`). 3. Inspect the ACL created for `ak`: the `topicA` entry now carries `Action.ALL` with `Decision.ALLOW` — the user is authorized for every action on `topicA` although ACL 1.0 denied it. At the unit level the same is reproduced with the existing `AuthMigratorTest` harness: feed a `PlainAccessConfig` whose `topicPerms` contains `topicA=ANY`, capture the `Acl` passed to `AuthorizationMetadataManager#createAcl` and assert the entry's decision. ### What Did You Expect to See? The migration preserves ACL 1.0 semantics: only the recognized allow values (`PUB`, `SUB`, `PUB|SUB`, `SUB|PUB`) produce an `ALLOW` entry, `DENY` produces a deny entry, and any other value leaves the resource denied (as `Permission.parsePermFromString` did) instead of granting `Action.ALL`. ### What Did You See Instead? An unrecognized permission becomes `Action.ALL` with `Decision.ALLOW`, i.e. a fail-open escalation of an entry that was a denial under ACL 1.0. In-tree, the sibling converter `broker/.../auth/converter/AclConverter.java:55-58` shows the intended handling of unknown action names (it skips them instead of granting `ALL`), and the migrator itself already pairs `ALL` with `DENY` in its `AclConstants.DENY` case, so the deny-shape is the established one. ### Additional Context Minimal fix: mirror `Permission.parsePermFromString` in `parseDecision` — return `Decision.ALLOW` only for `PUB`, `SUB`, `PUB|SUB`, `SUB|PUB`, and `Decision.DENY` otherwise (blank, `DENY`, unknown). `parseActions` can keep `Action.ALL` for its default case, because that list is what a deny entry should carry (exactly like the existing `DENY` case), so an unrecognized value becomes a deny-everything entry for that resource — the ACL 1.0 behaviour. I will open a PR with the change plus a regression test in `AuthMigratorTest` that migrates `topicA=ANY` and asserts the migrated entry does not authorize anything (red before the change: the captured entry has `Decision.ALLOW`). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
