From 9c351332e0735ab277dd23a6852b82407cfee6a1 Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Thu, 31 Jan 2019 19:24:15 +0200 Subject: [PATCH 1/6] Main code --- .../xpack/security/audit/logfile/LoggingAuditTrail.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java index 045140e331f28..664a4e0fef0f5 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java @@ -790,8 +790,10 @@ String toQuotedJsonArray(String[] values) { private static Optional indices(TransportMessage message) { if (message instanceof IndicesRequest) { final String[] indices = ((IndicesRequest) message).indices(); - if (indices != null) { - return Optional.of(((IndicesRequest) message).indices()); + if (indices == null || indices.length == 0 || indices[0] == null) { + return Optional.empty(); + } else { + return Optional.of(indices); } } return Optional.empty(); From 0353f2a7f69be4a1b401cf6ac5c098109aa229da Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Thu, 31 Jan 2019 20:02:01 +0200 Subject: [PATCH 2/6] mhm --- .../xpack/security/audit/logfile/LoggingAuditTrail.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java index 664a4e0fef0f5..417204f150804 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java @@ -985,8 +985,10 @@ static final class AuditEventMetaInfo { // conditions on the `principal` and `realm` fields // 2. reusability of the AuditEventMetaInfo instance: in this case Streams have // to be regenerated as they cannot be operated upon twice - this.roles = () -> roles.filter(r -> r.length != 0).map(Arrays::stream).orElse(Stream.of("")); - this.indices = () -> indices.filter(i -> i.length != 0).map(Arrays::stream).orElse(Stream.of("")); + this.roles = () -> roles.filter(r -> r.length > 0).filter(r -> r.length > 1 || r[0] != null).map(Arrays::stream) + .orElse(Stream.of("")); + this.indices = () -> indices.filter(i -> i.length > 0).filter(i -> i.length > 1 || i[0] != null).map(Arrays::stream) + .orElse(Stream.of("")); } AuditEventMetaInfo(Optional authenticationToken, Optional realm, Optional indices) { From 32da946d91b2b7c842ece2331fabb92ae890e178 Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Thu, 31 Jan 2019 20:02:22 +0200 Subject: [PATCH 3/6] Revert "Main code" This reverts commit 9c351332e0735ab277dd23a6852b82407cfee6a1. --- .../xpack/security/audit/logfile/LoggingAuditTrail.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java index 417204f150804..0796df7fed83e 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java @@ -790,10 +790,8 @@ String toQuotedJsonArray(String[] values) { private static Optional indices(TransportMessage message) { if (message instanceof IndicesRequest) { final String[] indices = ((IndicesRequest) message).indices(); - if (indices == null || indices.length == 0 || indices[0] == null) { - return Optional.empty(); - } else { - return Optional.of(indices); + if (indices != null) { + return Optional.of(((IndicesRequest) message).indices()); } } return Optional.empty(); From 69dd8cf8e2ac6cdf810fcba929bbf4085eb4b323 Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Thu, 31 Jan 2019 21:32:58 +0200 Subject: [PATCH 4/6] Tests --- .../audit/logfile/LoggingAuditTrail.java | 9 +- .../logfile/LoggingAuditTrailFilterTests.java | 83 ++++++++++++++++++- 2 files changed, 86 insertions(+), 6 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java index 0796df7fed83e..7127b28cb24fc 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java @@ -845,10 +845,11 @@ private static final class EventFilterPolicy { EventFilterPolicy(String name, Predicate ignorePrincipalsPredicate, Predicate ignoreRealmsPredicate, Predicate ignoreRolesPredicate, Predicate ignoreIndicesPredicate) { this.name = name; - this.ignorePrincipalsPredicate = ignorePrincipalsPredicate; - this.ignoreRealmsPredicate = ignoreRealmsPredicate; - this.ignoreRolesPredicate = ignoreRolesPredicate; - this.ignoreIndicesPredicate = ignoreIndicesPredicate; + // "null" values are "unexpected" and should not match any ignore policy + this.ignorePrincipalsPredicate = principal -> principal != null && ignorePrincipalsPredicate.test(principal); + this.ignoreRealmsPredicate = realm -> realm != null && ignoreRealmsPredicate.test(realm); + this.ignoreRolesPredicate = role -> role != null && ignoreRolesPredicate.test(role); + this.ignoreIndicesPredicate = index -> index != null && ignoreIndicesPredicate.test(index); } private EventFilterPolicy changePrincipalsFilter(List filtersList) { diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrailFilterTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrailFilterTests.java index 8cb3dbf01b247..6d4bb155ba704 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrailFilterTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrailFilterTests.java @@ -82,6 +82,78 @@ public void init() throws Exception { }).when(clusterService).addListener(Mockito.isA(LoggingAuditTrail.class)); } + public void testPolicyDoesNotMatchNullValuesInEvent() throws Exception { + final Logger logger = CapturingLogger.newCapturingLogger(Level.INFO, null); + final ThreadContext threadContext = new ThreadContext(Settings.EMPTY); + final Settings.Builder settingsBuilder = Settings.builder().put(settings); + // filter by username + final List filteredUsernames = randomNonEmptyListOfFilteredNames(); + final List filteredUsers = filteredUsernames.stream().map(u -> { + if (randomBoolean()) { + return new User(u); + } else { + return new User(new User(u), new User(UNFILTER_MARKER + randomAlphaOfLengthBetween(1, 4))); + } + }).collect(Collectors.toList()); + settingsBuilder.putList("xpack.security.audit.logfile.events.ignore_filters.userPolicy.users", filteredUsernames); + // filter by realms + final List filteredRealms = randomNonEmptyListOfFilteredNames(); + settingsBuilder.putList("xpack.security.audit.logfile.events.ignore_filters.realmsPolicy.realms", filteredRealms); + // filter by roles + final List filteredRoles = randomNonEmptyListOfFilteredNames(); + settingsBuilder.putList("xpack.security.audit.logfile.events.ignore_filters.rolesPolicy.roles", filteredRoles); + // filter by indices + final List filteredIndices = randomNonEmptyListOfFilteredNames(); + settingsBuilder.putList("xpack.security.audit.logfile.events.ignore_filters.indicesPolicy.indices", filteredIndices); + + final LoggingAuditTrail auditTrail = new LoggingAuditTrail(settingsBuilder.build(), clusterService, logger, threadContext); + + // user field matches + assertTrue("Matches the user filter predicate.", auditTrail.eventFilterPolicyRegistry.ignorePredicate().test( + new AuditEventMetaInfo(Optional.of(randomFrom(filteredUsers)), Optional.empty(), Optional.empty(), Optional.empty()))); + final User unfilteredUser; + if (randomBoolean()) { + unfilteredUser = new User(null); + } else { + unfilteredUser = new User(new User(null), new User(randomFrom(filteredUsers).principal())); + } + // null user field does NOT match + assertFalse("Does not match the user filter predicate because of null username.", + auditTrail.eventFilterPolicyRegistry.ignorePredicate() + .test(new AuditEventMetaInfo(Optional.of(unfilteredUser), Optional.empty(), Optional.empty(), Optional.empty()))); + // realm field matches + assertTrue("Matches the realm filter predicate.", auditTrail.eventFilterPolicyRegistry.ignorePredicate().test( + new AuditEventMetaInfo(Optional.empty(), Optional.of(randomFrom(filteredRealms)), Optional.empty(), Optional.empty()))); + // null realm field does NOT match + assertFalse("Does not match the realm filter predicate because of null realm.", + auditTrail.eventFilterPolicyRegistry.ignorePredicate() + .test(new AuditEventMetaInfo(Optional.empty(), Optional.ofNullable(null), Optional.empty(), Optional.empty()))); + // role field matches + assertTrue("Matches the role filter predicate.", auditTrail.eventFilterPolicyRegistry.ignorePredicate() + .test(new AuditEventMetaInfo(Optional.empty(), Optional.empty(), + Optional.of(randomSubsetOf(randomIntBetween(1, filteredRoles.size()), filteredRoles).toArray(new String[0])), + Optional.empty()))); + final List unfilteredRoles = new ArrayList<>(); + unfilteredRoles.add(null); + unfilteredRoles.addAll(randomSubsetOf(randomIntBetween(1, filteredRoles.size()), filteredRoles)); + // null role among roles field does NOT match + assertFalse("Does not match the role filter predicate because of null role.", + auditTrail.eventFilterPolicyRegistry.ignorePredicate().test(new AuditEventMetaInfo(Optional.empty(), Optional.empty(), + Optional.of(unfilteredRoles.toArray(new String[0])), Optional.empty()))); + // indices field matches + assertTrue("Matches the index filter predicate.", + auditTrail.eventFilterPolicyRegistry.ignorePredicate().test(new AuditEventMetaInfo(Optional.empty(), Optional.empty(), + Optional.empty(), + Optional.of(randomSubsetOf(randomIntBetween(1, filteredIndices.size()), filteredIndices).toArray(new String[0]))))); + final List unfilteredIndices = new ArrayList<>(); + unfilteredIndices.add(null); + unfilteredIndices.addAll(randomSubsetOf(randomIntBetween(1, filteredIndices.size()), filteredIndices)); + // null index among indices field does NOT match + assertFalse("Does not match the indices filter predicate because of null index.", + auditTrail.eventFilterPolicyRegistry.ignorePredicate().test(new AuditEventMetaInfo(Optional.empty(), Optional.empty(), + Optional.empty(), Optional.of(unfilteredIndices.toArray(new String[0]))))); + } + public void testSingleCompletePolicyPredicate() throws Exception { final Logger logger = CapturingLogger.newCapturingLogger(Level.INFO, null); final ThreadContext threadContext = new ThreadContext(Settings.EMPTY); @@ -265,11 +337,18 @@ public void testSingleCompleteWithEmptyFieldPolicyPredicate() throws Exception { .test(new AuditEventMetaInfo(Optional.of(randomFrom(filteredUsers)), Optional.of(randomFrom(filteredRealms)), Optional.of(randomSubsetOf(randomIntBetween(1, filteredRoles.size()), filteredRoles).toArray(new String[0])), Optional.of(someIndicesDoNotMatch.toArray(new String[0]))))); - final Optional emptyIndices = randomBoolean() ? Optional.empty() : Optional.of(new String[0]); assertTrue("Matches the filter predicate because of the empty indices.", auditTrail.eventFilterPolicyRegistry.ignorePredicate() .test(new AuditEventMetaInfo(Optional.of(randomFrom(filteredUsers)), Optional.of(randomFrom(filteredRealms)), Optional.of(randomSubsetOf(randomIntBetween(1, filteredRoles.size()), filteredRoles).toArray(new String[0])), - emptyIndices))); + Optional.empty()))); + assertTrue("Matches the filter predicate because of the empty indices.", auditTrail.eventFilterPolicyRegistry.ignorePredicate() + .test(new AuditEventMetaInfo(Optional.of(randomFrom(filteredUsers)), Optional.of(randomFrom(filteredRealms)), + Optional.of(randomSubsetOf(randomIntBetween(1, filteredRoles.size()), filteredRoles).toArray(new String[0])), + Optional.of(new String[0])))); + assertTrue("Matches the filter predicate because of the empty indices.", auditTrail.eventFilterPolicyRegistry.ignorePredicate() + .test(new AuditEventMetaInfo(Optional.of(randomFrom(filteredUsers)), Optional.of(randomFrom(filteredRealms)), + Optional.of(randomSubsetOf(randomIntBetween(1, filteredRoles.size()), filteredRoles).toArray(new String[0])), + Optional.of(new String[] { null })))); } public void testTwoPolicyPredicatesWithMissingFields() throws Exception { From daa35313631d60780993a8137a3ee9a64791d405 Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Sat, 2 Feb 2019 01:17:48 +0200 Subject: [PATCH 5/6] Follow Tim's suggestion --- .../xpack/security/audit/logfile/LoggingAuditTrail.java | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java index 7127b28cb24fc..c72905ddd1ef4 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java @@ -50,6 +50,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Optional; import java.util.TreeMap; import java.util.function.Function; @@ -984,10 +985,10 @@ static final class AuditEventMetaInfo { // conditions on the `principal` and `realm` fields // 2. reusability of the AuditEventMetaInfo instance: in this case Streams have // to be regenerated as they cannot be operated upon twice - this.roles = () -> roles.filter(r -> r.length > 0).filter(r -> r.length > 1 || r[0] != null).map(Arrays::stream) - .orElse(Stream.of("")); - this.indices = () -> indices.filter(i -> i.length > 0).filter(i -> i.length > 1 || i[0] != null).map(Arrays::stream) - .orElse(Stream.of("")); + this.roles = () -> roles.filter(r -> r.length > 0).filter(a -> Arrays.stream(a).anyMatch(Objects::nonNull)) + .map(Arrays::stream).orElse(Stream.of("")); + this.indices = () -> indices.filter(i -> i.length > 0).filter(a -> Arrays.stream(a).anyMatch(Objects::nonNull)) + .map(Arrays::stream).orElse(Stream.of("")); } AuditEventMetaInfo(Optional authenticationToken, Optional realm, Optional indices) { From 93dab367a1daaf35e4572a9366abad38eb5c3b32 Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Sat, 2 Feb 2019 16:58:50 +0200 Subject: [PATCH 6/6] Fix toString impacting AuditTrailSettingsUpdateTests --- .../security/audit/logfile/LoggingAuditTrail.java | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java index c72905ddd1ef4..d75eba4a42a0a 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java @@ -847,10 +847,10 @@ private static final class EventFilterPolicy { Predicate ignoreRolesPredicate, Predicate ignoreIndicesPredicate) { this.name = name; // "null" values are "unexpected" and should not match any ignore policy - this.ignorePrincipalsPredicate = principal -> principal != null && ignorePrincipalsPredicate.test(principal); - this.ignoreRealmsPredicate = realm -> realm != null && ignoreRealmsPredicate.test(realm); - this.ignoreRolesPredicate = role -> role != null && ignoreRolesPredicate.test(role); - this.ignoreIndicesPredicate = index -> index != null && ignoreIndicesPredicate.test(index); + this.ignorePrincipalsPredicate = ignorePrincipalsPredicate; + this.ignoreRealmsPredicate = ignoreRealmsPredicate; + this.ignoreRolesPredicate = ignoreRolesPredicate; + this.ignoreIndicesPredicate = ignoreIndicesPredicate; } private EventFilterPolicy changePrincipalsFilter(List filtersList) { @@ -896,8 +896,10 @@ private static List emptyStringBuildsEmptyAutomaton(List l) { * predicate of the corresponding field. */ Predicate ignorePredicate() { - return eventInfo -> ignorePrincipalsPredicate.test(eventInfo.principal) && ignoreRealmsPredicate.test(eventInfo.realm) - && eventInfo.roles.get().allMatch(ignoreRolesPredicate) && eventInfo.indices.get().allMatch(ignoreIndicesPredicate); + return eventInfo -> eventInfo.principal != null && ignorePrincipalsPredicate.test(eventInfo.principal) + && eventInfo.realm != null && ignoreRealmsPredicate.test(eventInfo.realm) + && eventInfo.roles.get().allMatch(role -> role != null && ignoreRolesPredicate.test(role)) + && eventInfo.indices.get().allMatch(index -> index != null && ignoreIndicesPredicate.test(index)); } @Override