From ecb16e64efb9accbca169cb5b19910f3a93ad058 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Slobodan=20Adamovi=C4=87?= Date: Sun, 16 Feb 2025 10:47:55 +0100 Subject: [PATCH] Fix internal cluster and single node security tests (#121466) This PR fixes SecuritySingleNodeTestCase and ProfileIntegTests tests. - The security single node test failures are solved by ensuring every test starts with security index created and available. This is in order to have consistent state for every test. With the changes introduce in the #120323 PR, only the first test would execute with .security index being created async. Subsequent tests would execute without security index creation due to the fact that whole cluster is wiped after each test. This caused a flakiness only for the first test, because there was no mechanism in place to ensure that the .security index is active before test execution. - The profile integration tests are solved by introducing an anonymous role which don't have application privileges. The application privileges are resolved from the .security index and assigned to all users, including the es_test_root user which is used during cluster wiping. Due to asynchronous nature of cluster setup and .security index creation, this now causes flakiness. The main problem is that wiping is done asynchronously and uses es_test_root which had assigned anonymous rac_role which depends on .security index being available for search in order to resolve application privileges. The application privilege resolution is done in buildRoleFromDescriptors which currently does not wait for security index availability(can be improved - but still wouldn't fix internal cluster tests). This wasn't a problem before just because we simply return empty results when .security index does not exist. There is some complexity in making internal clusters wait for availability of security shards before the test, so I think this solution is acceptable given that it's not required for this tests to have anonymous role with application privileges. Resolves #121022 Resolves #121096 Resolves #121101 Resolves #120988 Resolves #121108 Resolves #120983 Resolves #120987 Resolves #121179 Resolves #121183 Resolves #121346 Resolves #121151 Resolves #120985 Resolves #121039 Resolves #121483 Resolves #121116 Resolves #121258 Resolves #121486 (cherry picked from commit 369c6413024be706e4c0d824ea86898aaab0884c) # Conflicts: # muted-tests.yml --- muted-tests.yml | 47 +--------------- .../test/SecuritySingleNodeTestCase.java | 56 ++++++++++++++++++- ...ervedRealmElasticAutoconfigIntegTests.java | 52 +---------------- .../security/profile/ProfileIntegTests.java | 17 +++++- 4 files changed, 74 insertions(+), 98 deletions(-) diff --git a/muted-tests.yml b/muted-tests.yml index d70e808fc2407..ad0058aff5dbc 100644 --- a/muted-tests.yml +++ b/muted-tests.yml @@ -227,44 +227,17 @@ tests: - class: org.elasticsearch.action.search.SearchProgressActionListenerIT method: testSearchProgressWithQuery issue: https://github.com/elastic/elasticsearch/issues/120994 -- class: org.elasticsearch.xpack.security.profile.ProfileIntegTests - method: testSuggestProfilesWithName - issue: https://github.com/elastic/elasticsearch/issues/121022 -- class: org.elasticsearch.xpack.security.profile.ProfileIntegTests - method: testProfileAPIsWhenIndexNotCreated - issue: https://github.com/elastic/elasticsearch/issues/121096 -- class: org.elasticsearch.xpack.security.profile.ProfileIntegTests - method: testGetProfiles - issue: https://github.com/elastic/elasticsearch/issues/121101 -- class: org.elasticsearch.xpack.security.authc.service.ServiceAccountSingleNodeTests - method: testAuthenticateWithServiceFileToken - issue: https://github.com/elastic/elasticsearch/issues/120988 -- class: org.elasticsearch.xpack.security.profile.ProfileIntegTests - method: testUpdateProfileData - issue: https://github.com/elastic/elasticsearch/issues/121108 - class: org.elasticsearch.backwards.MixedClusterClientYamlTestSuiteIT method: test {p0=nodes.stats/11_indices_metrics/indices mappings exact count test for indices level} issue: https://github.com/elastic/elasticsearch/issues/120950 - class: org.elasticsearch.xpack.shutdown.AllocationFailuresResetOnShutdownIT method: testResetAllocationFailuresOnNodeShutdown issue: https://github.com/elastic/elasticsearch/issues/121129 -- class: org.elasticsearch.xpack.security.authc.jwt.JwtRealmSingleNodeTests - method: testActivateProfileForJWT - issue: https://github.com/elastic/elasticsearch/issues/120983 -- class: org.elasticsearch.xpack.security.profile.ProfileIntegTests - method: testProfileIndexAutoCreation - issue: https://github.com/elastic/elasticsearch/issues/120987 - class: org.elasticsearch.xpack.security.FileSettingsRoleMappingsRestartIT method: testFileSettingsReprocessedOnRestartWithoutVersionChange issue: https://github.com/elastic/elasticsearch/issues/120964 -- class: org.elasticsearch.xpack.security.profile.ProfileIntegTests - method: testGetUsersWithProfileUidWhenProfileIndexDoesNotExists - issue: https://github.com/elastic/elasticsearch/issues/121179 - class: org.elasticsearch.xpack.ml.integration.PyTorchModelIT issue: https://github.com/elastic/elasticsearch/issues/121165 -- class: org.elasticsearch.xpack.security.profile.ProfileIntegTests - method: testSetEnabled - issue: https://github.com/elastic/elasticsearch/issues/121183 - class: org.elasticsearch.xpack.test.rest.XPackRestIT method: test {p0=transform/*} issue: https://github.com/elastic/elasticsearch/issues/120816 @@ -293,30 +266,12 @@ tests: - class: org.elasticsearch.smoketest.DocsClientYamlTestSuiteIT method: test {yaml=reference/index-modules/slowlog/line_102} issue: https://github.com/elastic/elasticsearch/issues/121288 -- class: org.elasticsearch.xpack.security.profile.ProfileIntegTests - method: testSuggestProfilesWithHint - issue: https://github.com/elastic/elasticsearch/issues/121116 - class: org.elasticsearch.env.NodeEnvironmentTests method: testGetBestDowngradeVersion issue: https://github.com/elastic/elasticsearch/issues/121316 - class: org.elasticsearch.index.engine.ShuffleForcedMergePolicyTests method: testDiagnostics issue: https://github.com/elastic/elasticsearch/issues/121336 -- class: org.elasticsearch.xpack.security.authc.jwt.JwtRealmSingleNodeTests - method: testGrantApiKeyForJWT - issue: https://github.com/elastic/elasticsearch/issues/121039 -- class: org.elasticsearch.xpack.security.profile.ProfileIntegTests - method: testGetUsersWithProfileUid - issue: https://github.com/elastic/elasticsearch/issues/121483 -- class: org.elasticsearch.xpack.security.profile.ProfileIntegTests - method: testActivateProfile - issue: https://github.com/elastic/elasticsearch/issues/121151 -- class: org.elasticsearch.xpack.security.profile.ProfileIntegTests - method: testHasPrivileges - issue: https://github.com/elastic/elasticsearch/issues/121346 -- class: org.elasticsearch.xpack.security.profile.ProfileIntegTests - method: testSuggestProfileWithData - issue: https://github.com/elastic/elasticsearch/issues/121258 - class: org.elasticsearch.search.CrossClusterSearchUnavailableClusterIT method: testSearchSkipUnavailable issue: https://github.com/elastic/elasticsearch/issues/121497 @@ -367,4 +322,4 @@ tests: # issue: "https://github.com/elastic/elasticsearch/..." # - class: "org.elasticsearch.xpack.esql.**" # method: "test {union_types.MultiIndexIpStringStatsInline *}" -# issue: "https://github.com/elastic/elasticsearch/..." +# issue: "https://github.com/elastic/elasticsearch/..." \ No newline at end of file diff --git a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/test/SecuritySingleNodeTestCase.java b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/test/SecuritySingleNodeTestCase.java index 07bdd83c9a144..061ce6f3fc654 100644 --- a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/test/SecuritySingleNodeTestCase.java +++ b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/test/SecuritySingleNodeTestCase.java @@ -7,9 +7,16 @@ package org.elasticsearch.test; import org.apache.http.HttpHost; +import org.elasticsearch.ResourceAlreadyExistsException; import org.elasticsearch.action.admin.cluster.node.info.NodeInfo; import org.elasticsearch.action.admin.cluster.node.info.NodesInfoResponse; import org.elasticsearch.action.admin.cluster.node.info.PluginsAndModules; +import org.elasticsearch.action.admin.indices.create.CreateIndexRequest; +import org.elasticsearch.action.admin.indices.delete.DeleteIndexRequest; +import org.elasticsearch.action.admin.indices.get.GetIndexRequest; +import org.elasticsearch.action.admin.indices.get.GetIndexResponse; +import org.elasticsearch.action.support.ActiveShardCount; +import org.elasticsearch.action.support.IndicesOptions; import org.elasticsearch.client.RequestOptions; import org.elasticsearch.client.RestClient; import org.elasticsearch.client.RestClientBuilder; @@ -27,6 +34,7 @@ import org.elasticsearch.license.LicenseSettings; import org.elasticsearch.plugins.Plugin; import org.elasticsearch.xpack.core.security.authc.support.Hasher; +import org.elasticsearch.xpack.core.security.authc.support.UsernamePasswordToken; import org.elasticsearch.xpack.core.security.test.TestRestrictedIndices; import org.elasticsearch.xpack.security.LocalStateSecurity; import org.elasticsearch.xpack.security.support.SecurityMigrations; @@ -45,9 +53,12 @@ import java.util.stream.Collectors; import static org.elasticsearch.test.SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING; +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; import static org.elasticsearch.xpack.core.security.authc.support.UsernamePasswordToken.basicAuthHeaderValue; import static org.elasticsearch.xpack.security.support.SecurityIndexManager.getMigrationVersionFromIndexMetadata; +import static org.elasticsearch.xpack.security.support.SecuritySystemIndices.SECURITY_MAIN_ALIAS; import static org.hamcrest.Matchers.hasItem; +import static org.hamcrest.Matchers.is; /** * A test that starts a single node with security enabled. This test case allows for customization @@ -82,6 +93,13 @@ public static void destroyDefaultSettings() { tearDownRestClient(); } + @Override + public void setUp() throws Exception { + super.setUp(); + deleteSecurityIndexIfExists(); + createSecurityIndexWithWaitForActiveShards(); + } + @Override public void tearDown() throws Exception { awaitSecurityMigration(); @@ -100,7 +118,7 @@ private boolean isMigrationComplete(ClusterState state) { return getMigrationVersionFromIndexMetadata(indexMetadata) == SecurityMigrations.MIGRATIONS_BY_VERSION.lastKey(); } - private void awaitSecurityMigration() { + protected void awaitSecurityMigration() { final var latch = new CountDownLatch(1); ClusterService clusterService = getInstanceFromNode(ClusterService.class); clusterService.addListener((event) -> { @@ -362,4 +380,40 @@ private static RestClient createRestClient( } return builder.build(); } + + protected void deleteSecurityIndexIfExists() { + // delete the security index, if it exist + GetIndexRequest getIndexRequest = new GetIndexRequest(TEST_REQUEST_TIMEOUT); + getIndexRequest.indices(SECURITY_MAIN_ALIAS); + getIndexRequest.indicesOptions(IndicesOptions.lenientExpandOpen()); + GetIndexResponse getIndexResponse = client().admin().indices().getIndex(getIndexRequest).actionGet(); + if (getIndexResponse.getIndices().length > 0) { + assertThat(getIndexResponse.getIndices().length, is(1)); + assertThat(getIndexResponse.getIndices()[0], is(TestRestrictedIndices.INTERNAL_SECURITY_MAIN_INDEX_7)); + + // Security migration needs to finish before deleting the index + awaitSecurityMigration(); + DeleteIndexRequest deleteIndexRequest = new DeleteIndexRequest(getIndexResponse.getIndices()); + assertAcked(client().admin().indices().delete(deleteIndexRequest).actionGet()); + } + } + + protected void createSecurityIndexWithWaitForActiveShards() { + final Client client = client().filterWithHeader( + Collections.singletonMap( + "Authorization", + UsernamePasswordToken.basicAuthHeaderValue( + SecuritySettingsSource.ES_TEST_ROOT_USER, + SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING + ) + ) + ); + CreateIndexRequest createIndexRequest = new CreateIndexRequest(SECURITY_MAIN_ALIAS).waitForActiveShards(ActiveShardCount.ALL) + .masterNodeTimeout(TEST_REQUEST_TIMEOUT); + try { + client.admin().indices().create(createIndexRequest).actionGet(); + } catch (ResourceAlreadyExistsException e) { + logger.info("Security index already exists, ignoring.", e); + } + } } diff --git a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/esnative/ReservedRealmElasticAutoconfigIntegTests.java b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/esnative/ReservedRealmElasticAutoconfigIntegTests.java index 1d7e1da66a91f..8b8e6fa6b8ea8 100644 --- a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/esnative/ReservedRealmElasticAutoconfigIntegTests.java +++ b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/esnative/ReservedRealmElasticAutoconfigIntegTests.java @@ -8,17 +8,10 @@ package org.elasticsearch.xpack.security.authc.esnative; import org.elasticsearch.action.admin.cluster.settings.ClusterUpdateSettingsRequest; -import org.elasticsearch.action.admin.indices.delete.DeleteIndexRequest; -import org.elasticsearch.action.admin.indices.get.GetIndexRequest; -import org.elasticsearch.action.admin.indices.get.GetIndexResponse; -import org.elasticsearch.action.support.IndicesOptions; import org.elasticsearch.client.Request; import org.elasticsearch.client.RequestOptions; import org.elasticsearch.client.ResponseException; -import org.elasticsearch.cluster.ClusterState; -import org.elasticsearch.cluster.metadata.IndexMetadata; import org.elasticsearch.cluster.metadata.Metadata; -import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.Strings; import org.elasticsearch.common.settings.MockSecureSettings; import org.elasticsearch.common.settings.SecureString; @@ -29,14 +22,9 @@ import org.elasticsearch.xpack.core.security.action.user.PutUserRequest; import org.elasticsearch.xpack.core.security.authc.support.Hasher; import org.elasticsearch.xpack.core.security.authc.support.UsernamePasswordToken; -import org.elasticsearch.xpack.core.security.test.TestRestrictedIndices; import org.junit.BeforeClass; -import java.util.concurrent.CountDownLatch; - import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; -import static org.elasticsearch.xpack.core.security.action.UpdateIndexMigrationVersionAction.MIGRATION_VERSION_CUSTOM_KEY; -import static org.elasticsearch.xpack.security.support.SecuritySystemIndices.SECURITY_MAIN_ALIAS; import static org.hamcrest.Matchers.is; public class ReservedRealmElasticAutoconfigIntegTests extends SecuritySingleNodeTestCase { @@ -70,46 +58,10 @@ protected SecureString getBootstrapPassword() { return null; // no bootstrap password for this test } - private boolean isMigrationComplete(ClusterState state) { - IndexMetadata indexMetadata = state.metadata().getIndices().get(TestRestrictedIndices.INTERNAL_SECURITY_MAIN_INDEX_7); - return indexMetadata != null && indexMetadata.getCustomData(MIGRATION_VERSION_CUSTOM_KEY) != null; - } - - private void awaitSecurityMigrationRanOnce() { - final var latch = new CountDownLatch(1); - ClusterService clusterService = getInstanceFromNode(ClusterService.class); - clusterService.addListener((event) -> { - if (isMigrationComplete(event.state())) { - latch.countDown(); - } - }); - if (isMigrationComplete(clusterService.state())) { - latch.countDown(); - } - safeAwait(latch); - } - - private void deleteSecurityIndex() { - // delete the security index, if it exist - GetIndexRequest getIndexRequest = new GetIndexRequest(TEST_REQUEST_TIMEOUT); - getIndexRequest.indices(SECURITY_MAIN_ALIAS); - getIndexRequest.indicesOptions(IndicesOptions.lenientExpandOpen()); - GetIndexResponse getIndexResponse = client().admin().indices().getIndex(getIndexRequest).actionGet(); - if (getIndexResponse.getIndices().length > 0) { - assertThat(getIndexResponse.getIndices().length, is(1)); - assertThat(getIndexResponse.getIndices()[0], is(TestRestrictedIndices.INTERNAL_SECURITY_MAIN_INDEX_7)); - - // Security migration needs to finish before deleting the index - awaitSecurityMigrationRanOnce(); - DeleteIndexRequest deleteIndexRequest = new DeleteIndexRequest(getIndexResponse.getIndices()); - assertAcked(client().admin().indices().delete(deleteIndexRequest).actionGet()); - } - } - public void testAutoconfigFailedPasswordPromotion() throws Exception { try { // .security index is created automatically on node startup so delete the security index first - deleteSecurityIndex(); + deleteSecurityIndexIfExists(); // prevents the .security index from being created automatically (after elastic user authentication) ClusterUpdateSettingsRequest updateSettingsRequest = new ClusterUpdateSettingsRequest( TEST_REQUEST_TIMEOUT, @@ -176,7 +128,7 @@ public void testAutoconfigSucceedsAfterPromotionFailure() throws Exception { putUserRequest.roles(Strings.EMPTY_ARRAY); client().execute(PutUserAction.INSTANCE, putUserRequest).get(); // Security migration needs to finish before making the cluster read only - awaitSecurityMigrationRanOnce(); + awaitSecurityMigration(); // but then make the cluster read-only ClusterUpdateSettingsRequest updateSettingsRequest = new ClusterUpdateSettingsRequest( diff --git a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/profile/ProfileIntegTests.java b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/profile/ProfileIntegTests.java index 65f6f4f1a5b0a..4aaa2c4ee34e2 100644 --- a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/profile/ProfileIntegTests.java +++ b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/profile/ProfileIntegTests.java @@ -99,13 +99,28 @@ public class ProfileIntegTests extends AbstractProfileIntegTestCase { + protected static final String ANONYMOUS_ROLE = "anonymous_role"; + + @Override + protected String configRoles() { + return super.configRoles() + + "\n" + + ANONYMOUS_ROLE + + ":\n" + + " cluster:\n" + + " - 'manage_own_api_key'\n" + + " - 'manage_token'\n" + + " - 'manage_service_account'\n" + + " - 'monitor'\n"; + } + @Override protected Settings nodeSettings(int nodeOrdinal, Settings otherSettings) { final Settings.Builder builder = Settings.builder().put(super.nodeSettings(nodeOrdinal, otherSettings)); // This setting tests that the setting is registered builder.put("xpack.security.authc.domains.my_domain.realms", "file"); // enable anonymous - builder.putList(AnonymousUser.ROLES_SETTING.getKey(), RAC_ROLE); + builder.putList(AnonymousUser.ROLES_SETTING.getKey(), ANONYMOUS_ROLE); return builder.build(); }