From a4ddcbc1b51e5121426569287b970206c243c813 Mon Sep 17 00:00:00 2001 From: Slobodan Adamovic Date: Fri, 20 May 2022 13:16:18 +0200 Subject: [PATCH] Fix audit logging to consistently include port number in origin.address (#86732) This commit changes audit logging of `connection_denied` and `connection_granted` events in order to include a port number. Closes elastic/elasticsearch#86694 --- docs/changelog/86732.yaml | 5 ++++ .../en/security/auditing/event-types.asciidoc | 4 ++-- .../xpack/security/audit/AuditTrail.java | 9 ++++--- .../security/audit/AuditTrailService.java | 9 ++++--- .../audit/logfile/LoggingAuditTrail.java | 5 ++-- .../security/transport/filter/IPFilter.java | 6 ++--- .../audit/AuditTrailServiceTests.java | 5 ++-- .../logfile/LoggingAuditTrailFilterTests.java | 24 +++++++++++-------- .../audit/logfile/LoggingAuditTrailTests.java | 4 ++-- .../transport/filter/IPFilterTests.java | 8 +++---- 10 files changed, 43 insertions(+), 36 deletions(-) create mode 100644 docs/changelog/86732.yaml diff --git a/docs/changelog/86732.yaml b/docs/changelog/86732.yaml new file mode 100644 index 0000000000000..fff14e3cadb90 --- /dev/null +++ b/docs/changelog/86732.yaml @@ -0,0 +1,5 @@ +pr: 86732 +summary: Fix audit logging to consistently include port number in `origin.address` +area: Audit +type: bug +issues: [] diff --git a/x-pack/docs/en/security/auditing/event-types.asciidoc b/x-pack/docs/en/security/auditing/event-types.asciidoc index 2f92ee66c0c40..7cc041ff4ccf8 100644 --- a/x-pack/docs/en/security/auditing/event-types.asciidoc +++ b/x-pack/docs/en/security/auditing/event-types.asciidoc @@ -188,7 +188,7 @@ Logged when an incoming TCP connection does not pass the [source,js] {"type":"audit", "timestamp":"2020-12-30T21:47:31,526+0200", "node.id": "0RMNyghkQYCc_gVd1G6tZQ", "event.type":"ip_filter", "event.action": -"connection_denied", "origin.type":"rest", "origin.address":"10.10.0.20", +"connection_denied", "origin.type":"rest", "origin.address":"10.10.0.20:52314", "transport.profile":".http", "rule":"deny 10.10.0.0/16"} ==== @@ -203,7 +203,7 @@ for a specific profile. [source,js] {"type":"audit", "timestamp":"2020-12-30T21:47:31,526+0200", "node.id": "0RMNyghkQYCc_gVd1G6tZQ", "event.type":"ip_filter", "event.action": -"connection_granted", "origin.type":"rest", "origin.address":"::1", +"connection_granted", "origin.type":"rest", "origin.address":"[::1]:52314", "transport.profile":".http", "rule":"allow ::1,127.0.0.1"} ==== diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/AuditTrail.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/AuditTrail.java index d12c2080f5765..654c1dced5c30 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/AuditTrail.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/AuditTrail.java @@ -14,7 +14,6 @@ import org.elasticsearch.xpack.core.security.authz.AuthorizationEngine.AuthorizationInfo; import org.elasticsearch.xpack.security.transport.filter.SecurityIpFilterRule; -import java.net.InetAddress; import java.net.InetSocketAddress; public interface AuditTrail { @@ -66,14 +65,14 @@ void accessDenied( void tamperedRequest(String requestId, Authentication authentication, String action, TransportRequest transportRequest); /** - * The {@link #connectionGranted(InetAddress, String, SecurityIpFilterRule)} and - * {@link #connectionDenied(InetAddress, String, SecurityIpFilterRule)} methods do not have a requestId because they related to a + * The {@link #connectionGranted(InetSocketAddress, String, SecurityIpFilterRule)} and + * {@link #connectionDenied(InetSocketAddress, String, SecurityIpFilterRule)} methods do not have a requestId because they related to a * potentially long-lived TCP connection, not a single request. For both Transport and Rest connections, a single connection * granted/denied event is generated even if that connection is used for multiple Elasticsearch actions (potentially as different users) */ - void connectionGranted(InetAddress inetAddress, String profile, SecurityIpFilterRule rule); + void connectionGranted(InetSocketAddress inetAddress, String profile, SecurityIpFilterRule rule); - void connectionDenied(InetAddress inetAddress, String profile, SecurityIpFilterRule rule); + void connectionDenied(InetSocketAddress inetAddress, String profile, SecurityIpFilterRule rule); void runAsGranted( String requestId, diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/AuditTrailService.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/AuditTrailService.java index 7db7da5471d78..1c9a33971b3e8 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/AuditTrailService.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/AuditTrailService.java @@ -18,7 +18,6 @@ import org.elasticsearch.xpack.security.Security; import org.elasticsearch.xpack.security.transport.filter.SecurityIpFilterRule; -import java.net.InetAddress; import java.net.InetSocketAddress; import java.time.Duration; import java.time.Instant; @@ -150,10 +149,10 @@ public void tamperedRequest(String requestId, String action, TransportRequest tr public void tamperedRequest(String requestId, Authentication authentication, String action, TransportRequest transportRequest) {} @Override - public void connectionGranted(InetAddress inetAddress, String profile, SecurityIpFilterRule rule) {} + public void connectionGranted(InetSocketAddress inetAddress, String profile, SecurityIpFilterRule rule) {} @Override - public void connectionDenied(InetAddress inetAddress, String profile, SecurityIpFilterRule rule) {} + public void connectionDenied(InetSocketAddress inetAddress, String profile, SecurityIpFilterRule rule) {} @Override public void runAsGranted( @@ -362,14 +361,14 @@ public void tamperedRequest(String requestId, Authentication authentication, Str } @Override - public void connectionGranted(InetAddress inetAddress, String profile, SecurityIpFilterRule rule) { + public void connectionGranted(InetSocketAddress inetAddress, String profile, SecurityIpFilterRule rule) { for (AuditTrail auditTrail : auditTrails) { auditTrail.connectionGranted(inetAddress, profile, rule); } } @Override - public void connectionDenied(InetAddress inetAddress, String profile, SecurityIpFilterRule rule) { + public void connectionDenied(InetSocketAddress inetAddress, String profile, SecurityIpFilterRule rule) { for (AuditTrail auditTrail : auditTrails) { auditTrail.connectionDenied(inetAddress, profile, rule); } 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 47b64beaca394..6cf7655862b4e 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 @@ -98,7 +98,6 @@ import org.elasticsearch.xpack.security.transport.filter.SecurityIpFilterRule; import java.io.IOException; -import java.net.InetAddress; import java.net.InetSocketAddress; import java.util.ArrayList; import java.util.Arrays; @@ -915,7 +914,7 @@ public void tamperedRequest(String requestId, Authentication authentication, Str } @Override - public void connectionGranted(InetAddress inetAddress, String profile, SecurityIpFilterRule rule) { + public void connectionGranted(InetSocketAddress inetAddress, String profile, SecurityIpFilterRule rule) { if (events.contains(CONNECTION_GRANTED) && eventFilterPolicyRegistry.ignorePredicate().test(AuditEventMetaInfo.EMPTY) == false) { new LogEntryBuilder().with(EVENT_TYPE_FIELD_NAME, IP_FILTER_ORIGIN_FIELD_VALUE) .with(EVENT_ACTION_FIELD_NAME, "connection_granted") @@ -932,7 +931,7 @@ public void connectionGranted(InetAddress inetAddress, String profile, SecurityI } @Override - public void connectionDenied(InetAddress inetAddress, String profile, SecurityIpFilterRule rule) { + public void connectionDenied(InetSocketAddress inetAddress, String profile, SecurityIpFilterRule rule) { if (events.contains(CONNECTION_DENIED) && eventFilterPolicyRegistry.ignorePredicate().test(AuditEventMetaInfo.EMPTY) == false) { new LogEntryBuilder().with(EVENT_TYPE_FIELD_NAME, IP_FILTER_ORIGIN_FIELD_VALUE) .with(EVENT_ACTION_FIELD_NAME, "connection_denied") diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/filter/IPFilter.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/filter/IPFilter.java index 7efce53aa739b..28087e64bcc85 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/filter/IPFilter.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/filter/IPFilter.java @@ -286,15 +286,15 @@ public boolean accept(String profile, InetSocketAddress peerAddress) { if (rule.matches(peerAddress)) { boolean isAllowed = rule.ruleType() == IpFilterRuleType.ACCEPT; if (isAllowed) { - auditTrail.connectionGranted(peerAddress.getAddress(), profile, rule); + auditTrail.connectionGranted(peerAddress, profile, rule); } else { - auditTrail.connectionDenied(peerAddress.getAddress(), profile, rule); + auditTrail.connectionDenied(peerAddress, profile, rule); } return isAllowed; } } - auditTrail.connectionGranted(peerAddress.getAddress(), profile, DEFAULT_PROFILE_ACCEPT_ALL); + auditTrail.connectionGranted(peerAddress, profile, DEFAULT_PROFILE_ACCEPT_ALL); return true; } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/audit/AuditTrailServiceTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/audit/AuditTrailServiceTests.java index ac62b75c2df32..dd43f8fe27fa3 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/audit/AuditTrailServiceTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/audit/AuditTrailServiceTests.java @@ -27,6 +27,7 @@ import org.junit.Before; import java.net.InetAddress; +import java.net.InetSocketAddress; import java.time.Duration; import java.time.Instant; import java.util.ArrayList; @@ -259,7 +260,7 @@ public void testAccessDenied() throws Exception { } public void testConnectionGranted() throws Exception { - InetAddress inetAddress = InetAddress.getLoopbackAddress(); + InetSocketAddress inetAddress = new InetSocketAddress(InetAddress.getLoopbackAddress(), randomIntBetween(0, 65535)); SecurityIpFilterRule rule = randomBoolean() ? SecurityIpFilterRule.ACCEPT_ALL : IPFilter.DEFAULT_PROFILE_ACCEPT_ALL; service.get().connectionGranted(inetAddress, "client", rule); verify(licenseState).isAllowed(Security.AUDITING_FEATURE); @@ -273,7 +274,7 @@ public void testConnectionGranted() throws Exception { } public void testConnectionDenied() throws Exception { - InetAddress inetAddress = InetAddress.getLoopbackAddress(); + InetSocketAddress inetAddress = new InetSocketAddress(InetAddress.getLoopbackAddress(), randomIntBetween(0, 65535)); SecurityIpFilterRule rule = new SecurityIpFilterRule(false, "_all"); service.get().connectionDenied(inetAddress, "client", rule); verify(licenseState).isAllowed(Security.AUDITING_FEATURE); 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 11a06644e219c..769c9db13bf91 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 @@ -1112,7 +1112,7 @@ public void testUsersFilter() throws Exception { threadContext.stashContext(); // connection denied - auditTrail.connectionDenied(InetAddress.getLoopbackAddress(), "default", new SecurityIpFilterRule(false, "_all")); + auditTrail.connectionDenied(randomLoopbackInetSocketAddress(), "default", new SecurityIpFilterRule(false, "_all")); if (filterMissingUser) { assertThat("Connection denied: is not filtered out by the missing user filter", logOutput.size(), is(0)); } else { @@ -1122,7 +1122,7 @@ public void testUsersFilter() throws Exception { threadContext.stashContext(); // connection granted - auditTrail.connectionGranted(InetAddress.getLoopbackAddress(), "default", new SecurityIpFilterRule(false, "_all")); + auditTrail.connectionGranted(randomLoopbackInetSocketAddress(), "default", new SecurityIpFilterRule(false, "_all")); if (filterMissingUser) { assertThat("Connection granted: is not filtered out by the missing user filter", logOutput.size(), is(0)); } else { @@ -1561,7 +1561,7 @@ public void testRealmsFilter() throws Exception { threadContext.stashContext(); // connection denied - auditTrail.connectionDenied(InetAddress.getLoopbackAddress(), "default", new SecurityIpFilterRule(false, "_all")); + auditTrail.connectionDenied(randomLoopbackInetSocketAddress(), "default", new SecurityIpFilterRule(false, "_all")); if (filterMissingRealm) { assertThat("Connection denied: is not filtered out by the missing realm filter", logOutput.size(), is(0)); } else { @@ -1571,7 +1571,7 @@ public void testRealmsFilter() throws Exception { threadContext.stashContext(); // connection granted - auditTrail.connectionGranted(InetAddress.getLoopbackAddress(), "default", new SecurityIpFilterRule(false, "_all")); + auditTrail.connectionGranted(randomLoopbackInetSocketAddress(), "default", new SecurityIpFilterRule(false, "_all")); if (filterMissingRealm) { assertThat("Connection granted: is not filtered out by the missing realm filter", logOutput.size(), is(0)); } else { @@ -1896,7 +1896,7 @@ public void testRolesFilter() throws Exception { threadContext.stashContext(); // connection denied - auditTrail.connectionDenied(InetAddress.getLoopbackAddress(), "default", new SecurityIpFilterRule(false, "_all")); + auditTrail.connectionDenied(randomLoopbackInetSocketAddress(), "default", new SecurityIpFilterRule(false, "_all")); if (filterMissingRoles) { assertThat("Connection denied: is not filtered out by the missing roles filter", logOutput.size(), is(0)); } else { @@ -1906,7 +1906,7 @@ public void testRolesFilter() throws Exception { threadContext.stashContext(); // connection granted - auditTrail.connectionGranted(InetAddress.getLoopbackAddress(), "default", new SecurityIpFilterRule(false, "_all")); + auditTrail.connectionGranted(randomLoopbackInetSocketAddress(), "default", new SecurityIpFilterRule(false, "_all")); if (filterMissingRoles) { assertThat("Connection granted: is not filtered out by the missing roles filter", logOutput.size(), is(0)); } else { @@ -2352,7 +2352,7 @@ public void testIndicesFilter() throws Exception { threadContext.stashContext(); // connection denied - auditTrail.connectionDenied(InetAddress.getLoopbackAddress(), "default", new SecurityIpFilterRule(false, "_all")); + auditTrail.connectionDenied(randomLoopbackInetSocketAddress(), "default", new SecurityIpFilterRule(false, "_all")); if (filterMissingIndices) { assertThat("Connection denied: not filtered out by missing indices filter", logOutput.size(), is(0)); } else { @@ -2362,7 +2362,7 @@ public void testIndicesFilter() throws Exception { threadContext.stashContext(); // connection granted - auditTrail.connectionGranted(InetAddress.getLoopbackAddress(), "default", new SecurityIpFilterRule(false, "_all")); + auditTrail.connectionGranted(randomLoopbackInetSocketAddress(), "default", new SecurityIpFilterRule(false, "_all")); if (filterMissingIndices) { assertThat("Connection granted: not filtered out by missing indices filter", logOutput.size(), is(0)); } else { @@ -2634,7 +2634,7 @@ public void testActionsFilter() throws Exception { threadContext.stashContext(); // connection denied - auditTrail.connectionDenied(InetAddress.getLoopbackAddress(), "default", new SecurityIpFilterRule(false, "_all")); + auditTrail.connectionDenied(randomLoopbackInetSocketAddress(), "default", new SecurityIpFilterRule(false, "_all")); if (filterMissingAction) { assertThat("Connection denied: not filtered out by the missing action filter", logOutput.size(), is(0)); } else { @@ -2644,7 +2644,7 @@ public void testActionsFilter() throws Exception { threadContext.stashContext(); // connection granted - auditTrail.connectionGranted(InetAddress.getLoopbackAddress(), "default", new SecurityIpFilterRule(false, "_all")); + auditTrail.connectionGranted(randomLoopbackInetSocketAddress(), "default", new SecurityIpFilterRule(false, "_all")); if (filterMissingAction) { assertThat("Connection granted: not filtered out by the missing action filter", logOutput.size(), is(0)); } else { @@ -2734,6 +2734,10 @@ public void testActionsFilter() throws Exception { threadContext.stashContext(); } + private InetSocketAddress randomLoopbackInetSocketAddress() { + return new InetSocketAddress(InetAddress.getLoopbackAddress(), randomIntBetween(0, 65535)); + } + private List randomListFromLengthBetween(List l, int min, int max) { assert (min >= 0) && (min <= max) && (max <= l.size()); final int len = randomIntBetween(min, max); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrailTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrailTests.java index ec8dc4e63b1fa..443998fe44805 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrailTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrailTests.java @@ -2170,7 +2170,7 @@ public void testTamperedRequestWithUser() throws Exception { } public void testConnectionDenied() throws Exception { - final InetAddress inetAddress = InetAddress.getLoopbackAddress(); + final InetSocketAddress inetAddress = new InetSocketAddress(InetAddress.getLoopbackAddress(), randomIntBetween(0, 65535)); final SecurityIpFilterRule rule = new SecurityIpFilterRule(false, "_all"); final String profile = randomBoolean() ? IPFilter.HTTP_PROFILE_NAME : randomAlphaOfLengthBetween(1, 6); @@ -2202,7 +2202,7 @@ public void testConnectionDenied() throws Exception { } public void testConnectionGranted() throws Exception { - final InetAddress inetAddress = InetAddress.getLoopbackAddress(); + final InetSocketAddress inetAddress = new InetSocketAddress(InetAddress.getLoopbackAddress(), randomIntBetween(0, 65535)); final SecurityIpFilterRule rule = IPFilter.DEFAULT_PROFILE_ACCEPT_ALL; final String profile = randomBoolean() ? IPFilter.HTTP_PROFILE_NAME : randomAlphaOfLengthBetween(1, 6); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/transport/filter/IPFilterTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/transport/filter/IPFilterTests.java index 1555552543b55..2c80ac3679893 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/transport/filter/IPFilterTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/transport/filter/IPFilterTests.java @@ -291,8 +291,8 @@ public void testThatNodeStartsWithIPFilterDisabled() throws Exception { private void assertAddressIsAllowedForProfile(String profile, String... inetAddresses) { for (String inetAddress : inetAddresses) { String message = String.format(Locale.ROOT, "Expected address %s to be allowed", inetAddress); - InetAddress address = InetAddresses.forString(inetAddress); - assertTrue(message, ipFilter.accept(profile, new InetSocketAddress(address, 0))); + InetSocketAddress address = new InetSocketAddress(InetAddresses.forString(inetAddress), 0); + assertTrue(message, ipFilter.accept(profile, address)); ArgumentCaptor ruleCaptor = ArgumentCaptor.forClass(SecurityIpFilterRule.class); verify(auditTrail).connectionGranted(eq(address), eq(profile), ruleCaptor.capture()); assertNotNull(ruleCaptor.getValue()); @@ -306,8 +306,8 @@ private void assertAddressIsAllowed(String... inetAddresses) { private void assertAddressIsDeniedForProfile(String profile, String... inetAddresses) { for (String inetAddress : inetAddresses) { String message = String.format(Locale.ROOT, "Expected address %s to be denied", inetAddress); - InetAddress address = InetAddresses.forString(inetAddress); - assertFalse(message, ipFilter.accept(profile, new InetSocketAddress(address, 0))); + InetSocketAddress address = new InetSocketAddress(InetAddresses.forString(inetAddress), 0); + assertFalse(message, ipFilter.accept(profile, address)); ArgumentCaptor ruleCaptor = ArgumentCaptor.forClass(SecurityIpFilterRule.class); verify(auditTrail).connectionDenied(eq(address), eq(profile), ruleCaptor.capture()); assertNotNull(ruleCaptor.getValue());