From 0777e8d94f91af3b120148222f1106c85c402c0f Mon Sep 17 00:00:00 2001 From: uboness Date: Wed, 22 Oct 2014 19:14:46 +0200 Subject: [PATCH 1/4] Fixed a bug in Permissions with multiple indices permission groups The evalutation of the indices permission groups was wrong. Now, each index in the request is evaluated against all groups, such that: 1. for each index, at least one group must grant the request 2. all indices must be granted Along the way, also changed the audit logs structures such that: - moved the principal to "sit" next to the host - now, if we're logging an indices request, we also log the related indices (this provides more context to the actual request) Fixes elastic/elasticsearch#242 Original commit: elastic/x-pack-elasticsearch@95600d314865f29e18cc5a96b5061e89c3b1c55a --- .../audit/logfile/LoggingAuditTrail.java | 97 ++++++++++++++---- .../shield/authz/Permission.java | 39 ++++---- .../MultipleIndicesPermissionsTests.java | 98 +++++++++++++++++++ .../audit/logfile/LoggingAuditTrailTests.java | 98 +++++++++++++++---- 4 files changed, 276 insertions(+), 56 deletions(-) create mode 100644 src/test/java/org/elasticsearch/shield/MultipleIndicesPermissionsTests.java 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 dd25b1843d1..e9b3bf68c52 100644 --- a/src/main/java/org/elasticsearch/shield/audit/logfile/LoggingAuditTrail.java +++ b/src/main/java/org/elasticsearch/shield/audit/logfile/LoggingAuditTrail.java @@ -5,6 +5,8 @@ */ package org.elasticsearch.shield.audit.logfile; +import org.elasticsearch.action.IndicesRequest; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.logging.ESLogger; import org.elasticsearch.common.logging.Loggers; @@ -41,69 +43,126 @@ public class LoggingAuditTrail implements AuditTrail { @Override public void anonymousAccess(String action, TransportMessage message) { - if (logger.isDebugEnabled()) { - logger.debug("ANONYMOUS_ACCESS\thost=[{}], action=[{}], request=[{}]", message.remoteAddress(), action, message); + String indices = indices(message); + if (indices != null) { + if (logger.isDebugEnabled()) { + logger.debug("ANONYMOUS_ACCESS\thost=[{}], action=[{}], indices=[{}], request=[{}]", message.remoteAddress(), action, indices, message); + } else { + logger.warn("ANONYMOUS_ACCESS\thost=[{}], action=[{}], indices=[{}]", message.remoteAddress(), action, indices); + } } else { - logger.warn("ANONYMOUS_ACCESS\thost=[{}], action=[{}]", message.remoteAddress(), action); + if (logger.isDebugEnabled()) { + logger.debug("ANONYMOUS_ACCESS\thost=[{}], action=[{}], request=[{}]", message.remoteAddress(), action, message); + } else { + logger.warn("ANONYMOUS_ACCESS\thost=[{}], action=[{}]", message.remoteAddress(), action); + } } } @Override public void authenticationFailed(AuthenticationToken token, String action, TransportMessage message) { - if (logger.isDebugEnabled()) { - logger.debug("AUTHENTICATION_FAILED\thost=[{}], action=[{}], principal=[{}], request=[{}]", message.remoteAddress(), action, token.principal(), message); + String indices = indices(message); + if (indices != null) { + if (logger.isDebugEnabled()) { + logger.debug("AUTHENTICATION_FAILED\thost=[{}], principal=[{}], action=[{}], indices=[{}], request=[{}]", message.remoteAddress(), token.principal(), action, indices, message); + } else { + logger.error("AUTHENTICATION_FAILED\thost=[{}], principal=[{}], action=[{}], indices=[{}]", message.remoteAddress(), token.principal(), action, indices); + } } else { - logger.error("AUTHENTICATION_FAILED\thost=[{}], action=[{}], principal=[{}]", message.remoteAddress(), action, token.principal()); + if (logger.isDebugEnabled()) { + logger.debug("AUTHENTICATION_FAILED\thost=[{}], principal=[{}], action=[{}], request=[{}]", message.remoteAddress(), token.principal(), action, message); + } else { + logger.error("AUTHENTICATION_FAILED\thost=[{}], principal=[{}], action=[{}]", message.remoteAddress(), token.principal(), action); + } } } @Override public void authenticationFailed(AuthenticationToken token, RestRequest request) { if (logger.isDebugEnabled()) { - logger.debug("AUTHENTICATION_FAILED\thost=[{}], URI=[{}], principal=[{}], request=[{}]", request.getRemoteAddress(), request.uri(), token.principal(), request); + logger.debug("AUTHENTICATION_FAILED\thost=[{}], principal=[{}], URI=[{}], request=[{}]", request.getRemoteAddress(), token.principal(), request.uri(), request); } else { - logger.error("AUTHENTICATION_FAILED\thost=[{}], URI=[{}], principal=[{}]", request.getRemoteAddress(), request.uri(), token.principal()); + logger.error("AUTHENTICATION_FAILED\thost=[{}], principal=[{}], URI=[{}]", request.getRemoteAddress(), token.principal(), request.uri()); } } @Override public void authenticationFailed(String realm, AuthenticationToken token, String action, TransportMessage message) { if (logger.isTraceEnabled()) { - logger.trace("AUTHENTICATION_FAILED[{}]\thost=[{}], action=[{}], principal=[{}], request=[{}]", realm, message.remoteAddress(), action, token.principal(), message); + String indices = indices(message); + if (indices != null) { + logger.trace("AUTHENTICATION_FAILED[{}]\thost=[{}], principal=[{}], action=[{}], indices=[{}], request=[{}]", realm, message.remoteAddress(), token.principal(), action, indices, message); + } else { + logger.trace("AUTHENTICATION_FAILED[{}]\thost=[{}], principal=[{}], action=[{}], request=[{}]", realm, message.remoteAddress(), token.principal(), action, message); + } } } @Override public void authenticationFailed(String realm, AuthenticationToken token, RestRequest request) { if (logger.isTraceEnabled()) { - logger.trace("AUTHENTICATION_FAILED[{}]\thost=[{}], URI=[{}], principal=[{}], request=[{}]", realm, request.getRemoteAddress(), request.uri(), token.principal(), request); + logger.trace("AUTHENTICATION_FAILED[{}]\thost=[{}], principal=[{}], URI=[{}], request=[{}]", realm, request.getRemoteAddress(), token.principal(), request.uri(), request); } } @Override public void accessGranted(User user, String action, TransportMessage message) { - if (logger.isDebugEnabled()) { - logger.debug("ACCESS_GRANTED\thost=[{}], action=[{}], principal=[{}], request=[{}]", message.remoteAddress(), action, user.principal(), message); + String indices = indices(message); + if (indices != null) { + if (logger.isDebugEnabled()) { + logger.debug("ACCESS_GRANTED\thost=[{}], principal=[{}], action=[{}], indices=[{}], request=[{}]", message.remoteAddress(), user.principal(), action, indices, message); + } else { + logger.info("ACCESS_GRANTED\thost=[{}], principal=[{}], action=[{}], indices=[{}]", message.remoteAddress(), user.principal(), action, indices); + } } else { - logger.info("ACCESS_GRANTED\thost=[{}], action=[{}], principal=[{}]", message.remoteAddress(), action, user.principal()); + if (logger.isDebugEnabled()) { + logger.debug("ACCESS_GRANTED\thost=[{}], principal=[{}], action=[{}], request=[{}]", message.remoteAddress(), user.principal(), action, message); + } else { + logger.info("ACCESS_GRANTED\thost=[{}], principal=[{}], action=[{}]", message.remoteAddress(), user.principal(), action); + } } } @Override public void accessDenied(User user, String action, TransportMessage message) { - if (logger.isDebugEnabled()) { - logger.debug("ACCESS_DENIED\thost=[{}], action=[{}], principal=[{}], request=[{}]", message.remoteAddress(), action, user.principal(), message); + String indices = indices(message); + if (indices != null) { + if (logger.isDebugEnabled()) { + logger.debug("ACCESS_DENIED\thost=[{}], principal=[{}], action=[{}], indices=[{}], request=[{}]", message.remoteAddress(), user.principal(), action, indices, message); + } else { + logger.error("ACCESS_DENIED\thost=[{}], principal=[{}], action=[{}], indices=[{}]", message.remoteAddress(), user.principal(), action, indices); + } } else { - logger.error("ACCESS_DENIED\thost=[{}], action=[{}], principal=[{}]", message.remoteAddress(), action, user.principal()); + if (logger.isDebugEnabled()) { + logger.debug("ACCESS_DENIED\thost=[{}], principal=[{}], action=[{}], request=[{}]", message.remoteAddress(), user.principal(), action, message); + } else { + logger.error("ACCESS_DENIED\thost=[{}], principal=[{}], action=[{}]", message.remoteAddress(), user.principal(), action); + } } } @Override public void tamperedRequest(User user, String action, TransportRequest request) { - if (logger.isDebugEnabled()) { - logger.debug("TAMPERED REQUEST\thost=[{}], action=[{}], principal=[{}], request=[{}]", request.remoteAddress(), action, user.principal(), request); + String indices = indices(request); + if (indices != null) { + if (logger.isDebugEnabled()) { + logger.debug("TAMPERED REQUEST\thost=[{}], principal=[{}], action=[{}], indices=[{}], request=[{}]", request.remoteAddress(), user.principal(), action, indices, request); + } else { + logger.error("TAMPERED REQUEST\thost=[{}], principal=[{}], action=[{}], indices=[{}]", request.remoteAddress(), user.principal(), action, indices); + } } else { - logger.error("TAMPERED REQUEST\thost=[{}], action=[{}], principal=[{}]", request.remoteAddress(), action, user.principal()); + if (logger.isDebugEnabled()) { + logger.debug("TAMPERED REQUEST\thost=[{}], principal=[{}], action=[{}], request=[{}]", request.remoteAddress(), user.principal(), action, request); + } else { + logger.error("TAMPERED REQUEST\thost=[{}], principal=[{}], action=[{}]", request.remoteAddress(), user.principal(), action); + } } } + + private static String indices(TransportMessage message) { + if (message instanceof IndicesRequest) { + return Strings.arrayToCommaDelimitedString(((IndicesRequest) message).indices()); + } + return null; + } } diff --git a/src/main/java/org/elasticsearch/shield/authz/Permission.java b/src/main/java/org/elasticsearch/shield/authz/Permission.java index 40bfeb2fbed..433c719a64f 100644 --- a/src/main/java/org/elasticsearch/shield/authz/Permission.java +++ b/src/main/java/org/elasticsearch/shield/authz/Permission.java @@ -9,7 +9,6 @@ import org.elasticsearch.action.CompositeIndicesRequest; import org.elasticsearch.action.IndicesRequest; import org.elasticsearch.cluster.metadata.MetaData; import org.elasticsearch.common.base.Predicate; -import org.elasticsearch.common.cache.Cache; import org.elasticsearch.common.cache.CacheBuilder; import org.elasticsearch.common.cache.CacheLoader; import org.elasticsearch.common.cache.LoadingCache; @@ -233,12 +232,25 @@ public interface Permission { } } - for (Group group : groups) { - if (group.check(action, indices)) { - return true; + if (indices == null) { + return true; + } + + // for every index, at least one group should match it... otherwise denied + for (String index : indices) { + boolean grant = false; + for (Group group : groups) { + if (group.check(action, index)) { + grant = true; + break; + } + } + if (!grant) { + return false; } } - return false; + + return true; } public static class Group { @@ -264,21 +276,8 @@ public interface Permission { return indices; } - public boolean check(String action, Set indices) { - - if (!actionMatcher.apply(action)) { - return false; - } - - if (indices != null) { - for (String index : indices) { - if (!indexNameMatcher.apply(index)) { - return false; - } - } - } - - return true; + public boolean check(String action, String index) { + return actionMatcher.apply(action) && !(index != null && !indexNameMatcher.apply(index)); } diff --git a/src/test/java/org/elasticsearch/shield/MultipleIndicesPermissionsTests.java b/src/test/java/org/elasticsearch/shield/MultipleIndicesPermissionsTests.java new file mode 100644 index 00000000000..3cc2e2bae81 --- /dev/null +++ b/src/test/java/org/elasticsearch/shield/MultipleIndicesPermissionsTests.java @@ -0,0 +1,98 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ +package org.elasticsearch.shield; + +import org.elasticsearch.action.index.IndexResponse; +import org.elasticsearch.action.search.SearchResponse; +import org.elasticsearch.client.Client; +import org.elasticsearch.shield.authz.AuthorizationException; +import org.elasticsearch.shield.test.ShieldIntegrationTest; +import org.junit.Test; + +import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; +import static org.elasticsearch.index.query.QueryBuilders.indicesQuery; +import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery; +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount; +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertNoFailures; +import static org.hamcrest.Matchers.*; + +/** + * + */ +public class MultipleIndicesPermissionsTests extends ShieldIntegrationTest { + + public static final String ROLES = "user:\n" + + " cluster: all\n" + + " indices:\n" + + " '.*': manage\n" + + " '.*': write\n" + + " 'test': read\n" + + " 'test1': read\n"; + + @Override + protected String configRole() { + return ROLES; + } + + @Test + public void testDifferetCombinationsOfIndices() throws Exception { + IndexResponse indexResponse = index("test", "type", jsonBuilder() + .startObject() + .field("name", "value") + .endObject()); + assertThat(indexResponse.isCreated(), is(true)); + + + indexResponse = index("test1", "type", jsonBuilder() + .startObject() + .field("name", "value1") + .endObject()); + assertThat(indexResponse.isCreated(), is(true)); + + refresh(); + + Client client = internalCluster().transportClient(); + + // no specifying an index, should replace indices with the permitted ones (test & test1) + SearchResponse searchResponse = client.prepareSearch().setQuery(matchAllQuery()).get(); + assertNoFailures(searchResponse); + assertHitCount(searchResponse, 2); + + searchResponse = client.prepareSearch().setQuery(indicesQuery(matchAllQuery(), "test1")).get(); + assertNoFailures(searchResponse); + assertHitCount(searchResponse, 2); + + // _all should expand to all the permitted indices + searchResponse = client.prepareSearch("_all").setQuery(matchAllQuery()).get(); + assertNoFailures(searchResponse); + assertHitCount(searchResponse, 2); + + // wildcards should expand to all the permitted indices + searchResponse = client.prepareSearch("test*").setQuery(matchAllQuery()).get(); + assertNoFailures(searchResponse); + assertHitCount(searchResponse, 2); + + // specifying a permitted index, should only return results from that index + searchResponse = client.prepareSearch("test1").setQuery(indicesQuery(matchAllQuery(), "test1")).get(); + assertNoFailures(searchResponse); + assertHitCount(searchResponse, 1); + + // specifying a forbidden index, should throw an authorization exception + try { + client.prepareSearch("test2").setQuery(indicesQuery(matchAllQuery(), "test1")).get(); + fail("expected an authorization exception when searching a forbidden index"); + } catch (AuthorizationException ae) { + // expected + } + + try { + client.prepareSearch("test", "test2").setQuery(matchAllQuery()).get(); + fail("expected an authorization exception when one of mulitple indices is forbidden"); + } catch (AuthorizationException ae) { + // expected + } + } +} 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 652998edf73..ed26b08bd5b 100644 --- a/src/test/java/org/elasticsearch/shield/audit/logfile/LoggingAuditTrailTests.java +++ b/src/test/java/org/elasticsearch/shield/audit/logfile/LoggingAuditTrailTests.java @@ -5,6 +5,8 @@ */ package org.elasticsearch.shield.audit.logfile; +import org.elasticsearch.action.IndicesRequest; +import org.elasticsearch.action.support.IndicesOptions; import org.elasticsearch.common.transport.LocalTransportAddress; import org.elasticsearch.rest.RestRequest; import org.elasticsearch.shield.User; @@ -32,18 +34,27 @@ public class LoggingAuditTrailTests extends ElasticsearchTestCase { for (Level level : Level.values()) { CapturingLogger logger = new CapturingLogger(level); LoggingAuditTrail auditTrail = new LoggingAuditTrail(logger); - auditTrail.anonymousAccess("_action", new MockMessage()); + TransportMessage message = randomBoolean() ? new MockMessage() : new MockIndicesRequest(); + auditTrail.anonymousAccess("_action", message); switch (level) { case ERROR: assertEmptyLog(logger); break; case WARN: case INFO: - assertMsg(logger, Level.WARN, "ANONYMOUS_ACCESS\thost=[local[_host]], action=[_action]"); + if (message instanceof IndicesRequest) { + assertMsg(logger, Level.WARN, "ANONYMOUS_ACCESS\thost=[local[_host]], action=[_action], indices=[idx1,idx2]"); + } else { + assertMsg(logger, Level.WARN, "ANONYMOUS_ACCESS\thost=[local[_host]], action=[_action]"); + } break; case DEBUG: case TRACE: - assertMsg(logger, Level.DEBUG, "ANONYMOUS_ACCESS\thost=[local[_host]], action=[_action], request=[mock-message]"); + if (message instanceof IndicesRequest) { + assertMsg(logger, Level.DEBUG, "ANONYMOUS_ACCESS\thost=[local[_host]], action=[_action], indices=[idx1,idx2], request=[mock-message]"); + } else { + assertMsg(logger, Level.DEBUG, "ANONYMOUS_ACCESS\thost=[local[_host]], action=[_action], request=[mock-message]"); + } } } } @@ -53,16 +64,25 @@ public class LoggingAuditTrailTests extends ElasticsearchTestCase { for (Level level : Level.values()) { CapturingLogger logger = new CapturingLogger(level); LoggingAuditTrail auditTrail = new LoggingAuditTrail(logger); - auditTrail.authenticationFailed(new MockToken(), "_action", new MockMessage()); + TransportMessage message = randomBoolean() ? new MockMessage() : new MockIndicesRequest(); + auditTrail.authenticationFailed(new MockToken(), "_action", message); switch (level) { case ERROR: case WARN: case INFO: - assertMsg(logger, Level.ERROR, "AUTHENTICATION_FAILED\thost=[local[_host]], action=[_action], principal=[_principal]"); + if (message instanceof IndicesRequest) { + assertMsg(logger, Level.ERROR, "AUTHENTICATION_FAILED\thost=[local[_host]], principal=[_principal], action=[_action], indices=[idx1,idx2]"); + } else { + assertMsg(logger, Level.ERROR, "AUTHENTICATION_FAILED\thost=[local[_host]], principal=[_principal], action=[_action]"); + } break; case DEBUG: case TRACE: - assertMsg(logger, Level.DEBUG, "AUTHENTICATION_FAILED\thost=[local[_host]], action=[_action], principal=[_principal], request=[mock-message]"); + if (message instanceof IndicesRequest) { + assertMsg(logger, Level.DEBUG, "AUTHENTICATION_FAILED\thost=[local[_host]], principal=[_principal], action=[_action], indices=[idx1,idx2], request=[mock-message]"); + } else { + assertMsg(logger, Level.DEBUG, "AUTHENTICATION_FAILED\thost=[local[_host]], principal=[_principal], action=[_action], request=[mock-message]"); + } } } } @@ -81,11 +101,11 @@ public class LoggingAuditTrailTests extends ElasticsearchTestCase { case ERROR: case WARN: case INFO: - assertMsg(logger, Level.ERROR, "AUTHENTICATION_FAILED\thost=[_hostname:9200], URI=[_uri], principal=[_principal]"); + assertMsg(logger, Level.ERROR, "AUTHENTICATION_FAILED\thost=[_hostname:9200], principal=[_principal], URI=[_uri]"); break; case DEBUG: case TRACE: - assertMsg(logger, Level.DEBUG, "AUTHENTICATION_FAILED\thost=[_hostname:9200], URI=[_uri], principal=[_principal], request=[rest_request]"); + assertMsg(logger, Level.DEBUG, "AUTHENTICATION_FAILED\thost=[_hostname:9200], principal=[_principal], URI=[_uri], request=[rest_request]"); } } } @@ -95,7 +115,8 @@ public class LoggingAuditTrailTests extends ElasticsearchTestCase { for (Level level : Level.values()) { CapturingLogger logger = new CapturingLogger(level); LoggingAuditTrail auditTrail = new LoggingAuditTrail(logger); - auditTrail.authenticationFailed("_realm", new MockToken(), "_action", new MockMessage()); + TransportMessage message = randomBoolean() ? new MockMessage() : new MockIndicesRequest(); + auditTrail.authenticationFailed("_realm", new MockToken(), "_action", message); switch (level) { case ERROR: case WARN: @@ -104,7 +125,11 @@ public class LoggingAuditTrailTests extends ElasticsearchTestCase { assertEmptyLog(logger); break; case TRACE: - assertMsg(logger, Level.TRACE, "AUTHENTICATION_FAILED[_realm]\thost=[local[_host]], action=[_action], principal=[_principal], request=[mock-message]"); + if (message instanceof IndicesRequest) { + assertMsg(logger, Level.TRACE, "AUTHENTICATION_FAILED[_realm]\thost=[local[_host]], principal=[_principal], action=[_action], indices=[idx1,idx2], request=[mock-message]"); + } else { + assertMsg(logger, Level.TRACE, "AUTHENTICATION_FAILED[_realm]\thost=[local[_host]], principal=[_principal], action=[_action], request=[mock-message]"); + } } } } @@ -127,7 +152,7 @@ public class LoggingAuditTrailTests extends ElasticsearchTestCase { assertEmptyLog(logger); break; case TRACE: - assertMsg(logger, Level.TRACE, "AUTHENTICATION_FAILED[_realm]\thost=[_hostname:9200], URI=[_uri], principal=[_principal], request=[rest_request]"); + assertMsg(logger, Level.TRACE, "AUTHENTICATION_FAILED[_realm]\thost=[_hostname:9200], principal=[_principal], URI=[_uri], request=[rest_request]"); } } } @@ -137,18 +162,27 @@ public class LoggingAuditTrailTests extends ElasticsearchTestCase { for (Level level : Level.values()) { CapturingLogger logger = new CapturingLogger(level); LoggingAuditTrail auditTrail = new LoggingAuditTrail(logger); - auditTrail.accessGranted(new User.Simple("_username", "r1"), "_action", new MockMessage()); + TransportMessage message = randomBoolean() ? new MockMessage() : new MockIndicesRequest(); + auditTrail.accessGranted(new User.Simple("_username", "r1"), "_action", message); switch (level) { case ERROR: case WARN: assertEmptyLog(logger); break; case INFO: - assertMsg(logger, Level.INFO, "ACCESS_GRANTED\thost=[local[_host]], action=[_action], principal=[_username]"); + if (message instanceof IndicesRequest) { + assertMsg(logger, Level.INFO, "ACCESS_GRANTED\thost=[local[_host]], principal=[_username], action=[_action], indices=[idx1,idx2]"); + } else { + assertMsg(logger, Level.INFO, "ACCESS_GRANTED\thost=[local[_host]], principal=[_username], action=[_action]"); + } break; case DEBUG: case TRACE: - assertMsg(logger, Level.DEBUG, "ACCESS_GRANTED\thost=[local[_host]], action=[_action], principal=[_username], request=[mock-message]"); + if (message instanceof IndicesRequest) { + assertMsg(logger, Level.DEBUG, "ACCESS_GRANTED\thost=[local[_host]], principal=[_username], action=[_action], indices=[idx1,idx2], request=[mock-message]"); + } else { + assertMsg(logger, Level.DEBUG, "ACCESS_GRANTED\thost=[local[_host]], principal=[_username], action=[_action], request=[mock-message]"); + } } } } @@ -158,16 +192,25 @@ public class LoggingAuditTrailTests extends ElasticsearchTestCase { for (Level level : Level.values()) { CapturingLogger logger = new CapturingLogger(level); LoggingAuditTrail auditTrail = new LoggingAuditTrail(logger); - auditTrail.accessDenied(new User.Simple("_username", "r1"), "_action", new MockMessage()); + TransportMessage message = randomBoolean() ? new MockMessage() : new MockIndicesRequest(); + auditTrail.accessDenied(new User.Simple("_username", "r1"), "_action", message); switch (level) { case ERROR: case WARN: case INFO: - assertMsg(logger, Level.ERROR, "ACCESS_DENIED\thost=[local[_host]], action=[_action], principal=[_username]"); + if (message instanceof IndicesRequest) { + assertMsg(logger, Level.ERROR, "ACCESS_DENIED\thost=[local[_host]], principal=[_username], action=[_action], indices=[idx1,idx2]"); + } else { + assertMsg(logger, Level.ERROR, "ACCESS_DENIED\thost=[local[_host]], principal=[_username], action=[_action]"); + } break; case DEBUG: case TRACE: - assertMsg(logger, Level.DEBUG, "ACCESS_DENIED\thost=[local[_host]], action=[_action], principal=[_username], request=[mock-message]"); + if (message instanceof IndicesRequest) { + assertMsg(logger, Level.DEBUG, "ACCESS_DENIED\thost=[local[_host]], principal=[_username], action=[_action], indices=[idx1,idx2], request=[mock-message]"); + } else { + assertMsg(logger, Level.DEBUG, "ACCESS_DENIED\thost=[local[_host]], principal=[_username], action=[_action], request=[mock-message]"); + } } } } @@ -194,6 +237,27 @@ public class LoggingAuditTrailTests extends ElasticsearchTestCase { } } + private static class MockIndicesRequest extends TransportMessage implements IndicesRequest { + + private MockIndicesRequest() { + remoteAddress(new LocalTransportAddress("_host")); + } + + @Override + public String[] indices() { + return new String[] { "idx1", "idx2" }; + } + + @Override + public IndicesOptions indicesOptions() { + return IndicesOptions.strictExpandOpenAndForbidClosed(); + } + + @Override + public String toString() { + return "mock-message"; + } + } private static class MockToken implements AuthenticationToken { @Override From f517a6a8f31a4d6d2f592215dbdad777214d6178 Mon Sep 17 00:00:00 2001 From: c-a-m Date: Mon, 20 Oct 2014 14:15:48 -0600 Subject: [PATCH 2/4] Refactors "urls" -> "url" This lets the url be configured as a single element (the most likely usage) or as an array. This also checks that multiple urls are either all "ldaps", or all "ldap", as it is not possible to mix them. Original commit: elastic/x-pack-elasticsearch@b5a94b1d3544ac66f803a4c18edf1426f8fe4bf1 --- .../ActiveDirectoryConnectionFactory.java | 13 +++- .../authc/ldap/LdapConnectionFactory.java | 2 +- .../authc/ldap/LdapSslSocketFactory.java | 21 +++-- .../ldap/StandardLdapConnectionFactory.java | 4 + .../authc/ldap/LdapSSLSocketFactoryTests.java | 78 +++++++++++++++++++ 5 files changed, 105 insertions(+), 13 deletions(-) create mode 100644 src/test/java/org/elasticsearch/shield/authc/ldap/LdapSSLSocketFactoryTests.java diff --git a/src/main/java/org/elasticsearch/shield/authc/ldap/ActiveDirectoryConnectionFactory.java b/src/main/java/org/elasticsearch/shield/authc/ldap/ActiveDirectoryConnectionFactory.java index f376d7e4a13..5e19a0320a8 100644 --- a/src/main/java/org/elasticsearch/shield/authc/ldap/ActiveDirectoryConnectionFactory.java +++ b/src/main/java/org/elasticsearch/shield/authc/ldap/ActiveDirectoryConnectionFactory.java @@ -32,7 +32,6 @@ import java.util.Hashtable; public class ActiveDirectoryConnectionFactory extends AbstractComponent implements LdapConnectionFactory { public static final String AD_DOMAIN_NAME_SETTING = "domain_name"; - public static final String AD_PORT = "default_port"; public static final String AD_USER_SEARCH_BASEDN_SETTING = "user_search_dn"; static final String MODE_NAME = "active_directory"; @@ -48,9 +47,15 @@ public class ActiveDirectoryConnectionFactory extends AbstractComponent implemen throw new ShieldException("Missing [" + AD_DOMAIN_NAME_SETTING + "] setting for active directory"); } userSearchDN = componentSettings.get(AD_USER_SEARCH_BASEDN_SETTING, buildDnFromDomain(domainName)); - int port = componentSettings.getAsInt(AD_PORT, 636); - String protocol = port == 389 ? "ldap://" : "ldaps://"; - String[] ldapUrls = componentSettings.getAsArray(URLS_SETTING, new String[] { protocol + domainName + ":" + port }); + + String[] ldapUrls = componentSettings.getAsArray(URLS_SETTING); + if (ldapUrls == null) { + String url = componentSettings.get(URLS_SETTING); + ldapUrls = url == null ? null : new String[]{url}; + } + if (ldapUrls == null) { + ldapUrls = new String[] { "ldaps://" + domainName + ":636" }; + } ImmutableMap.Builder builder = ImmutableMap.builder() .put(Context.INITIAL_CONTEXT_FACTORY, "com.sun.jndi.ldap.LdapCtxFactory") diff --git a/src/main/java/org/elasticsearch/shield/authc/ldap/LdapConnectionFactory.java b/src/main/java/org/elasticsearch/shield/authc/ldap/LdapConnectionFactory.java index cf3cb9a71aa..d24c0a33b75 100644 --- a/src/main/java/org/elasticsearch/shield/authc/ldap/LdapConnectionFactory.java +++ b/src/main/java/org/elasticsearch/shield/authc/ldap/LdapConnectionFactory.java @@ -20,7 +20,7 @@ import org.elasticsearch.shield.authc.support.SecuredString; */ public interface LdapConnectionFactory { - static final String URLS_SETTING = "urls"; //comma separated + static final String URLS_SETTING = "url"; //comma separated /** * Password authenticated bind diff --git a/src/main/java/org/elasticsearch/shield/authc/ldap/LdapSslSocketFactory.java b/src/main/java/org/elasticsearch/shield/authc/ldap/LdapSslSocketFactory.java index a33f837d9f6..123a3dc342a 100644 --- a/src/main/java/org/elasticsearch/shield/authc/ldap/LdapSslSocketFactory.java +++ b/src/main/java/org/elasticsearch/shield/authc/ldap/LdapSslSocketFactory.java @@ -5,10 +5,12 @@ */ package org.elasticsearch.shield.authc.ldap; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.collect.ImmutableMap; import org.elasticsearch.common.logging.ESLogger; import org.elasticsearch.common.logging.ESLoggerFactory; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.shield.ShieldSettingsException; import org.elasticsearch.shield.transport.ssl.SSLTrustConfig; import javax.net.SocketFactory; @@ -28,6 +30,7 @@ import java.util.Locale; */ public class LdapSslSocketFactory extends SocketFactory { + static final String JAVA_NAMING_LDAP_FACTORY_SOCKET = "java.naming.ldap.factory.socket"; private static ESLogger logger = ESLoggerFactory.getLogger(LdapSslSocketFactory.class.getName()); private static LdapSslSocketFactory instance; @@ -51,7 +54,7 @@ public class LdapSslSocketFactory extends SocketFactory { /** * This is invoked by JNDI and the returned SocketFactory must be an LdapSslSocketFactory object - * @return + * @return a singleton instance of LdapSslSocketFactory set by calling the init static method. */ public static SocketFactory getDefault() { assert instance != null; @@ -92,19 +95,21 @@ public class LdapSslSocketFactory extends SocketFactory { /** * If one of the ldapUrls are SSL this will set the LdapSslSocketFactory as a socket provider on the builder - * @param ldapUrls + * @param ldapUrls array of ldap urls, either all SSL or none with SSL (no mixing) * @param builder set of jndi properties, that will + * @throws org.elasticsearch.shield.ShieldSettingsException if URLs have mixed protocols. */ public static void configureJndiSSL(String[] ldapUrls, ImmutableMap.Builder builder) { - boolean needsSSL = false; + boolean secureProtocol = ldapUrls[0].toLowerCase(Locale.getDefault()).startsWith("ldaps://"); for(String url: ldapUrls){ - if (url.toLowerCase(Locale.getDefault()).startsWith("ldaps://")) { - needsSSL = true; - break; + if (secureProtocol != url.toLowerCase(Locale.getDefault()).startsWith("ldaps://")) { + //this is because LdapSSLSocketFactory produces only SSL sockets and not clear text sockets + throw new ShieldSettingsException("Configured ldap protocols are not all equal " + + "(ldaps://.. and ldap://..): [" + Strings.arrayToCommaDelimitedString(ldapUrls) + "]"); } } - if (needsSSL && instance != null) { - builder.put("java.naming.ldap.factory.socket", LdapSslSocketFactory.class.getName()); + if (secureProtocol && instance != null) { + builder.put(JAVA_NAMING_LDAP_FACTORY_SOCKET, LdapSslSocketFactory.class.getName()); } else { logger.warn("LdapSslSocketFactory not used for LDAP connections"); } diff --git a/src/main/java/org/elasticsearch/shield/authc/ldap/StandardLdapConnectionFactory.java b/src/main/java/org/elasticsearch/shield/authc/ldap/StandardLdapConnectionFactory.java index 2ea852184b0..04bfb34487b 100644 --- a/src/main/java/org/elasticsearch/shield/authc/ldap/StandardLdapConnectionFactory.java +++ b/src/main/java/org/elasticsearch/shield/authc/ldap/StandardLdapConnectionFactory.java @@ -48,6 +48,10 @@ public class StandardLdapConnectionFactory extends AbstractComponent implements throw new ShieldException("Missing required ldap setting [" + USER_DN_TEMPLATES_SETTING + "]"); } String[] ldapUrls = componentSettings.getAsArray(URLS_SETTING); + if (ldapUrls == null) { + String url = componentSettings.get(URLS_SETTING); + ldapUrls = url == null ? null : new String[]{url}; + } if (ldapUrls == null) { throw new ShieldException("Missing required ldap setting [" + URLS_SETTING + "]"); } diff --git a/src/test/java/org/elasticsearch/shield/authc/ldap/LdapSSLSocketFactoryTests.java b/src/test/java/org/elasticsearch/shield/authc/ldap/LdapSSLSocketFactoryTests.java new file mode 100644 index 00000000000..49fe34ce8d5 --- /dev/null +++ b/src/test/java/org/elasticsearch/shield/authc/ldap/LdapSSLSocketFactoryTests.java @@ -0,0 +1,78 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ +package org.elasticsearch.shield.authc.ldap; + +import org.elasticsearch.common.collect.ImmutableMap; +import org.elasticsearch.common.settings.ImmutableSettings; +import org.elasticsearch.shield.ShieldSettingsException; +import org.elasticsearch.test.ElasticsearchTestCase; +import org.hamcrest.Matchers; +import org.junit.BeforeClass; +import org.junit.Test; + +import java.io.File; +import java.io.Serializable; +import java.net.URISyntaxException; + +import static org.hamcrest.Matchers.equalTo; + +public class LdapSslSocketFactoryTests extends ElasticsearchTestCase { + + @BeforeClass + public static void setTrustStore() throws URISyntaxException { + //LdapModule will set this up as a singleton normally + LdapSslSocketFactory.init(ImmutableSettings.builder() + .put("shield.authc.ldap.truststore", new File(LdapConnectionTests.class.getResource("ldaptrust.jks").toURI())) + .build()); + } + + @Test + public void testConfigure_1https(){ + String[] urls = new String[]{"ldaps://example.com:636"}; + + ImmutableMap.Builder builder = ImmutableMap.builder(); + LdapSslSocketFactory.configureJndiSSL(urls, builder); + ImmutableMap settings = builder.build(); + assertThat(settings.get(LdapSslSocketFactory.JAVA_NAMING_LDAP_FACTORY_SOCKET), + Matchers.equalTo("org.elasticsearch.shield.authc.ldap.LdapSslSocketFactory")); + } + + @Test + public void testConfigure_2https(){ + String[] urls = new String[]{"ldaps://primary.example.com:636", "LDAPS://secondary.example.com:10636"}; + + ImmutableMap.Builder builder = ImmutableMap.builder(); + LdapSslSocketFactory.configureJndiSSL(urls, builder); + ImmutableMap settings = builder.build(); + assertThat(settings.get(LdapSslSocketFactory.JAVA_NAMING_LDAP_FACTORY_SOCKET), Matchers.equalTo("org.elasticsearch.shield.authc.ldap.LdapSslSocketFactory")); + } + + @Test + public void testConfigure_2http(){ + String[] urls = new String[]{"ldap://primary.example.com:392", "LDAP://secondary.example.com:10392"}; + + ImmutableMap.Builder builder = ImmutableMap.builder(); + LdapSslSocketFactory.configureJndiSSL(urls, builder); + ImmutableMap settings = builder.build(); + assertThat(settings.get(LdapSslSocketFactory.JAVA_NAMING_LDAP_FACTORY_SOCKET), equalTo(null)); + } + + @Test(expected = ShieldSettingsException.class) + public void testConfigure_1httpS_1http(){ + String[] urls = new String[]{"LDAPS://primary.example.com:636", "ldap://secondary.example.com:392"}; + + ImmutableMap.Builder builder = ImmutableMap.builder(); + LdapSslSocketFactory.configureJndiSSL(urls, builder); + } + + @Test(expected = ShieldSettingsException.class) + public void testConfigure_1http_1https(){ + String[] urls = new String[]{"ldap://primary.example.com:392", "ldaps://secondary.example.com:636"}; + + ImmutableMap.Builder builder = ImmutableMap.builder(); + LdapSslSocketFactory.configureJndiSSL(urls, builder); + } +} From 4b0f7c4379fe45fe7f94c9fba7ddb2bf3de992ef Mon Sep 17 00:00:00 2001 From: c-a-m Date: Wed, 22 Oct 2014 10:33:52 -0600 Subject: [PATCH 3/4] Fixed the url settings to only call the toArray. Plus I changed one of the tests to use the single value style Original commit: elastic/x-pack-elasticsearch@16326d2b6c715a5f59b68f77c7926b4e22112898 --- .../authc/ldap/ActiveDirectoryConnectionFactory.java | 9 +-------- .../shield/authc/ldap/StandardLdapConnectionFactory.java | 4 ---- .../shield/authc/ldap/ActiveDirectoryFactoryTests.java | 2 +- 3 files changed, 2 insertions(+), 13 deletions(-) diff --git a/src/main/java/org/elasticsearch/shield/authc/ldap/ActiveDirectoryConnectionFactory.java b/src/main/java/org/elasticsearch/shield/authc/ldap/ActiveDirectoryConnectionFactory.java index 5e19a0320a8..35cbdf2b27b 100644 --- a/src/main/java/org/elasticsearch/shield/authc/ldap/ActiveDirectoryConnectionFactory.java +++ b/src/main/java/org/elasticsearch/shield/authc/ldap/ActiveDirectoryConnectionFactory.java @@ -48,14 +48,7 @@ public class ActiveDirectoryConnectionFactory extends AbstractComponent implemen } userSearchDN = componentSettings.get(AD_USER_SEARCH_BASEDN_SETTING, buildDnFromDomain(domainName)); - String[] ldapUrls = componentSettings.getAsArray(URLS_SETTING); - if (ldapUrls == null) { - String url = componentSettings.get(URLS_SETTING); - ldapUrls = url == null ? null : new String[]{url}; - } - if (ldapUrls == null) { - ldapUrls = new String[] { "ldaps://" + domainName + ":636" }; - } + String[] ldapUrls = componentSettings.getAsArray(URLS_SETTING, new String[] { "ldaps://" + domainName + ":636"}); ImmutableMap.Builder builder = ImmutableMap.builder() .put(Context.INITIAL_CONTEXT_FACTORY, "com.sun.jndi.ldap.LdapCtxFactory") diff --git a/src/main/java/org/elasticsearch/shield/authc/ldap/StandardLdapConnectionFactory.java b/src/main/java/org/elasticsearch/shield/authc/ldap/StandardLdapConnectionFactory.java index 04bfb34487b..2ea852184b0 100644 --- a/src/main/java/org/elasticsearch/shield/authc/ldap/StandardLdapConnectionFactory.java +++ b/src/main/java/org/elasticsearch/shield/authc/ldap/StandardLdapConnectionFactory.java @@ -48,10 +48,6 @@ public class StandardLdapConnectionFactory extends AbstractComponent implements throw new ShieldException("Missing required ldap setting [" + USER_DN_TEMPLATES_SETTING + "]"); } String[] ldapUrls = componentSettings.getAsArray(URLS_SETTING); - if (ldapUrls == null) { - String url = componentSettings.get(URLS_SETTING); - ldapUrls = url == null ? null : new String[]{url}; - } if (ldapUrls == null) { throw new ShieldException("Missing required ldap setting [" + URLS_SETTING + "]"); } diff --git a/src/test/java/org/elasticsearch/shield/authc/ldap/ActiveDirectoryFactoryTests.java b/src/test/java/org/elasticsearch/shield/authc/ldap/ActiveDirectoryFactoryTests.java index 4501969c942..4bdf7ec5cdc 100644 --- a/src/test/java/org/elasticsearch/shield/authc/ldap/ActiveDirectoryFactoryTests.java +++ b/src/test/java/org/elasticsearch/shield/authc/ldap/ActiveDirectoryFactoryTests.java @@ -114,7 +114,7 @@ public class ActiveDirectoryFactoryTests extends ElasticsearchTestCase { public static Settings buildAdSettings(String ldapUrl, String adDomainName) { return ImmutableSettings.builder() - .putArray(SETTINGS_PREFIX + ActiveDirectoryConnectionFactory.URLS_SETTING, ldapUrl) + .put(SETTINGS_PREFIX + ActiveDirectoryConnectionFactory.URLS_SETTING, ldapUrl) .put(SETTINGS_PREFIX + ActiveDirectoryConnectionFactory.AD_DOMAIN_NAME_SETTING, adDomainName) .build(); } From b5b6a1093c630575826a590da1cf72412bc25c4d Mon Sep 17 00:00:00 2001 From: c-a-m Date: Wed, 22 Oct 2014 17:12:46 -0600 Subject: [PATCH 4/4] Fixes filename case typo This fixes a file-name case typo for LdapSslSocketFactory Original commit: elastic/x-pack-elasticsearch@fb71a1116e6afd85e124672d1a0ebeaef8d672b6 --- ...pSSLSocketFactoryTests.java => LdapSslSocketFactoryTests.java} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename src/test/java/org/elasticsearch/shield/authc/ldap/{LdapSSLSocketFactoryTests.java => LdapSslSocketFactoryTests.java} (100%) diff --git a/src/test/java/org/elasticsearch/shield/authc/ldap/LdapSSLSocketFactoryTests.java b/src/test/java/org/elasticsearch/shield/authc/ldap/LdapSslSocketFactoryTests.java similarity index 100% rename from src/test/java/org/elasticsearch/shield/authc/ldap/LdapSSLSocketFactoryTests.java rename to src/test/java/org/elasticsearch/shield/authc/ldap/LdapSslSocketFactoryTests.java