Fix security related bug where the first matching rule 'by' clause is…

… evaluted and then subsequent policies are ignored.
rundeck · Jul 30, 2011 · a7d6f62 · a7d6f62
1 parent 51b2011
commit a7d6f62
Show file tree

Hide file tree

Showing 7 changed files with 77 additions and 18 deletions.
diff --git a/core/src/java/com/dtolabs/rundeck/core/authorization/providers/PoliciesDocument.java b/core/src/java/com/dtolabs/rundeck/core/authorization/providers/PoliciesDocument.java
@@ -30,6 +30,8 @@
 import org.w3c.dom.Document;
 import org.w3c.dom.Node;
 import org.w3c.dom.NodeList;
+import org.apache.log4j.Logger;
+
 
 import javax.naming.InvalidNameException;
 import javax.naming.ldap.LdapName;
@@ -46,6 +48,7 @@
  * @author Greg Schueler <a href="mailto:greg@dtosolutions.com">greg@dtosolutions.com</a>
  */
 public class PoliciesDocument implements PolicyCollection {
+
     private Document document;
     private File file;
     private ArrayList<String> groupNames;
@@ -184,7 +187,7 @@ static Collection<AclContext> policyMatcher(Subject subject, Collection<? extend
             // * The username matches exactly 1 in the context.
             // * 1 subject group matches 1 group.  non disjoint sets.
             //
-            // First match stops the search.
+            // First match stops the search, for this particular policy.
 
             // TODO: time of day check.
 
@@ -201,7 +204,7 @@ static Collection<AclContext> policyMatcher(Subject subject, Collection<? extend
 
                 if (!Collections.disjoint(policyUsers, usernamePrincipals)) {
                     matchedContexts.add(policy.getContext());
-                    break;
+                    continue;
                 }
             }
 

diff --git a/core/src/java/com/dtolabs/rundeck/core/authorization/providers/PoliciesXml.java b/core/src/java/com/dtolabs/rundeck/core/authorization/providers/PoliciesXml.java
@@ -296,7 +296,7 @@ private void buildNodeString(Node node, StringBuilder sb) {
 
         @Override
         public String toString() {
-            return "Context";
+            return "Context: " + this.generatePolicyName(this.policy);
         }
     }
 

diff --git a/core/src/java/com/dtolabs/rundeck/core/authorization/providers/PoliciesYaml.java b/core/src/java/com/dtolabs/rundeck/core/authorization/providers/PoliciesYaml.java
@@ -96,6 +96,11 @@ public Set<Object> getGroups() {
         @Override
         public AclContext getContext() {
             return new AclContext() {
+                private String description = "Not Evaluated: " + super.toString();
+
+                public String toString() {
+                    return "Context: " + description;
+                }
 
                 @SuppressWarnings("rawtypes")
                 @Override
@@ -108,7 +113,7 @@ public ContextDecision includes(Map<String, String> resourceMap, String action)
                         return new ContextDecision(Code.REJECTED_NO_DESCRIPTION_PROVIDED, false, evaluations);
                     }
 
-                    String description = (String)descriptionValue;
+                    description = (String)descriptionValue;
 
                     Object rulesValue = rawInput.get("rules");
                     if( !(rulesValue instanceof Map) ) {

diff --git a/core/src/java/com/dtolabs/rundeck/core/authorization/providers/Policy.java b/core/src/java/com/dtolabs/rundeck/core/authorization/providers/Policy.java
@@ -23,17 +23,46 @@
 */
 package com.dtolabs.rundeck.core.authorization.providers;
 
+import java.util.Collections;
 import java.util.Set;
 
 /**
- * Policy is ...
+ * Policy is contains a set of {@link AclContext} with corresponding usernames and/or groups
+ * associated with the each Acl.
+ * 
+ * The policy is a reference to a phycial policy stored on persistantly.
  *
- * @author Greg Schueler <a href="mailto:greg@dtosolutions.com">greg@dtosolutions.com</a>
+ * @author noahcampbell
  */
 public interface Policy {
-    public Set<String> getUsernames();
-
+
+    /**
+     * Return the {@link AclContext} for this policy representation.
+     * 
+     * @return context
+     */
     AclContext getContext();
 
+    /**
+     * Return a list of usernames as strings associated with this policy.  The backing set should 
+     * rely on the natural sorting order of HashSet<String> otherwise unexpect behavior will occur.  
+     * 
+     * See {@link Collections#disjoint(java.util.Collection, java.util.Collection)} if you need
+     * to muck with the ordering.
+     * 
+     * @return usernames
+     */
+    public Set<String> getUsernames();
+
+    /**
+     * 
+     * Return a list of group objects associated with this policy.  The backing set should rely on
+     * the natural sorting order of HashSet<String> otherwise unexpect behavior will occur.  
+     * 
+     * See {@link Collections#disjoint(java.util.Collection, java.util.Collection)} if you need
+     * to muck with the ordering.
+     * 
+     * @return groups
+     */
     public Set<Object> getGroups();
 }
diff --git a/core/src/test/com/dtolabs/rundeck/core/authorization/TestPolicies.java b/core/src/test/com/dtolabs/rundeck/core/authorization/TestPolicies.java
@@ -58,7 +58,7 @@ public static File getPath(String name) {
     }
 
     public void testPoliciesStructural() throws Exception {
-        assertEquals("Policy count mismatch", 9, policies.count());
+        assertEquals("Policy count mismatch", 10, policies.count());
     }
 
     public void testSelectOnPrincipal() throws Exception {
@@ -72,7 +72,8 @@ public void testSelectOnPrincipal() throws Exception {
         Set<Attribute> environment = new HashSet<Attribute>();
         List<AclContext> contexts = policies.narrowContext(formalSubject, environment);
         assertNotNull("Context is null.", contexts);
-        assertEquals("Incorrect number of contexts returned when matching on username.", 1, contexts.size());
+        assertEquals("Incorrect number of contexts returned when matching on username.", 2, contexts.size());
+
 
         formalSubject = new Subject();
         formalSubject.getPrincipals().add(new Username("yml_usr_1"));
@@ -96,7 +97,7 @@ public void testSelectOnPrincipal() throws Exception {
 
     public void testListAllRoles() throws Exception {
         List<String> results = policies.listAllRoles();
-        assertEquals("Results did not return the correct number of policies.", 9, results.size());
+        assertEquals("Results did not return the correct number of policies.", 10, results.size());
         results.containsAll(Arrays.asList(new String[]{"admin","foo","admin-environment","ou=Foo,dn=example,dn=com"}));
     }
 }
diff --git a/core/src/test/com/dtolabs/rundeck/core/authorization/TestSAREAuthorization.java b/core/src/test/com/dtolabs/rundeck/core/authorization/TestSAREAuthorization.java
@@ -199,6 +199,18 @@ public void testActionAuthorizationYmlInvalid() throws Exception {
 
     }
 
+    public void testActionAuthorizationYmlNoMatchIssue() throws Exception {
+        Map<String,String> resource = declareScript("Script_123", "/AB3");
+        Subject subject = createSubject("yml_usr_2", "issue_not_match");
+
+        Decision decision = authorization.evaluate(resource, subject, "foobar", null);
+        decision.explain().describe(System.out);
+        System.out.flush();
+        assertEquals("Decision for authoraztion for action: foobar is not GRANTED_ACTIONS_AND_COMMANDS_MATCHED.",
+                Code.GRANTED_ACTIONS_AND_COMMANDS_MATCHED, decision.explain().getCode());
+        assertTrue("Action granted authorization.", decision.isAuthorized());
+    }
+
     public void testActionAuthorization() throws Exception {
         Map<String,String> resource = declareScript("myScript", "bar/baz/boo");
         Subject subject = createSubject("testActionAuthorization", "admin-action");
@@ -215,18 +227,13 @@ public void testActionAuthorization() throws Exception {
                 Code.REJECTED_COMMAND_NOT_MATCHED,  decision.explain().getCode());
         assertFalse("Action bobble_head should not have been authorized", decision.isAuthorized());
 
-        System.out.println(decision);
-        decision.explain().describe(System.out);
 
         /* Empty actions never match. */
         decision = authorization.evaluate(resource, subject, "", environment);
         assertEquals("Decision for empty action does not match", Code.REJECTED_NO_ACTION_PROVIDED,
                 decision.explain().getCode());
         assertFalse("An empty action should not select", decision.isAuthorized());
-
-        System.out.println(decision);
-        decision.explain().describe(System.out);        
-
+
         /* The given job=anyaction of group=foobar should allow any action. */
         decision = authorization.evaluate(declareScript("anyaction", "foobar"), subject, 
                 "my_wacky_action", environment);
@@ -295,6 +302,8 @@ public void off_testNodeTarget() throws Exception {
 
 
     /**
+     * Create a RunDeck equivalent Job.
+     * 
      * @param scriptName
      * @param scriptGroup
      */

diff --git a/core/src/test/com/dtolabs/rundeck/core/authorization/admintest.yml.aclpolicy b/core/src/test/com/dtolabs/rundeck/core/authorization/admintest.yml.aclpolicy
@@ -58,4 +58,16 @@ id: d5e598ae-403b-45c4-8f91-36c255101091
 
 by:
     username: yml_usr_3
-    group: missing_rules
+    group: missing_rules
+
+---
+
+description: Yaml Policy 4
+id: CD055D12-DAEF-4E4D-90B7-C100ADF8FD5F
+
+rules:
+    /AB3/.*:
+        actions: '*'
+
+by:
+    group: issue_not_match