Skip to content

Commit 83fed50

Browse files
albertzaharovitsalbertzaharovits
authored
Fix NPE in Logfile Audit Filter (#38120) (#38274)
The culprit in #38097 is an `IndicesRequest` that has no indices, but instead of `request.indices()` returning `null` or `String[0]` it returned `String[] {null}` . This tripped the audit filter. I have addressed this in two ways: 1. `request.indices()` returning `String[] {null}` is treated as `null` or `String[0]`, i.e. no indices 2. `null` values among the roles and indices lists, which are unexpected, will never again stumble the audit filter; `null` values are treated as special values that will not match any policy, i.e. their events will always be printed. Closes #38097
1 parent 610161b commit 83fed50

File tree

2 files changed

+91
-6
lines changed

2 files changed

+91
-6
lines changed

‎x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@
5252
import java.util.HashMap;
5353
import java.util.List;
5454
import java.util.Map;
55+
import java.util.Objects;
5556
import java.util.Optional;
5657
import java.util.TreeMap;
5758
import java.util.function.Function;
@@ -807,6 +808,7 @@ static final class EventFilterPolicy {
807808
EventFilterPolicy(String name, Predicate<String> ignorePrincipalsPredicate, Predicate<String> ignoreRealmsPredicate,
808809
Predicate<String> ignoreRolesPredicate, Predicate<String> ignoreIndicesPredicate) {
809810
this.name = name;
811+
// "null" values are "unexpected" and should not match any ignore policy
810812
this.ignorePrincipalsPredicate = ignorePrincipalsPredicate;
811813
this.ignoreRealmsPredicate = ignoreRealmsPredicate;
812814
this.ignoreRolesPredicate = ignoreRolesPredicate;
@@ -856,8 +858,10 @@ private static List<String> emptyStringBuildsEmptyAutomaton(List<String> l) {
856858
* predicate of the corresponding field.
857859
*/
858860
Predicate<AuditEventMetaInfo> ignorePredicate() {
859-
return eventInfo -> ignorePrincipalsPredicate.test(eventInfo.principal) && ignoreRealmsPredicate.test(eventInfo.realm)
860-
&& eventInfo.roles.get().allMatch(ignoreRolesPredicate) && eventInfo.indices.get().allMatch(ignoreIndicesPredicate);
861+
return eventInfo -> eventInfo.principal != null && ignorePrincipalsPredicate.test(eventInfo.principal)
862+
&& eventInfo.realm != null && ignoreRealmsPredicate.test(eventInfo.realm)
863+
&& eventInfo.roles.get().allMatch(role -> role != null && ignoreRolesPredicate.test(role))
864+
&& eventInfo.indices.get().allMatch(index -> index != null && ignoreIndicesPredicate.test(index));
861865
}
862866

863867
@Override
@@ -945,8 +949,10 @@ static final class AuditEventMetaInfo {
945949
// conditions on the `principal` and `realm` fields
946950
// 2. reusability of the AuditEventMetaInfo instance: in this case Streams have
947951
// to be regenerated as they cannot be operated upon twice
948-
this.roles = () -> roles.filter(r -> r.length != 0).map(Arrays::stream).orElse(Stream.of(""));
949-
this.indices = () -> indices.filter(i -> i.length != 0).map(Arrays::stream).orElse(Stream.of(""));
952+
this.roles = () -> roles.filter(r -> r.length > 0).filter(a -> Arrays.stream(a).anyMatch(Objects::nonNull))
953+
.map(Arrays::stream).orElse(Stream.of(""));
954+
this.indices = () -> indices.filter(i -> i.length > 0).filter(a -> Arrays.stream(a).anyMatch(Objects::nonNull))
955+
.map(Arrays::stream).orElse(Stream.of(""));
950956
}
951957

952958
AuditEventMetaInfo(Optional<AuthenticationToken> authenticationToken, Optional<String> realm, Optional<String[]> indices) {

‎x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrailFilterTests.java‎

Lines changed: 81 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,78 @@ public void init() throws Exception {
8282
}).when(clusterService).addListener(Mockito.isA(LoggingAuditTrail.class));
8383
}
8484

85+
public void testPolicyDoesNotMatchNullValuesInEvent() throws Exception {
86+
final Logger logger = CapturingLogger.newCapturingLogger(Level.INFO, null);
87+
final ThreadContext threadContext = new ThreadContext(Settings.EMPTY);
88+
final Settings.Builder settingsBuilder = Settings.builder().put(settings);
89+
// filter by username
90+
final List<String> filteredUsernames = randomNonEmptyListOfFilteredNames();
91+
final List<User> filteredUsers = filteredUsernames.stream().map(u -> {
92+
if (randomBoolean()) {
93+
return new User(u);
94+
} else {
95+
return new User(new User(u), new User(UNFILTER_MARKER + randomAlphaOfLengthBetween(1, 4)));
96+
}
97+
}).collect(Collectors.toList());
98+
settingsBuilder.putList("xpack.security.audit.logfile.events.ignore_filters.userPolicy.users", filteredUsernames);
99+
// filter by realms
100+
final List<String> filteredRealms = randomNonEmptyListOfFilteredNames();
101+
settingsBuilder.putList("xpack.security.audit.logfile.events.ignore_filters.realmsPolicy.realms", filteredRealms);
102+
// filter by roles
103+
final List<String> filteredRoles = randomNonEmptyListOfFilteredNames();
104+
settingsBuilder.putList("xpack.security.audit.logfile.events.ignore_filters.rolesPolicy.roles", filteredRoles);
105+
// filter by indices
106+
final List<String> filteredIndices = randomNonEmptyListOfFilteredNames();
107+
settingsBuilder.putList("xpack.security.audit.logfile.events.ignore_filters.indicesPolicy.indices", filteredIndices);
108+
109+
final LoggingAuditTrail auditTrail = new LoggingAuditTrail(settingsBuilder.build(), clusterService, logger, threadContext);
110+
111+
// user field matches
112+
assertTrue("Matches the user filter predicate.", auditTrail.eventFilterPolicyRegistry.ignorePredicate().test(
113+
new AuditEventMetaInfo(Optional.of(randomFrom(filteredUsers)), Optional.empty(), Optional.empty(), Optional.empty())));
114+
final User unfilteredUser;
115+
if (randomBoolean()) {
116+
unfilteredUser = new User(null);
117+
} else {
118+
unfilteredUser = new User(new User(null), new User(randomFrom(filteredUsers).principal()));
119+
}
120+
// null user field does NOT match
121+
assertFalse("Does not match the user filter predicate because of null username.",
122+
auditTrail.eventFilterPolicyRegistry.ignorePredicate()
123+
.test(new AuditEventMetaInfo(Optional.of(unfilteredUser), Optional.empty(), Optional.empty(), Optional.empty())));
124+
// realm field matches
125+
assertTrue("Matches the realm filter predicate.", auditTrail.eventFilterPolicyRegistry.ignorePredicate().test(
126+
new AuditEventMetaInfo(Optional.empty(), Optional.of(randomFrom(filteredRealms)), Optional.empty(), Optional.empty())));
127+
// null realm field does NOT match
128+
assertFalse("Does not match the realm filter predicate because of null realm.",
129+
auditTrail.eventFilterPolicyRegistry.ignorePredicate()
130+
.test(new AuditEventMetaInfo(Optional.empty(), Optional.ofNullable(null), Optional.empty(), Optional.empty())));
131+
// role field matches
132+
assertTrue("Matches the role filter predicate.", auditTrail.eventFilterPolicyRegistry.ignorePredicate()
133+
.test(new AuditEventMetaInfo(Optional.empty(), Optional.empty(),
134+
Optional.of(randomSubsetOf(randomIntBetween(1, filteredRoles.size()), filteredRoles).toArray(new String[0])),
135+
Optional.empty())));
136+
final List<String> unfilteredRoles = new ArrayList<>();
137+
unfilteredRoles.add(null);
138+
unfilteredRoles.addAll(randomSubsetOf(randomIntBetween(1, filteredRoles.size()), filteredRoles));
139+
// null role among roles field does NOT match
140+
assertFalse("Does not match the role filter predicate because of null role.",
141+
auditTrail.eventFilterPolicyRegistry.ignorePredicate().test(new AuditEventMetaInfo(Optional.empty(), Optional.empty(),
142+
Optional.of(unfilteredRoles.toArray(new String[0])), Optional.empty())));
143+
// indices field matches
144+
assertTrue("Matches the index filter predicate.",
145+
auditTrail.eventFilterPolicyRegistry.ignorePredicate().test(new AuditEventMetaInfo(Optional.empty(), Optional.empty(),
146+
Optional.empty(),
147+
Optional.of(randomSubsetOf(randomIntBetween(1, filteredIndices.size()), filteredIndices).toArray(new String[0])))));
148+
final List<String> unfilteredIndices = new ArrayList<>();
149+
unfilteredIndices.add(null);
150+
unfilteredIndices.addAll(randomSubsetOf(randomIntBetween(1, filteredIndices.size()), filteredIndices));
151+
// null index among indices field does NOT match
152+
assertFalse("Does not match the indices filter predicate because of null index.",
153+
auditTrail.eventFilterPolicyRegistry.ignorePredicate().test(new AuditEventMetaInfo(Optional.empty(), Optional.empty(),
154+
Optional.empty(), Optional.of(unfilteredIndices.toArray(new String[0])))));
155+
}
156+
85157
public void testSingleCompletePolicyPredicate() throws Exception {
86158
final Logger logger = CapturingLogger.newCapturingLogger(Level.INFO, null);
87159
final ThreadContext threadContext = new ThreadContext(Settings.EMPTY);
@@ -265,11 +337,18 @@ public void testSingleCompleteWithEmptyFieldPolicyPredicate() throws Exception {
265337
.test(new AuditEventMetaInfo(Optional.of(randomFrom(filteredUsers)), Optional.of(randomFrom(filteredRealms)),
266338
Optional.of(randomSubsetOf(randomIntBetween(1, filteredRoles.size()), filteredRoles).toArray(new String[0])),
267339
Optional.of(someIndicesDoNotMatch.toArray(new String[0])))));
268-
final Optional<String[]> emptyIndices = randomBoolean() ? Optional.empty() : Optional.of(new String[0]);
269340
assertTrue("Matches the filter predicate because of the empty indices.", auditTrail.eventFilterPolicyRegistry.ignorePredicate()
270341
.test(new AuditEventMetaInfo(Optional.of(randomFrom(filteredUsers)), Optional.of(randomFrom(filteredRealms)),
271342
Optional.of(randomSubsetOf(randomIntBetween(1, filteredRoles.size()), filteredRoles).toArray(new String[0])),
272-
emptyIndices)));
343+
Optional.empty())));
344+
assertTrue("Matches the filter predicate because of the empty indices.", auditTrail.eventFilterPolicyRegistry.ignorePredicate()
345+
.test(new AuditEventMetaInfo(Optional.of(randomFrom(filteredUsers)), Optional.of(randomFrom(filteredRealms)),
346+
Optional.of(randomSubsetOf(randomIntBetween(1, filteredRoles.size()), filteredRoles).toArray(new String[0])),
347+
Optional.of(new String[0]))));
348+
assertTrue("Matches the filter predicate because of the empty indices.", auditTrail.eventFilterPolicyRegistry.ignorePredicate()
349+
.test(new AuditEventMetaInfo(Optional.of(randomFrom(filteredUsers)), Optional.of(randomFrom(filteredRealms)),
350+
Optional.of(randomSubsetOf(randomIntBetween(1, filteredRoles.size()), filteredRoles).toArray(new String[0])),
351+
Optional.of(new String[] { null }))));
273352
}
274353

275354
public void testTwoPolicyPredicatesWithMissingFields() throws Exception {

0 commit comments

Comments
 (0)