Improve cache invalidation in IdP SP cache (elastic#128890)

tvernum · valeriy42 · commit 59207c278490 · 2025-06-12T13:27:35.000+02:00
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.
diff --git a/docs/changelog/128890.yaml 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
@@ -49,6 +49,7 @@
 import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.not;
 import static org.hamcrest.Matchers.notNullValue;
 
 public class IdentityProviderAuthenticationIT extends IdpRestTestCase {
@@ -89,6 +90,52 @@ public void testRegistrationAndIdpInitiatedSso() throws Exception {
         authenticateWithSamlResponse(samlResponse, null);
     }
 
+    public void testUpdateExistingServiceProvider() throws Exception {
+        final Map<String, Object> 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<String, Object> 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 testCustomAttributesInIdpInitiatedSso() throws Exception {
         final Map<String, Object> request = Map.ofEntries(
             Map.entry("name", "Test SP With Custom Attributes"),
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
@@ -107,6 +107,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
@@ -26,13 +26,15 @@
 import org.elasticsearch.cluster.ClusterState;
 import org.elasticsearch.cluster.ClusterStateListener;
 import org.elasticsearch.cluster.metadata.IndexAbstraction;
+import org.elasticsearch.cluster.metadata.ProjectMetadata;
 import org.elasticsearch.cluster.service.ClusterService;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.ValidationException;
 import org.elasticsearch.common.bytes.BytesReference;
 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 +53,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 +155,21 @@ private void checkForAliasStateChange(ClusterState state) {
         }
     }
 
+    Index getIndex(ClusterState state) {
+        final ProjectMetadata project = state.getMetadata().getProject();
+        final SortedMap<String, IndexAbstraction> indicesLookup = project.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 +273,12 @@ public void refresh(ActionListener<Void> listener) {
 
     private void findDocuments(QueryBuilder query, ActionListener<Set<DocumentSupplier>> 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
@@ -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<String, CachedServiceProvider> 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
@@ -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<SamlServiceProvider> future = new PlainActionFuture<>();
         resolver.resolve(entityId, future);
