Repository: incubator-ranger Updated Branches: refs/heads/ranger-0.5 de019a805 -> 2c54590a8
RANGER-529 Policy Validation: resources of a policy must match one of the resource hierarchies of the service def Signed-off-by: Madhan Neethiraj <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/incubator-ranger/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-ranger/commit/cfc0f39a Tree: http://git-wip-us.apache.org/repos/asf/incubator-ranger/tree/cfc0f39a Diff: http://git-wip-us.apache.org/repos/asf/incubator-ranger/diff/cfc0f39a Branch: refs/heads/ranger-0.5 Commit: cfc0f39acfeab1d03eb186492f43e6cc0e93c2b2 Parents: de019a8 Author: Alok Lal <[email protected]> Authored: Wed Jun 3 13:58:43 2015 -0700 Committer: Madhan Neethiraj <[email protected]> Committed: Wed Jun 3 17:31:41 2015 -0700 ---------------------------------------------------------------------- .../model/validation/RangerPolicyValidator.java | 129 +++++++++++++------ .../validation/RangerServiceDefHelper.java | 20 ++- .../validation/TestRangerPolicyValidator.java | 30 ++++- .../validation/TestRangerServiceDefHelper.java | 4 +- 4 files changed, 140 insertions(+), 43 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-ranger/blob/cfc0f39a/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerPolicyValidator.java ---------------------------------------------------------------------- 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 caaf68f..cea3e05 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 @@ -20,7 +20,7 @@ package org.apache.ranger.plugin.model.validation; import java.util.ArrayList; -import java.util.Iterator; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -39,9 +39,6 @@ import org.apache.ranger.plugin.model.RangerServiceDef; import org.apache.ranger.plugin.model.RangerServiceDef.RangerResourceDef; import org.apache.ranger.plugin.store.ServiceStore; -import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Sets; - public class RangerPolicyValidator extends RangerValidator { private static final Log LOG = LogFactory.getLog(RangerPolicyValidator.class); @@ -304,51 +301,38 @@ public class RangerPolicyValidator extends RangerValidator { RangerServiceDefHelper defHelper = new RangerServiceDefHelper(serviceDef); Set<List<RangerResourceDef>> hierarchies = defHelper.getResourceHierarchies(); // this can be empty but not null! if (hierarchies.isEmpty()) { - LOG.debug("RangerPolicyValidator.isValidResourceNames: serviceDef does not have any resource hierarchies! Skipping this check!"); + LOG.warn("RangerPolicyValidator.isValidResourceNames: serviceDef does not have any resource hierarchies, possibly due to a old/migrated service def! Skipping this check!"); } else { - Iterator<List<RangerResourceDef>> iterator = hierarchies.iterator(); - boolean foundHierarchyMatch = false; - Set<String> missingResourcesCopyForErrorMessage = null; // used to give more helpful error message to the user - while (iterator.hasNext() && !foundHierarchyMatch) { - List<RangerResourceDef> aHierarchy = iterator.next(); - Set<String> mandatoryResources = defHelper.getMandatoryResourceNames(aHierarchy); - Set<String> missingResources = Sets.difference(mandatoryResources, policyResources); - if (missingResources.isEmpty()) { - foundHierarchyMatch = true; - } else { - /* - * Since user does not specify which hierarchy the policy is for, it is tricky to guess her intention and give error message about - * what resource is really missing. For example if only db is speicifed then it is tricky to say if just UDF was missed or TBL and COL were missed. - * So we punt and capture the first hierarchy that does not match. And if policy does not match any hierarchies then we use this cached value - * to report error back to the user. Ideal solution is to require user to specify hierarhcy in policy in case of multi-hierarchy policies. - */ - missingResourcesCopyForErrorMessage = ImmutableSet.copyOf(missingResources); - } - } /* - * We have evaluated all valid resource hierarchies for the service. But policy did not have mandatory resources required by any of those hierarchies! + * A policy is for a single hierarchy however, it doesn't specify which one. So we have to guess which hierarchy(s) it possibly be for. First, see if the policy could be for + * any of the known hierarchies? A candidate hierarchy is one whose resource levels are a superset of those in the policy. + * Why? What we want to catch at this stage is policies that straddles multiple hierarchies, e.g. db, udf and column for a hive policy. + * This has the side effect of catch spurious levels specified on the policy, e.g. having a level "blah" on a hive policy. */ - if (!foundHierarchyMatch) { + Set<List<RangerResourceDef>> candidateHierarchies = filterHierarchies_hierarchyHasAllPolicyResources(policyResources, hierarchies, defHelper); + if (candidateHierarchies.isEmpty()) { + // let's build a helpful message for user failures.add(new ValidationFailureDetailsBuilder() .field("resources") - .subField(missingResourcesCopyForErrorMessage.iterator().next()) // We return any one missing resource here! - .isMissing() - .becauseOf("required resources [" + missingResourcesCopyForErrorMessage + "] are missing") + .subField("incompatible") + .isSemanticallyIncorrect() + .becauseOf(String.format("policy resources [%s] were incompatible with all the hierarchies for this service defs! Valid hierarchies are: %s", + policyResources.toString(), toStringHierarchies_all(hierarchies, defHelper))) .build()); valid = false; } /* - * Since policy does not specify which hierarchy it belongs to, we can't reliably check if a policy is specifying levels not relevant for it. However, we can - * do a secular check if we got a level that is not applicable to any hierarchy for the service. + * Among the candidate hierarchies there should be at least one for which policy specifies all of the mandatory resources. Note that there could be multiple + * hierarchies that meet that criteria, e.g. a hive policy that specified only DB. It is not clear if it belongs to DB->UDF or DB->TBL->COL hierarchy. + * However, if both UDF and TBL were required then we can detect that policy does not specify mandatory levels for any of the candidate hierarchies. */ - Set<String> allResource = defHelper.getResorceMap().keySet(); - Set<String> unknownResources = Sets.difference(policyResources, allResource); - if (!unknownResources.isEmpty()) { + Set<List<RangerResourceDef>> validHierarchies = filterHierarchies_mandatoryResourcesSpecifiedInPolicy(policyResources, candidateHierarchies, defHelper); + if (validHierarchies.isEmpty()) { failures.add(new ValidationFailureDetailsBuilder() .field("resources") - .subField(unknownResources.iterator().next()) // we return any one parameter! + .subField("missing mandatory") .isSemanticallyIncorrect() - .becauseOf("resource[" + unknownResources + "] is not valid for service-def[" + serviceDef.getName() + "]") + .becauseOf("policy is missing required resources. Mandatory fields of potential hierarchies are: " + toStringHierarchies_mandatory(candidateHierarchies, defHelper)) .build()); valid = false; } @@ -360,6 +344,79 @@ public class RangerPolicyValidator extends RangerValidator { return valid; } + /** + * String representation of mandatory resources of all the hierarchies suitable of showing to user. Mandatory resources within a hierarchy are not ordered per the hierarchy. + * @param hierarchies + * @param defHelper + * @return + */ + String toStringHierarchies_mandatory(Set<List<RangerResourceDef>> hierarchies, RangerServiceDefHelper defHelper) { + + // helper function skipping sanity checks of getting null arguments passed + StringBuilder builder = new StringBuilder(); + for (List<RangerResourceDef> aHierarchy : hierarchies) { + builder.append(defHelper.getMandatoryResourceNames(aHierarchy)); + builder.append(" "); + } + return builder.toString(); + } + + /** + * String representation of all resources of all hierarchies. Resources within a hierarchy are ordered per the hierarchy. + * @param hierarchies + * @param defHelper + * @return + */ + String toStringHierarchies_all(Set<List<RangerResourceDef>> hierarchies, RangerServiceDefHelper defHelper) { + + // helper function skipping sanity checks of getting null arguments passed + StringBuilder builder = new StringBuilder(); + for (List<RangerResourceDef> aHierarchy : hierarchies) { + builder.append(defHelper.getAllResourceNamesOrdered(aHierarchy)); + builder.append(" "); + } + return builder.toString(); + } + /** + * Returns the subset of all hierarchies that are a superset of the policy's resources. + * @param policyResources + * @param hierarchies + * @return + */ + Set<List<RangerResourceDef>> filterHierarchies_hierarchyHasAllPolicyResources(Set<String> policyResources, Set<List<RangerResourceDef>> hierarchies, RangerServiceDefHelper defHelper) { + + // helper function skipping sanity checks of getting null arguments passed + Set<List<RangerResourceDef>> result = new HashSet<List<RangerResourceDef>>(hierarchies.size()); + for (List<RangerResourceDef> aHierarchy : hierarchies) { + Set<String> hierarchyResources = defHelper.getAllResourceNames(aHierarchy); + if (hierarchyResources.containsAll(policyResources)) { + result.add(aHierarchy); + } + } + return result; + } + + /** + * Returns the subset of hierarchies all of whose mandatory resources were found in policy's resource set. candidate hierarchies are expected to have passed + * <code>filterHierarchies_hierarchyHasAllPolicyResources</code> check first. + * @param policyResources + * @param hierarchies + * @param defHelper + * @return + */ + Set<List<RangerResourceDef>> filterHierarchies_mandatoryResourcesSpecifiedInPolicy(Set<String> policyResources, Set<List<RangerResourceDef>> hierarchies, RangerServiceDefHelper defHelper) { + + // helper function skipping sanity checks of getting null arguments passed + Set<List<RangerResourceDef>> result = new HashSet<List<RangerResourceDef>>(hierarchies.size()); + for (List<RangerResourceDef> aHierarchy : hierarchies) { + Set<String> mandatoryResources = defHelper.getMandatoryResourceNames(aHierarchy); + if (policyResources.containsAll(mandatoryResources)) { + result.add(aHierarchy); + } + } + return result; + } + boolean isValidResourceFlags(final Map<String, RangerPolicyResource> inputPolicyResources, final List<ValidationFailureDetails> failures, final List<RangerResourceDef> resourceDefs, final String serviceDefName, final String policyName, boolean isAdmin) { if(LOG.isDebugEnabled()) { http://git-wip-us.apache.org/repos/asf/incubator-ranger/blob/cfc0f39a/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceDefHelper.java ---------------------------------------------------------------------- diff --git a/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceDefHelper.java b/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceDefHelper.java index d3bcc1a..53df193 100644 --- a/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceDefHelper.java +++ b/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceDefHelper.java @@ -119,7 +119,25 @@ public class RangerServiceDefHelper { return result; } - public List<String> getAllResourceNames(List<RangerResourceDef> hierarchy) { + /** + * Set view of a hierarchy's resource names for efficient searching + * @param hierarchy + * @return + */ + public Set<String> getAllResourceNames(List<RangerResourceDef> hierarchy) { + Set<String> result = new HashSet<String>(hierarchy.size()); + for (RangerResourceDef resourceDef : hierarchy) { + result.add(resourceDef.getName()); + } + return result; + } + + /** + * Resources names matching the order of list of resource defs passed in. + * @param hierarchy + * @return + */ + public List<String> getAllResourceNamesOrdered(List<RangerResourceDef> hierarchy) { List<String> result = new ArrayList<String>(hierarchy.size()); for (RangerResourceDef resourceDef : hierarchy) { result.add(resourceDef.getName()); http://git-wip-us.apache.org/repos/asf/incubator-ranger/blob/cfc0f39a/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerPolicyValidator.java ---------------------------------------------------------------------- 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 27c6318..6236d71 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 @@ -142,6 +142,19 @@ public class TestRangerPolicyValidator { {"extra", new String[] { "extra1", "extra2" }, null, null } // spurious "extra" specified }; + private final Object[][] policyResourceMap_bad_multiple_hierarchies = new Object[][] { + // resource-name, values, excludes, recursive + { "db", new String[] { "db1", "db2" }, null, true }, + { "tbl", new String[] { "tbl11", "tbl2" }, null, true }, + { "col", new String[] { "col1", "col2" }, true, true }, + { "udf", new String[] { "extra1", "extra2" }, null, null } // either udf or tbl/db/col should be specified, not both + }; + + private final Object[][] policyResourceMap_bad_multiple_hierarchies_missing_mandatory = new Object[][] { + // resource-name, values, excludes, recursive + { "db", new String[] { "db1", "db2" }, null, true } + }; + @Test public final void testIsValid_long() throws Exception { // this validation should be removed if we start supporting other than delete action @@ -454,8 +467,6 @@ public class TestRangerPolicyValidator { _utils.checkFailureForSemanticError(_failures, "resource-values", "col"); // for spurious resource: "extra" _utils.checkFailureForSemanticError(_failures, "isRecursive", "db"); // for specifying it as true when def did not allow it _utils.checkFailureForSemanticError(_failures, "isExcludes", "col"); // for specifying it as true when def did not allow it - _utils.checkFailureForMissingValue(_failures, "resources", "tbl"); // for missing resource: tbl - _utils.checkFailureForSemanticError(_failures, "resources", "extra"); // for spurious resource: "extra" } } @@ -755,8 +766,19 @@ public class TestRangerPolicyValidator { Map<String, RangerPolicyResource> policyResources = _utils.createPolicyResourceMap(policyResourceMap_bad); when(_policy.getResources()).thenReturn(policyResources); assertFalse("Missing required resource and unknown resource", _validator.isValidResourceNames(_policy, _failures, _serviceDef)); - _utils.checkFailureForMissingValue(_failures, "resources", new String[] {"tbl", "udf"} ); - _utils.checkFailureForSemanticError(_failures, "resources", "extra"); + _utils.checkFailureForSemanticError(_failures, "resources"); + + // another bad resource map that straddles multiple hierarchies + policyResources = _utils.createPolicyResourceMap(policyResourceMap_bad_multiple_hierarchies); + when(_policy.getResources()).thenReturn(policyResources); + _failures.clear(); assertFalse("Policy with resources for multiple hierarchies", _validator.isValidResourceNames(_policy, _failures, _serviceDef)); + _utils.checkFailureForSemanticError(_failures, "resources", "incompatible"); + + // another bad policy resource map that could match multiple hierarchies but is short on mandatory resources for all of those matches + policyResources = _utils.createPolicyResourceMap(policyResourceMap_bad_multiple_hierarchies_missing_mandatory); + when(_policy.getResources()).thenReturn(policyResources); + _failures.clear(); assertFalse("Policy with resources for multiple hierarchies missing mandatory resources for all pontential matches", _validator.isValidResourceNames(_policy, _failures, _serviceDef)); + _utils.checkFailureForSemanticError(_failures, "resources", "missing mandatory"); } http://git-wip-us.apache.org/repos/asf/incubator-ranger/blob/cfc0f39a/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerServiceDefHelper.java ---------------------------------------------------------------------- diff --git a/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerServiceDefHelper.java b/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerServiceDefHelper.java index 883b808..d9e50e4 100644 --- a/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerServiceDefHelper.java +++ b/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerServiceDefHelper.java @@ -145,7 +145,7 @@ public class TestRangerServiceDefHelper { expectedHierarchies.add(Lists.newArrayList("namespace", "function")); for (List<RangerResourceDef> aHierarchy : hierarchies) { - List<String> resourceNames = _helper.getAllResourceNames(aHierarchy); + List<String> resourceNames = _helper.getAllResourceNamesOrdered(aHierarchy); assertTrue(expectedHierarchies.contains(resourceNames)); expectedHierarchies.remove(resourceNames); } @@ -186,7 +186,7 @@ public class TestRangerServiceDefHelper { expectedHierarchies.add(Lists.newArrayList("namespace", "function")); for (List<RangerResourceDef> aHierarchy : hierarchies) { - List<String> resourceNames = _helper.getAllResourceNames(aHierarchy); + List<String> resourceNames = _helper.getAllResourceNamesOrdered(aHierarchy); assertTrue(expectedHierarchies.contains(resourceNames)); expectedHierarchies.remove(resourceNames); }
