Add exclude_roles configuration parameter to LDAP authorization bac…

…kend (opensearch-project#4025) Signed-off-by: Maciej Mierzwa <[email protected]> Signed-off-by: Craig Perkins <[email protected]> Co-authored-by: Maciej Mierzwa <[email protected]>
cwperks · Feb 12, 2024 · 0bb31ca · 0bb31ca
1 parent 69bf786
commit 0bb31ca
Show file tree

Hide file tree

Showing 8 changed files with 130 additions and 22 deletions.
diff --git a/src/main/java/com/amazon/dlic/auth/ldap/backend/LDAPAuthorizationBackend.java b/src/main/java/com/amazon/dlic/auth/ldap/backend/LDAPAuthorizationBackend.java
@@ -101,6 +101,7 @@ public class LDAPAuthorizationBackend implements AuthorizationBackend {
     protected static final Logger log = LogManager.getLogger(LDAPAuthorizationBackend.class);
     private final Settings settings;
     private final WildcardMatcher skipUsersMatcher;
+    private final WildcardMatcher excludeRolesMatcher;
     private final WildcardMatcher nestedRoleMatcher;
     private final Path configPath;
     private final List<Map.Entry<String, Settings>> roleBaseSettings;
@@ -112,6 +113,7 @@ public class LDAPAuthorizationBackend implements AuthorizationBackend {
     public LDAPAuthorizationBackend(final Settings settings, final Path configPath) {
         this.settings = settings;
         this.skipUsersMatcher = WildcardMatcher.from(settings.getAsList(ConfigConstants.LDAP_AUTHZ_SKIP_USERS));
+        this.excludeRolesMatcher = WildcardMatcher.from(settings.getAsList(ConfigConstants.LDAP_AUTHZ_EXCLUDE_ROLES));
         this.nestedRoleMatcher = settings.getAsBoolean(ConfigConstants.LDAP_AUTHZ_RESOLVE_NESTED_ROLES, false)
             ? WildcardMatcher.from(settings.getAsList(ConfigConstants.LDAP_AUTHZ_NESTEDROLEFILTER))
             : null;
@@ -962,10 +964,12 @@ public void fillRoles(final User user, final AuthCredentials optionalAuthCreds)
                 for (final LdapName roleLdapName : nestedReturn) {
                     final String role = getRoleFromEntry(connection, roleLdapName, roleName);
 
-                    if (!Strings.isNullOrEmpty(role)) {
-                        user.addRole(role);
+                    if (excludeRolesMatcher.test(role)) {
+                        if (isDebugEnabled) {
+                            log.debug("Role was excluded or empty, attribute: '{}' for entry: {}", roleName, roleLdapName);
+                        }
                     } else {
-                        log.warn("No or empty attribute '{}' for entry {}", roleName, roleLdapName);
+                        user.addRole(role);
                     }
                 }
 
@@ -974,10 +978,12 @@ public void fillRoles(final User user, final AuthCredentials optionalAuthCreds)
                 for (final LdapName roleLdapName : ldapRoles) {
                     final String role = getRoleFromEntry(connection, roleLdapName, roleName);
 
-                    if (!Strings.isNullOrEmpty(role)) {
-                        user.addRole(role);
+                    if (excludeRolesMatcher.test(role)) {
+                        if (isDebugEnabled) {
+                            log.debug("Role was excluded or empty, attribute: '{}' for entry: {}", roleName, roleLdapName);
+                        }
                     } else {
-                        log.warn("No or empty attribute '{}' for entry {}", roleName, roleLdapName);
+                        user.addRole(role);
                     }
                 }
 

diff --git a/src/main/java/com/amazon/dlic/auth/ldap/util/ConfigConstants.java b/src/main/java/com/amazon/dlic/auth/ldap/util/ConfigConstants.java
@@ -29,6 +29,7 @@ public final class ConfigConstants {
     public static final String LDAP_AUTHZ_USERROLEATTRIBUTE = "userroleattribute";// multi-value
     public static final String LDAP_AUTHZ_USERROLENAME = "userrolename";// multi-value
     public static final String LDAP_AUTHZ_SKIP_USERS = "skip_users";
+    public static final String LDAP_AUTHZ_EXCLUDE_ROLES = "exclude_roles";
     public static final String LDAP_AUTHZ_ROLESEARCH_ENABLED = "rolesearch_enabled";
     public static final String LDAP_AUTHZ_NESTEDROLEFILTER = "nested_role_filter";
     public static final String LDAP_AUTHZ_MAX_NESTED_DEPTH = "max_nested_depth";

diff --git a/src/main/java/com/amazon/dlic/auth/ldap/util/LdapHelper.java b/src/main/java/com/amazon/dlic/auth/ldap/util/LdapHelper.java
@@ -122,7 +122,6 @@ private static Object escapeForwardSlash(Object input) {
         } else {
             return input;
         }
-
     }
 
 }
diff --git a/src/main/java/com/amazon/dlic/auth/ldap2/LDAPAuthorizationBackend2.java b/src/main/java/com/amazon/dlic/auth/ldap2/LDAPAuthorizationBackend2.java
@@ -69,6 +69,7 @@ public class LDAPAuthorizationBackend2 implements AuthorizationBackend, Destroya
     protected static final Logger log = LogManager.getLogger(LDAPAuthorizationBackend2.class);
     private final Settings settings;
     private final WildcardMatcher skipUsersMatcher;
+    private final WildcardMatcher excludeRolesMatcher;
     private final WildcardMatcher nestedRoleMatcher;
     private final List<Map.Entry<String, Settings>> roleBaseSettings;
     private ConnectionPool connectionPool;
@@ -80,6 +81,7 @@ public class LDAPAuthorizationBackend2 implements AuthorizationBackend, Destroya
     public LDAPAuthorizationBackend2(final Settings settings, final Path configPath) throws SSLConfigException {
         this.settings = settings;
         this.skipUsersMatcher = WildcardMatcher.from(settings.getAsList(ConfigConstants.LDAP_AUTHZ_SKIP_USERS));
+        this.excludeRolesMatcher = WildcardMatcher.from(settings.getAsList(ConfigConstants.LDAP_AUTHZ_EXCLUDE_ROLES));
         this.nestedRoleMatcher = settings.getAsBoolean(ConfigConstants.LDAP_AUTHZ_RESOLVE_NESTED_ROLES, false)
             ? WildcardMatcher.from(settings.getAsList(ConfigConstants.LDAP_AUTHZ_NESTEDROLEFILTER))
             : null;
@@ -329,8 +331,10 @@ private void fillRoles0(final User user, final AuthCredentials optionalAuthCreds
                         for (final Iterator<LdapEntry> iterator = rolesResult.iterator(); iterator.hasNext();) {
                             LdapEntry searchResultEntry = iterator.next();
                             LdapName ldapName = new LdapName(searchResultEntry.getDn());
-                            ldapRoles.add(ldapName);
-                            resultRoleSearchBaseKeys.put(ldapName, roleSearchSettingsEntry);
+                            if (!excludeRolesMatcher.test(searchResultEntry.getDn())) {
+                                ldapRoles.add(ldapName);
+                                resultRoleSearchBaseKeys.put(ldapName, roleSearchSettingsEntry);
+                            }
                         }
                     }
                 }
@@ -376,10 +380,12 @@ private void fillRoles0(final User user, final AuthCredentials optionalAuthCreds
                 for (final LdapName roleLdapName : nestedReturn) {
                     final String role = getRoleFromEntry(connection, roleLdapName, roleName);
 
-                    if (!Strings.isNullOrEmpty(role)) {
-                        user.addRole(role);
+                    if (excludeRolesMatcher.test(role)) {
+                        if (isDebugEnabled) {
+                            log.debug("Role was excluded or empty attribute '{}' for entry {}", roleName, roleLdapName);
+                        }
                     } else {
-                        log.warn("No or empty attribute '{}' for entry {}", roleName, roleLdapName);
+                        user.addRole(role);
                     }
                 }
 
@@ -388,10 +394,12 @@ private void fillRoles0(final User user, final AuthCredentials optionalAuthCreds
                 for (final LdapName roleLdapName : ldapRoles) {
                     final String role = getRoleFromEntry(connection, roleLdapName, roleName);
 
-                    if (!Strings.isNullOrEmpty(role)) {
-                        user.addRole(role);
+                    if (excludeRolesMatcher.test(role)) {
+                        if (isDebugEnabled) {
+                            log.debug("Role was excluded or empty attribute '{}' for entry {}", roleName, roleLdapName);
+                        }
                     } else {
-                        log.warn("No or empty attribute '{}' for entry {}", roleName, roleLdapName);
+                        user.addRole(role);
                     }
                 }
 

diff --git a/src/main/java/org/opensearch/security/support/WildcardMatcher.java b/src/main/java/org/opensearch/security/support/WildcardMatcher.java
@@ -150,7 +150,9 @@ public String toString() {
     };
 
     public static WildcardMatcher from(String pattern, boolean caseSensitive) {
-        if (pattern.equals("*")) {
+        if (pattern == null) {
+            return NONE;
+        } else if (pattern.equals("*")) {
             return ANY;
         } else if (pattern.startsWith("/") && pattern.endsWith("/")) {
             return new RegexMatcher(pattern, caseSensitive);
@@ -168,7 +170,9 @@ public static WildcardMatcher from(String pattern) {
     // This may in future use more optimized techniques to combine multiple WildcardMatchers in a single automaton
     public static <T> WildcardMatcher from(Stream<T> stream, boolean caseSensitive) {
         Collection<WildcardMatcher> matchers = stream.map(t -> {
-            if (t instanceof String) {
+            if (t == null) {
+                return NONE;
+            } else if (t instanceof String) {
                 return WildcardMatcher.from(((String) t), caseSensitive);
             } else if (t instanceof WildcardMatcher) {
                 return ((WildcardMatcher) t);

diff --git a/src/test/java/com/amazon/dlic/auth/ldap/LdapBackendTest.java b/src/test/java/com/amazon/dlic/auth/ldap/LdapBackendTest.java
@@ -14,6 +14,7 @@
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Paths;
 import java.util.Arrays;
+import java.util.List;
 
 import org.hamcrest.MatcherAssert;
 import org.junit.AfterClass;
@@ -39,6 +40,7 @@
 import org.ldaptive.ReturnAttributes;
 
 import static org.hamcrest.Matchers.hasItem;
+import static org.hamcrest.Matchers.not;
 
 public class LdapBackendTest {
 
@@ -579,6 +581,90 @@ public void testLdapAuthorizationNested() throws Exception {
         MatcherAssert.assertThat(user.getRoles(), hasItem("nested1"));
     }
 
+    @Test
+    public void testLdapNestedRoleFiltering() {
+
+        final Settings settings = Settings.builder()
+            .putList(ConfigConstants.LDAP_HOSTS, "localhost:" + ldapPort)
+            .put(ConfigConstants.LDAP_AUTHC_USERSEARCH, "(uid={0})")
+            .put(ConfigConstants.LDAP_AUTHC_USERBASE, "ou=people,o=TEST")
+            .put(ConfigConstants.LDAP_AUTHZ_ROLEBASE, "ou=groups,o=TEST")
+            .put(ConfigConstants.LDAP_AUTHZ_ROLENAME, "cn")
+            .put(ConfigConstants.LDAP_AUTHZ_RESOLVE_NESTED_ROLES, true)
+            .put(ConfigConstants.LDAP_AUTHZ_ROLESEARCH, "(uniqueMember={0})")
+            .putList(ConfigConstants.LDAP_AUTHZ_EXCLUDE_ROLES, List.of("nested1", "nested2"))
+            .build();
+
+        final User user = new User("spock");
+
+        new LDAPAuthorizationBackend(settings, null).fillRoles(user, null);
+
+        Assert.assertNotNull(user);
+        Assert.assertEquals("spock", user.getName());
+        Assert.assertEquals(2, user.getRoles().size());
+        // filtered out
+        MatcherAssert.assertThat(user.getRoles(), not(hasItem("nested1")));
+        MatcherAssert.assertThat(user.getRoles(), not(hasItem("nested2")));
+        MatcherAssert.assertThat(user.getRoles(), hasItem("role2"));
+        MatcherAssert.assertThat(user.getRoles(), hasItem("ceo"));
+    }
+
+    @Test
+    public void testLdapNestedRoleFilteringWithExcludedRolesWildcard() {
+
+        final Settings settings = Settings.builder()
+            .putList(ConfigConstants.LDAP_HOSTS, "localhost:" + ldapPort)
+            .put(ConfigConstants.LDAP_AUTHC_USERSEARCH, "(uid={0})")
+            .put(ConfigConstants.LDAP_AUTHC_USERBASE, "ou=people,o=TEST")
+            .put(ConfigConstants.LDAP_AUTHZ_ROLEBASE, "ou=groups,o=TEST")
+            .put(ConfigConstants.LDAP_AUTHZ_ROLENAME, "cn")
+            .put(ConfigConstants.LDAP_AUTHZ_RESOLVE_NESTED_ROLES, true)
+            .put(ConfigConstants.LDAP_AUTHZ_ROLESEARCH, "(uniqueMember={0})")
+            .putList(ConfigConstants.LDAP_AUTHZ_EXCLUDE_ROLES, List.of("nested*"))
+            .build();
+
+        final User user = new User("spock");
+
+        new LDAPAuthorizationBackend(settings, null).fillRoles(user, null);
+
+        Assert.assertNotNull(user);
+        Assert.assertEquals("spock", user.getName());
+        Assert.assertEquals(2, user.getRoles().size());
+        // filtered out
+        MatcherAssert.assertThat(user.getRoles(), not(hasItem("nested1")));
+        MatcherAssert.assertThat(user.getRoles(), not(hasItem("nested2")));
+        MatcherAssert.assertThat(user.getRoles(), hasItem("role2"));
+        MatcherAssert.assertThat(user.getRoles(), hasItem("ceo"));
+    }
+
+    @Test
+    public void testLdapdRoleFiltering() {
+
+        final Settings settings = Settings.builder()
+            .putList(ConfigConstants.LDAP_HOSTS, "localhost:" + ldapPort)
+            .put(ConfigConstants.LDAP_AUTHC_USERSEARCH, "(uid={0})")
+            .put(ConfigConstants.LDAP_AUTHC_USERBASE, "ou=people,o=TEST")
+            .put(ConfigConstants.LDAP_AUTHZ_ROLEBASE, "ou=groups,o=TEST")
+            .put(ConfigConstants.LDAP_AUTHZ_ROLENAME, "cn")
+            .put(ConfigConstants.LDAP_AUTHZ_RESOLVE_NESTED_ROLES, true)
+            .put(ConfigConstants.LDAP_AUTHZ_ROLESEARCH, "(uniqueMember={0})")
+            .putList(ConfigConstants.LDAP_AUTHZ_EXCLUDE_ROLES, List.of("ceo", "role1", "role2"))
+            .build();
+
+        final User user = new User("spock");
+
+        new LDAPAuthorizationBackend(settings, null).fillRoles(user, null);
+
+        Assert.assertNotNull(user);
+        Assert.assertEquals("spock", user.getName());
+        Assert.assertEquals(2, user.getRoles().size());
+        MatcherAssert.assertThat(user.getRoles(), hasItem("nested1"));
+        MatcherAssert.assertThat(user.getRoles(), hasItem("nested2"));
+        // filtered out
+        MatcherAssert.assertThat(user.getRoles(), not(hasItem("role2")));
+        MatcherAssert.assertThat(user.getRoles(), not(hasItem("ceo")));
+    }
+
     @Test
     public void testLdapAuthorizationNestedFilter() throws Exception {
 

diff --git a/src/test/java/com/amazon/dlic/auth/ldap2/LdapBackendTestNewStyleConfig2.java b/src/test/java/com/amazon/dlic/auth/ldap2/LdapBackendTestNewStyleConfig2.java
@@ -15,6 +15,7 @@
 import java.nio.file.Paths;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.List;
 import java.util.TreeSet;
 
 import org.apache.commons.lang3.exception.ExceptionUtils;
@@ -564,16 +565,18 @@ public void testLdapAuthorizationNested() throws Exception {
             .put(ConfigConstants.LDAP_AUTHZ_ROLENAME, "cn")
             .put(ConfigConstants.LDAP_AUTHZ_RESOLVE_NESTED_ROLES, true)
             .put("roles.g1.search", "(uniqueMember={0})")
+            .putList(ConfigConstants.LDAP_AUTHZ_EXCLUDE_ROLES, List.of("nested2"))
             .build();
 
         final User user = new User("spock");
 
-        new LDAPAuthorizationBackend(settings, null).fillRoles(user, null);
+        new LDAPAuthorizationBackend2(settings, null).fillRoles(user, null);
 
         Assert.assertNotNull(user);
         Assert.assertEquals("spock", user.getName());
-        Assert.assertEquals(4, user.getRoles().size());
-        Assert.assertEquals("nested1", new ArrayList<>(new TreeSet<>(user.getRoles())).get(1));
+        Assert.assertEquals(3, user.getRoles().size());
+        Assert.assertTrue(user.getRoles().contains("nested1"));
+        Assert.assertFalse(user.getRoles().contains("nested2"));
     }
 
     @Test
@@ -759,7 +762,7 @@ public void testLdapAuthorizationNestedAttrFilter() throws Exception {
     }
 
     @Test
-    public void testLdapAuthorizationNestedAttrFilterAll() throws Exception {
+    public void testLdapAuthorizationNestedAttrFilterAll() {
 
         final Settings settings = createBaseSettings().putList(ConfigConstants.LDAP_HOSTS, "localhost:" + ldapPort)
             .put("users.u1.search", "(uid={0})")
@@ -780,7 +783,6 @@ public void testLdapAuthorizationNestedAttrFilterAll() throws Exception {
         Assert.assertNotNull(user);
         Assert.assertEquals("spock", user.getName());
         Assert.assertEquals(4, user.getRoles().size());
-
     }
 
     @Test

diff --git a/src/test/java/org/opensearch/security/UtilTests.java b/src/test/java/org/opensearch/security/UtilTests.java
@@ -70,6 +70,8 @@ public void testWildcardMatcherClasses() {
         assertTrue(wc("/\\S+/").test("abc"));
         assertTrue(wc("abc").test("abc"));
         assertFalse(wc("ABC").test("abc"));
+        assertFalse(wc(null).test("abc"));
+        assertTrue(WildcardMatcher.from(null, "abc").test("abc"));
     }
 
     @Test
-Original file line number
+Diff line change
@@ Expand Up / @@ -122,7 +122,6 @@ private static Object escapeForwardSlash(Object input) { @@
             } else {
                 return input;
             }
         }
     }