vyommani commented on code in PR #1229:
URL: https://github.com/apache/ranger/pull/1229#discussion_r4032857078
##########
security-admin/src/main/java/org/apache/ranger/rest/SecurityZoneREST.java:
##########
@@ -695,20 +697,60 @@ private void blockAdminFromKMSService(RangerSecurityZone
securityZone) {
if (serviceMap != null) {
for (String serviceName : serviceMap.keySet()) {
- XXService xService =
daoManager.getXXService().findByName(serviceName);
-
- if (xService != null) {
- XXServiceDef xServiceDef =
daoManager.getXXServiceDef().getById(xService.getType());
+ String serviceType =
daoManager.getXXServiceDef().findServiceDefTypeByServiceName(serviceName);
- if
(EmbeddedServiceDefsUtil.KMS_IMPL_CLASS_NAME.equals(xServiceDef.getImplclassname()))
{
- throw restErrorUtil.createRESTException("KMS
Services/Service-Defs are not accessible for Zone operations",
MessageEnums.OPER_NOT_ALLOWED_FOR_ENTITY);
- }
+ if
(EmbeddedServiceDefsUtil.EMBEDDED_SERVICEDEF_KMS_NAME.equals(serviceType)) {
+ throw restErrorUtil.createRESTException("KMS
Services/Service-Defs are not accessible for Zone operations",
MessageEnums.OPER_NOT_ALLOWED_FOR_ENTITY);
}
}
}
}
}
+ private void blockAdminFromKMSServiceOnUpdate(RangerSecurityZone
submittedZone, Long zoneId) {
+ if (bizUtil.isAdmin()) {
+ blockAdminFromKMSService(submittedZone);
+
+ if (zoneId != null) {
Review 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.
--
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]
