Skip to content

Commit c34f8b6

Browse files
tvernumtvernum
authored
Improve cache invalidation in IdP SP cache (#128890)
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.
1 parent 923f029 commit c34f8b6

File tree

6 files changed

+132
-2
lines changed

6 files changed

+132
-2
lines changed

‎docs/changelog/128890.yaml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 128890
2+
summary: Improve cache invalidation in IdP SP cache
3+
area: IdentityProvider
4+
type: bug
5+
issues: []

‎x-pack/plugin/identity-provider/qa/idp-rest-tests/src/javaRestTest/java/org/elasticsearch/xpack/idp/IdentityProviderAuthenticationIT.java‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@
4949
import static org.hamcrest.Matchers.hasSize;
5050
import static org.hamcrest.Matchers.instanceOf;
5151
import static org.hamcrest.Matchers.is;
52+
import static org.hamcrest.Matchers.not;
5253
import static org.hamcrest.Matchers.notNullValue;
5354

5455
public class IdentityProviderAuthenticationIT extends IdpRestTestCase {
@@ -89,6 +90,52 @@ public void testRegistrationAndIdpInitiatedSso() throws Exception {
8990
authenticateWithSamlResponse(samlResponse, null);
9091
}
9192

93+
public void testUpdateExistingServiceProvider() throws Exception {
94+
final Map<String, Object> request1 = Map.ofEntries(
95+
Map.entry("name", "Test SP [v1]"),
96+
Map.entry("acs", SP_ACS),
97+
Map.entry("privileges", Map.ofEntries(Map.entry("resource", SP_ENTITY_ID), Map.entry("roles", List.of("sso:(\\w+)")))),
98+
Map.entry(
99+
"attributes",
100+
Map.ofEntries(
101+
Map.entry("principal", "https://idp.test.es.elasticsearch.org/attribute/principal"),
102+
Map.entry("name", "https://idp.test.es.elasticsearch.org/attribute/name"),
103+
Map.entry("email", "https://idp.test.es.elasticsearch.org/attribute/email"),
104+
Map.entry("roles", "https://idp.test.es.elasticsearch.org/attribute/roles")
105+
)
106+
)
107+
);
108+
final SamlServiceProviderIndex.DocumentVersion docVersion1 = createServiceProvider(SP_ENTITY_ID, request1);
109+
checkIndexDoc(docVersion1);
110+
ensureGreen(SamlServiceProviderIndex.INDEX_NAME);
111+
112+
final String samlResponse1 = generateSamlResponse(SP_ENTITY_ID, SP_ACS, null);
113+
assertThat(samlResponse1, containsString("https://idp.test.es.elasticsearch.org/attribute/principal"));
114+
assertThat(samlResponse1, not(containsString("https://idp.test.es.elasticsearch.org/attribute/username")));
115+
116+
final Map<String, Object> request = Map.ofEntries(
117+
Map.entry("name", "Test SP [v2]"),
118+
Map.entry("acs", SP_ACS),
119+
Map.entry("privileges", Map.ofEntries(Map.entry("resource", SP_ENTITY_ID), Map.entry("roles", List.of("sso:(\\w+)")))),
120+
Map.entry(
121+
"attributes",
122+
Map.ofEntries(
123+
Map.entry("principal", "https://idp.test.es.elasticsearch.org/attribute/username"),
124+
Map.entry("name", "https://idp.test.es.elasticsearch.org/attribute/name"),
125+
Map.entry("email", "https://idp.test.es.elasticsearch.org/attribute/email"),
126+
Map.entry("roles", "https://idp.test.es.elasticsearch.org/attribute/roles")
127+
)
128+
)
129+
);
130+
final SamlServiceProviderIndex.DocumentVersion docVersion2 = createServiceProvider(SP_ENTITY_ID, request);
131+
checkIndexDoc(docVersion2);
132+
ensureGreen(SamlServiceProviderIndex.INDEX_NAME);
133+
134+
final String samlResponse2 = generateSamlResponse(SP_ENTITY_ID, SP_ACS, null);
135+
assertThat(samlResponse2, containsString("https://idp.test.es.elasticsearch.org/attribute/username"));
136+
assertThat(samlResponse2, not(containsString("https://idp.test.es.elasticsearch.org/attribute/principal")));
137+
}
138+
92139
public void testCustomAttributesInIdpInitiatedSso() throws Exception {
93140
final Map<String, Object> request = Map.ofEntries(
94141
Map.entry("name", "Test SP With Custom Attributes"),

‎x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/IdentityProviderPlugin.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,8 @@ public Collection<?> createComponents(PluginServices services) {
107107
index,
108108
serviceProviderFactory
109109
);
110+
services.clusterService().addListener(registeredServiceProviderResolver);
111+
110112
final WildcardServiceProviderResolver wildcardServiceProviderResolver = WildcardServiceProviderResolver.create(
111113
services.environment(),
112114
services.resourceWatcherService(),

‎x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/sp/SamlServiceProviderIndex.java‎

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,13 +26,15 @@
2626
import org.elasticsearch.cluster.ClusterState;
2727
import org.elasticsearch.cluster.ClusterStateListener;
2828
import org.elasticsearch.cluster.metadata.IndexAbstraction;
29+
import org.elasticsearch.cluster.metadata.ProjectMetadata;
2930
import org.elasticsearch.cluster.service.ClusterService;
3031
import org.elasticsearch.common.Strings;
3132
import org.elasticsearch.common.ValidationException;
3233
import org.elasticsearch.common.bytes.BytesReference;
3334
import org.elasticsearch.common.util.CachedSupplier;
3435
import org.elasticsearch.common.xcontent.LoggingDeprecationHandler;
3536
import org.elasticsearch.common.xcontent.XContentHelper;
37+
import org.elasticsearch.index.Index;
3638
import org.elasticsearch.index.IndexNotFoundException;
3739
import org.elasticsearch.index.get.GetResult;
3840
import org.elasticsearch.index.query.QueryBuilder;
@@ -51,6 +53,7 @@
5153
import java.util.Arrays;
5254
import java.util.Objects;
5355
import java.util.Set;
56+
import java.util.SortedMap;
5457
import java.util.function.Supplier;
5558
import java.util.stream.Collectors;
5659
import java.util.stream.Stream;
@@ -152,6 +155,21 @@ private void checkForAliasStateChange(ClusterState state) {
152155
}
153156
}
154157

158+
Index getIndex(ClusterState state) {
159+
final ProjectMetadata project = state.getMetadata().getProject();
160+
final SortedMap<String, IndexAbstraction> indicesLookup = project.getIndicesLookup();
161+
162+
IndexAbstraction indexAbstraction = indicesLookup.get(ALIAS_NAME);
163+
if (indexAbstraction == null) {
164+
indexAbstraction = indicesLookup.get(INDEX_NAME);
165+
}
166+
if (indexAbstraction == null) {
167+
return null;
168+
} else {
169+
return indexAbstraction.getWriteIndex();
170+
}
171+
}
172+
155173
@Override
156174
public void close() {
157175
logger.debug("Closing ... removing cluster state listener");
@@ -255,7 +273,12 @@ public void refresh(ActionListener<Void> listener) {
255273

256274
private void findDocuments(QueryBuilder query, ActionListener<Set<DocumentSupplier>> listener) {
257275
logger.trace("Searching [{}] for [{}]", ALIAS_NAME, query);
258-
final SearchRequest request = client.prepareSearch(ALIAS_NAME).setQuery(query).setSize(1000).setFetchSource(true).request();
276+
final SearchRequest request = client.prepareSearch(ALIAS_NAME)
277+
.setQuery(query)
278+
.setSize(1000)
279+
.setFetchSource(true)
280+
.seqNoAndPrimaryTerm(true)
281+
.request();
259282
client.search(request, ActionListener.wrap(response -> {
260283
if (logger.isTraceEnabled()) {
261284
logger.trace("Search hits: [{}] [{}]", response.getHits().getTotalHits(), Arrays.toString(response.getHits().getHits()));

‎x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/sp/SamlServiceProviderResolver.java‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,16 +7,22 @@
77

88
package org.elasticsearch.xpack.idp.saml.sp;
99

10+
import org.apache.logging.log4j.LogManager;
11+
import org.apache.logging.log4j.Logger;
1012
import org.elasticsearch.action.ActionListener;
13+
import org.elasticsearch.cluster.ClusterChangedEvent;
14+
import org.elasticsearch.cluster.ClusterStateListener;
1115
import org.elasticsearch.common.cache.Cache;
1216
import org.elasticsearch.common.settings.Settings;
1317
import org.elasticsearch.common.util.iterable.Iterables;
18+
import org.elasticsearch.index.Index;
1419
import org.elasticsearch.xpack.idp.saml.sp.SamlServiceProviderIndex.DocumentSupplier;
1520
import org.elasticsearch.xpack.idp.saml.sp.SamlServiceProviderIndex.DocumentVersion;
1621

22+
import java.util.Objects;
1723
import java.util.stream.Collectors;
1824

19-
public class SamlServiceProviderResolver {
25+
public class SamlServiceProviderResolver implements ClusterStateListener {
2026

2127
private final Cache<String, CachedServiceProvider> cache;
2228
private final SamlServiceProviderIndex index;
@@ -32,6 +38,8 @@ public SamlServiceProviderResolver(
3238
this.serviceProviderFactory = serviceProviderFactory;
3339
}
3440

41+
private final Logger logger = LogManager.getLogger(getClass());
42+
3543
/**
3644
* Find a {@link SamlServiceProvider} by entity-id.
3745
*
@@ -75,6 +83,16 @@ private void populateCacheAndReturn(String entityId, DocumentSupplier doc, Actio
7583
listener.onResponse(serviceProvider);
7684
}
7785

86+
@Override
87+
public void clusterChanged(ClusterChangedEvent event) {
88+
final Index previousIndex = index.getIndex(event.previousState());
89+
final Index currentIndex = index.getIndex(event.state());
90+
if (Objects.equals(previousIndex, currentIndex) == false) {
91+
logger.info("Index has changed [{}] => [{}], clearing cache", previousIndex, currentIndex);
92+
this.cache.invalidateAll();
93+
}
94+
}
95+
7896
private class CachedServiceProvider {
7997
private final String entityId;
8098
private final DocumentVersion documentVersion;

‎x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/sp/SamlServiceProviderResolverTests.java‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,11 @@
99

1010
import org.elasticsearch.action.ActionListener;
1111
import org.elasticsearch.action.support.PlainActionFuture;
12+
import org.elasticsearch.cluster.ClusterChangedEvent;
13+
import org.elasticsearch.cluster.ClusterName;
14+
import org.elasticsearch.cluster.ClusterState;
1215
import org.elasticsearch.common.settings.Settings;
16+
import org.elasticsearch.index.Index;
1317
import org.elasticsearch.test.ESTestCase;
1418
import org.elasticsearch.xpack.idp.saml.idp.SamlIdentityProvider;
1519
import org.elasticsearch.xpack.idp.saml.sp.SamlServiceProviderIndex.DocumentVersion;
@@ -135,6 +139,37 @@ public void testResolveIgnoresCacheWhenDocumentVersionChanges() throws Exception
135139
assertThat(serviceProvider2.getPrivileges().getResource(), equalTo(document2.privileges.resource));
136140
}
137141

142+
public void testCacheIsClearedWhenIndexChanges() throws Exception {
143+
final SamlServiceProviderDocument document1 = SamlServiceProviderTestUtils.randomDocument(1);
144+
final SamlServiceProviderDocument document2 = SamlServiceProviderTestUtils.randomDocument(2);
145+
document2.entityId = document1.entityId;
146+
147+
final DocumentVersion docVersion = new DocumentVersion(randomAlphaOfLength(12), 1, 1);
148+
149+
mockDocument(document1.entityId, docVersion, document1);
150+
final SamlServiceProvider serviceProvider1a = resolveServiceProvider(document1.entityId);
151+
final SamlServiceProvider serviceProvider1b = resolveServiceProvider(document1.entityId);
152+
assertThat(serviceProvider1b, sameInstance(serviceProvider1a));
153+
154+
final ClusterState oldState = ClusterState.builder(ClusterName.DEFAULT).build();
155+
final ClusterState newState = ClusterState.builder(ClusterName.DEFAULT).build();
156+
when(index.getIndex(oldState)).thenReturn(new Index(SamlServiceProviderIndex.INDEX_NAME, randomUUID()));
157+
when(index.getIndex(newState)).thenReturn(new Index(SamlServiceProviderIndex.INDEX_NAME, randomUUID()));
158+
resolver.clusterChanged(new ClusterChangedEvent(getTestName(), newState, oldState));
159+
160+
mockDocument(document1.entityId, docVersion, document2);
161+
final SamlServiceProvider serviceProvider2 = resolveServiceProvider(document1.entityId);
162+
163+
assertThat(serviceProvider2, not(sameInstance(serviceProvider1a)));
164+
assertThat(serviceProvider2.getEntityId(), equalTo(document2.entityId));
165+
assertThat(serviceProvider2.getAssertionConsumerService().toString(), equalTo(document2.acs));
166+
assertThat(serviceProvider2.getAttributeNames().principal, equalTo(document2.attributeNames.principal));
167+
assertThat(serviceProvider2.getAttributeNames().name, equalTo(document2.attributeNames.name));
168+
assertThat(serviceProvider2.getAttributeNames().email, equalTo(document2.attributeNames.email));
169+
assertThat(serviceProvider2.getAttributeNames().roles, equalTo(document2.attributeNames.roles));
170+
assertThat(serviceProvider2.getPrivileges().getResource(), equalTo(document2.privileges.resource));
171+
}
172+
138173
private SamlServiceProvider resolveServiceProvider(String entityId) {
139174
final PlainActionFuture<SamlServiceProvider> future = new PlainActionFuture<>();
140175
resolver.resolve(entityId, future);

0 commit comments

Comments
 (0)