From 46c7b5ee6e53e33aa6f91c1cbd873810d319a400 Mon Sep 17 00:00:00 2001 From: jaymode Date: Wed, 17 Oct 2018 10:37:40 -0600 Subject: [PATCH] Revert "Security: don't call prepare index for reads (#34246)" This reverts commit 0b4e8db1d3341d096e02bbdbb44fd8e5d221722a as some issues have been identified with the changed handling of a primary shard of the security index not being available. --- .../xpack/security/authc/TokenService.java | 170 ++++++++---------- .../authc/esnative/NativeUsersStore.java | 109 ++++++----- .../mapper/NativeRoleMappingStore.java | 44 ++--- .../authz/store/NativePrivilegeStore.java | 96 +++++----- .../authz/store/NativeRolesStore.java | 24 ++- .../support/SecurityIndexManager.java | 17 -- ...sportSamlInvalidateSessionActionTests.java | 5 - .../saml/TransportSamlLogoutActionTests.java | 5 - .../authc/AuthenticationServiceTests.java | 10 -- .../security/authc/TokenServiceTests.java | 62 +------ .../authc/esnative/NativeUsersStoreTests.java | 5 - .../store/NativePrivilegeStoreTests.java | 6 - 12 files changed, 207 insertions(+), 346 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java index 6ac7f687e2446..c72cf7e8c4f4f 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java @@ -366,50 +366,45 @@ void decodeToken(String token, ActionListener listener) throws IOExce final Cipher cipher = getDecryptionCipher(iv, decodeKey, version, decodedSalt); if (version.onOrAfter(Version.V_6_2_0)) { // we only have the id and need to get the token from the doc! - decryptTokenId(in, cipher, version, ActionListener.wrap(tokenId -> { - if (securityIndex.isAvailable() == false) { - logger.warn("failed to get token [{}] since index is not available", tokenId); - listener.onResponse(null); - } else { - securityIndex.checkIndexVersionThenExecute( - ex -> listener.onFailure(traceLog("prepare security index", tokenId, ex)), - () -> { - final GetRequest getRequest = client.prepareGet(SecurityIndexManager.SECURITY_INDEX_NAME, TYPE, - getTokenDocumentId(tokenId)).request(); - Consumer onFailure = ex -> listener.onFailure(traceLog("decode token", tokenId, ex)); - executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN, getRequest, - ActionListener.wrap(response -> { - if (response.isExists()) { - Map accessTokenSource = - (Map) response.getSource().get("access_token"); - if (accessTokenSource == null) { - onFailure.accept(new IllegalStateException( - "token document is missing the access_token field")); - } else if (accessTokenSource.containsKey("user_token") == false) { - onFailure.accept(new IllegalStateException( - "token document is missing the user_token field")); - } else { - Map userTokenSource = - (Map) accessTokenSource.get("user_token"); - listener.onResponse(UserToken.fromSourceMap(userTokenSource)); - } - } else { - onFailure.accept( - new IllegalStateException("token document is missing and must be present")); - } - }, e -> { - // if the index or the shard is not there / available we assume that - // the token is not valid - if (isShardNotAvailableException(e)) { - logger.warn("failed to get token [{}] since index is not available", tokenId); - listener.onResponse(null); + decryptTokenId(in, cipher, version, ActionListener.wrap(tokenId -> + securityIndex.prepareIndexIfNeededThenExecute( + ex -> listener.onFailure(traceLog("prepare security index", tokenId, ex)), + () -> { + final GetRequest getRequest = client.prepareGet(SecurityIndexManager.SECURITY_INDEX_NAME, TYPE, + getTokenDocumentId(tokenId)).request(); + Consumer onFailure = ex -> listener.onFailure(traceLog("decode token", tokenId, ex)); + executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN, getRequest, + ActionListener.wrap(response -> { + if (response.isExists()) { + Map accessTokenSource = + (Map) response.getSource().get("access_token"); + if (accessTokenSource == null) { + onFailure.accept(new IllegalStateException( + "token document is missing the access_token field")); + } else if (accessTokenSource.containsKey("user_token") == false) { + onFailure.accept(new IllegalStateException( + "token document is missing the user_token field")); } else { - logger.error(new ParameterizedMessage("failed to get token [{}]", tokenId), e); - listener.onFailure(e); + Map userTokenSource = + (Map) accessTokenSource.get("user_token"); + listener.onResponse(UserToken.fromSourceMap(userTokenSource)); } - }), client::get); - }); - }}, listener::onFailure)); + } else { + onFailure.accept( + new IllegalStateException("token document is missing and must be present")); + } + }, e -> { + // if the index or the shard is not there / available we assume that + // the token is not valid + if (isShardNotAvailableException(e)) { + logger.warn("failed to get token [{}] since index is not available", tokenId); + listener.onResponse(null); + } else { + logger.error(new ParameterizedMessage("failed to get token [{}]", tokenId), e); + listener.onFailure(e); + } + }), client::get); + }), listener::onFailure)); } else { decryptToken(in, cipher, version, listener); } @@ -696,37 +691,31 @@ private void findTokenFromRefreshToken(String refreshToken, ActionListener onFailure = ex -> listener.onFailure(traceLog("find by refresh token", refreshToken, ex)); - securityIndex.checkIndexVersionThenExecute(listener::onFailure, () -> - executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN, request, - ActionListener.wrap(searchResponse -> { - if (searchResponse.isTimedOut()) { - attemptCount.incrementAndGet(); - findTokenFromRefreshToken(refreshToken, listener, attemptCount); - } else if (searchResponse.getHits().getHits().length < 1) { - logger.info("could not find token document with refresh_token [{}]", refreshToken); - onFailure.accept(invalidGrantException("could not refresh the requested token")); - } else if (searchResponse.getHits().getHits().length > 1) { - onFailure.accept(new IllegalStateException("multiple tokens share the same refresh token")); - } else { - listener.onResponse(new Tuple<>(searchResponse, attemptCount)); - } - }, e -> { - if (isShardNotAvailableException(e)) { - logger.debug("failed to search for token document, retrying", e); - attemptCount.incrementAndGet(); - findTokenFromRefreshToken(refreshToken, listener, attemptCount); - } else { - onFailure.accept(e); - } - }), - client::search)); - } + Consumer onFailure = ex -> listener.onFailure(traceLog("find by refresh token", refreshToken, ex)); + securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () -> + executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN, request, + ActionListener.wrap(searchResponse -> { + if (searchResponse.isTimedOut()) { + attemptCount.incrementAndGet(); + findTokenFromRefreshToken(refreshToken, listener, attemptCount); + } else if (searchResponse.getHits().getHits().length < 1) { + logger.info("could not find token document with refresh_token [{}]", refreshToken); + onFailure.accept(invalidGrantException("could not refresh the requested token")); + } else if (searchResponse.getHits().getHits().length > 1) { + onFailure.accept(new IllegalStateException("multiple tokens share the same refresh token")); + } else { + listener.onResponse(new Tuple<>(searchResponse, attemptCount)); + } + }, e -> { + if (isShardNotAvailableException(e)) { + logger.debug("failed to search for token document, retrying", e); + attemptCount.incrementAndGet(); + findTokenFromRefreshToken(refreshToken, listener, attemptCount); + } else { + onFailure.accept(e); + } + }), + client::search)); } } @@ -863,22 +852,22 @@ public void findActiveTokensForRealm(String realmName, ActionListener supplier = client.threadPool().getThreadContext().newRestorableContext(false); - securityIndex.checkIndexVersionThenExecute(listener::onFailure, () -> - ScrollHelper.fetchAllByEntity(client, request, new ContextPreservingActionListener<>(supplier, listener), this::parseHit)); - } + final Supplier supplier = client.threadPool().getThreadContext().newRestorableContext(false); + securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () -> + ScrollHelper.fetchAllByEntity(client, request, new ContextPreservingActionListener<>(supplier, listener), this::parseHit)); } private Tuple parseHit(SearchHit hit) { @@ -956,12 +944,10 @@ private void ensureEnabled() { */ private void checkIfTokenIsRevoked(UserToken userToken, ActionListener listener) { if (securityIndex.indexExists() == false) { - // index doesn't exist so the token is considered valid. it is important to note that - // we do not use isAvailable as the lack of a shard being available is not equivalent - // to the index not existing in the case of revocation checking. + // index doesn't exist so the token is considered valid. listener.onResponse(userToken); } else { - securityIndex.checkIndexVersionThenExecute(listener::onFailure, () -> { + securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () -> { MultiGetRequest mGetRequest = client.prepareMultiGet() .add(SecurityIndexManager.SECURITY_INDEX_NAME, TYPE, getInvalidatedTokenDocumentId(userToken)) .add(SecurityIndexManager.SECURITY_INDEX_NAME, TYPE, getTokenDocumentId(userToken)) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/NativeUsersStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/NativeUsersStore.java index 196a48416a4cd..620c3817ebb0b 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/NativeUsersStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/NativeUsersStore.java @@ -118,7 +118,8 @@ public void getUsers(String[] userNames, final ActionListener> } }; - if (securityIndex.isAvailable() == false) { + if (securityIndex.indexExists() == false) { + // TODO remove this short circuiting and fix tests that fail without this! listener.onResponse(Collections.emptyList()); } else if (userNames.length == 1) { // optimization for single user lookup final String username = userNames[0]; @@ -126,7 +127,7 @@ public void getUsers(String[] userNames, final ActionListener> (uap) -> listener.onResponse(uap == null ? Collections.emptyList() : Collections.singletonList(uap.user())), handleException)); } else { - securityIndex.checkIndexVersionThenExecute(listener::onFailure, () -> { + securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () -> { final QueryBuilder query; if (userNames == null || userNames.length == 0) { query = QueryBuilders.termQuery(Fields.TYPE.getPreferredName(), USER_DOC_TYPE); @@ -154,10 +155,10 @@ public void getUsers(String[] userNames, final ActionListener> } void getUserCount(final ActionListener listener) { - if (securityIndex.isAvailable() == false) { + if (securityIndex.indexExists() == false) { listener.onResponse(0L); } else { - securityIndex.checkIndexVersionThenExecute(listener::onFailure, () -> + securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () -> executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN, client.prepareSearch(SECURITY_INDEX_NAME) .setQuery(QueryBuilders.termQuery(Fields.TYPE.getPreferredName(), USER_DOC_TYPE)) @@ -181,10 +182,11 @@ public void onFailure(Exception e) { * Async method to retrieve a user and their password */ private void getUserAndPassword(final String user, final ActionListener listener) { - if (securityIndex.isAvailable() == false) { + if (securityIndex.indexExists() == false) { + // TODO remove this short circuiting and fix tests that fail without this! listener.onResponse(null); } else { - securityIndex.checkIndexVersionThenExecute(listener::onFailure, () -> + securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () -> executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN, client.prepareGet(SECURITY_INDEX_NAME, INDEX_TYPE, getIdForUser(USER_DOC_TYPE, user)).request(), @@ -457,19 +459,16 @@ public void onFailure(Exception e) { } public void deleteUser(final DeleteUserRequest deleteUserRequest, final ActionListener listener) { - if (securityIndex.isAvailable() == false) { - listener.onResponse(false); - } else { - securityIndex.checkIndexVersionThenExecute(listener::onFailure, () -> { - DeleteRequest request = client.prepareDelete(SECURITY_INDEX_NAME, + securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () -> { + DeleteRequest request = client.prepareDelete(SECURITY_INDEX_NAME, INDEX_TYPE, getIdForUser(USER_DOC_TYPE, deleteUserRequest.username())).request(); - request.setRefreshPolicy(deleteUserRequest.getRefreshPolicy()); - executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN, request, + request.setRefreshPolicy(deleteUserRequest.getRefreshPolicy()); + executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN, request, new ActionListener() { @Override public void onResponse(DeleteResponse deleteResponse) { clearRealmCache(deleteUserRequest.username(), listener, - deleteResponse.getResult() == DocWriteResponse.Result.DELETED); + deleteResponse.getResult() == DocWriteResponse.Result.DELETED); } @Override @@ -477,8 +476,7 @@ public void onFailure(Exception e) { listener.onFailure(e); } }, client::delete); - }); - } + }); } /** @@ -500,10 +498,11 @@ void verifyPassword(String username, final SecureString password, ActionListener } void getReservedUserInfo(String username, ActionListener listener) { - if (securityIndex.isAvailable() == false) { + if (securityIndex.indexExists() == false) { + // TODO remove this short circuiting and fix tests that fail without this! listener.onResponse(null); } else { - securityIndex.checkIndexVersionThenExecute(listener::onFailure, () -> + securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () -> executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN, client.prepareGet(SECURITY_INDEX_NAME, INDEX_TYPE, getIdForUser(RESERVED_USER_TYPE, username)).request(), @@ -542,53 +541,49 @@ public void onFailure(Exception e) { } void getAllReservedUserInfo(ActionListener> listener) { - if (securityIndex.isAvailable() == false) { - listener.onResponse(Collections.emptyMap()); - } else { - securityIndex.checkIndexVersionThenExecute(listener::onFailure, () -> - executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN, - client.prepareSearch(SECURITY_INDEX_NAME) + securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () -> + executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN, + client.prepareSearch(SECURITY_INDEX_NAME) .setQuery(QueryBuilders.termQuery(Fields.TYPE.getPreferredName(), RESERVED_USER_TYPE)) .setFetchSource(true).request(), - new ActionListener() { - @Override - public void onResponse(SearchResponse searchResponse) { - Map userInfos = new HashMap<>(); - assert searchResponse.getHits().getTotalHits() <= 10 : + new ActionListener() { + @Override + public void onResponse(SearchResponse searchResponse) { + Map userInfos = new HashMap<>(); + assert searchResponse.getHits().getTotalHits() <= 10 : "there are more than 10 reserved users we need to change this to retrieve them all!"; - for (SearchHit searchHit : searchResponse.getHits().getHits()) { - Map sourceMap = searchHit.getSourceAsMap(); - String password = (String) sourceMap.get(Fields.PASSWORD.getPreferredName()); - Boolean enabled = (Boolean) sourceMap.get(Fields.ENABLED.getPreferredName()); - final String id = searchHit.getId(); - assert id != null && id.startsWith(RESERVED_USER_TYPE) : + for (SearchHit searchHit : searchResponse.getHits().getHits()) { + Map sourceMap = searchHit.getSourceAsMap(); + String password = (String) sourceMap.get(Fields.PASSWORD.getPreferredName()); + Boolean enabled = (Boolean) sourceMap.get(Fields.ENABLED.getPreferredName()); + final String id = searchHit.getId(); + assert id != null && id.startsWith(RESERVED_USER_TYPE) : "id [" + id + "] does not start with reserved-user prefix"; - final String username = id.substring(RESERVED_USER_TYPE.length() + 1); - if (password == null) { - listener.onFailure(new IllegalStateException("password hash must not be null!")); - return; - } else if (enabled == null) { - listener.onFailure(new IllegalStateException("enabled must not be null!")); - return; - } else { - userInfos.put(username, new ReservedUserInfo(password.toCharArray(), enabled, false)); - } + final String username = id.substring(RESERVED_USER_TYPE.length() + 1); + if (password == null) { + listener.onFailure(new IllegalStateException("password hash must not be null!")); + return; + } else if (enabled == null) { + listener.onFailure(new IllegalStateException("enabled must not be null!")); + return; + } else { + userInfos.put(username, new ReservedUserInfo(password.toCharArray(), enabled, false)); } - listener.onResponse(userInfos); } + listener.onResponse(userInfos); + } - @Override - public void onFailure(Exception e) { - if (e instanceof IndexNotFoundException) { - logger.trace("could not retrieve built in users since security index does not exist", e); - listener.onResponse(Collections.emptyMap()); - } else { - logger.error("failed to retrieve built in users", e); - listener.onFailure(e); - } + @Override + public void onFailure(Exception e) { + if (e instanceof IndexNotFoundException) { + logger.trace("could not retrieve built in users since security index does not exist", e); + listener.onResponse(Collections.emptyMap()); + } else { + logger.error("failed to retrieve built in users", e); + listener.onFailure(e); } - }, client::search)); - } + } + }, client::search)); } private void clearRealmCache(String username, ActionListener listener, Response response) { diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/mapper/NativeRoleMappingStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/mapper/NativeRoleMappingStore.java index ba3e5ee3793a0..b45de8184d6e3 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/mapper/NativeRoleMappingStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/mapper/NativeRoleMappingStore.java @@ -220,32 +220,32 @@ public void onFailure(Exception e) { }); } - private void innerDeleteMapping(DeleteRoleMappingRequest request, ActionListener listener) { - if (securityIndex.isAvailable() == false) { - listener.onResponse(false); - } else { - securityIndex.checkIndexVersionThenExecute(listener::onFailure, () -> { - executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN, - client.prepareDelete(SECURITY_INDEX_NAME, SECURITY_GENERIC_TYPE, getIdForName(request.getName())) + private void innerDeleteMapping(DeleteRoleMappingRequest request, ActionListener listener) throws IOException { + if (securityIndex.isIndexUpToDate() == false) { + listener.onFailure(new IllegalStateException( + "Security index is not on the current version - the native realm will not be operational until " + + "the upgrade API is run on the security index")); + return; + } + executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN, + client.prepareDelete(SECURITY_INDEX_NAME, SECURITY_GENERIC_TYPE, getIdForName(request.getName())) .setRefreshPolicy(request.getRefreshPolicy()) .request(), - new ActionListener() { + new ActionListener() { - @Override - public void onResponse(DeleteResponse deleteResponse) { - boolean deleted = deleteResponse.getResult() == DELETED; - listener.onResponse(deleted); - } + @Override + public void onResponse(DeleteResponse deleteResponse) { + boolean deleted = deleteResponse.getResult() == DELETED; + listener.onResponse(deleted); + } - @Override - public void onFailure(Exception e) { - logger.error(new ParameterizedMessage("failed to delete role-mapping [{}]", request.getName()), e); - listener.onFailure(e); + @Override + public void onFailure(Exception e) { + logger.error(new ParameterizedMessage("failed to delete role-mapping [{}]", request.getName()), e); + listener.onFailure(e); - } - }, client::delete); - }); - } + } + }, client::delete); } /** @@ -301,7 +301,7 @@ private void getMappings(ActionListener> listener) { * */ public void usageStats(ActionListener> listener) { - if (securityIndex.isAvailable() == false) { + if (securityIndex.indexExists() == false) { reportStats(listener, Collections.emptyList()); } else { getMappings(ActionListener.wrap(mappings -> reportStats(listener, mappings), listener::onFailure)); diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java index e7a27855f5da6..2cfa89b647ceb 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java @@ -88,15 +88,13 @@ public NativePrivilegeStore(Settings settings, Client client, SecurityIndexManag public void getPrivileges(Collection applications, Collection names, ActionListener> listener) { - if (securityIndexManager.isAvailable() == false) { - listener.onResponse(Collections.emptyList()); - } else if (applications != null && applications.size() == 1 && names != null && names.size() == 1) { + if (applications != null && applications.size() == 1 && names != null && names.size() == 1) { getPrivilege(Objects.requireNonNull(Iterables.get(applications, 0)), Objects.requireNonNull(Iterables.get(names, 0)), ActionListener.wrap(privilege -> listener.onResponse(privilege == null ? Collections.emptyList() : Collections.singletonList(privilege)), listener::onFailure)); } else { - securityIndexManager.checkIndexVersionThenExecute(listener::onFailure, () -> { + securityIndexManager.prepareIndexIfNeededThenExecute(listener::onFailure, () -> { final QueryBuilder query; final TermQueryBuilder typeQuery = QueryBuilders .termQuery(ApplicationPrivilegeDescriptor.Fields.TYPE.getPreferredName(), DOC_TYPE_VALUE); @@ -136,37 +134,33 @@ private static boolean isEmpty(Collection collection) { return collection == null || collection.isEmpty(); } - void getPrivilege(String application, String name, ActionListener listener) { - if (securityIndexManager.isAvailable() == false) { - listener.onResponse(null); - } else { - securityIndexManager.checkIndexVersionThenExecute(listener::onFailure, - () -> executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN, - client.prepareGet(SECURITY_INDEX_NAME, "doc", toDocId(application, name)).request(), - new ActionListener() { - @Override - public void onResponse(GetResponse response) { - if (response.isExists()) { - listener.onResponse(buildPrivilege(response.getId(), response.getSourceAsBytesRef())); - } else { - listener.onResponse(null); - } + public void getPrivilege(String application, String name, ActionListener listener) { + securityIndexManager.prepareIndexIfNeededThenExecute(listener::onFailure, + () -> executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN, + client.prepareGet(SECURITY_INDEX_NAME, "doc", toDocId(application, name)).request(), + new ActionListener() { + @Override + public void onResponse(GetResponse response) { + if (response.isExists()) { + listener.onResponse(buildPrivilege(response.getId(), response.getSourceAsBytesRef())); + } else { + listener.onResponse(null); } + } - @Override - public void onFailure(Exception e) { - // if the index or the shard is not there / available we just claim the privilege is not there - if (TransportActions.isShardNotAvailableException(e)) { - logger.warn(new ParameterizedMessage("failed to load privilege [{}] index not available", name), e); - listener.onResponse(null); - } else { - logger.error(new ParameterizedMessage("failed to load privilege [{}]", name), e); - listener.onFailure(e); - } + @Override + public void onFailure(Exception e) { + // if the index or the shard is not there / available we just claim the privilege is not there + if (TransportActions.isShardNotAvailableException(e)) { + logger.warn(new ParameterizedMessage("failed to load privilege [{}] index not available", name), e); + listener.onResponse(null); + } else { + logger.error(new ParameterizedMessage("failed to load privilege [{}]", name), e); + listener.onFailure(e); } - }, - client::get)); - } + } + }, + client::get)); } public void putPrivileges(Collection privileges, WriteRequest.RefreshPolicy refreshPolicy, @@ -206,27 +200,23 @@ private void innerPutPrivilege(ApplicationPrivilegeDescriptor privilege, WriteRe public void deletePrivileges(String application, Collection names, WriteRequest.RefreshPolicy refreshPolicy, ActionListener>> listener) { - if (securityIndexManager.isAvailable() == false) { - listener.onResponse(Collections.emptyMap()); - } else { - securityIndexManager.checkIndexVersionThenExecute(listener::onFailure, () -> { - ActionListener groupListener = new GroupedActionListener<>( - ActionListener.wrap(responses -> { - final Map> deletedNames = responses.stream() - .filter(r -> r.getResult() == DocWriteResponse.Result.DELETED) - .map(r -> r.getId()) - .map(NativePrivilegeStore::nameFromDocId) - .collect(TUPLES_TO_MAP); - clearRolesCache(listener, deletedNames); - }, listener::onFailure), names.size(), Collections.emptyList()); - for (String name : names) { - ClientHelper.executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN, - client.prepareDelete(SECURITY_INDEX_NAME, "doc", toDocId(application, name)) - .setRefreshPolicy(refreshPolicy) - .request(), groupListener, client::delete); - } - }); - } + securityIndexManager.prepareIndexIfNeededThenExecute(listener::onFailure, () -> { + ActionListener groupListener = new GroupedActionListener<>( + ActionListener.wrap(responses -> { + final Map> deletedNames = responses.stream() + .filter(r -> r.getResult() == DocWriteResponse.Result.DELETED) + .map(r -> r.getId()) + .map(NativePrivilegeStore::nameFromDocId) + .collect(TUPLES_TO_MAP); + clearRolesCache(listener, deletedNames); + }, listener::onFailure), names.size(), Collections.emptyList()); + for (String name : names) { + ClientHelper.executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN, + client.prepareDelete(SECURITY_INDEX_NAME, "doc", toDocId(application, name)) + .setRefreshPolicy(refreshPolicy) + .request(), groupListener, client::delete); + } + }); } private void clearRolesCache(ActionListener listener, T value) { diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java index c5be68f41996e..d604d166812c8 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java @@ -116,7 +116,7 @@ public void getRoleDescriptors(Set names, final ActionListener { + securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () -> { QueryBuilder query; if (names == null || names.isEmpty()) { query = QueryBuilders.termQuery(RoleDescriptor.Fields.TYPE.getPreferredName(), ROLE_TYPE); @@ -144,19 +144,16 @@ public void getRoleDescriptors(Set names, final ActionListener listener) { - if (securityIndex.isAvailable() == false) { - listener.onResponse(false); - } else { - securityIndex.checkIndexVersionThenExecute(listener::onFailure, () -> { - DeleteRequest request = client.prepareDelete(SecurityIndexManager.SECURITY_INDEX_NAME, + securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () -> { + DeleteRequest request = client.prepareDelete(SecurityIndexManager.SECURITY_INDEX_NAME, ROLE_DOC_TYPE, getIdForUser(deleteRoleRequest.name())).request(); - request.setRefreshPolicy(deleteRoleRequest.getRefreshPolicy()); - executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN, request, + request.setRefreshPolicy(deleteRoleRequest.getRefreshPolicy()); + executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN, request, new ActionListener() { @Override public void onResponse(DeleteResponse deleteResponse) { clearRoleCache(deleteRoleRequest.name(), listener, - deleteResponse.getResult() == DocWriteResponse.Result.DELETED); + deleteResponse.getResult() == DocWriteResponse.Result.DELETED); } @Override @@ -165,8 +162,7 @@ public void onFailure(Exception e) { listener.onFailure(e); } }, client::delete); - }); - } + }); } public void putRole(final PutRoleRequest request, final RoleDescriptor role, final ActionListener listener) { @@ -214,13 +210,13 @@ public void onFailure(Exception e) { public void usageStats(ActionListener> listener) { Map usageStats = new HashMap<>(3); - if (securityIndex.isAvailable() == false) { + if (securityIndex.indexExists() == false) { usageStats.put("size", 0L); usageStats.put("fls", false); usageStats.put("dls", false); listener.onResponse(usageStats); } else { - securityIndex.checkIndexVersionThenExecute(listener::onFailure, () -> + securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () -> executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN, client.prepareMultiSearch() .add(client.prepareSearch(SecurityIndexManager.SECURITY_INDEX_NAME) @@ -302,7 +298,7 @@ public void onFailure(Exception e) { } private void executeGetRoleRequest(String role, ActionListener listener) { - securityIndex.checkIndexVersionThenExecute(listener::onFailure, () -> + securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () -> executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN, client.prepareGet(SECURITY_INDEX_NAME, ROLE_DOC_TYPE, getIdForUser(role)).request(), diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/SecurityIndexManager.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/SecurityIndexManager.java index 18d86ab028cd2..d02b569a7440e 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/SecurityIndexManager.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/SecurityIndexManager.java @@ -253,23 +253,6 @@ private static Version readMappingVersion(String indexName, MappingMetaData mapp } } - /** - * Validates the security index is up to date and does not need to migrated. If it is not, the - * consumer is called with an exception. If the security index is up to date, the runnable will - * be executed. NOTE: this method does not check the availability of the index; this check - * is left to the caller so that this condition can be handled appropriately. - */ - public void checkIndexVersionThenExecute(final Consumer consumer, final Runnable andThen) { - final State indexState = this.indexState; // use a local copy so all checks execute against the same state! - if (indexState.indexExists && indexState.isIndexUpToDate == false) { - consumer.accept(new IllegalStateException( - "Security index is not on the current version. Security features relying on the index will not be available until " + - "the upgrade API is run on the security index")); - } else { - andThen.run(); - } - } - /** * Prepares the index by creating it if it doesn't exist or updating the mappings if the mappings are * out of date. After any tasks have been executed, the runnable is then executed. diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/saml/TransportSamlInvalidateSessionActionTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/saml/TransportSamlInvalidateSessionActionTests.java index 65f95ecb2f89f..17a45f2389303 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/saml/TransportSamlInvalidateSessionActionTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/saml/TransportSamlInvalidateSessionActionTests.java @@ -163,11 +163,6 @@ void doExecute(Action action, Request request, ActionListener { - ((Runnable) inv.getArguments()[1]).run(); - return null; - }).when(securityIndex).checkIndexVersionThenExecute(any(Consumer.class), any(Runnable.class)); - when(securityIndex.isAvailable()).thenReturn(true); final ClusterService clusterService = ClusterServiceUtils.createClusterService(threadPool); tokenService = new TokenService(settings, Clock.systemUTC(), client, securityIndex, clusterService); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/saml/TransportSamlLogoutActionTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/saml/TransportSamlLogoutActionTests.java index f2f94176edc15..291c102f396c5 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/saml/TransportSamlLogoutActionTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/saml/TransportSamlLogoutActionTests.java @@ -178,11 +178,6 @@ public void setup() throws Exception { ((Runnable) inv.getArguments()[1]).run(); return null; }).when(securityIndex).prepareIndexIfNeededThenExecute(any(Consumer.class), any(Runnable.class)); - doAnswer(inv -> { - ((Runnable) inv.getArguments()[1]).run(); - return null; - }).when(securityIndex).checkIndexVersionThenExecute(any(Consumer.class), any(Runnable.class)); - when(securityIndex.isAvailable()).thenReturn(true); final ClusterService clusterService = ClusterServiceUtils.createClusterService(threadPool); tokenService = new TokenService(settings, Clock.systemUTC(), client, securityIndex, clusterService); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/AuthenticationServiceTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/AuthenticationServiceTests.java index 549083896108e..ef5b0386bc23f 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/AuthenticationServiceTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/AuthenticationServiceTests.java @@ -87,7 +87,6 @@ import static org.elasticsearch.test.SecurityTestsUtils.assertAuthenticationException; import static org.elasticsearch.xpack.core.security.support.Exceptions.authenticationError; -import static org.elasticsearch.xpack.security.authc.TokenServiceTests.mockCheckTokenInvalidationFromId; import static org.elasticsearch.xpack.security.authc.TokenServiceTests.mockGetTokenFromId; import static org.hamcrest.Matchers.arrayContaining; import static org.hamcrest.Matchers.contains; @@ -188,11 +187,6 @@ licenseState, threadContext, mock(ReservedRealm.class), Arrays.asList(firstRealm runnable.run(); return null; }).when(securityIndex).prepareIndexIfNeededThenExecute(any(Consumer.class), any(Runnable.class)); - doAnswer(invocationOnMock -> { - Runnable runnable = (Runnable) invocationOnMock.getArguments()[1]; - runnable.run(); - return null; - }).when(securityIndex).checkIndexVersionThenExecute(any(Consumer.class), any(Runnable.class)); ClusterService clusterService = ClusterServiceUtils.createClusterService(threadPool); tokenService = new TokenService(settings, Clock.systemUTC(), client, securityIndex, clusterService); service = new AuthenticationService(settings, realms, auditTrail, @@ -904,11 +898,7 @@ public void testAuthenticateWithToken() throws Exception { tokenService.createUserToken(expected, originatingAuth, tokenFuture, Collections.emptyMap(), true); } String token = tokenService.getUserTokenString(tokenFuture.get().v1()); - when(client.prepareMultiGet()).thenReturn(new MultiGetRequestBuilder(client, MultiGetAction.INSTANCE)); mockGetTokenFromId(tokenFuture.get().v1(), client); - mockCheckTokenInvalidationFromId(tokenFuture.get().v1(), client); - when(securityIndex.isAvailable()).thenReturn(true); - when(securityIndex.indexExists()).thenReturn(true); try (ThreadContext.StoredContext ignore = threadContext.stashContext()) { threadContext.putHeader("Authorization", "Bearer " + token); service.authenticate("_action", message, (User)null, ActionListener.wrap(result -> { diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/TokenServiceTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/TokenServiceTests.java index 7926b44a38cb8..213def0f0fe6d 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/TokenServiceTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/TokenServiceTests.java @@ -137,13 +137,6 @@ public void setupClient() { runnable.run(); return null; }).when(securityIndex).prepareIndexIfNeededThenExecute(any(Consumer.class), any(Runnable.class)); - doAnswer(invocationOnMock -> { - Runnable runnable = (Runnable) invocationOnMock.getArguments()[1]; - runnable.run(); - return null; - }).when(securityIndex).checkIndexVersionThenExecute(any(Consumer.class), any(Runnable.class)); - when(securityIndex.indexExists()).thenReturn(true); - when(securityIndex.isAvailable()).thenReturn(true); this.clusterService = ClusterServiceUtils.createClusterService(threadPool); } @@ -168,7 +161,6 @@ public void testAttachAndGetToken() throws Exception { final UserToken token = tokenFuture.get().v1(); assertNotNull(token); mockGetTokenFromId(token); - mockCheckTokenInvalidationFromId(token); ThreadContext requestContext = new ThreadContext(Settings.EMPTY); requestContext.putHeader("Authorization", randomFrom("Bearer ", "BEARER ", "bearer ") + tokenService.getUserTokenString(token)); @@ -215,7 +207,6 @@ public void testRotateKey() throws Exception { final UserToken token = tokenFuture.get().v1(); assertNotNull(token); mockGetTokenFromId(token); - mockCheckTokenInvalidationFromId(token); ThreadContext requestContext = new ThreadContext(Settings.EMPTY); requestContext.putHeader("Authorization", "Bearer " + tokenService.getUserTokenString(token)); @@ -275,7 +266,6 @@ public void testKeyExchange() throws Exception { final UserToken token = tokenFuture.get().v1(); assertNotNull(token); mockGetTokenFromId(token); - mockCheckTokenInvalidationFromId(token); ThreadContext requestContext = new ThreadContext(Settings.EMPTY); requestContext.putHeader("Authorization", "Bearer " + tokenService.getUserTokenString(token)); @@ -306,7 +296,6 @@ public void testPruneKeys() throws Exception { final UserToken token = tokenFuture.get().v1(); assertNotNull(token); mockGetTokenFromId(token); - mockCheckTokenInvalidationFromId(token); ThreadContext requestContext = new ThreadContext(Settings.EMPTY); requestContext.putHeader("Authorization", "Bearer " + tokenService.getUserTokenString(token)); @@ -368,7 +357,6 @@ public void testPassphraseWorks() throws Exception { final UserToken token = tokenFuture.get().v1(); assertNotNull(token); mockGetTokenFromId(token); - mockCheckTokenInvalidationFromId(token); ThreadContext requestContext = new ThreadContext(Settings.EMPTY); requestContext.putHeader("Authorization", "Bearer " + tokenService.getUserTokenString(token)); @@ -466,7 +454,6 @@ public void testTokenExpiry() throws Exception { tokenService.createUserToken(authentication, authentication, tokenFuture, Collections.emptyMap(), true); final UserToken token = tokenFuture.get().v1(); mockGetTokenFromId(token); - mockCheckTokenInvalidationFromId(token); ThreadContext requestContext = new ThreadContext(Settings.EMPTY); requestContext.putHeader("Authorization", "Bearer " + tokenService.getUserTokenString(token)); @@ -590,25 +577,14 @@ public void testIndexNotAvailable() throws Exception { try (ThreadContext.StoredContext ignore = requestContext.newStoredContext(true)) { PlainActionFuture future = new PlainActionFuture<>(); tokenService.getAndValidateToken(requestContext, future); - assertNull(future.get()); + UserToken serialized = future.get(); + assertEquals(authentication, serialized.getAuthentication()); when(securityIndex.isAvailable()).thenReturn(false); when(securityIndex.indexExists()).thenReturn(true); future = new PlainActionFuture<>(); tokenService.getAndValidateToken(requestContext, future); assertNull(future.get()); - - when(securityIndex.indexExists()).thenReturn(false); - future = new PlainActionFuture<>(); - tokenService.getAndValidateToken(requestContext, future); - assertNull(future.get()); - - when(securityIndex.isAvailable()).thenReturn(true); - when(securityIndex.indexExists()).thenReturn(true); - mockCheckTokenInvalidationFromId(token); - future = new PlainActionFuture<>(); - tokenService.getAndValidateToken(requestContext, future); - assertEquals(token.getAuthentication(), future.get().getAuthentication()); } } @@ -649,38 +625,4 @@ public static void mockGetTokenFromId(UserToken userToken, Client client) { return Void.TYPE; }).when(client).get(any(GetRequest.class), any(ActionListener.class)); } - - private void mockCheckTokenInvalidationFromId(UserToken userToken) { - mockCheckTokenInvalidationFromId(userToken, client); - } - - public static void mockCheckTokenInvalidationFromId(UserToken userToken, Client client) { - doAnswer(invocationOnMock -> { - MultiGetRequest request = (MultiGetRequest) invocationOnMock.getArguments()[0]; - ActionListener listener = (ActionListener) invocationOnMock.getArguments()[1]; - MultiGetResponse response = mock(MultiGetResponse.class); - MultiGetItemResponse[] responses = new MultiGetItemResponse[2]; - when(response.getResponses()).thenReturn(responses); - GetResponse legacyResponse = mock(GetResponse.class); - responses[0] = new MultiGetItemResponse(legacyResponse, null); - when(legacyResponse.isExists()).thenReturn(false); - GetResponse tokenResponse = mock(GetResponse.class); - if (userToken.getId().equals(request.getItems().get(1).id().replace("token_", ""))) { - when(tokenResponse.isExists()).thenReturn(true); - Map sourceMap = new HashMap<>(); - try (XContentBuilder builder = XContentBuilder.builder(XContentType.JSON.xContent())) { - userToken.toXContent(builder, ToXContent.EMPTY_PARAMS); - Map accessTokenMap = new HashMap<>(); - accessTokenMap.put("user_token", - XContentHelper.convertToMap(XContentType.JSON.xContent(), Strings.toString(builder), false)); - accessTokenMap.put("invalidated", false); - sourceMap.put("access_token", accessTokenMap); - } - when(tokenResponse.getSource()).thenReturn(sourceMap); - } - responses[1] = new MultiGetItemResponse(tokenResponse, null); - listener.onResponse(response); - return Void.TYPE; - }).when(client).multiGet(any(MultiGetRequest.class), any(ActionListener.class)); - } } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/esnative/NativeUsersStoreTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/esnative/NativeUsersStoreTests.java index 425bebc060095..f280e85f4ab1a 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/esnative/NativeUsersStoreTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/esnative/NativeUsersStoreTests.java @@ -242,11 +242,6 @@ private NativeUsersStore startNativeUsersStore() { action.run(); return null; }).when(securityIndex).prepareIndexIfNeededThenExecute(any(Consumer.class), any(Runnable.class)); - doAnswer((i) -> { - Runnable action = (Runnable) i.getArguments()[1]; - action.run(); - return null; - }).when(securityIndex).checkIndexVersionThenExecute(any(Consumer.class), any(Runnable.class)); return new NativeUsersStore(Settings.EMPTY, client, securityIndex); } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java index 9b08691f4153c..89058cf4a8bb9 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java @@ -96,12 +96,6 @@ void doExecute(Action action, Request request, ActionListener { - assertThat(invocationOnMock.getArguments().length, equalTo(2)); - assertThat(invocationOnMock.getArguments()[1], instanceOf(Runnable.class)); - ((Runnable) invocationOnMock.getArguments()[1]).run(); - return null; - }).when(securityIndex).checkIndexVersionThenExecute(any(Consumer.class), any(Runnable.class)); store = new NativePrivilegeStore(Settings.EMPTY, client, securityIndex); }