From d1548abe027e1108c21c35bc25d717edf854ed57 Mon Sep 17 00:00:00 2001 From: Jay Modi Date: Thu, 14 Feb 2019 07:34:29 -0700 Subject: [PATCH] Use consistent view of realms for authentication (#38815) This change updates the authentication service to use a consistent view of the realms based on the license state at the start of authentication. Without this, the license can change during authentication of a request and it will result in a failure if the realm that extracted the token is no longer in the realm list. This manifests in some tests as an authentication failure that should never really happen; one example would be the test framework's transport client user should always have a succesful authentication but in the LicensingTests this can fail and will show up as a NoNodeAvailableException. Additionally, the licensing tests have been updated to ensure that there is consistency when changing the license. The license is changed by modifying the internal xpack license state on each node, which has no protection against be changed by some pending cluster action. The methods to disable and enable now ensure we have a green cluster and that the cluster is consistent before returning. Closes #30301 --- .../action/filter/SecurityActionFilter.java | 10 +- .../saml/TransportSamlAuthenticateAction.java | 1 + .../token/TransportCreateTokenAction.java | 6 +- .../security/authc/AuthenticationService.java | 58 +++++++----- .../SecurityServerTransportInterceptor.java | 4 +- .../transport/ServerTransportFilter.java | 17 +++- .../elasticsearch/license/LicensingTests.java | 91 +++++++++++++------ .../authc/AuthenticationServiceTests.java | 7 +- .../transport/ServerTransportFilterTests.java | 5 +- 9 files changed, 135 insertions(+), 64 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/filter/SecurityActionFilter.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/filter/SecurityActionFilter.java index 8252c7f9a9dce..98d935d637c80 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/filter/SecurityActionFilter.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/filter/SecurityActionFilter.java @@ -154,7 +154,15 @@ it to the action without an associated user (not via REST or transport - this is */ final String securityAction = actionMapper.action(action, request); authcService.authenticate(securityAction, request, SystemUser.INSTANCE, - ActionListener.wrap((authc) -> authorizeRequest(authc, securityAction, request, listener), listener::onFailure)); + ActionListener.wrap((authc) -> { + if (authc != null) { + authorizeRequest(authc, securityAction, request, listener); + } else if (licenseState.isAuthAllowed() == false) { + listener.onResponse(null); + } else { + listener.onFailure(new IllegalStateException("no authentication present but auth is allowed")); + } + }, listener::onFailure)); } private void authorizeRequest(Authentication authentication, String securityAction, Request request, diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlAuthenticateAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlAuthenticateAction.java index 616f6184c3884..2cd55ea150cc8 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlAuthenticateAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlAuthenticateAction.java @@ -60,6 +60,7 @@ protected void doExecute(SamlAuthenticateRequest request, listener.onFailure(new IllegalStateException("Cannot find AuthenticationResult on thread context")); return; } + assert authentication != null : "authentication should never be null at this point"; final Map tokenMeta = (Map) result.getMetadata().get(SamlRealm.CONTEXT_TOKEN_DATA); tokenService.createUserToken(authentication, originatingAuthentication, ActionListener.wrap(tuple -> { diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/token/TransportCreateTokenAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/token/TransportCreateTokenAction.java index 566bc20966b3b..5d94ae5e8a61f 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/token/TransportCreateTokenAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/token/TransportCreateTokenAction.java @@ -73,7 +73,11 @@ private void authenticateAndCreateToken(CreateTokenRequest request, ActionListen authenticationService.authenticate(CreateTokenAction.NAME, request, authToken, ActionListener.wrap(authentication -> { request.getPassword().close(); - createToken(request, authentication, originatingAuthentication, true, listener); + if (authentication != null) { + createToken(request, authentication, originatingAuthentication, true, listener); + } else { + listener.onFailure(new UnsupportedOperationException("cannot create token if authentication is not allowed")); + } }, e -> { // clear the request password request.getPassword().close(); diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/AuthenticationService.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/AuthenticationService.java index e9756925ce6de..734fde7cca937 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/AuthenticationService.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/AuthenticationService.java @@ -135,8 +135,9 @@ class Authenticator { private final AuditableRequest request; private final User fallbackUser; - + private final List defaultOrderedRealmList; private final ActionListener listener; + private RealmRef authenticatedBy = null; private RealmRef lookedupBy = null; private AuthenticationToken authenticationToken = null; @@ -154,6 +155,7 @@ class Authenticator { private Authenticator(AuditableRequest auditableRequest, User fallbackUser, ActionListener listener) { this.request = auditableRequest; this.fallbackUser = fallbackUser; + this.defaultOrderedRealmList = realms.asList(); this.listener = listener; } @@ -172,27 +174,33 @@ private Authenticator(AuditableRequest auditableRequest, User fallbackUser, Acti * */ private void authenticateAsync() { - lookForExistingAuthentication((authentication) -> { - if (authentication != null) { - listener.onResponse(authentication); - } else { - tokenService.getAndValidateToken(threadContext, ActionListener.wrap(userToken -> { - if (userToken != null) { - writeAuthToContext(userToken.getAuthentication()); - } else { - extractToken(this::consumeToken); - } - }, e -> { - if (e instanceof ElasticsearchSecurityException && + if (defaultOrderedRealmList.isEmpty()) { + // this happens when the license state changes between the call to authenticate and the actual invocation + // to get the realm list + listener.onResponse(null); + } else { + lookForExistingAuthentication((authentication) -> { + if (authentication != null) { + listener.onResponse(authentication); + } else { + tokenService.getAndValidateToken(threadContext, ActionListener.wrap(userToken -> { + if (userToken != null) { + writeAuthToContext(userToken.getAuthentication()); + } else { + extractToken(this::consumeToken); + } + }, e -> { + if (e instanceof ElasticsearchSecurityException && tokenService.isExpiredTokenException((ElasticsearchSecurityException) e) == false) { - // intentionally ignore the returned exception; we call this primarily - // for the auditing as we already have a purpose built exception - request.tamperedRequest(); - } - listener.onFailure(e); - })); - } - }); + // intentionally ignore the returned exception; we call this primarily + // for the auditing as we already have a purpose built exception + request.tamperedRequest(); + } + listener.onFailure(e); + })); + } + }); + } } /** @@ -233,7 +241,7 @@ void extractToken(Consumer consumer) { if (authenticationToken != null) { action = () -> consumer.accept(authenticationToken); } else { - for (Realm realm : realms) { + for (Realm realm : defaultOrderedRealmList) { final AuthenticationToken token = realm.token(threadContext); if (token != null) { action = () -> consumer.accept(token); @@ -260,7 +268,6 @@ private void consumeToken(AuthenticationToken token) { handleNullToken(); } else { authenticationToken = token; - final List realmsList = realms.asList(); final Map> messages = new LinkedHashMap<>(); final BiConsumer> realmAuthenticatingConsumer = (realm, userListener) -> { if (realm.supports(authenticationToken)) { @@ -297,11 +304,12 @@ private void consumeToken(AuthenticationToken token) { userListener.onResponse(null); } }; + final IteratingActionListener authenticatingListener = new IteratingActionListener<>(ContextPreservingActionListener.wrapPreservingContext(ActionListener.wrap( (user) -> consumeUser(user, messages), (e) -> listener.onFailure(request.exceptionProcessingRequest(e, token))), threadContext), - realmAuthenticatingConsumer, realmsList, threadContext); + realmAuthenticatingConsumer, defaultOrderedRealmList, threadContext); try { authenticatingListener.run(); } catch (Exception e) { @@ -388,7 +396,7 @@ private void consumeUser(User user, Map> message * names of users that exist using a timing attack */ private void lookupRunAsUser(final User user, String runAsUsername, Consumer userConsumer) { - final RealmUserLookup lookup = new RealmUserLookup(realms.asList(), threadContext); + final RealmUserLookup lookup = new RealmUserLookup(defaultOrderedRealmList, threadContext); lookup.lookup(runAsUsername, ActionListener.wrap(tuple -> { if (tuple == null) { // the user does not exist, but we still create a User object, which will later be rejected by authz diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/SecurityServerTransportInterceptor.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/SecurityServerTransportInterceptor.java index b59e656c5b92d..f4de04c729f5f 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/SecurityServerTransportInterceptor.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/SecurityServerTransportInterceptor.java @@ -187,12 +187,12 @@ protected Map initializeProfileFilters(Destructiv case "client": profileFilters.put(entry.getKey(), new ServerTransportFilter.ClientProfile(authcService, authzService, threadPool.getThreadContext(), extractClientCert, destructiveOperations, reservedRealmEnabled, - securityContext)); + securityContext, licenseState)); break; case "node": profileFilters.put(entry.getKey(), new ServerTransportFilter.NodeProfile(authcService, authzService, threadPool.getThreadContext(), extractClientCert, destructiveOperations, reservedRealmEnabled, - securityContext)); + securityContext, licenseState)); break; default: throw new IllegalStateException("unknown profile type: " + type); diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/ServerTransportFilter.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/ServerTransportFilter.java index fa8b094da7d0f..98ecef7ce62eb 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/ServerTransportFilter.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/ServerTransportFilter.java @@ -19,6 +19,7 @@ import org.elasticsearch.action.admin.indices.open.OpenIndexAction; import org.elasticsearch.action.support.DestructiveOperations; import org.elasticsearch.common.util.concurrent.ThreadContext; +import org.elasticsearch.license.XPackLicenseState; import org.elasticsearch.transport.TaskTransportChannel; import org.elasticsearch.transport.TcpTransportChannel; import org.elasticsearch.transport.TransportChannel; @@ -78,10 +79,11 @@ class NodeProfile implements ServerTransportFilter { private final DestructiveOperations destructiveOperations; private final boolean reservedRealmEnabled; private final SecurityContext securityContext; + private final XPackLicenseState licenseState; NodeProfile(AuthenticationService authcService, AuthorizationService authzService, ThreadContext threadContext, boolean extractClientCert, DestructiveOperations destructiveOperations, - boolean reservedRealmEnabled, SecurityContext securityContext) { + boolean reservedRealmEnabled, SecurityContext securityContext, XPackLicenseState licenseState) { this.authcService = authcService; this.authzService = authzService; this.threadContext = threadContext; @@ -89,6 +91,7 @@ class NodeProfile implements ServerTransportFilter { this.destructiveOperations = destructiveOperations; this.reservedRealmEnabled = reservedRealmEnabled; this.securityContext = securityContext; + this.licenseState = licenseState; } @Override @@ -129,6 +132,7 @@ requests from all the nodes are attached with a user (either a serialize final Version version = transportChannel.getVersion().equals(Version.V_5_4_0) ? Version.CURRENT : transportChannel.getVersion(); authcService.authenticate(securityAction, request, (User)null, ActionListener.wrap((authentication) -> { + if (authentication != null) { if (reservedRealmEnabled && authentication.getVersion().before(Version.V_5_2_0) && KibanaUser.NAME.equals(authentication.getUser().authenticatedUser().principal())) { executeAsCurrentVersionKibanaUser(securityAction, request, transportChannel, listener, authentication); @@ -156,7 +160,12 @@ requests from all the nodes are attached with a user (either a serialize }); asyncAuthorizer.authorize(authzService); } - }, listener::onFailure)); + } else if (licenseState.isAuthAllowed() == false) { + listener.onResponse(null); + } else { + listener.onFailure(new IllegalStateException("no authentication present but auth is allowed")); + } + }, listener::onFailure)); } private void executeAsCurrentVersionKibanaUser(String securityAction, TransportRequest request, TransportChannel transportChannel, @@ -220,9 +229,9 @@ class ClientProfile extends NodeProfile { ClientProfile(AuthenticationService authcService, AuthorizationService authzService, ThreadContext threadContext, boolean extractClientCert, DestructiveOperations destructiveOperations, - boolean reservedRealmEnabled, SecurityContext securityContext) { + boolean reservedRealmEnabled, SecurityContext securityContext, XPackLicenseState licenseState) { super(authcService, authzService, threadContext, extractClientCert, destructiveOperations, reservedRealmEnabled, - securityContext); + securityContext, licenseState); } @Override diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/license/LicensingTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/license/LicensingTests.java index f2b0840ada806..c777514a3d820 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/license/LicensingTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/license/LicensingTests.java @@ -21,12 +21,12 @@ import org.elasticsearch.client.ResponseException; import org.elasticsearch.client.transport.NoNodeAvailableException; import org.elasticsearch.client.transport.TransportClient; -import org.elasticsearch.cluster.routing.ShardRoutingState; import org.elasticsearch.common.network.NetworkModule; import org.elasticsearch.common.settings.SecureString; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.util.concurrent.ThreadContext; import org.elasticsearch.discovery.DiscoveryModule; +import org.elasticsearch.license.License.OperationMode; import org.elasticsearch.node.MockNode; import org.elasticsearch.node.Node; import org.elasticsearch.plugins.Plugin; @@ -55,6 +55,7 @@ import java.util.Arrays; import java.util.Collection; import java.util.List; +import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; @@ -69,7 +70,7 @@ @TestLogging("org.elasticsearch.cluster.service:TRACE,org.elasticsearch.discovery.zen:TRACE,org.elasticsearch.action.search:TRACE," + "org.elasticsearch.search:TRACE") public class LicensingTests extends SecurityIntegTestCase { - public static final String ROLES = + private static final String ROLES = SecuritySettingsSource.TEST_ROLE + ":\n" + " cluster: [ all ]\n" + " indices:\n" + @@ -92,7 +93,7 @@ public class LicensingTests extends SecurityIntegTestCase { " - names: 'b'\n" + " privileges: [all]\n"; - public static final String USERS_ROLES = + private static final String USERS_ROLES = SecuritySettingsSource.CONFIG_STANDARD_USER_ROLES + "role_a:user_a,user_b\n" + "role_b:user_b\n"; @@ -135,8 +136,8 @@ protected int maxNumberOfNodes() { } @Before - public void resetLicensing() { - enableLicensing(); + public void resetLicensing() throws InterruptedException { + enableLicensing(OperationMode.BASIC); } @After @@ -159,11 +160,7 @@ public void testEnableDisableBehaviour() throws Exception { assertEquals(DocWriteResponse.Result.CREATED, indexResponse.getResult()); refresh(); - // wait for all replicas to be started (to make sure that there are no more cluster state updates when we disable licensing) - assertBusy(() -> assertTrue(client().admin().cluster().prepareState().get().getState().routingTable() - .shardsWithState(ShardRoutingState.INITIALIZING).isEmpty())); - - Client client = internalCluster().transportClient(); + final Client client = internalCluster().transportClient(); disableLicensing(); @@ -277,7 +274,6 @@ public void testTransportClientAuthenticationByLicenseType() throws Exception { public void testNodeJoinWithoutSecurityExplicitlyEnabled() throws Exception { License.OperationMode mode = randomFrom(License.OperationMode.GOLD, License.OperationMode.PLATINUM, License.OperationMode.STANDARD); enableLicensing(mode); - ensureGreen(); final List unicastHostsList = internalCluster().masterClient().admin().cluster().nodesInfo(new NodesInfoRequest()).get() .getNodes().stream().map(n -> n.getTransport().getAddress().publishAddress().toString()).distinct() @@ -312,23 +308,64 @@ private static void assertElasticsearchSecurityException(ThrowingRunnable runnab assertThat(ee.status(), is(RestStatus.FORBIDDEN)); } - public static void disableLicensing() { - disableLicensing(License.OperationMode.BASIC); - } - - public static void disableLicensing(License.OperationMode operationMode) { - for (XPackLicenseState licenseState : internalCluster().getInstances(XPackLicenseState.class)) { - licenseState.update(operationMode, false, null); - } + private void disableLicensing() throws InterruptedException { + // This method first makes sure licensing is enabled everywhere so that we can execute + // monitoring actions to ensure we have a stable cluster and only then do we disable. + // This is done in an await busy since there is a chance that the enabling of the license + // is overwritten by some other cluster activity and the node throws an exception while we + // wait for things to stabilize! + final boolean success = awaitBusy(() -> { + try { + for (XPackLicenseState licenseState : internalCluster().getInstances(XPackLicenseState.class)) { + if (licenseState.isAuthAllowed() == false) { + enableLicensing(OperationMode.BASIC); + break; + } + } + + ensureGreen(); + ensureClusterSizeConsistency(); + ensureClusterStateConsistency(); + + // apply the disabling of the license once the cluster is stable + for (XPackLicenseState licenseState : internalCluster().getInstances(XPackLicenseState.class)) { + licenseState.update(OperationMode.BASIC, false, null); + } + } catch (Exception e) { + logger.error("Caught exception while disabling license", e); + return false; + } + return true; + }, 30L, TimeUnit.SECONDS); + assertTrue(success); } - public static void enableLicensing() { - enableLicensing(License.OperationMode.BASIC); - } - - public static void enableLicensing(License.OperationMode operationMode) { - for (XPackLicenseState licenseState : internalCluster().getInstances(XPackLicenseState.class)) { - licenseState.update(operationMode, true, null); - } + private void enableLicensing(License.OperationMode operationMode) throws InterruptedException { + // do this in an await busy since there is a chance that the enabling of the license is + // overwritten by some other cluster activity and the node throws an exception while we + // wait for things to stabilize! + final boolean success = awaitBusy(() -> { + try { + // first update the license so we can execute monitoring actions + for (XPackLicenseState licenseState : internalCluster().getInstances(XPackLicenseState.class)) { + licenseState.update(operationMode, true, null); + } + + ensureGreen(); + ensureClusterSizeConsistency(); + ensureClusterStateConsistency(); + + // re-apply the update in case any node received an updated cluster state that triggered the license state + // to change + for (XPackLicenseState licenseState : internalCluster().getInstances(XPackLicenseState.class)) { + licenseState.update(operationMode, true, null); + } + } catch (Exception e) { + logger.error("Caught exception while enabling license", e); + return false; + } + return true; + }, 30L, TimeUnit.SECONDS); + assertTrue(success); } } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/AuthenticationServiceTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/AuthenticationServiceTests.java index becd6069a2526..35b44ba9579bf 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/AuthenticationServiceTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/AuthenticationServiceTests.java @@ -109,6 +109,7 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.reset; +import static org.mockito.Mockito.spy; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.verifyZeroInteractions; @@ -159,9 +160,9 @@ public void init() throws Exception { XPackLicenseState licenseState = mock(XPackLicenseState.class); when(licenseState.allowedRealmType()).thenReturn(XPackLicenseState.AllowedRealmType.ALL); when(licenseState.isAuthAllowed()).thenReturn(true); - realms = new TestRealms(Settings.EMPTY, TestEnvironment.newEnvironment(settings), Collections.emptyMap(), + realms = spy(new TestRealms(Settings.EMPTY, TestEnvironment.newEnvironment(settings), Collections.emptyMap(), licenseState, threadContext, mock(ReservedRealm.class), Arrays.asList(firstRealm, secondRealm), - Collections.singletonList(firstRealm)); + Collections.singletonList(firstRealm))); auditTrail = mock(AuditTrailService.class); client = mock(Client.class); @@ -264,6 +265,8 @@ public void testAuthenticateBothSupportSecondSucceeds() throws Exception { }, this::logAndFail)); assertTrue(completed.get()); verify(auditTrail).authenticationFailed(reqId, firstRealm.name(), token, "_action", message); + verify(realms).asList(); + verifyNoMoreInteractions(realms); } public void testAuthenticateFirstNotSupportingSecondSucceeds() throws Exception { diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/transport/ServerTransportFilterTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/transport/ServerTransportFilterTests.java index 8624b780a9ced..6a9d33ae39ef2 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/transport/ServerTransportFilterTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/transport/ServerTransportFilterTests.java @@ -18,6 +18,7 @@ import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.util.concurrent.ThreadContext; +import org.elasticsearch.license.XPackLicenseState; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.transport.TransportChannel; import org.elasticsearch.transport.TransportRequest; @@ -311,13 +312,13 @@ private ServerTransportFilter.ClientProfile getClientFilter(boolean reservedReal Settings settings = Settings.builder().put("path.home", createTempDir()).build(); ThreadContext threadContext = new ThreadContext(settings); return new ServerTransportFilter.ClientProfile(authcService, authzService, threadContext, false, destructiveOperations, - reservedRealmEnabled, new SecurityContext(settings, threadContext)); + reservedRealmEnabled, new SecurityContext(settings, threadContext), new XPackLicenseState(settings)); } private ServerTransportFilter.NodeProfile getNodeFilter(boolean reservedRealmEnabled) throws IOException { Settings settings = Settings.builder().put("path.home", createTempDir()).build(); ThreadContext threadContext = new ThreadContext(settings); return new ServerTransportFilter.NodeProfile(authcService, authzService, threadContext, false, destructiveOperations, - reservedRealmEnabled, new SecurityContext(settings, threadContext)); + reservedRealmEnabled, new SecurityContext(settings, threadContext), new XPackLicenseState(settings)); } }