From c3e0ae24d377f9fccd6edb326fee995dd2e6a8b4 Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Mon, 22 Apr 2019 10:25:24 +0300 Subject: [PATCH] Fix role mapping DN field wildcards for users with NULL DNs (#41343) The `DistinguishedNamePredicate`, used for matching users to role mapping expressions, should handle users with null DNs. But it fails to do so (and this is a NPE bug), if the role mapping expression contains a lucene regexp or a wildcard. The fix simplifies `DistinguishedNamePredicate` to not handle null DNs at all, and instead use the `ExpressionModel#NULL_PREDICATE` for the DN field, just like any other missing user field. --- .../authc/support/UserRoleMapper.java | 25 ++++++----- .../DistinguishedNamePredicateTests.java | 10 ++--- .../mapper/ExpressionRoleMappingTests.java | 45 +++++++++++++++++-- 3 files changed, 58 insertions(+), 22 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/UserRoleMapper.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/UserRoleMapper.java index e55530bb5de..dbc32381061 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/UserRoleMapper.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/UserRoleMapper.java @@ -79,8 +79,12 @@ public interface UserRoleMapper { public ExpressionModel asModel() { final ExpressionModel model = new ExpressionModel(); model.defineField("username", username); - model.defineField("dn", dn, new DistinguishedNamePredicate(dn)); + if (dn != null) { + // null dn fields get the default NULL_PREDICATE + model.defineField("dn", dn, new DistinguishedNamePredicate(dn)); + } model.defineField("groups", groups, groups.stream() + .filter(group -> group != null) .>map(DistinguishedNamePredicate::new) .reduce(Predicate::or) .orElse(fieldValue -> false) @@ -165,22 +169,19 @@ public interface UserRoleMapper { private final DN dn; public DistinguishedNamePredicate(String string) { + assert string != null : "DN string should not be null. Use the dedicated NULL_PREDICATE for every user null field."; this.string = string; this.dn = parseDn(string); } private static DN parseDn(String string) { - if (string == null) { - return null; - } else { - try { - return new DN(string); - } catch (LDAPException | LDAPSDKUsageException e) { - if (LOGGER.isTraceEnabled()) { - LOGGER.trace(new ParameterizedMessage("failed to parse [{}] as a DN", string), e); - } - return null; + try { + return new DN(string); + } catch (LDAPException | LDAPSDKUsageException e) { + if (LOGGER.isTraceEnabled()) { + LOGGER.trace(new ParameterizedMessage("failed to parse [{}] as a DN", string), e); } + return null; } } @@ -240,7 +241,7 @@ public interface UserRoleMapper { } return testString.equalsIgnoreCase(dn.toNormalizedString()); } - return string == null && fieldValue.getValue() == null; + return false; } } } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/DistinguishedNamePredicateTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/DistinguishedNamePredicateTests.java index d04f0ad7f93..51ea82fc0e4 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/DistinguishedNamePredicateTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/DistinguishedNamePredicateTests.java @@ -49,27 +49,25 @@ public class DistinguishedNamePredicateTests extends ESTestCase { } public void testParsingMalformedInput() { - Predicate predicate = new UserRoleMapper.DistinguishedNamePredicate(null); - assertPredicate(predicate, null, true); - assertPredicate(predicate, "", false); - assertPredicate(predicate, randomAlphaOfLengthBetween(1, 8), false); - - predicate = new UserRoleMapper.DistinguishedNamePredicate(""); + Predicate predicate = new UserRoleMapper.DistinguishedNamePredicate(""); assertPredicate(predicate, null, false); assertPredicate(predicate, "", true); assertPredicate(predicate, randomAlphaOfLengthBetween(1, 8), false); + assertPredicate(predicate, randomAlphaOfLengthBetween(1, 8) + "*", false); predicate = new UserRoleMapper.DistinguishedNamePredicate("foo="); assertPredicate(predicate, null, false); assertPredicate(predicate, "foo", false); assertPredicate(predicate, "foo=", true); assertPredicate(predicate, randomAlphaOfLengthBetween(5, 12), false); + assertPredicate(predicate, randomAlphaOfLengthBetween(5, 12) + "*", false); predicate = new UserRoleMapper.DistinguishedNamePredicate("=bar"); assertPredicate(predicate, null, false); assertPredicate(predicate, "bar", false); assertPredicate(predicate, "=bar", true); assertPredicate(predicate, randomAlphaOfLengthBetween(5, 12), false); + assertPredicate(predicate, randomAlphaOfLengthBetween(5, 12) + "*", false); } private void assertPredicate(Predicate predicate, Object value, boolean expected) { diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/mapper/ExpressionRoleMappingTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/mapper/ExpressionRoleMappingTests.java index 57db6005119..42652676d39 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/mapper/ExpressionRoleMappingTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/mapper/ExpressionRoleMappingTests.java @@ -31,6 +31,7 @@ import org.elasticsearch.xpack.core.security.authc.RealmConfig; import org.elasticsearch.xpack.core.security.authc.support.mapper.ExpressionRoleMapping; import org.elasticsearch.xpack.core.security.authc.support.mapper.TemplateRoleName; import org.elasticsearch.xpack.core.security.authc.support.mapper.expressiondsl.AllExpression; +import org.elasticsearch.xpack.core.security.authc.support.mapper.expressiondsl.AnyExpression; import org.elasticsearch.xpack.core.security.authc.support.mapper.expressiondsl.FieldExpression; import org.elasticsearch.xpack.security.authc.support.UserRoleMapper; import org.hamcrest.Matchers; @@ -59,7 +60,7 @@ public class ExpressionRoleMappingTests extends ESTestCase { Settings.EMPTY, Mockito.mock(Environment.class), new ThreadContext(Settings.EMPTY)); } - public void testParseValidJsonWithFixedRoleNames() throws Exception { + public void testValidExpressionWithFixedRoleNames() throws Exception { String json = "{" + "\"roles\": [ \"kibana_user\", \"sales\" ], " + "\"enabled\": true, " @@ -69,7 +70,7 @@ public class ExpressionRoleMappingTests extends ESTestCase { + " { \"except\": { \"field\": { \"metadata.active\" : false } } }" + " ]}" + "}"; - final ExpressionRoleMapping mapping = parse(json, "ldap_sales"); + ExpressionRoleMapping mapping = parse(json, "ldap_sales"); assertThat(mapping.getRoles(), Matchers.containsInAnyOrder("kibana_user", "sales")); assertThat(mapping.getExpression(), instanceOf(AllExpression.class)); @@ -96,12 +97,48 @@ public class ExpressionRoleMappingTests extends ESTestCase { Collections.emptyList(), Collections.singletonMap("active", true), realm ); + final UserRoleMapper.UserData user4 = new UserRoleMapper.UserData( + "peter.null", null, Collections.emptyList(), Collections.singletonMap("active", true), realm + ); + assertThat(mapping.getExpression().match(user1a.asModel()), equalTo(true)); assertThat(mapping.getExpression().match(user1b.asModel()), equalTo(true)); assertThat(mapping.getExpression().match(user1c.asModel()), equalTo(true)); assertThat(mapping.getExpression().match(user1d.asModel()), equalTo(true)); - assertThat(mapping.getExpression().match(user2.asModel()), equalTo(false)); - assertThat(mapping.getExpression().match(user3.asModel()), equalTo(false)); + assertThat(mapping.getExpression().match(user2.asModel()), equalTo(false)); // metadata.active == false + assertThat(mapping.getExpression().match(user3.asModel()), equalTo(false)); // dn != ou=sales,dc=example,dc=com + assertThat(mapping.getExpression().match(user4.asModel()), equalTo(false)); // dn == null + + // expression without dn + json = "{" + + "\"roles\": [ \"superuser\", \"system_admin\", \"admin\" ], " + + "\"enabled\": true, " + + "\"rules\": { " + + " \"any\": [ " + + " { \"field\": { \"username\" : \"tony.stark\" } }, " + + " { \"field\": { \"groups\": \"cn=admins,dc=stark-enterprises,dc=com\" } }" + + " ]}" + + "}"; + mapping = parse(json, "stark_admin"); + assertThat(mapping.getRoles(), Matchers.containsInAnyOrder("superuser", "system_admin", "admin")); + assertThat(mapping.getExpression(), instanceOf(AnyExpression.class)); + + final UserRoleMapper.UserData userTony = new UserRoleMapper.UserData( + "tony.stark", null, Collections.singletonList("Audi R8 owners"), Collections.singletonMap("boss", true), realm + ); + final UserRoleMapper.UserData userPepper = new UserRoleMapper.UserData( + "pepper.potts", null, Arrays.asList("marvel", "cn=admins,dc=stark-enterprises,dc=com"), null, realm + ); + final UserRoleMapper.UserData userMax = new UserRoleMapper.UserData( + "max.rockatansky", null, Collections.singletonList("bronze"), Collections.singletonMap("mad", true), realm + ); + final UserRoleMapper.UserData userFinn = new UserRoleMapper.UserData( + "finn.hackleberry", null, Arrays.asList("hacker", null), null, realm + ); + assertThat(mapping.getExpression().match(userTony.asModel()), equalTo(true)); + assertThat(mapping.getExpression().match(userPepper.asModel()), equalTo(true)); + assertThat(mapping.getExpression().match(userMax.asModel()), equalTo(false)); + assertThat(mapping.getExpression().match(userFinn.asModel()), equalTo(false)); } public void testParseValidJsonWithTemplatedRoleNames() throws Exception {