From 7c67b49aa505498e11e51e82529931835d3fb1fc Mon Sep 17 00:00:00 2001 From: jaymode Date: Fri, 12 Jun 2015 08:33:46 -0400 Subject: [PATCH] system actions are only logged at the correct audit level Today, some system actions could be logged by default when the actions performed are not internal. Additionally for internal actions, we never checked if the user was the system user. This adds a check to ensure the user is the System user and that the actions that are being suppressed are known system actions. Closes elastic/elasticsearch#902 Original commit: elastic/x-pack-elasticsearch@b107994692e5d9be51aa77dfccbacb7cef0e124d --- .../shield/audit/index/IndexAuditTrail.java | 2 +- .../audit/logfile/LoggingAuditTrail.java | 2 +- .../elasticsearch/shield/authz/Privilege.java | 4 -- .../audit/index/IndexAuditTrailTests.java | 36 ++++++++++++++++-- .../audit/logfile/LoggingAuditTrailTests.java | 37 +++++++++++++++++-- 5 files changed, 68 insertions(+), 13 deletions(-) diff --git a/src/main/java/org/elasticsearch/shield/audit/index/IndexAuditTrail.java b/src/main/java/org/elasticsearch/shield/audit/index/IndexAuditTrail.java index 4226aff7ea2..1ee5426638f 100644 --- a/src/main/java/org/elasticsearch/shield/audit/index/IndexAuditTrail.java +++ b/src/main/java/org/elasticsearch/shield/audit/index/IndexAuditTrail.java @@ -193,7 +193,7 @@ public class IndexAuditTrail implements AuditTrail { if (enabled.contains(Level.ACCESS_GRANTED)) { if (!principalIsAuditor(user.principal())) { // special treatment for internal system actions - only log if explicitly told to - if (Privilege.SYSTEM.internalActionPredicate().apply(action)) { + if (user.isSystem() && Privilege.SYSTEM.predicate().apply(action)) { if (enabled.contains(Level.SYSTEM_ACCESS_GRANTED)) { try { processor.submit(message("access_granted", action, user.principal(), null, indices(message), message)); diff --git a/src/main/java/org/elasticsearch/shield/audit/logfile/LoggingAuditTrail.java b/src/main/java/org/elasticsearch/shield/audit/logfile/LoggingAuditTrail.java index 0380625f055..5afae352c66 100644 --- a/src/main/java/org/elasticsearch/shield/audit/logfile/LoggingAuditTrail.java +++ b/src/main/java/org/elasticsearch/shield/audit/logfile/LoggingAuditTrail.java @@ -140,7 +140,7 @@ public class LoggingAuditTrail implements AuditTrail { String indices = indices(message); // special treatment for internal system actions - only log on trace - if (Privilege.SYSTEM.internalActionPredicate().apply(action)) { + if (user.isSystem() && Privilege.SYSTEM.predicate().apply(action)) { if (logger.isTraceEnabled()) { if (indices != null) { logger.trace("{}[transport] [access_granted]\t{}, principal=[{}], action=[{}], indices=[{}], request=[{}]", prefix, originAttributes(message), user.principal(), action, indices, message.getClass().getSimpleName()); diff --git a/src/main/java/org/elasticsearch/shield/authz/Privilege.java b/src/main/java/org/elasticsearch/shield/authz/Privilege.java index c3c0b13fa8c..e7ac4e7c4de 100644 --- a/src/main/java/org/elasticsearch/shield/authz/Privilege.java +++ b/src/main/java/org/elasticsearch/shield/authz/Privilege.java @@ -103,10 +103,6 @@ public abstract class Privilege

> { return PREDICATE; } - public Predicate internalActionPredicate() { - return INTERNAL_PREDICATE; - } - @Override public boolean implies(System other) { return true; diff --git a/src/test/java/org/elasticsearch/shield/audit/index/IndexAuditTrailTests.java b/src/test/java/org/elasticsearch/shield/audit/index/IndexAuditTrailTests.java index 94ef315c849..21a5cc9a54c 100644 --- a/src/test/java/org/elasticsearch/shield/audit/index/IndexAuditTrailTests.java +++ b/src/test/java/org/elasticsearch/shield/audit/index/IndexAuditTrailTests.java @@ -80,17 +80,19 @@ public class IndexAuditTrailTests extends ShieldIntegrationTest { .build(); } - private Settings mutedSettings(String... muted) { + private Settings mutedSettings(boolean systemEnabled, String... muted) { Settings.Builder builder = Settings.builder(); for (String mute : muted) { builder.put("shield.audit.index.events." + mute, false); } + + builder.put("shield.audit.index.events.system.access_granted", systemEnabled); return builder.build(); } - private Settings settings(IndexNameResolver.Rollover rollover, String... muted) { + private Settings settings(IndexNameResolver.Rollover rollover, boolean systemEnabled, String... muted) { Settings.Builder builder = Settings.builder(); - builder.put(mutedSettings(muted)); + builder.put(mutedSettings(systemEnabled, muted)); builder.put(commonSettings(rollover)); return builder.build(); } @@ -110,9 +112,12 @@ public class IndexAuditTrailTests extends ShieldIntegrationTest { } private void initialize(String... muted) { + initialize(false, muted); + } + private void initialize(boolean systemEnabled, String... muted) { rollover = randomFrom(HOURLY, DAILY, WEEKLY, MONTHLY); - Settings settings = settings(rollover, muted); + Settings settings = settings(rollover, systemEnabled, muted); remoteIndexing = randomBoolean(); if (remoteIndexing) { @@ -336,6 +341,29 @@ public class IndexAuditTrailTests extends ShieldIntegrationTest { getClient().prepareExists(resolveIndexName()).execute().actionGet(); } + @Test + public void testSystemAccessGranted() throws Exception { + initialize(true); + TransportMessage message = randomBoolean() ? new RemoteHostMockMessage() : new LocalHostMockMessage(); + auditor.accessGranted(User.SYSTEM, "internal:_action", message); + awaitIndexCreation(resolveIndexName()); + + SearchHit hit = getIndexedAuditMessage(); + assertAuditMessage(hit, "transport", "access_granted"); + assertEquals("transport", hit.field("origin_type").getValue()); + assertEquals(User.SYSTEM.principal(), hit.field("principal").getValue()); + assertEquals("internal:_action", hit.field("action").getValue()); + } + + @Test(expected = IndexMissingException.class) + public void testSystemAccessGranted_Muted() throws Exception { + initialize(); + TransportMessage message = randomBoolean() ? new RemoteHostMockMessage() : new LocalHostMockMessage(); + auditor.accessGranted(User.SYSTEM, "internal:_action", message); + getClient().prepareExists(resolveIndexName()).execute().actionGet(); + awaitIndexCreation(resolveIndexName()); + } + @Test public void testAccessDenied() throws Exception { diff --git a/src/test/java/org/elasticsearch/shield/audit/logfile/LoggingAuditTrailTests.java b/src/test/java/org/elasticsearch/shield/audit/logfile/LoggingAuditTrailTests.java index f732974e6c2..fd50b847b06 100644 --- a/src/test/java/org/elasticsearch/shield/audit/logfile/LoggingAuditTrailTests.java +++ b/src/test/java/org/elasticsearch/shield/audit/logfile/LoggingAuditTrailTests.java @@ -307,7 +307,7 @@ public class LoggingAuditTrailTests extends ElasticsearchTestCase { LoggingAuditTrail auditTrail = new LoggingAuditTrail(settings, logger); TransportMessage message = randomBoolean() ? new MockMessage() : new MockIndicesRequest(); String origins = LoggingAuditTrail.originAttributes(message); - auditTrail.accessGranted(new User.Simple("_username", "r1"), "internal:_action", message); + auditTrail.accessGranted(User.SYSTEM, "internal:_action", message); switch (level) { case ERROR: case WARN: @@ -317,9 +317,40 @@ public class LoggingAuditTrailTests extends ElasticsearchTestCase { break; case TRACE: if (message instanceof IndicesRequest) { - assertMsg(logger, Level.TRACE, prefix + "[transport] [access_granted]\t" + origins + ", principal=[_username], action=[internal:_action], indices=[idx1,idx2], request=[MockIndicesRequest]"); + assertMsg(logger, Level.TRACE, prefix + "[transport] [access_granted]\t" + origins + ", principal=[" + User.SYSTEM.principal() + "], action=[internal:_action], indices=[idx1,idx2], request=[MockIndicesRequest]"); } else { - assertMsg(logger, Level.TRACE, prefix + "[transport] [access_granted]\t" + origins + ", principal=[_username], action=[internal:_action], request=[MockMessage]"); + assertMsg(logger, Level.TRACE, prefix + "[transport] [access_granted]\t" + origins + ", principal=[" + User.SYSTEM.principal() + "], action=[internal:_action], request=[MockMessage]"); + } + } + } + } + + @Test + public void testAccessGranted_InternalSystemAction_NonSystemUser() throws Exception { + for (Level level : Level.values()) { + CapturingLogger logger = new CapturingLogger(level); + LoggingAuditTrail auditTrail = new LoggingAuditTrail(settings, logger); + TransportMessage message = randomBoolean() ? new MockMessage() : new MockIndicesRequest(); + String origins = LoggingAuditTrail.originAttributes(message); + auditTrail.accessGranted(new User.Simple("_username"), "internal:_action", message); + switch (level) { + case ERROR: + case WARN: + assertEmptyLog(logger); + break; + case INFO: + if (message instanceof IndicesRequest) { + assertMsg(logger, Level.INFO, prefix + "[transport] [access_granted]\t" + origins + ", principal=[_username], action=[internal:_action], indices=[idx1,idx2]"); + } else { + assertMsg(logger, Level.INFO, prefix + "[transport] [access_granted]\t" + origins + ", principal=[_username], action=[internal:_action]"); + } + break; + case DEBUG: + case TRACE: + if (message instanceof IndicesRequest) { + assertMsg(logger, Level.DEBUG, prefix + "[transport] [access_granted]\t" + origins + ", principal=[_username], action=[internal:_action], indices=[idx1,idx2], request=[MockIndicesRequest]"); + } else { + assertMsg(logger, Level.DEBUG, prefix + "[transport] [access_granted]\t" + origins + ", principal=[_username], action=[internal:_action], request=[MockMessage]"); } } }