Skip to content

Commit c234912

Browse files
tvernumtvernum
authored
Improve cache invalidation in IdP SP cache (#129025)
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
1 parent 7e2f23b commit c234912

File tree

6 files changed

+130
-2
lines changed

6 files changed

+130
-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
@@ -35,6 +35,7 @@
3535
import static org.hamcrest.Matchers.equalTo;
3636
import static org.hamcrest.Matchers.hasSize;
3737
import static org.hamcrest.Matchers.instanceOf;
38+
import static org.hamcrest.Matchers.not;
3839

3940
public class IdentityProviderAuthenticationIT extends IdpRestTestCase {
4041

@@ -74,6 +75,52 @@ public void testRegistrationAndIdpInitiatedSso() throws Exception {
7475
authenticateWithSamlResponse(samlResponse, null);
7576
}
7677

78+
public void testUpdateExistingServiceProvider() throws Exception {
79+
final Map<String, Object> request1 = Map.ofEntries(
80+
Map.entry("name", "Test SP [v1]"),
81+
Map.entry("acs", SP_ACS),
82+
Map.entry("privileges", Map.ofEntries(Map.entry("resource", SP_ENTITY_ID), Map.entry("roles", List.of("sso:(\\w+)")))),
83+
Map.entry(
84+
"attributes",
85+
Map.ofEntries(
86+
Map.entry("principal", "https://idp.test.es.elasticsearch.org/attribute/principal"),
87+
Map.entry("name", "https://idp.test.es.elasticsearch.org/attribute/name"),
88+
Map.entry("email", "https://idp.test.es.elasticsearch.org/attribute/email"),
89+
Map.entry("roles", "https://idp.test.es.elasticsearch.org/attribute/roles")
90+
)
91+
)
92+
);
93+
final SamlServiceProviderIndex.DocumentVersion docVersion1 = createServiceProvider(SP_ENTITY_ID, request1);
94+
checkIndexDoc(docVersion1);
95+
ensureGreen(SamlServiceProviderIndex.INDEX_NAME);
96+
97+
final String samlResponse1 = generateSamlResponse(SP_ENTITY_ID, SP_ACS, null);
98+
assertThat(samlResponse1, containsString("https://idp.test.es.elasticsearch.org/attribute/principal"));
99+
assertThat(samlResponse1, not(containsString("https://idp.test.es.elasticsearch.org/attribute/username")));
100+
101+
final Map<String, Object> request = Map.ofEntries(
102+
Map.entry("name", "Test SP [v2]"),
103+
Map.entry("acs", SP_ACS),
104+
Map.entry("privileges", Map.ofEntries(Map.entry("resource", SP_ENTITY_ID), Map.entry("roles", List.of("sso:(\\w+)")))),
105+
Map.entry(
106+
"attributes",
107+
Map.ofEntries(
108+
Map.entry("principal", "https://idp.test.es.elasticsearch.org/attribute/username"),
109+
Map.entry("name", "https://idp.test.es.elasticsearch.org/attribute/name"),
110+
Map.entry("email", "https://idp.test.es.elasticsearch.org/attribute/email"),
111+
Map.entry("roles", "https://idp.test.es.elasticsearch.org/attribute/roles")
112+
)
113+
)
114+
);
115+
final SamlServiceProviderIndex.DocumentVersion docVersion2 = createServiceProvider(SP_ENTITY_ID, request);
116+
checkIndexDoc(docVersion2);
117+
ensureGreen(SamlServiceProviderIndex.INDEX_NAME);
118+
119+
final String samlResponse2 = generateSamlResponse(SP_ENTITY_ID, SP_ACS, null);
120+
assertThat(samlResponse2, containsString("https://idp.test.es.elasticsearch.org/attribute/username"));
121+
assertThat(samlResponse2, not(containsString("https://idp.test.es.elasticsearch.org/attribute/principal")));
122+
}
123+
77124
public void testRegistrationAndSpInitiatedSso() throws Exception {
78125
final Map<String, Object> request = Map.ofEntries(
79126
Map.entry("name", "Test SP"),

‎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
@@ -108,6 +108,8 @@ public Collection<?> createComponents(PluginServices services) {
108108
index,
109109
serviceProviderFactory
110110
);
111+
services.clusterService().addListener(registeredServiceProviderResolver);
112+
111113
final WildcardServiceProviderResolver wildcardServiceProviderResolver = WildcardServiceProviderResolver.create(
112114
services.environment(),
113115
services.resourceWatcherService(),

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

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
import org.elasticsearch.common.util.CachedSupplier;
3434
import org.elasticsearch.common.xcontent.LoggingDeprecationHandler;
3535
import org.elasticsearch.common.xcontent.XContentHelper;
36+
import org.elasticsearch.index.Index;
3637
import org.elasticsearch.index.IndexNotFoundException;
3738
import org.elasticsearch.index.get.GetResult;
3839
import org.elasticsearch.index.query.QueryBuilder;
@@ -51,6 +52,7 @@
5152
import java.util.Arrays;
5253
import java.util.Objects;
5354
import java.util.Set;
55+
import java.util.SortedMap;
5456
import java.util.function.Supplier;
5557
import java.util.stream.Collectors;
5658
import java.util.stream.Stream;
@@ -152,6 +154,20 @@ private void checkForAliasStateChange(ClusterState state) {
152154
}
153155
}
154156

157+
Index getIndex(ClusterState state) {
158+
final SortedMap<String, IndexAbstraction> indicesLookup = state.metadata().getIndicesLookup();
159+
160+
IndexAbstraction indexAbstraction = indicesLookup.get(ALIAS_NAME);
161+
if (indexAbstraction == null) {
162+
indexAbstraction = indicesLookup.get(INDEX_NAME);
163+
}
164+
if (indexAbstraction == null) {
165+
return null;
166+
} else {
167+
return indexAbstraction.getWriteIndex();
168+
}
169+
}
170+
155171
@Override
156172
public void close() {
157173
logger.debug("Closing ... removing cluster state listener");
@@ -255,7 +271,12 @@ public void refresh(ActionListener<Void> listener) {
255271

256272
private void findDocuments(QueryBuilder query, ActionListener<Set<DocumentSupplier>> listener) {
257273
logger.trace("Searching [{}] for [{}]", ALIAS_NAME, query);
258-
final SearchRequest request = client.prepareSearch(ALIAS_NAME).setQuery(query).setSize(1000).setFetchSource(true).request();
274+
final SearchRequest request = client.prepareSearch(ALIAS_NAME)
275+
.setQuery(query)
276+
.setSize(1000)
277+
.setFetchSource(true)
278+
.seqNoAndPrimaryTerm(true)
279+
.request();
259280
client.search(request, ActionListener.wrap(response -> {
260281
if (logger.isTraceEnabled()) {
261282
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)