From d9158d5e7fe0e4b5f3ca2248ca77e1800eeed219 Mon Sep 17 00:00:00 2001 From: Tim Vernum Date: Fri, 6 Jun 2025 14:46:01 +1000 Subject: [PATCH] Improve cache invalidation in IdP SP cache The Identity Provider's Service Provider cache had two issues: 1. It checked for identity based on sequence numbers, but didn't include the `seq_no_primary_term` parameter on searches, which means the sequence would always by `-2` 2. It didn't track whether the index was deleted, which means it could be caching values from an old version of the index This commit fixes both of these issues. In practice neither issue was a problem because there are no deployments that use index-based service providers, however the 2nd issue did cause some challenges for testing. Backport of: #128890 --- docs/changelog/128890.yaml | 5 ++ .../idp/IdentityProviderAuthenticationIT.java | 47 +++++++++++++++++++ .../xpack/idp/IdentityProviderPlugin.java | 2 + .../idp/saml/sp/SamlServiceProviderIndex.java | 23 ++++++++- .../saml/sp/SamlServiceProviderResolver.java | 20 +++++++- .../sp/SamlServiceProviderResolverTests.java | 35 ++++++++++++++ 6 files changed, 130 insertions(+), 2 deletions(-) create mode 100644 docs/changelog/128890.yaml diff --git a/docs/changelog/128890.yaml b/docs/changelog/128890.yaml new file mode 100644 index 0000000000000..79fa83c3e55e4 --- /dev/null +++ b/docs/changelog/128890.yaml @@ -0,0 +1,5 @@ +pr: 128890 +summary: Improve cache invalidation in IdP SP cache +area: IdentityProvider +type: bug +issues: [] diff --git a/x-pack/plugin/identity-provider/qa/idp-rest-tests/src/javaRestTest/java/org/elasticsearch/xpack/idp/IdentityProviderAuthenticationIT.java b/x-pack/plugin/identity-provider/qa/idp-rest-tests/src/javaRestTest/java/org/elasticsearch/xpack/idp/IdentityProviderAuthenticationIT.java index c065e8d7e1d12..46f9bde5df32f 100644 --- a/x-pack/plugin/identity-provider/qa/idp-rest-tests/src/javaRestTest/java/org/elasticsearch/xpack/idp/IdentityProviderAuthenticationIT.java +++ b/x-pack/plugin/identity-provider/qa/idp-rest-tests/src/javaRestTest/java/org/elasticsearch/xpack/idp/IdentityProviderAuthenticationIT.java @@ -35,6 +35,7 @@ import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.instanceOf; +import static org.hamcrest.Matchers.not; public class IdentityProviderAuthenticationIT extends IdpRestTestCase { @@ -74,6 +75,52 @@ public void testRegistrationAndIdpInitiatedSso() throws Exception { authenticateWithSamlResponse(samlResponse, null); } + public void testUpdateExistingServiceProvider() throws Exception { + final Map request1 = Map.ofEntries( + Map.entry("name", "Test SP [v1]"), + Map.entry("acs", SP_ACS), + Map.entry("privileges", Map.ofEntries(Map.entry("resource", SP_ENTITY_ID), Map.entry("roles", List.of("sso:(\\w+)")))), + Map.entry( + "attributes", + Map.ofEntries( + Map.entry("principal", "https://idp.test.es.elasticsearch.org/attribute/principal"), + Map.entry("name", "https://idp.test.es.elasticsearch.org/attribute/name"), + Map.entry("email", "https://idp.test.es.elasticsearch.org/attribute/email"), + Map.entry("roles", "https://idp.test.es.elasticsearch.org/attribute/roles") + ) + ) + ); + final SamlServiceProviderIndex.DocumentVersion docVersion1 = createServiceProvider(SP_ENTITY_ID, request1); + checkIndexDoc(docVersion1); + ensureGreen(SamlServiceProviderIndex.INDEX_NAME); + + final String samlResponse1 = generateSamlResponse(SP_ENTITY_ID, SP_ACS, null); + assertThat(samlResponse1, containsString("https://idp.test.es.elasticsearch.org/attribute/principal")); + assertThat(samlResponse1, not(containsString("https://idp.test.es.elasticsearch.org/attribute/username"))); + + final Map request = Map.ofEntries( + Map.entry("name", "Test SP [v2]"), + Map.entry("acs", SP_ACS), + Map.entry("privileges", Map.ofEntries(Map.entry("resource", SP_ENTITY_ID), Map.entry("roles", List.of("sso:(\\w+)")))), + Map.entry( + "attributes", + Map.ofEntries( + Map.entry("principal", "https://idp.test.es.elasticsearch.org/attribute/username"), + Map.entry("name", "https://idp.test.es.elasticsearch.org/attribute/name"), + Map.entry("email", "https://idp.test.es.elasticsearch.org/attribute/email"), + Map.entry("roles", "https://idp.test.es.elasticsearch.org/attribute/roles") + ) + ) + ); + final SamlServiceProviderIndex.DocumentVersion docVersion2 = createServiceProvider(SP_ENTITY_ID, request); + checkIndexDoc(docVersion2); + ensureGreen(SamlServiceProviderIndex.INDEX_NAME); + + final String samlResponse2 = generateSamlResponse(SP_ENTITY_ID, SP_ACS, null); + assertThat(samlResponse2, containsString("https://idp.test.es.elasticsearch.org/attribute/username")); + assertThat(samlResponse2, not(containsString("https://idp.test.es.elasticsearch.org/attribute/principal"))); + } + public void testRegistrationAndSpInitiatedSso() throws Exception { final Map request = Map.ofEntries( Map.entry("name", "Test SP"), diff --git a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/IdentityProviderPlugin.java b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/IdentityProviderPlugin.java index 5e6bc5f703879..4a857766e1f7c 100644 --- a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/IdentityProviderPlugin.java +++ b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/IdentityProviderPlugin.java @@ -108,6 +108,8 @@ public Collection createComponents(PluginServices services) { index, serviceProviderFactory ); + services.clusterService().addListener(registeredServiceProviderResolver); + final WildcardServiceProviderResolver wildcardServiceProviderResolver = WildcardServiceProviderResolver.create( services.environment(), services.resourceWatcherService(), diff --git a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/sp/SamlServiceProviderIndex.java b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/sp/SamlServiceProviderIndex.java index 1eb6c5586a48b..ac9264ca69313 100644 --- a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/sp/SamlServiceProviderIndex.java +++ b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/sp/SamlServiceProviderIndex.java @@ -33,6 +33,7 @@ import org.elasticsearch.common.util.CachedSupplier; import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; import org.elasticsearch.common.xcontent.XContentHelper; +import org.elasticsearch.index.Index; import org.elasticsearch.index.IndexNotFoundException; import org.elasticsearch.index.get.GetResult; import org.elasticsearch.index.query.QueryBuilder; @@ -51,6 +52,7 @@ import java.util.Arrays; import java.util.Objects; import java.util.Set; +import java.util.SortedMap; import java.util.function.Supplier; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -152,6 +154,20 @@ private void checkForAliasStateChange(ClusterState state) { } } + Index getIndex(ClusterState state) { + final SortedMap indicesLookup = state.metadata().getIndicesLookup(); + + IndexAbstraction indexAbstraction = indicesLookup.get(ALIAS_NAME); + if (indexAbstraction == null) { + indexAbstraction = indicesLookup.get(INDEX_NAME); + } + if (indexAbstraction == null) { + return null; + } else { + return indexAbstraction.getWriteIndex(); + } + } + @Override public void close() { logger.debug("Closing ... removing cluster state listener"); @@ -255,7 +271,12 @@ public void refresh(ActionListener listener) { private void findDocuments(QueryBuilder query, ActionListener> listener) { logger.trace("Searching [{}] for [{}]", ALIAS_NAME, query); - final SearchRequest request = client.prepareSearch(ALIAS_NAME).setQuery(query).setSize(1000).setFetchSource(true).request(); + final SearchRequest request = client.prepareSearch(ALIAS_NAME) + .setQuery(query) + .setSize(1000) + .setFetchSource(true) + .seqNoAndPrimaryTerm(true) + .request(); client.search(request, ActionListener.wrap(response -> { if (logger.isTraceEnabled()) { logger.trace("Search hits: [{}] [{}]", response.getHits().getTotalHits(), Arrays.toString(response.getHits().getHits())); diff --git a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/sp/SamlServiceProviderResolver.java b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/sp/SamlServiceProviderResolver.java index 4e3e76e539a95..621068c0db840 100644 --- a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/sp/SamlServiceProviderResolver.java +++ b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/sp/SamlServiceProviderResolver.java @@ -7,16 +7,22 @@ package org.elasticsearch.xpack.idp.saml.sp; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.elasticsearch.action.ActionListener; +import org.elasticsearch.cluster.ClusterChangedEvent; +import org.elasticsearch.cluster.ClusterStateListener; import org.elasticsearch.common.cache.Cache; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.util.iterable.Iterables; +import org.elasticsearch.index.Index; import org.elasticsearch.xpack.idp.saml.sp.SamlServiceProviderIndex.DocumentSupplier; import org.elasticsearch.xpack.idp.saml.sp.SamlServiceProviderIndex.DocumentVersion; +import java.util.Objects; import java.util.stream.Collectors; -public class SamlServiceProviderResolver { +public class SamlServiceProviderResolver implements ClusterStateListener { private final Cache cache; private final SamlServiceProviderIndex index; @@ -32,6 +38,8 @@ public SamlServiceProviderResolver( this.serviceProviderFactory = serviceProviderFactory; } + private final Logger logger = LogManager.getLogger(getClass()); + /** * Find a {@link SamlServiceProvider} by entity-id. * @@ -75,6 +83,16 @@ private void populateCacheAndReturn(String entityId, DocumentSupplier doc, Actio listener.onResponse(serviceProvider); } + @Override + public void clusterChanged(ClusterChangedEvent event) { + final Index previousIndex = index.getIndex(event.previousState()); + final Index currentIndex = index.getIndex(event.state()); + if (Objects.equals(previousIndex, currentIndex) == false) { + logger.info("Index has changed [{}] => [{}], clearing cache", previousIndex, currentIndex); + this.cache.invalidateAll(); + } + } + private class CachedServiceProvider { private final String entityId; private final DocumentVersion documentVersion; diff --git a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/sp/SamlServiceProviderResolverTests.java b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/sp/SamlServiceProviderResolverTests.java index 6b76deb08aad6..cc387703cd453 100644 --- a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/sp/SamlServiceProviderResolverTests.java +++ b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/sp/SamlServiceProviderResolverTests.java @@ -9,7 +9,11 @@ import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.support.PlainActionFuture; +import org.elasticsearch.cluster.ClusterChangedEvent; +import org.elasticsearch.cluster.ClusterName; +import org.elasticsearch.cluster.ClusterState; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.index.Index; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xpack.idp.saml.idp.SamlIdentityProvider; import org.elasticsearch.xpack.idp.saml.sp.SamlServiceProviderIndex.DocumentVersion; @@ -135,6 +139,37 @@ public void testResolveIgnoresCacheWhenDocumentVersionChanges() throws Exception assertThat(serviceProvider2.getPrivileges().getResource(), equalTo(document2.privileges.resource)); } + public void testCacheIsClearedWhenIndexChanges() throws Exception { + final SamlServiceProviderDocument document1 = SamlServiceProviderTestUtils.randomDocument(1); + final SamlServiceProviderDocument document2 = SamlServiceProviderTestUtils.randomDocument(2); + document2.entityId = document1.entityId; + + final DocumentVersion docVersion = new DocumentVersion(randomAlphaOfLength(12), 1, 1); + + mockDocument(document1.entityId, docVersion, document1); + final SamlServiceProvider serviceProvider1a = resolveServiceProvider(document1.entityId); + final SamlServiceProvider serviceProvider1b = resolveServiceProvider(document1.entityId); + assertThat(serviceProvider1b, sameInstance(serviceProvider1a)); + + final ClusterState oldState = ClusterState.builder(ClusterName.DEFAULT).build(); + final ClusterState newState = ClusterState.builder(ClusterName.DEFAULT).build(); + when(index.getIndex(oldState)).thenReturn(new Index(SamlServiceProviderIndex.INDEX_NAME, randomUUID())); + when(index.getIndex(newState)).thenReturn(new Index(SamlServiceProviderIndex.INDEX_NAME, randomUUID())); + resolver.clusterChanged(new ClusterChangedEvent(getTestName(), newState, oldState)); + + mockDocument(document1.entityId, docVersion, document2); + final SamlServiceProvider serviceProvider2 = resolveServiceProvider(document1.entityId); + + assertThat(serviceProvider2, not(sameInstance(serviceProvider1a))); + assertThat(serviceProvider2.getEntityId(), equalTo(document2.entityId)); + assertThat(serviceProvider2.getAssertionConsumerService().toString(), equalTo(document2.acs)); + assertThat(serviceProvider2.getAttributeNames().principal, equalTo(document2.attributeNames.principal)); + assertThat(serviceProvider2.getAttributeNames().name, equalTo(document2.attributeNames.name)); + assertThat(serviceProvider2.getAttributeNames().email, equalTo(document2.attributeNames.email)); + assertThat(serviceProvider2.getAttributeNames().roles, equalTo(document2.attributeNames.roles)); + assertThat(serviceProvider2.getPrivileges().getResource(), equalTo(document2.privileges.resource)); + } + private SamlServiceProvider resolveServiceProvider(String entityId) { final PlainActionFuture future = new PlainActionFuture<>(); resolver.resolve(entityId, future);