From c63cbc1e02efc31e4aa9a2d032f32f1b029d5f87 Mon Sep 17 00:00:00 2001 From: Jay Modi Date: Wed, 13 Jun 2018 12:09:55 -0600 Subject: [PATCH] Security: fix token bwc with pre 6.0.0-beta2 (#31254) This commit fixes a backwards compatibility bug in the token service that causes token decoding to fail when there is a pre 6.0.0-beta2 node in the cluster. The token encoding is actually the culprit as a version check is missing around the serialization of the key hash bytes. This value was added in 6.0.0-beta2 and cannot be sent to nodes that do not know about this value. The version check has been added and the token service unit tests have been enhanced to randomly run with some 5.6.x nodes in the cluster service. Additionally, a small change was made to the way we check to see if the token metadata needs to be installed. Previously we would pass the metadata to the install method and check that the token metadata is null. This null check is now done prior to checking if the metadata can be installed. Relates #30743 Closes #31195 --- .../xpack/security/authc/TokenService.java | 63 ++++++++-------- .../security/authc/TokenServiceTests.java | 72 +++++++++++++++---- x-pack/qa/rolling-upgrade/build.gradle | 1 - .../TokenBackwardsCompatibilityIT.java | 5 +- 4 files changed, 90 insertions(+), 51 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java index 99680da2bba6a..4aa4c866c8976 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java @@ -9,7 +9,6 @@ import org.apache.lucene.util.BytesRef; import org.apache.lucene.util.BytesRefBuilder; import org.elasticsearch.cluster.ClusterStateUpdateTask; -import org.elasticsearch.cluster.metadata.MetaData; import org.elasticsearch.common.Priority; import org.elasticsearch.core.internal.io.IOUtils; import org.apache.lucene.util.UnicodeUtil; @@ -1041,7 +1040,9 @@ public String getUserTokenString(UserToken userToken) throws IOException, Genera KeyAndCache keyAndCache = keyCache.activeKeyCache; Version.writeVersion(userToken.getVersion(), out); out.writeByteArray(keyAndCache.getSalt().bytes); - out.writeByteArray(keyAndCache.getKeyHash().bytes); + if (userToken.getVersion().onOrAfter(Version.V_6_0_0_beta2)) { + out.writeByteArray(keyAndCache.getKeyHash().bytes); + } final byte[] initializationVector = getNewInitializationVector(); out.writeByteArray(initializationVector); try (CipherOutputStream encryptedOutput = @@ -1369,16 +1370,18 @@ private void initialize(ClusterService clusterService) { return; } + TokenMetaData custom = event.state().custom(TokenMetaData.TYPE); if (state.nodes().isLocalNodeElectedMaster()) { - if (XPackPlugin.isReadyForXPackCustomMetadata(state)) { - installTokenMetadata(state.metaData()); - } else { - logger.debug("cannot add token metadata to cluster as the following nodes might not understand the metadata: {}", - () -> XPackPlugin.nodesNotReadyForXPackCustomMetadata(state)); + if (custom == null) { + if (XPackPlugin.isReadyForXPackCustomMetadata(state)) { + installTokenMetadata(); + } else { + logger.debug("cannot add token metadata to cluster as the following nodes might not understand the metadata: {}", + () -> XPackPlugin.nodesNotReadyForXPackCustomMetadata(state)); + } } } - TokenMetaData custom = event.state().custom(TokenMetaData.TYPE); if (custom != null && custom.equals(getTokenMetaData()) == false) { logger.info("refresh keys"); try { @@ -1394,33 +1397,31 @@ private void initialize(ClusterService clusterService) { // to prevent too many cluster state update tasks to be queued for doing the same update private final AtomicBoolean installTokenMetadataInProgress = new AtomicBoolean(false); - private void installTokenMetadata(MetaData metaData) { - if (metaData.custom(TokenMetaData.TYPE) == null) { - if (installTokenMetadataInProgress.compareAndSet(false, true)) { - clusterService.submitStateUpdateTask("install-token-metadata", new ClusterStateUpdateTask(Priority.URGENT) { - @Override - public ClusterState execute(ClusterState currentState) { - XPackPlugin.checkReadyForXPackCustomMetadata(currentState); + private void installTokenMetadata() { + if (installTokenMetadataInProgress.compareAndSet(false, true)) { + clusterService.submitStateUpdateTask("install-token-metadata", new ClusterStateUpdateTask(Priority.URGENT) { + @Override + public ClusterState execute(ClusterState currentState) { + XPackPlugin.checkReadyForXPackCustomMetadata(currentState); - if (currentState.custom(TokenMetaData.TYPE) == null) { - return ClusterState.builder(currentState).putCustom(TokenMetaData.TYPE, getTokenMetaData()).build(); - } else { - return currentState; - } + if (currentState.custom(TokenMetaData.TYPE) == null) { + return ClusterState.builder(currentState).putCustom(TokenMetaData.TYPE, getTokenMetaData()).build(); + } else { + return currentState; } + } - @Override - public void onFailure(String source, Exception e) { - installTokenMetadataInProgress.set(false); - logger.error("unable to install token metadata", e); - } + @Override + public void onFailure(String source, Exception e) { + installTokenMetadataInProgress.set(false); + logger.error("unable to install token metadata", e); + } - @Override - public void clusterStateProcessed(String source, ClusterState oldState, ClusterState newState) { - installTokenMetadataInProgress.set(false); - } - }); - } + @Override + public void clusterStateProcessed(String source, ClusterState oldState, ClusterState newState) { + installTokenMetadataInProgress.set(false); + } + }); } } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/TokenServiceTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/TokenServiceTests.java index 07276e33b4efe..dbce97e31def4 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/TokenServiceTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/TokenServiceTests.java @@ -26,6 +26,9 @@ import org.elasticsearch.action.update.UpdateAction; import org.elasticsearch.action.update.UpdateRequestBuilder; import org.elasticsearch.client.Client; +import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.cluster.node.DiscoveryNode; +import org.elasticsearch.cluster.node.DiscoveryNodes; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.settings.MockSecureSettings; import org.elasticsearch.common.Strings; @@ -44,6 +47,7 @@ import org.elasticsearch.test.ClusterServiceUtils; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.test.EqualsHashCodeTestUtils; +import org.elasticsearch.test.VersionUtils; import org.elasticsearch.threadpool.FixedExecutorBuilder; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.xpack.core.XPackSettings; @@ -53,6 +57,7 @@ import org.elasticsearch.xpack.core.security.user.User; import org.elasticsearch.xpack.core.watcher.watch.ClockMock; import org.elasticsearch.xpack.security.support.SecurityIndexManager; +import org.junit.After; import org.junit.AfterClass; import org.junit.Before; import org.junit.BeforeClass; @@ -66,6 +71,7 @@ import java.time.temporal.ChronoUnit; import java.util.Base64; import java.util.Collections; +import java.util.EnumSet; import java.util.HashMap; import java.util.Map; import java.util.concurrent.ExecutionException; @@ -91,6 +97,7 @@ public class TokenServiceTests extends ESTestCase { private Client client; private SecurityIndexManager securityIndex; private ClusterService clusterService; + private boolean mixedCluster; private Settings tokenServiceEnabledSettings = Settings.builder() .put(XPackSettings.TOKEN_SERVICE_ENABLED_SETTING.getKey(), true).build(); @@ -141,6 +148,25 @@ public void setupClient() { return null; }).when(securityIndex).prepareIndexIfNeededThenExecute(any(Consumer.class), any(Runnable.class)); this.clusterService = ClusterServiceUtils.createClusterService(threadPool); + this.mixedCluster = randomBoolean(); + if (mixedCluster) { + Version version = VersionUtils.randomVersionBetween(random(), Version.V_5_6_0, Version.V_5_6_10); + logger.info("adding a node with version [{}] to the cluster service", version); + ClusterState updatedState = ClusterState.builder(clusterService.state()) + .nodes(DiscoveryNodes.builder(clusterService.state().nodes()) + .add(new DiscoveryNode("56node", ESTestCase.buildNewFakeTransportAddress(), Collections.emptyMap(), + EnumSet.allOf(DiscoveryNode.Role.class), version)) + .build()) + .build(); + ClusterServiceUtils.setState(clusterService, updatedState); + } + } + + @After + public void stopClusterService() { + if (clusterService != null) { + clusterService.close(); + } } @BeforeClass @@ -172,7 +198,7 @@ public void testAttachAndGetToken() throws Exception { PlainActionFuture future = new PlainActionFuture<>(); tokenService.getAndValidateToken(requestContext, future); UserToken serialized = future.get(); - assertEquals(authentication, serialized.getAuthentication()); + assertAuthenticationEquals(authentication, serialized.getAuthentication()); } try (ThreadContext.StoredContext ignore = requestContext.newStoredContext(true)) { @@ -183,11 +209,12 @@ public void testAttachAndGetToken() throws Exception { PlainActionFuture future = new PlainActionFuture<>(); anotherService.getAndValidateToken(requestContext, future); UserToken fromOtherService = future.get(); - assertEquals(authentication, fromOtherService.getAuthentication()); + assertAuthenticationEquals(authentication, fromOtherService.getAuthentication()); } } public void testRotateKey() throws Exception { + assumeFalse("internally managed keys do not work in a mixed cluster", mixedCluster); TokenService tokenService = new TokenService(tokenServiceEnabledSettings, systemUTC(), client, securityIndex, clusterService); Authentication authentication = new Authentication(new User("joe", "admin"), new RealmRef("native_realm", "native", "node1"), null); PlainActionFuture> tokenFuture = new PlainActionFuture<>(); @@ -203,7 +230,7 @@ public void testRotateKey() throws Exception { PlainActionFuture future = new PlainActionFuture<>(); tokenService.getAndValidateToken(requestContext, future); UserToken serialized = future.get(); - assertEquals(authentication, serialized.getAuthentication()); + assertAuthenticationEquals(authentication, serialized.getAuthentication()); } rotateKeys(tokenService); @@ -211,7 +238,7 @@ public void testRotateKey() throws Exception { PlainActionFuture future = new PlainActionFuture<>(); tokenService.getAndValidateToken(requestContext, future); UserToken serialized = future.get(); - assertEquals(authentication, serialized.getAuthentication()); + assertAuthenticationEquals(authentication, serialized.getAuthentication()); } PlainActionFuture> newTokenFuture = new PlainActionFuture<>(); @@ -240,6 +267,7 @@ private void rotateKeys(TokenService tokenService) { } public void testKeyExchange() throws Exception { + assumeFalse("internally managed keys do not work in a mixed cluster", mixedCluster); TokenService tokenService = new TokenService(tokenServiceEnabledSettings, systemUTC(), client, securityIndex, clusterService); int numRotations = 0;randomIntBetween(1, 5); for (int i = 0; i < numRotations; i++) { @@ -261,7 +289,7 @@ public void testKeyExchange() throws Exception { PlainActionFuture future = new PlainActionFuture<>(); otherTokenService.getAndValidateToken(requestContext, future); UserToken serialized = future.get(); - assertEquals(authentication, serialized.getAuthentication()); + assertAuthenticationEquals(authentication, serialized.getAuthentication()); } rotateKeys(tokenService); @@ -272,11 +300,12 @@ public void testKeyExchange() throws Exception { PlainActionFuture future = new PlainActionFuture<>(); otherTokenService.getAndValidateToken(requestContext, future); UserToken serialized = future.get(); - assertEquals(authentication, serialized.getAuthentication()); + assertAuthenticationEquals(authentication, serialized.getAuthentication()); } } public void testPruneKeys() throws Exception { + assumeFalse("internally managed keys do not work in a mixed cluster", mixedCluster); TokenService tokenService = new TokenService(tokenServiceEnabledSettings, systemUTC(), client, securityIndex, clusterService); Authentication authentication = new Authentication(new User("joe", "admin"), new RealmRef("native_realm", "native", "node1"), null); PlainActionFuture> tokenFuture = new PlainActionFuture<>(); @@ -292,7 +321,7 @@ public void testPruneKeys() throws Exception { PlainActionFuture future = new PlainActionFuture<>(); tokenService.getAndValidateToken(requestContext, future); UserToken serialized = future.get(); - assertEquals(authentication, serialized.getAuthentication()); + assertAuthenticationEquals(authentication, serialized.getAuthentication()); } TokenMetaData metaData = tokenService.pruneKeys(randomIntBetween(0, 100)); tokenService.refreshMetaData(metaData); @@ -306,7 +335,7 @@ public void testPruneKeys() throws Exception { PlainActionFuture future = new PlainActionFuture<>(); tokenService.getAndValidateToken(requestContext, future); UserToken serialized = future.get(); - assertEquals(authentication, serialized.getAuthentication()); + assertAuthenticationEquals(authentication, serialized.getAuthentication()); } PlainActionFuture> newTokenFuture = new PlainActionFuture<>(); @@ -332,7 +361,7 @@ public void testPruneKeys() throws Exception { PlainActionFuture future = new PlainActionFuture<>(); tokenService.getAndValidateToken(requestContext, future); UserToken serialized = future.get(); - assertEquals(authentication, serialized.getAuthentication()); + assertAuthenticationEquals(authentication, serialized.getAuthentication()); } } @@ -353,7 +382,7 @@ public void testPassphraseWorks() throws Exception { PlainActionFuture future = new PlainActionFuture<>(); tokenService.getAndValidateToken(requestContext, future); UserToken serialized = future.get(); - assertEquals(authentication, serialized.getAuthentication()); + assertAuthenticationEquals(authentication, serialized.getAuthentication()); } try (ThreadContext.StoredContext ignore = requestContext.newStoredContext(true)) { @@ -454,7 +483,7 @@ public void testTokenExpiry() throws Exception { // the clock is still frozen, so the cookie should be valid PlainActionFuture future = new PlainActionFuture<>(); tokenService.getAndValidateToken(requestContext, future); - assertEquals(authentication, future.get().getAuthentication()); + assertAuthenticationEquals(authentication, future.get().getAuthentication()); } final TimeValue defaultExpiration = TokenService.TOKEN_EXPIRATION.get(Settings.EMPTY); @@ -464,7 +493,7 @@ public void testTokenExpiry() throws Exception { clock.fastForwardSeconds(fastForwardAmount); PlainActionFuture future = new PlainActionFuture<>(); tokenService.getAndValidateToken(requestContext, future); - assertEquals(authentication, future.get().getAuthentication()); + assertAuthenticationEquals(authentication, future.get().getAuthentication()); } try (ThreadContext.StoredContext ignore = requestContext.newStoredContext(true)) { @@ -473,7 +502,7 @@ public void testTokenExpiry() throws Exception { clock.rewind(TimeValue.timeValueNanos(clock.instant().getNano())); // trim off nanoseconds since don't store them in the index PlainActionFuture future = new PlainActionFuture<>(); tokenService.getAndValidateToken(requestContext, future); - assertEquals(authentication, future.get().getAuthentication()); + assertAuthenticationEquals(authentication, future.get().getAuthentication()); } try (ThreadContext.StoredContext ignore = requestContext.newStoredContext(true)) { @@ -569,7 +598,7 @@ public void testIndexNotAvailable() throws Exception { PlainActionFuture future = new PlainActionFuture<>(); tokenService.getAndValidateToken(requestContext, future); UserToken serialized = future.get(); - assertEquals(authentication, serialized.getAuthentication()); + assertAuthenticationEquals(authentication, serialized.getAuthentication()); when(securityIndex.isAvailable()).thenReturn(false); when(securityIndex.indexExists()).thenReturn(true); @@ -601,6 +630,7 @@ public void testDecodePre6xToken() throws GeneralSecurityException, ExecutionExc assertWarnings("[xpack.security.authc.token.passphrase] setting was deprecated in Elasticsearch and will be removed in a future" + " release! See the breaking changes documentation for the next major version."); } + public void testGetAuthenticationWorksWithExpiredToken() throws Exception { TokenService tokenService = new TokenService(tokenServiceEnabledSettings, Clock.systemUTC(), client, securityIndex, clusterService); @@ -611,7 +641,7 @@ public void testGetAuthenticationWorksWithExpiredToken() throws Exception { PlainActionFuture>> authFuture = new PlainActionFuture<>(); tokenService.getAuthenticationAndMetaData(userTokenString, authFuture); Authentication retrievedAuth = authFuture.actionGet().v1(); - assertEquals(authentication, retrievedAuth); + assertAuthenticationEquals(authentication, retrievedAuth); } private void mockGetTokenFromId(UserToken userToken) { @@ -638,4 +668,16 @@ public static void mockGetTokenFromId(UserToken userToken, Client client) { return Void.TYPE; }).when(client).get(any(GetRequest.class), any(ActionListener.class)); } + + private void assertAuthenticationEquals(Authentication expected, Authentication actual) { + if (mixedCluster) { + assertNotNull(expected); + assertNotNull(actual); + assertEquals(expected.getUser(), actual.getUser()); + assertEquals(expected.getAuthenticatedBy(), actual.getAuthenticatedBy()); + assertEquals(expected.getLookedUpBy(), actual.getLookedUpBy()); + } else { + assertEquals(expected, actual); + } + } } diff --git a/x-pack/qa/rolling-upgrade/build.gradle b/x-pack/qa/rolling-upgrade/build.gradle index e53c34f42e042..54033b0422c72 100644 --- a/x-pack/qa/rolling-upgrade/build.gradle +++ b/x-pack/qa/rolling-upgrade/build.gradle @@ -146,7 +146,6 @@ subprojects { if (version.onOrAfter('6.0.0') == false) { // this is needed since in 5.6 we don't bootstrap the token service if there is no explicit initial password keystoreSetting 'xpack.security.authc.token.passphrase', 'xpack_token_passphrase' - setting 'xpack.security.authc.token.enabled', 'true' } dependsOn copyTestNodeKeystore extraConfigFile 'testnode.jks', new File(outputDir + '/testnode.jks') diff --git a/x-pack/qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/TokenBackwardsCompatibilityIT.java b/x-pack/qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/TokenBackwardsCompatibilityIT.java index 2ba388e9852dc..d5e87cca5cfc2 100644 --- a/x-pack/qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/TokenBackwardsCompatibilityIT.java +++ b/x-pack/qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/TokenBackwardsCompatibilityIT.java @@ -74,9 +74,6 @@ public void testMixedCluster() throws Exception { assumeTrue("the master must be on the latest version before we can write", isMasterOnLatestVersion()); assumeFalse("can't be run twice because it invalidates a token so we skip the first attempt", Booleans.parseBoolean(System.getProperty("tests.first_round"))); - Version upgradeFromVersion = Version.fromString(System.getProperty("tests.upgrade_from_version")); - assumeFalse("this test fails for unknown reasons when run before 5.6.0", - upgradeFromVersion.before(Version.V_6_0_0)); Response getResponse = client().performRequest("GET", "token_backwards_compatibility_it/doc/old_cluster_token2"); assertOK(getResponse); @@ -124,7 +121,7 @@ public void testMixedCluster() throws Exception { } public void testUpgradedCluster() throws Exception { - assumeTrue("this test should only run against the mixed cluster", CLUSTER_TYPE == ClusterType.UPGRADED); + assumeTrue("this test should only run against the upgraded cluster", CLUSTER_TYPE == ClusterType.UPGRADED); Response getResponse = client().performRequest("GET", "token_backwards_compatibility_it/doc/old_cluster_token2"); assertOK(getResponse); Map source = (Map) entityAsMap(getResponse).get("_source");