Skip to content

RANGER-5787 : Refactoring the security zone API - #1229

Open
dhavalshah9131 wants to merge 4 commits into
masterfrom
ds_RANGER-5787
Open

dhavalshah9131 wants to merge 4 commits into
masterfrom
ds_RANGER-5787

Conversation

@dhavalshah9131

Copy link
Copy Markdown
Contributor

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

Comment thread security-admin/src/main/java/org/apache/ranger/rest/SecurityZoneREST.java Outdated
if (bizUtil.isAdmin()) {
blockAdminFromKMSService(submittedZone);

if (zoneId != null) {

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.

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).

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.

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.

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