You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ranger.apache.org by ab...@apache.org on 2021/11/24 16:02:45 UTC
[ranger] branch master updated: RANGER-3490: Make policy resource signature is unique in a service
This is an automated email from the ASF dual-hosted git repository.
abhay pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/ranger.git
The following commit(s) were added to refs/heads/master by this push:
new 856571c RANGER-3490: Make policy resource signature is unique in a service
856571c is described below
commit 856571c4348e31725498c0922338339c76ebba02
Author: Abhay Kulkarni <ab...@apache.org>
AuthorDate: Wed Nov 24 07:38:20 2021 -0800
RANGER-3490: Make policy resource signature is unique in a service
---
.../model/RangerPolicyResourceSignature.java | 5 ++++
.../model/validation/RangerPolicyValidator.java | 35 ++++++++++------------
.../plugin/model/validation/RangerValidator.java | 21 +++++++++----
.../model/TestRangerPolicyResourceSignature.java | 18 +++++++++++
.../validation/TestRangerPolicyValidator.java | 24 +++++++++------
.../model/validation/TestRangerValidator.java | 4 +--
.../java/org/apache/ranger/biz/ServiceDBStore.java | 30 ++++++++++++-------
.../org/apache/ranger/biz/TestServiceDBStore.java | 4 +++
8 files changed, 95 insertions(+), 46 deletions(-)
diff --git a/agents-common/src/main/java/org/apache/ranger/plugin/model/RangerPolicyResourceSignature.java b/agents-common/src/main/java/org/apache/ranger/plugin/model/RangerPolicyResourceSignature.java
index 312005e..c84d0bc 100644
--- a/agents-common/src/main/java/org/apache/ranger/plugin/model/RangerPolicyResourceSignature.java
+++ b/agents-common/src/main/java/org/apache/ranger/plugin/model/RangerPolicyResourceSignature.java
@@ -121,6 +121,8 @@ public class RangerPolicyResourceSignature {
LOG.debug("isPolicyValidForResourceSignatureComputation: resources collection on policy was null!");
} else if (_policy.getResources().containsKey(null)) {
LOG.debug("isPolicyValidForResourceSignatureComputation: resources collection has resource with null name!");
+ } else if (StringUtils.isEmpty(_policy.getGuid())) {
+ LOG.debug("isPolicyValidForResourceSignatureComputation: policy GUID is empty!");
} else {
valid = true;
}
@@ -163,6 +165,9 @@ public class RangerPolicyResourceSignature {
CustomConditionSerialiser customConditionSerialiser = new CustomConditionSerialiser(_policy.getConditions());
resource += customConditionSerialiser.toString();
}
+ if (!_policy.getIsEnabled()) {
+ resource += _policy.getGuid();
+ }
String result = String.format("{version=%d,type=%d,resource=%s}", _SignatureVersion, type, resource);
return result;
diff --git a/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerPolicyValidator.java b/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerPolicyValidator.java
index 0ba1fb9..0519227 100644
--- a/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerPolicyValidator.java
+++ b/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerPolicyValidator.java
@@ -539,25 +539,22 @@ public class RangerPolicyValidator extends RangerValidator {
}
boolean valid = true;
- if (!Boolean.TRUE.equals(policy.getIsEnabled())) {
- LOG.debug("Policy is disabled. Skipping resource uniqueness validation.");
- } else {
- RangerPolicyResourceSignature policySignature = _factory.createPolicyResourceSignature(policy);
- String signature = policySignature.getSignature();
- List<RangerPolicy> policies = getPoliciesForResourceSignature(policy.getService(), signature);
- if (CollectionUtils.isNotEmpty(policies)) {
- ValidationErrorCode error = ValidationErrorCode.POLICY_VALIDATION_ERR_DUPLICATE_POLICY_RESOURCE;
- RangerPolicy matchedPolicy = policies.iterator().next();
- // there shouldn't be a matching policy for create. During update only match should be to itself
- if (action == Action.CREATE || (action == Action.UPDATE && (policies.size() > 1 || !matchedPolicy.getId().equals(policy.getId())))) {
- failures.add(new ValidationFailureDetailsBuilder()
- .field("resources")
- .isSemanticallyIncorrect()
- .becauseOf(error.getMessage(matchedPolicy.getName(), policy.getService()))
- .errorCode(error.getErrorCode())
- .build());
- valid = false;
- }
+
+ RangerPolicyResourceSignature policySignature = _factory.createPolicyResourceSignature(policy);
+ String signature = policySignature.getSignature();
+ List<RangerPolicy> policies = getPoliciesForResourceSignature(policy.getService(), signature);
+ if (CollectionUtils.isNotEmpty(policies)) {
+ ValidationErrorCode error = ValidationErrorCode.POLICY_VALIDATION_ERR_DUPLICATE_POLICY_RESOURCE;
+ RangerPolicy matchedPolicy = policies.iterator().next();
+ // there shouldn't be a matching policy for create. During update only match should be to itself
+ if (action == Action.CREATE || (action == Action.UPDATE && !matchedPolicy.getId().equals(policy.getId()))) {
+ failures.add(new ValidationFailureDetailsBuilder()
+ .field("resources")
+ .isSemanticallyIncorrect()
+ .becauseOf(error.getMessage(matchedPolicy.getName(), policy.getService()))
+ .errorCode(error.getErrorCode())
+ .build());
+ valid = false;
}
}
diff --git a/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerValidator.java b/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerValidator.java
index b02090b..e99598d 100644
--- a/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerValidator.java
+++ b/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerValidator.java
@@ -346,19 +346,30 @@ public abstract class RangerValidator {
if(LOG.isDebugEnabled()) {
LOG.debug(String.format("==> RangerValidator.getPoliciesForResourceSignature(%s, %s)", serviceName, policySignature));
}
+ final List<RangerPolicy> ret;
- List<RangerPolicy> policies = null;
+ List<RangerPolicy> enabledPolicies = new ArrayList<>();
+ List<RangerPolicy> disabledPolicies = new ArrayList<>();
try {
- policies = _store.getPoliciesByResourceSignature(serviceName, policySignature, true); // only look for enabled policies
+ enabledPolicies = _store.getPoliciesByResourceSignature(serviceName, policySignature, true);
+ disabledPolicies = _store.getPoliciesByResourceSignature(serviceName, policySignature, false);
} catch (Exception e) {
LOG.debug("Encountred exception while retrieving policies from service store!", e);
}
+ if (CollectionUtils.isEmpty(enabledPolicies)) {
+ ret = disabledPolicies;
+ } else if (CollectionUtils.isEmpty(disabledPolicies)) {
+ ret = enabledPolicies;
+ } else {
+ ret = enabledPolicies;
+ ret.addAll(disabledPolicies);
+ }
if(LOG.isDebugEnabled()) {
- int count = policies == null ? 0 : policies.size();
- LOG.debug(String.format("<== RangerValidator.getPoliciesForResourceSignature(%s, %s): count[%d], %s", serviceName, policySignature, count, policies));
+ int count = ret == null ? 0 : ret.size();
+ LOG.debug(String.format("<== RangerValidator.getPoliciesForResourceSignature(%s, %s): count[%d], %s", serviceName, policySignature, count, ret));
}
- return policies;
+ return ret;
}
RangerSecurityZone getSecurityZone(Long id) {
diff --git a/agents-common/src/test/java/org/apache/ranger/plugin/model/TestRangerPolicyResourceSignature.java b/agents-common/src/test/java/org/apache/ranger/plugin/model/TestRangerPolicyResourceSignature.java
index bc9df31..87075ec 100644
--- a/agents-common/src/test/java/org/apache/ranger/plugin/model/TestRangerPolicyResourceSignature.java
+++ b/agents-common/src/test/java/org/apache/ranger/plugin/model/TestRangerPolicyResourceSignature.java
@@ -89,6 +89,7 @@ public class TestRangerPolicyResourceSignature {
// empty resources map is ok!
Map<String, RangerPolicyResource> policyResources = new HashMap<>();
when(rangerPolicy.getResources()).thenReturn(policyResources);
+ when(rangerPolicy.getGuid()).thenReturn("TEST_GUID");
policySerializer = new PolicySerializer(rangerPolicy);
Assert.assertTrue("policy.getResources().isEmpty()", policySerializer.isPolicyValidForResourceSignatureComputation());
@@ -159,6 +160,8 @@ public class TestRangerPolicyResourceSignature {
Map<String, RangerPolicyResource> policyResources = _utils.createPolicyResourceMap(first);
when(policy.getResources()).thenReturn(policyResources);
when(policy.getZoneName()).thenReturn(null);
+ when(policy.getGuid()).thenReturn("TEST_GUID");
+ when(policy.getIsEnabled()).thenReturn(true);
serializer = new PolicySerializer(policy);
String expectedVersion = "version=1";
String expectedType = "type=0";
@@ -190,6 +193,9 @@ public class TestRangerPolicyResourceSignature {
RangerPolicy policy2 = createPolicy(first_recursive_null_or_false);
RangerPolicyResourceSignature signature1 = new RangerPolicyResourceSignature(policy1);
RangerPolicyResourceSignature signature2 = new RangerPolicyResourceSignature(policy2);
+ when(policy1.getGuid()).thenReturn("TEST_GUID-1");
+ when(policy2.getGuid()).thenReturn("TEST_GUID-2");
+
Assert.assertEquals("Recursive flag: null is same as false", signature1.toString(), signature2.toString());
}
@@ -198,6 +204,8 @@ public class TestRangerPolicyResourceSignature {
// create two policies with resources that differ only in the recursive flag, i.e. null/false in one and true in another
RangerPolicy policy1 = createPolicy(first);
RangerPolicy policy2 = createPolicy(first_recursive_flag_different);
+ when(policy1.getGuid()).thenReturn("TEST_GUID-1");
+ when(policy2.getGuid()).thenReturn("TEST_GUID-2");
RangerPolicyResourceSignature signature1 = new RangerPolicyResourceSignature(policy1);
RangerPolicyResourceSignature signature2 = new RangerPolicyResourceSignature(policy2);
Assert.assertFalse("Resources differ only by recursive flag true vs false/null", signature1.toString().equals(signature2.toString()));
@@ -208,6 +216,8 @@ public class TestRangerPolicyResourceSignature {
// create two policies with resources that differ only in the excludes flag such that flags are null in one and false in another
RangerPolicy policy1 = createPolicy(first);
RangerPolicy policy2 = createPolicy(first_excludes_null_or_false);
+ when(policy1.getGuid()).thenReturn("TEST_GUID-1");
+ when(policy2.getGuid()).thenReturn("TEST_GUID-2");
RangerPolicyResourceSignature signature1 = new RangerPolicyResourceSignature(policy1);
RangerPolicyResourceSignature signature2 = new RangerPolicyResourceSignature(policy2);
Assert.assertEquals("Excludes flag: null is same as false", signature1.toString(), signature2.toString());
@@ -218,6 +228,8 @@ public class TestRangerPolicyResourceSignature {
// create two policies with resources that differ only in the excludes flag, i.e. null/false in one and true in another
RangerPolicy policy1 = createPolicy(first);
RangerPolicy policy2 = createPolicy(first_excludes_flag_different);
+ when(policy1.getGuid()).thenReturn("TEST_GUID-1");
+ when(policy2.getGuid()).thenReturn("TEST_GUID-2");
RangerPolicyResourceSignature signature1 = new RangerPolicyResourceSignature(policy1);
RangerPolicyResourceSignature signature2 = new RangerPolicyResourceSignature(policy2);
Assert.assertFalse("Resources differ only by excludes flag true vs false/null", signature1.toString().equals(signature2.toString()));
@@ -227,6 +239,7 @@ public class TestRangerPolicyResourceSignature {
RangerPolicy policy = mock(RangerPolicy.class);
Map<String, RangerPolicyResource> resources = _utils.createPolicyResourceMap(data);
when(policy.getResources()).thenReturn(resources);
+ when(policy.getIsEnabled()).thenReturn(true);
return policy;
}
@@ -236,11 +249,16 @@ public class TestRangerPolicyResourceSignature {
RangerPolicy aPolicy = mock(RangerPolicy.class);
Map<String, RangerPolicyResource> resources = _utils.createPolicyResourceMap(first);
when(aPolicy.getResources()).thenReturn(resources);
+ when(aPolicy.getIsEnabled()).thenReturn(true);
+ when(aPolicy.getGuid()).thenReturn("TEST_GUID-1");
RangerPolicyResourceSignature signature = new RangerPolicyResourceSignature(aPolicy);
RangerPolicy anotherPolicy = mock(RangerPolicy.class);
resources = _utils.createPolicyResourceMap(data_second);
when(anotherPolicy.getResources()).thenReturn(resources);
+ when(anotherPolicy.getIsEnabled()).thenReturn(true);
+ when(anotherPolicy.getGuid()).thenReturn("TEST_GUID-2");
+
RangerPolicyResourceSignature anotherSignature = new RangerPolicyResourceSignature(anotherPolicy);
Assert.assertTrue(signature.equals(anotherSignature));
Assert.assertTrue(anotherSignature.equals(signature));
diff --git a/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerPolicyValidator.java b/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerPolicyValidator.java
index f0e2d07..0c6c4c2 100644
--- a/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerPolicyValidator.java
+++ b/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerPolicyValidator.java
@@ -205,6 +205,12 @@ public class TestRangerPolicyValidator {
when(_store.getServiceByName("service-name2")).thenReturn(service2);
when(_policy.getService()).thenReturn("service-name2");
+
+ RangerPolicyResourceSignature policySignature = mock(RangerPolicyResourceSignature.class);
+ when(policySignature.getSignature()).thenReturn("hash-1");
+
+ when(_factory.createPolicyResourceSignature(_policy)).thenReturn(policySignature);
+
when(_store.getServiceByName("service-name2")).thenReturn(service2);
action = Action.UPDATE;
@@ -730,26 +736,26 @@ public class TestRangerPolicyValidator {
when(_policy.getIsEnabled()).thenReturn(true); // ensure policy is enabled
_failures.clear(); Assert.assertFalse(_validator.isPolicyResourceUnique(_policy, _failures, Action.CREATE));
_utils.checkFailureForSemanticError(_failures, "resources");
- // same check should pass if the policy is disabled
+ // same check should fail even if the policy is disabled
when(_policy.getIsEnabled()).thenReturn(false);
- _failures.clear(); Assert.assertTrue(_validator.isPolicyResourceUnique(_policy, _failures, Action.CREATE));
- Assert.assertTrue("failures collection wasn't empty!", _failures.isEmpty());
+ _failures.clear(); Assert.assertFalse(_validator.isPolicyResourceUnique(_policy, _failures, Action.CREATE));
+ _utils.checkFailureForSemanticError(_failures, "resources");
// For Update match with itself is not a problem as long as it isn't itself, i.e. same id.
when(_policy.getIsEnabled()).thenReturn(true); // ensure policy is enabled
when(policy1.getId()).thenReturn(103L);
when(_policy.getId()).thenReturn(103L);
Assert.assertTrue(_validator.isPolicyResourceUnique(_policy, _failures, Action.UPDATE));
-
+
// matching policy can't be some other policy (i.e. different id) because that implies a conflict.
when(policy1.getId()).thenReturn(104L);
Assert.assertFalse(_validator.isPolicyResourceUnique(_policy, _failures, Action.UPDATE));
_utils.checkFailureForSemanticError(_failures, "resources");
// same check should pass if the policy is disabled
when(_policy.getIsEnabled()).thenReturn(false);
- _failures.clear(); Assert.assertTrue(_validator.isPolicyResourceUnique(_policy, _failures, Action.UPDATE));
- Assert.assertTrue("failures collection wasn't empty!", _failures.isEmpty());
-
+ _failures.clear(); Assert.assertFalse(_validator.isPolicyResourceUnique(_policy, _failures, Action.UPDATE));
+ _utils.checkFailureForSemanticError(_failures, "resources");
+
// And validation should never pass if there are more than one policies with matching signature, regardless of their ID!!
RangerPolicy policy2 = mock(RangerPolicy.class);
when(policy2.getId()).thenReturn(103L); // has same id as the policy being tested (_policy)
@@ -759,8 +765,8 @@ public class TestRangerPolicyValidator {
_utils.checkFailureForSemanticError(_failures, "resources");
// same check should pass if the policy is disabled
when(_policy.getIsEnabled()).thenReturn(false);
- _failures.clear(); Assert.assertTrue(_validator.isPolicyResourceUnique(_policy, _failures, Action.UPDATE));
- Assert.assertTrue("failures collection wasn't empty!", _failures.isEmpty());
+ _failures.clear(); Assert.assertFalse(_validator.isPolicyResourceUnique(_policy, _failures, Action.UPDATE));
+ _utils.checkFailureForSemanticError(_failures, "resources");
}
@Test
diff --git a/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerValidator.java b/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerValidator.java
index 6de590b..6114225 100644
--- a/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerValidator.java
+++ b/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerValidator.java
@@ -186,9 +186,9 @@ public class TestRangerValidator {
String serviceName = "service-name";
boolean isPolicyEnabled = true;
when(_store.getPoliciesByResourceSignature(serviceName, hexSignature, isPolicyEnabled)).thenReturn(null);
- Assert.assertNull(_validator.getPoliciesForResourceSignature(serviceName, hexSignature));
+ Assert.assertNotNull(_validator.getPoliciesForResourceSignature(serviceName, hexSignature));
when(_store.getPoliciesByResourceSignature(serviceName, hexSignature, isPolicyEnabled)).thenThrow(new Exception());
- Assert.assertNull(_validator.getPoliciesForResourceSignature(serviceName, hexSignature));
+ Assert.assertNotNull(_validator.getPoliciesForResourceSignature(serviceName, hexSignature));
// what ever store returns should come back
hexSignature = "anotherSignature";
diff --git a/security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java b/security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java
index f13cef7..cb57b99 100644
--- a/security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java
+++ b/security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java
@@ -65,6 +65,7 @@ import org.apache.ranger.authorization.utils.JsonUtils;
import org.apache.ranger.biz.ServiceDBStore.METRIC_TYPE;
import org.apache.ranger.common.AppConstants;
import org.apache.ranger.common.ContextUtil;
+import org.apache.ranger.common.GUIDUtil;
import org.apache.ranger.common.MessageEnums;
import org.apache.ranger.common.RangerCommonEnums;
import org.apache.ranger.common.db.RangerTransactionSynchronizationAdapter;
@@ -347,6 +348,9 @@ public class ServiceDBStore extends AbstractServiceStore {
@Autowired
SecurityZoneDBStore securityZoneStore;
+ @Autowired
+ GUIDUtil guidUtil;
+
private static volatile boolean legacyServiceDefsInitDone = false;
private Boolean populateExistingBaseFields = false;
@@ -2029,11 +2033,6 @@ public class ServiceDBStore extends AbstractServiceStore {
policy.setVersion(Long.valueOf(1));
updatePolicySignature(policy);
- boolean updateServiceInfoRoleVersion = false;
- if (isSupportsRolesDownloadByService()) {
- updateServiceInfoRoleVersion = isRoleDownloadRequired(policy, service);
- }
-
if(populateExistingBaseFields) {
assignedIdPolicyService.setPopulateExistingBaseFields(true);
daoMgr.getXXPolicy().setIdentityInsert(true);
@@ -2050,8 +2049,12 @@ public class ServiceDBStore extends AbstractServiceStore {
XXPolicy xCreatedPolicy = daoMgr.getXXPolicy().getById(policy.getId());
policyRefUpdater.createNewPolMappingForRefTable(policy, xCreatedPolicy, xServiceDef);
createOrMapLabels(xCreatedPolicy, uniquePolicyLabels);
- RangerPolicy createdPolicy = policyService.getPopulatedViewObject(xCreatedPolicy);
+ RangerPolicy createdPolicy = policyService.getPopulatedViewObject(xCreatedPolicy);
+ boolean updateServiceInfoRoleVersion = false;
+ if (isSupportsRolesDownloadByService()) {
+ updateServiceInfoRoleVersion = isRoleDownloadRequired(createdPolicy, service);
+ }
handlePolicyUpdate(service, RangerPolicyDelta.CHANGE_TYPE_POLICY_CREATE, createdPolicy, updateServiceInfoRoleVersion);
dataHistService.createObjectDataHistory(createdPolicy, RangerDataHistService.ACTION_CREATE);
@@ -2175,11 +2178,6 @@ public class ServiceDBStore extends AbstractServiceStore {
updatePolicySignature(policy);
- boolean updateServiceInfoRoleVersion = false;
- if (isSupportsRolesDownloadByService()) {
- updateServiceInfoRoleVersion = isRoleDownloadRequired(policy, service);
- }
-
policy = policyService.update(policy);
XXPolicy newUpdPolicy = daoMgr.getXXPolicy().getById(policy.getId());
@@ -2189,6 +2187,11 @@ public class ServiceDBStore extends AbstractServiceStore {
policyRefUpdater.createNewPolMappingForRefTable(policy, newUpdPolicy, xServiceDef);
createOrMapLabels(newUpdPolicy, uniquePolicyLabels);
RangerPolicy updPolicy = policyService.getPopulatedViewObject(newUpdPolicy);
+
+ boolean updateServiceInfoRoleVersion = false;
+ if (isSupportsRolesDownloadByService()) {
+ updateServiceInfoRoleVersion = isRoleDownloadRequired(updPolicy, service);
+ }
handlePolicyUpdate(service, RangerPolicyDelta.CHANGE_TYPE_POLICY_UPDATE, updPolicy, updateServiceInfoRoleVersion);
dataHistService.createObjectDataHistory(updPolicy, RangerDataHistService.ACTION_UPDATE);
@@ -3831,6 +3834,11 @@ public class ServiceDBStore extends AbstractServiceStore {
}
void updatePolicySignature(RangerPolicy policy) {
+ String guid = policy.getGuid();
+ if (StringUtils.isEmpty(guid)) {
+ guid = guidUtil.genGUID();
+ policy.setGuid(guid);
+ }
RangerPolicyResourceSignature policySignature = factory.createPolicyResourceSignature(policy);
String signature = policySignature.getSignature();
policy.setResourceSignature(signature);
diff --git a/security-admin/src/test/java/org/apache/ranger/biz/TestServiceDBStore.java b/security-admin/src/test/java/org/apache/ranger/biz/TestServiceDBStore.java
index ef0d3b4..dfb5814 100644
--- a/security-admin/src/test/java/org/apache/ranger/biz/TestServiceDBStore.java
+++ b/security-admin/src/test/java/org/apache/ranger/biz/TestServiceDBStore.java
@@ -25,6 +25,7 @@ import java.util.Map;
import org.apache.commons.collections.ListUtils;
import org.apache.ranger.common.ContextUtil;
+import org.apache.ranger.common.GUIDUtil;
import org.apache.ranger.common.JSONUtil;
import org.apache.ranger.common.RESTErrorUtil;
import org.apache.ranger.common.RangerFactory;
@@ -150,6 +151,9 @@ public class TestServiceDBStore {
@Mock
JSONUtil jsonUtil;
+ @Mock
+ GUIDUtil guidUtil;
+
@Rule
public ExpectedException thrown = ExpectedException.none();