Skip to content

Commit 3b0cd57

Browse files
slobodanadamovicslobodanadamovic
committed
Fix SAML private attributes predicate
The way predicate was constructed could result in a `NullPointerException` when `name` or `friendlyName` are null. This is because the `contains` method of `ImmutableCollections` does not accept nulls. This PR fixes this and adds more test coverage for these cases.
1 parent 500c4ff commit 3b0cd57

File tree

3 files changed

+169
-17
lines changed

3 files changed

+169
-17
lines changed

‎x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlPrivateAttributePredicate.java‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License
4+
* 2.0; you may not use this file except in compliance with the Elastic License
5+
* 2.0.
6+
*/
7+
8+
package org.elasticsearch.xpack.security.authc.saml;
9+
10+
import org.apache.logging.log4j.LogManager;
11+
import org.apache.logging.log4j.Logger;
12+
import org.elasticsearch.xpack.core.security.authc.RealmConfig;
13+
import org.elasticsearch.xpack.core.security.authc.saml.SamlRealmSettings;
14+
import org.elasticsearch.xpack.security.authc.saml.SamlAttributes.SamlPrivateAttribute;
15+
import org.opensaml.saml.saml2.core.Attribute;
16+
17+
import java.util.List;
18+
import java.util.Set;
19+
import java.util.function.Predicate;
20+
import java.util.stream.Collectors;
21+
22+
import static org.elasticsearch.xpack.core.security.authc.saml.SamlRealmSettings.PRIVATE_ATTRIBUTES;
23+
24+
/**
25+
* The predicate which is constructed based on the values of {@link SamlRealmSettings#PRIVATE_ATTRIBUTES} setting.
26+
* When the setting is configured, the attributes whose {@code Attribute#getName} or {@code Attribute#getFriendlyName} match,
27+
* will be treated as private ({@link SamlPrivateAttribute}).
28+
*/
29+
public class SamlPrivateAttributePredicate implements Predicate<Attribute> {
30+
31+
private static final Logger logger = LogManager.getLogger(SamlPrivateAttributePredicate.class);
32+
33+
private final Predicate<Attribute> predicate;
34+
35+
private SamlPrivateAttributePredicate(RealmConfig config) {
36+
this.predicate = buildPrivateAttributesPredicate(config);
37+
}
38+
39+
public static SamlPrivateAttributePredicate create(RealmConfig config) {
40+
return new SamlPrivateAttributePredicate(config);
41+
}
42+
43+
private static Predicate<Attribute> buildPrivateAttributesPredicate(RealmConfig config) {
44+
45+
if (false == config.hasSetting(PRIVATE_ATTRIBUTES)) {
46+
logger.trace("No SAML private attributes setting configured.");
47+
return attribute -> false;
48+
}
49+
50+
final List<String> attributesList = config.getSetting(PRIVATE_ATTRIBUTES);
51+
if (attributesList == null || attributesList.isEmpty()) {
52+
logger.trace("No SAML private attributes configured for setting [{}].", PRIVATE_ATTRIBUTES);
53+
return attribute -> false;
54+
}
55+
56+
final Set<String> attributesSet = attributesList.stream()
57+
.filter(name -> name != null && false == name.isBlank())
58+
.collect(Collectors.toUnmodifiableSet());
59+
60+
if (attributesSet.isEmpty()) {
61+
return attribute -> false;
62+
}
63+
64+
logger.trace("SAML private attributes configured: {}", attributesSet);
65+
return new Predicate<>() {
66+
67+
@Override
68+
public boolean test(Attribute attribute) {
69+
if (attribute == null) {
70+
return false;
71+
}
72+
if (attribute.getName() != null && attributesSet.contains(attribute.getName())) {
73+
return true;
74+
}
75+
return attribute.getFriendlyName() != null && attributesSet.contains(attribute.getFriendlyName());
76+
}
77+
78+
@Override
79+
public String toString() {
80+
return "SAML private attributes predicate for: " + attributesSet;
81+
}
82+
};
83+
}
84+
85+
@Override
86+
public boolean test(Attribute attribute) {
87+
return predicate.test(attribute);
88+
}
89+
90+
}

‎x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlRealm.java‎

Lines changed: 2 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,6 @@
135135
import static org.elasticsearch.xpack.core.security.authc.saml.SamlRealmSettings.NAME_ATTRIBUTE;
136136
import static org.elasticsearch.xpack.core.security.authc.saml.SamlRealmSettings.POPULATE_USER_METADATA;
137137
import static org.elasticsearch.xpack.core.security.authc.saml.SamlRealmSettings.PRINCIPAL_ATTRIBUTE;
138-
import static org.elasticsearch.xpack.core.security.authc.saml.SamlRealmSettings.PRIVATE_ATTRIBUTES;
139138
import static org.elasticsearch.xpack.core.security.authc.saml.SamlRealmSettings.SIGNING_KEY_ALIAS;
140139
import static org.elasticsearch.xpack.core.security.authc.saml.SamlRealmSettings.SIGNING_MESSAGE_TYPES;
141140
import static org.elasticsearch.xpack.core.security.authc.saml.SamlRealmSettings.SIGNING_SETTING_KEY;
@@ -223,13 +222,13 @@ public static SamlRealm create(
223222
final Clock clock = Clock.systemUTC();
224223
final IdpConfiguration idpConfiguration = getIdpConfiguration(config, metadataResolver, idpDescriptor);
225224
final TimeValue maxSkew = config.getSetting(CLOCK_SKEW);
226-
final Predicate<Attribute> secureAttributePredicate = secureAttributePredicate(config);
225+
final Predicate<Attribute> privateAttributePredicate = SamlPrivateAttributePredicate.create(config);
227226
final SamlAuthenticator authenticator = new SamlAuthenticator(
228227
clock,
229228
idpConfiguration,
230229
serviceProvider,
231230
maxSkew,
232-
secureAttributePredicate
231+
privateAttributePredicate
233232
);
234233
final SamlLogoutRequestHandler logoutHandler = new SamlLogoutRequestHandler(clock, idpConfiguration, serviceProvider, maxSkew);
235234
final SamlLogoutResponseHandler logoutResponseHandler = new SamlLogoutResponseHandler(
@@ -258,20 +257,6 @@ public static SamlRealm create(
258257
return realm;
259258
}
260259

261-
static Predicate<Attribute> secureAttributePredicate(RealmConfig config) {
262-
if (false == config.hasSetting(PRIVATE_ATTRIBUTES)) {
263-
return attribute -> false;
264-
}
265-
final List<String> secureAttributeNames = config.getSetting(PRIVATE_ATTRIBUTES);
266-
if (secureAttributeNames == null || secureAttributeNames.isEmpty()) {
267-
return attribute -> false;
268-
}
269-
270-
final Set<String> secureAttributeNamesSet = Set.copyOf(secureAttributeNames);
271-
return attribute -> attribute != null
272-
&& (secureAttributeNamesSet.contains(attribute.getName()) || secureAttributeNamesSet.contains(attribute.getFriendlyName()));
273-
}
274-
275260
public SpConfiguration getServiceProvider() {
276261
return serviceProvider;
277262
}

‎x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlSecureAttributePredicateTests.java‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License
4+
* 2.0; you may not use this file except in compliance with the Elastic License
5+
* 2.0.
6+
*/
7+
8+
package org.elasticsearch.xpack.security.authc.saml;
9+
10+
import org.elasticsearch.test.ESTestCase;
11+
import org.elasticsearch.xpack.core.security.authc.RealmConfig;
12+
import org.mockito.Mockito;
13+
import org.opensaml.saml.saml2.core.Attribute;
14+
15+
import java.util.List;
16+
17+
import static org.elasticsearch.xpack.core.security.authc.saml.SamlRealmSettings.PRIVATE_ATTRIBUTES;
18+
import static org.hamcrest.Matchers.is;
19+
import static org.mockito.Mockito.doReturn;
20+
import static org.mockito.Mockito.mock;
21+
import static org.mockito.Mockito.when;
22+
23+
public class SamlSecureAttributePredicateTests extends ESTestCase {
24+
25+
public void testPredicateWithSettingConfigured() {
26+
27+
final List<String> privateAttributes = List.of("private", "http://elastic.co/confidential");
28+
final RealmConfig config = realmConfig(privateAttributes);
29+
final SamlPrivateAttributePredicate predicate = SamlPrivateAttributePredicate.create(config);
30+
31+
final String privateAttribute = randomFrom(privateAttributes);
32+
final String nonPrivateAttribute = randomFrom(new String[] { null, " ", randomAlphaOfLengthBetween(0, 3) });
33+
34+
assertThat(predicate.test(attribute("private", "http://elastic.co/confidential")), is(true));
35+
assertThat(predicate.test(attribute(privateAttribute, nonPrivateAttribute)), is(true));
36+
assertThat(predicate.test(attribute(nonPrivateAttribute, privateAttribute)), is(true));
37+
38+
assertThat(predicate.test(attribute(privateAttribute, null)), is(true));
39+
assertThat(predicate.test(attribute(null, privateAttribute)), is(true));
40+
41+
assertThat(predicate.test(attribute(nonPrivateAttribute, null)), is(false));
42+
assertThat(predicate.test(attribute(null, nonPrivateAttribute)), is(false));
43+
assertThat(predicate.test(attribute(null, null)), is(false));
44+
45+
assertThat(predicate.test(attribute("something", "else")), is(false));
46+
assertThat(predicate.test(attribute("", "")), is(false));
47+
48+
}
49+
50+
public void testPredicateWhenSettingIsNotConfigured() {
51+
52+
List<String> privateAttributes = randomBoolean() ? List.of() : null;
53+
RealmConfig config = realmConfig(privateAttributes);
54+
SamlPrivateAttributePredicate predicate = SamlPrivateAttributePredicate.create(config);
55+
56+
String name = randomFrom(randomAlphaOfLengthBetween(0, 5), null);
57+
String friendlyName = randomFrom(randomAlphaOfLengthBetween(0, 5), null);
58+
59+
assertThat(predicate.test(attribute(name, friendlyName)), is(false));
60+
61+
}
62+
63+
private static Attribute attribute(String name, String friendlyName) {
64+
Attribute attribute = mock(Attribute.class);
65+
when(attribute.getName()).thenReturn(name);
66+
when(attribute.getFriendlyName()).thenReturn(friendlyName);
67+
return attribute;
68+
}
69+
70+
private static RealmConfig realmConfig(List<String> privateAttributeNames) {
71+
RealmConfig config = Mockito.mock(RealmConfig.class);
72+
when(config.hasSetting(PRIVATE_ATTRIBUTES)).thenReturn(privateAttributeNames != null);
73+
doReturn(privateAttributeNames).when(config).getSetting(PRIVATE_ATTRIBUTES);
74+
return config;
75+
}
76+
77+
}

0 commit comments

Comments
 (0)