RANGER-5787 : Refactoring the security zone API - #1229
dhavalshah9131 wants to merge 4 commits into
Conversation
| if (bizUtil.isAdmin()) { | ||
| blockAdminFromKMSService(submittedZone); | ||
|
|
||
| if (zoneId != null) { |
There was a problem hiding this comment.
The existing-zone check here blocks the only way to fix a zone that already contains a KMS service (e.g. one created on an older build before this check existed). Delete is blocked below, and ROLE_KEY_ADMIN can't operate on zones, so an admin who submits this zone with the KMS service removed is still rejected — the zone can't be edited or deleted through the API.
The check also adds nothing: if the submitted zone still contains the KMS service, blockAdminFromKMSService(submittedZone) catches it; if it doesn't, that's the removal we want to allow. Suggest checking only the submitted zone on update, dropping the securityZoneStore.getSecurityZone(zoneId) read, and adding a test that an admin can remove a KMS service from an existing zone (PUT without KMS → DELETE should then succeed).
There was a problem hiding this comment.
Walk through it with a concrete scenario.
The scenario. A cluster running Ranger 2.9 has zone Z1, and at some point an admin used PUT /zones/{id} (which had no KMS check) to add dev_kms to it. So the DB now has Z1.services = {dev_hive, dev_kms}. Now the cluster upgrades to a build with this PR.
What happens under the PR's code. The admin realises dev_kms shouldn't be there and tries to remove it: PUT /zones/Z1 with body services = {dev_hive} (no KMS). blockAdminFromKMSServiceOnUpdate runs:
blockAdminFromKMSService(submittedZone) — submitted has only dev_hive, passes.
existingZone = securityZoneStore.getSecurityZone(zoneId) — loads the DB copy, which still has dev_kms.
blockAdminFromKMSService(existingZone) — finds dev_kms, throws "KMS Services/Service-Defs are not accessible for Zone operations".
So the removal is rejected. The admin then tries DELETE /zones/Z1 — blockAdminFromKMSServiceOnDelete loads the same DB copy, finds dev_kms, throws. Nobody else can help: ensureAdminAccess() requires ROLE_SYS_ADMIN, so a key admin can't update or delete zones at all. Z1 is now permanently frozen — can't be edited, can't be deleted — and dev_kms stays zoned, which is exactly the state the fix is meant to prevent. The only way out is editing x_security_zone in the DB by hand.
Why the existing-zone check adds nothing. Consider the two possible updates an admin can send for Z1:
Submitted zone still contains dev_kms → step 1 already throws. Step 3 never runs.
Submitted zone doesn't contain dev_kms → this is the admin removing it. We want this to succeed. Step 3 is the only thing stopping it.
So step 3 never blocks anything step 1 didn't already block; its only effect is to prevent removal.
What changes were proposed in this pull request?
Improvement and refactoring the security zone API
How was this patch tested?
Successfully validated the Security Zone API.
Build succeeded with unit tests