Skip to content

Commit 9e3e7e1

Browse files
committed
Security: don't call prepare index for reads (#34246)
The security native stores follow a pattern where `SecurityIndexManager#prepareIndexIfNeededThenExecute` wraps most calls made for the security index. The reasoning behind this was to check if the security index had been upgraded to the latest version in a consistent manner. However, this has the potential side effect that a read will trigger the creation of the security index or an updating of its mappings, which can lead to issues such as failures due to put mapping requests timing out even though we might have been able to read from the index and get the data necessary. This change introduces a new method, `checkIndexVersionThenExecute`, that provides the consistent checking of the security index to make sure it has been upgraded. That is the only check that this method performs prior to running the passed in operation, which removes the possible triggering of index creation and mapping updates for reads. Additionally, areas where we do reads now check the availability of the security index and can short circuit requests. Availability in this context means that the index exists and all primaries are active. Relates #33205
1 parent 10b3ed5 commit 9e3e7e1

File tree

12 files changed

+335
-196
lines changed

12 files changed

+335
-196
lines changed

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java

Lines changed: 64 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -375,30 +375,34 @@ void decodeToken(String token, ActionListener<UserToken> listener) throws IOExce
375375
final Cipher cipher = getDecryptionCipher(iv, decodeKey, version, decodedSalt);
376376
if (version.onOrAfter(Version.V_6_2_0)) {
377377
// we only have the id and need to get the token from the doc!
378-
decryptTokenId(in, cipher, version, ActionListener.wrap(tokenId ->
379-
securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () -> {
380-
final GetRequest getRequest =
378+
decryptTokenId(in, cipher, version, ActionListener.wrap(tokenId -> {
379+
if (securityIndex.isAvailable() == false) {
380+
logger.warn("failed to get token [{}] since index is not available", tokenId);
381+
listener.onResponse(null);
382+
} else {
383+
securityIndex.checkIndexVersionThenExecute(listener::onFailure, () -> {
384+
final GetRequest getRequest =
381385
client.prepareGet(SecurityIndexManager.SECURITY_INDEX_NAME, TYPE,
382-
getTokenDocumentId(tokenId)).request();
383-
executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN, getRequest,
386+
getTokenDocumentId(tokenId)).request();
387+
executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN, getRequest,
384388
ActionListener.<GetResponse>wrap(response -> {
385389
if (response.isExists()) {
386390
Map<String, Object> accessTokenSource =
387-
(Map<String, Object>) response.getSource().get("access_token");
391+
(Map<String, Object>) response.getSource().get("access_token");
388392
if (accessTokenSource == null) {
389393
listener.onFailure(new IllegalStateException("token document is missing " +
390-
"the access_token field"));
394+
"the access_token field"));
391395
} else if (accessTokenSource.containsKey("user_token") == false) {
392396
listener.onFailure(new IllegalStateException("token document is missing " +
393-
"the user_token field"));
397+
"the user_token field"));
394398
} else {
395399
Map<String, Object> userTokenSource =
396-
(Map<String, Object>) accessTokenSource.get("user_token");
400+
(Map<String, Object>) accessTokenSource.get("user_token");
397401
listener.onResponse(UserToken.fromSourceMap(userTokenSource));
398402
}
399403
} else {
400404
listener.onFailure(
401-
new IllegalStateException("token document is missing and must be present"));
405+
new IllegalStateException("token document is missing and must be present"));
402406
}
403407
}, e -> {
404408
// if the index or the shard is not there / available we assume that
@@ -411,7 +415,8 @@ void decodeToken(String token, ActionListener<UserToken> listener) throws IOExce
411415
listener.onFailure(e);
412416
}
413417
}), client::get);
414-
}), listener::onFailure));
418+
});
419+
}}, listener::onFailure));
415420
} else {
416421
decryptToken(in, cipher, version, listener);
417422
}
@@ -687,30 +692,36 @@ private void findTokenFromRefreshToken(String refreshToken, ActionListener<Tuple
687692
.setVersion(true)
688693
.request();
689694

690-
securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () ->
695+
if (securityIndex.isAvailable() == false) {
696+
logger.debug("security index is not available to find token from refresh token, retrying");
697+
attemptCount.incrementAndGet();
698+
findTokenFromRefreshToken(refreshToken, listener, attemptCount);
699+
} else {
700+
securityIndex.checkIndexVersionThenExecute(listener::onFailure, () ->
691701
executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN, request,
692-
ActionListener.<SearchResponse>wrap(searchResponse -> {
693-
if (searchResponse.isTimedOut()) {
694-
attemptCount.incrementAndGet();
695-
findTokenFromRefreshToken(refreshToken, listener, attemptCount);
696-
} else if (searchResponse.getHits().getHits().length < 1) {
697-
logger.info("could not find token document with refresh_token [{}]", refreshToken);
698-
listener.onFailure(invalidGrantException("could not refresh the requested token"));
699-
} else if (searchResponse.getHits().getHits().length > 1) {
700-
listener.onFailure(new IllegalStateException("multiple tokens share the same refresh token"));
701-
} else {
702-
listener.onResponse(new Tuple<>(searchResponse, attemptCount));
703-
}
704-
}, e -> {
705-
if (isShardNotAvailableException(e)) {
706-
logger.debug("failed to search for token document, retrying", e);
707-
attemptCount.incrementAndGet();
708-
findTokenFromRefreshToken(refreshToken, listener, attemptCount);
709-
} else {
710-
listener.onFailure(e);
711-
}
712-
}),
713-
client::search));
702+
ActionListener.<SearchResponse>wrap(searchResponse -> {
703+
if (searchResponse.isTimedOut()) {
704+
attemptCount.incrementAndGet();
705+
findTokenFromRefreshToken(refreshToken, listener, attemptCount);
706+
} else if (searchResponse.getHits().getHits().length < 1) {
707+
logger.info("could not find token document with refresh_token [{}]", refreshToken);
708+
listener.onFailure(invalidGrantException("could not refresh the requested token"));
709+
} else if (searchResponse.getHits().getHits().length > 1) {
710+
listener.onFailure(new IllegalStateException("multiple tokens share the same refresh token"));
711+
} else {
712+
listener.onResponse(new Tuple<>(searchResponse, attemptCount));
713+
}
714+
}, e -> {
715+
if (isShardNotAvailableException(e)) {
716+
logger.debug("failed to search for token document, retrying", e);
717+
attemptCount.incrementAndGet();
718+
findTokenFromRefreshToken(refreshToken, listener, attemptCount);
719+
} else {
720+
listener.onFailure(e);
721+
}
722+
}),
723+
client::search));
724+
}
714725
}
715726
}
716727

@@ -845,32 +856,33 @@ public void findActiveTokensForRealm(String realmName, ActionListener<Collection
845856

846857
if (Strings.isNullOrEmpty(realmName)) {
847858
listener.onFailure(new IllegalArgumentException("Realm name is required"));
848-
return;
849-
}
850-
851-
final Instant now = clock.instant();
852-
final BoolQueryBuilder boolQuery = QueryBuilders.boolQuery()
859+
} else if (securityIndex.isAvailable() == false) {
860+
listener.onResponse(Collections.emptyList());
861+
} else {
862+
final Instant now = clock.instant();
863+
final BoolQueryBuilder boolQuery = QueryBuilders.boolQuery()
853864
.filter(QueryBuilders.termQuery("doc_type", "token"))
854865
.filter(QueryBuilders.termQuery("access_token.realm", realmName))
855866
.filter(QueryBuilders.boolQuery()
856-
.should(QueryBuilders.boolQuery()
857-
.must(QueryBuilders.termQuery("access_token.invalidated", false))
858-
.must(QueryBuilders.rangeQuery("access_token.user_token.expiration_time").gte(now.toEpochMilli()))
859-
)
860-
.should(QueryBuilders.termQuery("refresh_token.invalidated", false))
867+
.should(QueryBuilders.boolQuery()
868+
.must(QueryBuilders.termQuery("access_token.invalidated", false))
869+
.must(QueryBuilders.rangeQuery("access_token.user_token.expiration_time").gte(now.toEpochMilli()))
870+
)
871+
.should(QueryBuilders.termQuery("refresh_token.invalidated", false))
861872
);
862873

863-
final SearchRequest request = client.prepareSearch(SecurityIndexManager.SECURITY_INDEX_NAME)
874+
final SearchRequest request = client.prepareSearch(SecurityIndexManager.SECURITY_INDEX_NAME)
864875
.setScroll(DEFAULT_KEEPALIVE_SETTING.get(settings))
865876
.setQuery(boolQuery)
866877
.setVersion(false)
867878
.setSize(1000)
868879
.setFetchSource(true)
869880
.request();
870881

871-
final Supplier<ThreadContext.StoredContext> supplier = client.threadPool().getThreadContext().newRestorableContext(false);
872-
securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () ->
873-
ScrollHelper.fetchAllByEntity(client, request, new ContextPreservingActionListener<>(supplier, listener), this::parseHit));
882+
final Supplier<ThreadContext.StoredContext> supplier = client.threadPool().getThreadContext().newRestorableContext(false);
883+
securityIndex.checkIndexVersionThenExecute(listener::onFailure, () ->
884+
ScrollHelper.fetchAllByEntity(client, request, new ContextPreservingActionListener<>(supplier, listener), this::parseHit));
885+
}
874886
}
875887

876888
private Tuple<UserToken, String> parseHit(SearchHit hit) {
@@ -937,10 +949,12 @@ private void ensureEnabled() {
937949
*/
938950
private void checkIfTokenIsRevoked(UserToken userToken, ActionListener<UserToken> listener) {
939951
if (securityIndex.indexExists() == false) {
940-
// index doesn't exist so the token is considered valid.
952+
// index doesn't exist so the token is considered valid. it is important to note that
953+
// we do not use isAvailable as the lack of a shard being available is not equivalent
954+
// to the index not existing in the case of revocation checking.
941955
listener.onResponse(userToken);
942956
} else {
943-
securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () -> {
957+
securityIndex.checkIndexVersionThenExecute(listener::onFailure, () -> {
944958
MultiGetRequest mGetRequest = client.prepareMultiGet()
945959
.add(SecurityIndexManager.SECURITY_INDEX_NAME, TYPE, getInvalidatedTokenDocumentId(userToken))
946960
.add(SecurityIndexManager.SECURITY_INDEX_NAME, TYPE, getTokenDocumentId(userToken))

0 commit comments

Comments
 (0)