From 52e647ca6df7c583fbf12dde02bb6be1e98b827e Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Thu, 31 Aug 2023 17:40:29 +0000 Subject: [PATCH] Verify updated configuration is newer on reload Signed-off-by: Peter Nied --- .../test/framework/cluster/LocalCluster.java | 2 +- .../configupdate/ConfigUpdateRequest.java | 73 +++++++++++++--- .../ConfigUpdateRequestBuilder.java | 46 ---------- .../TransportConfigUpdateAction.java | 11 +-- .../ConfigurationRepository.java | 83 +++++++++++++------ .../dlic/rest/api/AbstractApiAction.java | 17 ++-- .../dlic/rest/api/FlushCacheApiAction.java | 2 +- .../dlic/rest/api/MigrateApiAction.java | 58 ++++++------- .../dlic/rest/api/ValidateApiAction.java | 3 +- .../rest/SecurityConfigUpdateAction.java | 6 +- .../security/rest/TenantInfoAction.java | 6 +- .../security/securityconf/impl/CType.java | 4 +- .../opensearch/security/user/UserService.java | 6 +- .../security/HttpIntegrationTests.java | 15 +++- .../InitializationIntegrationTests.java | 8 +- .../opensearch/security/IntegrationTests.java | 8 +- .../security/SnapshotRestoreTests.java | 6 +- .../api/AbstractApiActionValidationTest.java | 7 +- .../RolesMappingApiActionValidationTest.java | 7 +- .../test/AbstractSecurityUnitTest.java | 5 +- 20 files changed, 200 insertions(+), 173 deletions(-) delete mode 100644 src/main/java/org/opensearch/security/action/configupdate/ConfigUpdateRequestBuilder.java diff --git a/src/integrationTest/java/org/opensearch/test/framework/cluster/LocalCluster.java b/src/integrationTest/java/org/opensearch/test/framework/cluster/LocalCluster.java index 64207ead5b..5c181b92a3 100644 --- a/src/integrationTest/java/org/opensearch/test/framework/cluster/LocalCluster.java +++ b/src/integrationTest/java/org/opensearch/test/framework/cluster/LocalCluster.java @@ -290,7 +290,7 @@ public void updateUserConfiguration(List users) { private static void triggerConfigurationReload(Client client) { ConfigUpdateResponse configUpdateResponse = client.execute( ConfigUpdateAction.INSTANCE, - new ConfigUpdateRequest(CType.lcStringValues().toArray(new String[0])) + new ConfigUpdateRequest(CType.values(), null) ).actionGet(); if (configUpdateResponse.hasFailures()) { throw new RuntimeException("ConfigUpdateResponse produced failures: " + configUpdateResponse.failures()); diff --git a/src/main/java/org/opensearch/security/action/configupdate/ConfigUpdateRequest.java b/src/main/java/org/opensearch/security/action/configupdate/ConfigUpdateRequest.java index d4e860569d..76c871138b 100644 --- a/src/main/java/org/opensearch/security/action/configupdate/ConfigUpdateRequest.java +++ b/src/main/java/org/opensearch/security/action/configupdate/ConfigUpdateRequest.java @@ -27,42 +27,55 @@ package org.opensearch.security.action.configupdate; import java.io.IOException; +import java.util.Map; +import java.util.stream.Collectors; +import java.util.stream.IntStream; import org.opensearch.action.ActionRequestValidationException; import org.opensearch.action.support.nodes.BaseNodesRequest; import org.opensearch.core.common.io.stream.StreamInput; import org.opensearch.core.common.io.stream.StreamOutput; +import org.opensearch.security.securityconf.impl.CType; public class ConfigUpdateRequest extends BaseNodesRequest { - private String[] configTypes; + private CType[] configTypes; + private Long[] sequenceIds; public ConfigUpdateRequest(StreamInput in) throws IOException { super(in); - this.configTypes = in.readStringArray(); + this.configTypes = CType.fromStringValues(in.readStringArray()); + // For backwards compability during mixed cluster scenarios need to cleanly deserialize + if (in.available() != 0) { + this.sequenceIds = ConfigUpdateRequest.longArrayFromStringValues(in.readStringArray()); + } } public ConfigUpdateRequest() { super(new String[0]); } - public ConfigUpdateRequest(String[] configTypes) { + /** + * @param configTypes The types of configuration that should be reloaded + * @param sequenceIds Sequence numbers that should be smaller than the values from reloaded configuration, if null ignored. + */ + public ConfigUpdateRequest(final CType[] configTypes, final Long[] sequenceIds) { this(); - setConfigTypes(configTypes); + this.configTypes = configTypes; + this.sequenceIds = sequenceIds; } @Override public void writeTo(final StreamOutput out) throws IOException { super.writeTo(out); - out.writeStringArray(configTypes); - } - - public String[] getConfigTypes() { - return configTypes; + out.writeStringArray(prepToSerialize(configTypes)); + out.writeStringArray(prepToSerialize(sequenceIds)); } - public void setConfigTypes(final String[] configTypes) { - this.configTypes = configTypes; + public Map getTypeAndSequenceIdMap() { + return IntStream.range(0, configTypes.length) + .boxed() + .collect(Collectors.toMap(i -> configTypes[i], i -> sequenceIds != null ? sequenceIds[i] : null)); } @Override @@ -72,4 +85,42 @@ public ActionRequestValidationException validate() { } return null; } + + // TODO BEFORE-MERGE: Need to be sure unit test capture these scenarios + + private static String[] prepToSerialize(final CType[] ctypes) { + if (ctypes == null) { + return null; + } + + final String[] serializedReady = new String[ctypes.length]; + for (int i = 0; i < ctypes.length; i++) { + serializedReady[i] = ctypes[i] + ""; + } + return serializedReady; + } + + private static String[] prepToSerialize(final Long[] longs) { + if (longs == null) { + return null; + } + + final String[] serializedReady = new String[longs.length]; + for (int i = 0; i < longs.length; i++) { + serializedReady[i] = longs[i] != null ? longs[i] + "" : null; + } + return serializedReady; + } + + private static Long[] longArrayFromStringValues(final String[] longsAsStrings) { + if (longsAsStrings == null) { + return null; + } + + final Long[] asLongs = new Long[longsAsStrings.length]; + for (int i = 0; i < longsAsStrings.length; i++) { + asLongs[i] = longsAsStrings[i] != null ? Long.parseLong(longsAsStrings[i]) : null; + } + return asLongs; + } } diff --git a/src/main/java/org/opensearch/security/action/configupdate/ConfigUpdateRequestBuilder.java b/src/main/java/org/opensearch/security/action/configupdate/ConfigUpdateRequestBuilder.java deleted file mode 100644 index edfe1f10bb..0000000000 --- a/src/main/java/org/opensearch/security/action/configupdate/ConfigUpdateRequestBuilder.java +++ /dev/null @@ -1,46 +0,0 @@ -/* - * Copyright 2015-2018 _floragunn_ GmbH - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - * - * Modifications Copyright OpenSearch Contributors. See - * GitHub history for details. - */ - -package org.opensearch.security.action.configupdate; - -import org.opensearch.action.ActionType; -import org.opensearch.action.support.nodes.NodesOperationRequestBuilder; -import org.opensearch.client.OpenSearchClient; - -public class ConfigUpdateRequestBuilder extends NodesOperationRequestBuilder< - ConfigUpdateRequest, - ConfigUpdateResponse, - ConfigUpdateRequestBuilder> { - - protected ConfigUpdateRequestBuilder(OpenSearchClient client, ActionType action) { - super(client, action, new ConfigUpdateRequest()); - } - - public ConfigUpdateRequestBuilder setShardId(final String[] configTypes) { - request.setConfigTypes(configTypes); - return this; - } -} diff --git a/src/main/java/org/opensearch/security/action/configupdate/TransportConfigUpdateAction.java b/src/main/java/org/opensearch/security/action/configupdate/TransportConfigUpdateAction.java index 1e5b5e4056..3570948884 100644 --- a/src/main/java/org/opensearch/security/action/configupdate/TransportConfigUpdateAction.java +++ b/src/main/java/org/opensearch/security/action/configupdate/TransportConfigUpdateAction.java @@ -43,8 +43,6 @@ import org.opensearch.common.settings.Settings; import org.opensearch.security.auth.BackendRegistry; import org.opensearch.security.configuration.ConfigurationRepository; -import org.opensearch.security.securityconf.DynamicConfigFactory; -import org.opensearch.security.securityconf.impl.CType; import org.opensearch.threadpool.ThreadPool; import org.opensearch.transport.TransportRequest; import org.opensearch.transport.TransportService; @@ -58,7 +56,6 @@ public class TransportConfigUpdateAction extends TransportNodesAction< protected Logger logger = LogManager.getLogger(getClass()); private final Provider backendRegistry; private final ConfigurationRepository configurationRepository; - private DynamicConfigFactory dynamicConfigFactory; @Inject public TransportConfigUpdateAction( @@ -68,8 +65,7 @@ public TransportConfigUpdateAction( final TransportService transportService, final ConfigurationRepository configurationRepository, final ActionFilters actionFilters, - Provider backendRegistry, - DynamicConfigFactory dynamicConfigFactory + Provider backendRegistry ) { super( ConfigUpdateAction.NAME, @@ -85,7 +81,6 @@ public TransportConfigUpdateAction( this.configurationRepository = configurationRepository; this.backendRegistry = backendRegistry; - this.dynamicConfigFactory = dynamicConfigFactory; } public static class NodeConfigUpdateRequest extends TransportRequest { @@ -125,9 +120,9 @@ protected ConfigUpdateResponse newResponse( @Override protected ConfigUpdateNodeResponse nodeOperation(final NodeConfigUpdateRequest request) { - configurationRepository.reloadConfiguration(CType.fromStringValues((request.request.getConfigTypes()))); + configurationRepository.reloadConfiguration(request.request); backendRegistry.get().invalidateCache(); - return new ConfigUpdateNodeResponse(clusterService.localNode(), request.request.getConfigTypes(), null); + return new ConfigUpdateNodeResponse(clusterService.localNode(), new String[] {}, null); } @Override diff --git a/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java b/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java index 17ea48f46c..5c12ff3bcc 100644 --- a/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java +++ b/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java @@ -30,17 +30,17 @@ import java.nio.file.Path; import java.text.SimpleDateFormat; import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collection; import java.util.Date; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.locks.Lock; import java.util.concurrent.locks.ReentrantLock; +import java.util.stream.Collectors; import com.google.common.cache.Cache; import com.google.common.cache.CacheBuilder; @@ -66,6 +66,7 @@ import org.opensearch.core.xcontent.MediaTypeRegistry; import org.opensearch.env.Environment; import org.opensearch.core.rest.RestStatus; +import org.opensearch.security.action.configupdate.ConfigUpdateRequest; import org.opensearch.security.auditlog.AuditLog; import org.opensearch.security.auditlog.config.AuditConfig; import org.opensearch.security.securityconf.DynamicConfigFactory; @@ -218,7 +219,7 @@ private ConfigurationRepository( while (!dynamicConfigFactory.isInitialized()) { try { LOGGER.debug("Try to load config ..."); - reloadConfiguration(Arrays.asList(CType.values())); + reloadConfiguration(new ConfigUpdateRequest(CType.values(), null)); break; } catch (Exception e) { LOGGER.debug("Unable to load configuration due to {}", String.valueOf(ExceptionUtils.getRootCause(e))); @@ -372,11 +373,11 @@ public SecurityDynamicConfiguration getConfiguration(CType configurationType) private final Lock LOCK = new ReentrantLock(); - public void reloadConfiguration(Collection configTypes) throws ConfigUpdateAlreadyInProgressException { + public void reloadConfiguration(final ConfigUpdateRequest updateRequest) throws ConfigUpdateAlreadyInProgressException { try { if (LOCK.tryLock(60, TimeUnit.SECONDS)) { try { - reloadConfiguration0(configTypes, this.acceptInvalid); + reloadConfiguration0(updateRequest, this.acceptInvalid); } finally { LOCK.unlock(); } @@ -389,8 +390,8 @@ public void reloadConfiguration(Collection configTypes) throws ConfigUpda } } - private void reloadConfiguration0(Collection configTypes, boolean acceptInvalid) { - final Map> loaded = getConfigurationsFromIndex(configTypes, false, acceptInvalid); + private void reloadConfiguration0(final ConfigUpdateRequest updateRequest, boolean acceptInvalid) { + final Map> loaded = getConfigurationsFromIndex(updateRequest, false, acceptInvalid); configCache.putAll(loaded); notifyAboutChanges(loaded); } @@ -417,21 +418,25 @@ private synchronized void notifyAboutChanges(Map> getConfigurationsFromIndex( - Collection configTypes, - boolean logComplianceEvent - ) { - return getConfigurationsFromIndex(configTypes, logComplianceEvent, this.acceptInvalid); + public SecurityDynamicConfiguration getConfigurationFromIndex(CType configType, boolean logComplianceEvent) { + return getConfigurationFromIndex(configType, logComplianceEvent, this.acceptInvalid); + } + + public SecurityDynamicConfiguration getConfigurationFromIndex(CType configType, boolean logComplianceEvent, boolean acceptInvalid) { + final ConfigUpdateRequest updateRequest = new ConfigUpdateRequest(new CType[] { configType }, null); + return getConfigurationsFromIndex(updateRequest, logComplianceEvent, acceptInvalid).get(configType); } public Map> getConfigurationsFromIndex( - Collection configTypes, - boolean logComplianceEvent, - boolean acceptInvalid + final ConfigUpdateRequest updateRequest, + final boolean logComplianceEvent, + final boolean acceptInvalid ) { final ThreadContext threadContext = threadPool.getThreadContext(); final Map> retVal = new HashMap<>(); + final Map typeAndSequenceIdMap = updateRequest.getTypeAndSequenceIdMap(); + final CType[] configTypes = typeAndSequenceIdMap.keySet().toArray(new CType[0]); try (StoredContext ctx = threadContext.stashContext()) { threadContext.putHeader(ConfigConstants.OPENDISTRO_SECURITY_CONF_REQUEST_HEADER, "true"); @@ -445,25 +450,28 @@ public Map> getConfigurationsFromIndex( } else { LOGGER.debug("security index exists and was created with ES 7 (new layout)"); } - retVal.putAll( - validate(cl.load(configTypes.toArray(new CType[0]), 10, TimeUnit.SECONDS, acceptInvalid), configTypes.size()) - ); + retVal.putAll(validate(cl.load(configTypes, 10, TimeUnit.SECONDS, acceptInvalid), typeAndSequenceIdMap)); } else { // wait (and use new layout) LOGGER.debug("security index not exists (yet)"); - retVal.putAll( - validate(cl.load(configTypes.toArray(new CType[0]), 10, TimeUnit.SECONDS, acceptInvalid), configTypes.size()) - ); + retVal.putAll(validate(cl.load(configTypes, 10, TimeUnit.SECONDS, acceptInvalid), typeAndSequenceIdMap)); } } catch (Exception e) { throw new OpenSearchException(e); } + // TODO BEFORE-MERGE: need to make sure this is unit tested/validated if (logComplianceEvent && auditLog.getComplianceConfig().isEnabled()) { - CType configurationType = configTypes.iterator().next(); - Map fields = new HashMap(); + if (typeAndSequenceIdMap.size() != 1) { + throw new RuntimeException("Unexpected configuration log event that is missing critical details"); + } + final CType configurationType = typeAndSequenceIdMap.keySet() + .stream() + .findFirst() + .orElseThrow(() -> new RuntimeException("Loaded configuation without expected CType!")); + final Map fields = new HashMap(); fields.put(configurationType.toLCString(), Strings.toString(MediaTypeRegistry.JSON, retVal.get(configurationType))); auditLog.logDocumentRead(this.securityIndex, configurationType.toLCString(), null, fields); } @@ -471,13 +479,36 @@ public Map> getConfigurationsFromIndex( return retVal; } - private Map> validate(Map> conf, int expectedSize) - throws InvalidConfigException { + private Map> validate( + Map> conf, + Map expectedSequences + ) throws InvalidConfigException { - if (conf == null || conf.size() != expectedSize) { + if (conf == null || conf.size() != expectedSequences.size()) { throw new InvalidConfigException("Retrieved only partial configuration"); } + // TODO BEFORE-MERGE: Need unit tests + final List configurationSeqIssues = expectedSequences.entrySet().stream().map(kvp -> { + if (kvp.getValue() != null) { + final SecurityDynamicConfiguration configEntry = conf.get(kvp.getKey()); + if (configEntry.getSeqNo() > kvp.getValue()) { + return "Configuration " + + kvp.getKey() + + ", sequence number (" + + configEntry.getSeqNo() + + ") was lower than expected (" + + kvp.getValue() + + ")"; + } + } + return null; + }).filter(Objects::nonNull).collect(Collectors.toList()); + + if (!configurationSeqIssues.isEmpty()) { + throw new InvalidConfigException(configurationSeqIssues.stream().collect(Collectors.joining("\n"))); + } + return conf; } diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java index 6d3710871b..833a87eebc 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java +++ b/src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java @@ -57,7 +57,6 @@ import org.opensearch.threadpool.ThreadPool; import java.io.IOException; -import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Optional; @@ -430,8 +429,7 @@ public RequestContentValidator createRequestContentValidator(Object... params) { protected final SecurityDynamicConfiguration load(final CType config, boolean logComplianceEvent) { SecurityDynamicConfiguration loaded = securityApiDependencies.configurationRepository() - .getConfigurationsFromIndex(List.of(config), logComplianceEvent) - .get(config) + .getConfigurationFromIndex(config, logComplianceEvent) .deepClone(); return DynamicConfigFactory.addStatics(loaded); } @@ -479,7 +477,7 @@ public static void saveAndUpdateConfigs( .setIfSeqNo(configuration.getSeqNo()) .setIfPrimaryTerm(configuration.getPrimaryTerm()) .source(id, XContentHelper.toXContent(configuration, XContentType.JSON, false)), - new ConfigUpdatingActionListener<>(new String[] { id }, client, actionListener) + new ConfigUpdatingActionListener<>(Map.of(cType, Long.valueOf(configuration.getSeqNo())), client, actionListener) ); } catch (IOException e) { throw ExceptionsHelper.convertToOpenSearchException(e); @@ -487,12 +485,12 @@ public static void saveAndUpdateConfigs( } protected static class ConfigUpdatingActionListener implements ActionListener { - private final String[] cTypes; + private final Map typesAndSequenceIds; private final Client client; private final ActionListener delegate; - public ConfigUpdatingActionListener(String[] cTypes, Client client, ActionListener delegate) { - this.cTypes = Objects.requireNonNull(cTypes, "cTypes must not be null"); + public ConfigUpdatingActionListener(Map typesAndSequenceIds, Client client, ActionListener delegate) { + this.typesAndSequenceIds = Objects.requireNonNull(typesAndSequenceIds, "cTypes must not be null"); this.client = Objects.requireNonNull(client, "client must not be null"); this.delegate = Objects.requireNonNull(delegate, "delegate must not be null"); } @@ -500,7 +498,10 @@ public ConfigUpdatingActionListener(String[] cTypes, Client client, ActionListen @Override public void onResponse(Response response) { - final ConfigUpdateRequest cur = new ConfigUpdateRequest(cTypes); + final ConfigUpdateRequest cur = new ConfigUpdateRequest( + typesAndSequenceIds.keySet().toArray(new CType[0]), + typesAndSequenceIds.values().toArray(new Long[0]) + ); client.execute(ConfigUpdateAction.INSTANCE, cur, new ActionListener() { @Override diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/FlushCacheApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/FlushCacheApiAction.java index 640e52df6e..2b3cc1a678 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/api/FlushCacheApiAction.java +++ b/src/main/java/org/opensearch/security/dlic/rest/api/FlushCacheApiAction.java @@ -65,7 +65,7 @@ private void flushCacheApiRequestHandlers(RequestHandler.RequestHandlersBuilder Method.DELETE, (channel, request, client) -> client.execute( ConfigUpdateAction.INSTANCE, - new ConfigUpdateRequest(CType.lcStringValues().toArray(new String[0])), + new ConfigUpdateRequest(CType.values(), null), new ActionListener<>() { @Override diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/MigrateApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/MigrateApiAction.java index 1f3871c4ca..83a01f2b9a 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/api/MigrateApiAction.java +++ b/src/main/java/org/opensearch/security/dlic/rest/api/MigrateApiAction.java @@ -14,6 +14,8 @@ // CS-SUPPRESS-SINGLE: RegexpSingleline https://github.com/opensearch-project/OpenSearch/issues/3663 import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; + import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.opensearch.action.admin.indices.create.CreateIndexResponse; @@ -190,7 +192,7 @@ public void onResponse(AcknowledgedResponse response) { @Override public void onResponse(CreateIndexResponse response) { final List> dynamicConfigurations = builder.build(); - final ImmutableList.Builder cTypes = ImmutableList.builderWithExpectedSize( + final ImmutableMap.Builder cTypes = ImmutableMap.builderWithExpectedSize( dynamicConfigurations.size() ); final BulkRequestBuilder br = client.prepareBulk(securityApiDependencies.securityIndexName()); @@ -204,7 +206,7 @@ public void onResponse(CreateIndexResponse response) { false ); br.add(new IndexRequest().id(id).source(id, xContent)); - cTypes.add(id); + cTypes.put(dynamicConfiguration.getCType(), Long.valueOf(dynamicConfiguration.getSeqNo())); } } catch (final IOException e1) { LOGGER.error("Unable to create bulk request " + e1, e1); @@ -212,38 +214,28 @@ public void onResponse(CreateIndexResponse response) { return; } - br.execute( - new ConfigUpdatingActionListener( - cTypes.build().toArray(new String[0]), - client, - new ActionListener() { - - @Override - public void onResponse(BulkResponse response) { - if (response.hasFailures()) { - LOGGER.error( - "Unable to upload migrated configuration because of " - + response.buildFailureMessage() - ); - internalSeverError( - channel, - "Unable to upload migrated configuration (bulk index failed)." - ); - } else { - LOGGER.debug("Migration completed"); - ok(channel, "Migration completed."); - } - - } - - @Override - public void onFailure(Exception e) { - LOGGER.error("Unable to upload migrated configuration because of " + e, e); - internalSeverError(channel, "Unable to upload migrated configuration."); - } + br.execute(new ConfigUpdatingActionListener(cTypes.build(), client, new ActionListener() { + + @Override + public void onResponse(BulkResponse response) { + if (response.hasFailures()) { + LOGGER.error( + "Unable to upload migrated configuration because of " + response.buildFailureMessage() + ); + internalSeverError(channel, "Unable to upload migrated configuration (bulk index failed)."); + } else { + LOGGER.debug("Migration completed"); + ok(channel, "Migration completed."); } - ) - ); + + } + + @Override + public void onFailure(Exception e) { + LOGGER.error("Unable to upload migrated configuration because of " + e, e); + internalSeverError(channel, "Unable to upload migrated configuration."); + } + })); } diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/ValidateApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/ValidateApiAction.java index 8f764e94c3..5e8635a3a5 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/api/ValidateApiAction.java +++ b/src/main/java/org/opensearch/security/dlic/rest/api/ValidateApiAction.java @@ -131,8 +131,7 @@ protected void validate(RestChannel channel, RestRequest request) throws IOExcep private SecurityDynamicConfiguration load(final CType config, boolean logComplianceEvent, boolean acceptInvalid) { SecurityDynamicConfiguration loaded = securityApiDependencies.configurationRepository() - .getConfigurationsFromIndex(Collections.singleton(config), logComplianceEvent, acceptInvalid) - .get(config) + .getConfigurationFromIndex(config, logComplianceEvent, acceptInvalid) .deepClone(); return DynamicConfigFactory.addStatics(loaded); } diff --git a/src/main/java/org/opensearch/security/rest/SecurityConfigUpdateAction.java b/src/main/java/org/opensearch/security/rest/SecurityConfigUpdateAction.java index c582c9f51b..c9594fcde1 100644 --- a/src/main/java/org/opensearch/security/rest/SecurityConfigUpdateAction.java +++ b/src/main/java/org/opensearch/security/rest/SecurityConfigUpdateAction.java @@ -29,6 +29,7 @@ import org.opensearch.security.action.configupdate.ConfigUpdateAction; import org.opensearch.security.action.configupdate.ConfigUpdateRequest; import org.opensearch.security.configuration.AdminDNs; +import org.opensearch.security.securityconf.impl.CType; import org.opensearch.security.ssl.transport.PrincipalExtractor; import org.opensearch.security.ssl.util.SSLRequestHelper; import org.opensearch.security.support.ConfigConstants; @@ -71,7 +72,8 @@ public List routes() { @Override protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { - String[] configTypes = request.paramAsStringArrayOrEmptyIfAll("config_types"); + final String[] rawConfigTypes = request.paramAsStringArrayOrEmptyIfAll("config_types"); + final CType[] configTypes = CType.fromStringValues(rawConfigTypes); SSLRequestHelper.SSLInfo sslInfo = SSLRequestHelper.getSSLInfo(settings, configPath, request, principalExtractor); @@ -85,7 +87,7 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli if (user == null || !adminDns.isAdmin(user)) { return channel -> channel.sendResponse(new BytesRestResponse(RestStatus.FORBIDDEN, "")); } else { - ConfigUpdateRequest configUpdateRequest = new ConfigUpdateRequest(configTypes); + ConfigUpdateRequest configUpdateRequest = new ConfigUpdateRequest(configTypes, null); return channel -> { client.execute(ConfigUpdateAction.INSTANCE, configUpdateRequest, new NodesResponseRestListener<>(channel)); }; diff --git a/src/main/java/org/opensearch/security/rest/TenantInfoAction.java b/src/main/java/org/opensearch/security/rest/TenantInfoAction.java index f3afc0f006..c5bf856b05 100644 --- a/src/main/java/org/opensearch/security/rest/TenantInfoAction.java +++ b/src/main/java/org/opensearch/security/rest/TenantInfoAction.java @@ -27,7 +27,6 @@ package org.opensearch.security.rest; import java.io.IOException; -import java.util.Collections; import java.util.List; import java.util.SortedMap; @@ -178,10 +177,7 @@ private boolean isAuthorized() { } private final SecurityDynamicConfiguration load(final CType config, boolean logComplianceEvent) { - SecurityDynamicConfiguration loaded = configurationRepository.getConfigurationsFromIndex( - Collections.singleton(config), - logComplianceEvent - ).get(config).deepClone(); + SecurityDynamicConfiguration loaded = configurationRepository.getConfigurationFromIndex(config, logComplianceEvent).deepClone(); return DynamicConfigFactory.addStatics(loaded); } diff --git a/src/main/java/org/opensearch/security/securityconf/impl/CType.java b/src/main/java/org/opensearch/security/securityconf/impl/CType.java index 4e5e2de496..3b36744c65 100644 --- a/src/main/java/org/opensearch/security/securityconf/impl/CType.java +++ b/src/main/java/org/opensearch/security/securityconf/impl/CType.java @@ -83,8 +83,8 @@ public static Set lcStringValues() { return Arrays.stream(CType.values()).map(c -> c.toLCString()).collect(Collectors.toSet()); } - public static Set fromStringValues(String[] strings) { - return Arrays.stream(strings).map(c -> CType.fromString(c)).collect(Collectors.toSet()); + public static CType[] fromStringValues(String[] strings) { + return Arrays.stream(strings).map(c -> CType.fromString(c)).collect(Collectors.toList()).toArray(new CType[0]); } private static Map> toMap(Object... objects) { diff --git a/src/main/java/org/opensearch/security/user/UserService.java b/src/main/java/org/opensearch/security/user/UserService.java index bf2e3e0273..a2d52de407 100644 --- a/src/main/java/org/opensearch/security/user/UserService.java +++ b/src/main/java/org/opensearch/security/user/UserService.java @@ -15,7 +15,6 @@ import java.nio.charset.StandardCharsets; import java.util.Arrays; import java.util.Base64; -import java.util.Collections; import java.util.List; import java.util.Optional; import java.util.Random; @@ -103,10 +102,7 @@ public UserService(ClusterService clusterService, ConfigurationRepository config * @return configuration loaded with given CType data */ protected static final SecurityDynamicConfiguration load(final CType config, boolean logComplianceEvent) { - SecurityDynamicConfiguration loaded = configurationRepository.getConfigurationsFromIndex( - Collections.singleton(config), - logComplianceEvent - ).get(config).deepClone(); + SecurityDynamicConfiguration loaded = configurationRepository.getConfigurationFromIndex(config, logComplianceEvent).deepClone(); return DynamicConfigFactory.addStatics(loaded); } diff --git a/src/test/java/org/opensearch/security/HttpIntegrationTests.java b/src/test/java/org/opensearch/security/HttpIntegrationTests.java index 94994875d8..06615f088e 100644 --- a/src/test/java/org/opensearch/security/HttpIntegrationTests.java +++ b/src/test/java/org/opensearch/security/HttpIntegrationTests.java @@ -44,6 +44,7 @@ import org.opensearch.security.action.configupdate.ConfigUpdateAction; import org.opensearch.security.action.configupdate.ConfigUpdateRequest; import org.opensearch.security.action.configupdate.ConfigUpdateResponse; +import org.opensearch.security.securityconf.impl.CType; import org.opensearch.security.ssl.util.SSLConfigConstants; import org.opensearch.security.support.ConfigConstants; import org.opensearch.security.test.DynamicSecurityConfig; @@ -287,7 +288,7 @@ public void testHTTPBasic() throws Exception { .setRefreshPolicy(RefreshPolicy.IMMEDIATE) .source("roles", FileHelper.readYamlContent("roles_deny.yml")) ).actionGet(); - ConfigUpdateResponse cur = tc.execute(ConfigUpdateAction.INSTANCE, new ConfigUpdateRequest(new String[] { "roles" })) + ConfigUpdateResponse cur = tc.execute(ConfigUpdateAction.INSTANCE, new ConfigUpdateRequest(new CType[] { CType.ROLES }, null)) .actionGet(); Assert.assertEquals(clusterInfo.numNodes, cur.getNodes().size()); } @@ -303,7 +304,7 @@ public void testHTTPBasic() throws Exception { .setRefreshPolicy(RefreshPolicy.IMMEDIATE) .source("roles", FileHelper.readYamlContent("roles.yml")) ).actionGet(); - ConfigUpdateResponse cur = tc.execute(ConfigUpdateAction.INSTANCE, new ConfigUpdateRequest(new String[] { "roles" })) + ConfigUpdateResponse cur = tc.execute(ConfigUpdateAction.INSTANCE, new ConfigUpdateRequest(new CType[] { CType.ROLES }, null)) .actionGet(); Assert.assertEquals(clusterInfo.numNodes, cur.getNodes().size()); } @@ -495,7 +496,10 @@ public void testHTTPAnon() throws Exception { ).actionGet(); ConfigUpdateResponse cur = tc.execute( ConfigUpdateAction.INSTANCE, - new ConfigUpdateRequest(new String[] { "config", "roles", "rolesmapping", "internalusers", "actiongroups" }) + new ConfigUpdateRequest( + new CType[] { CType.CONFIG, CType.ROLES, CType.ROLESMAPPING, CType.INTERNALUSERS, CType.ACTIONGROUPS }, + null + ) ).actionGet(); Assert.assertFalse(cur.hasFailures()); Assert.assertEquals(clusterInfo.numNodes, cur.getNodes().size()); @@ -529,7 +533,10 @@ public void testHTTPClientCert() throws Exception { ConfigUpdateResponse cur = tc.execute( ConfigUpdateAction.INSTANCE, - new ConfigUpdateRequest(new String[] { "config", "roles", "rolesmapping", "internalusers", "actiongroups" }) + new ConfigUpdateRequest( + new CType[] { CType.CONFIG, CType.ROLES, CType.ROLESMAPPING, CType.INTERNALUSERS, CType.ACTIONGROUPS }, + null + ) ).actionGet(); Assert.assertFalse(cur.hasFailures()); Assert.assertEquals(clusterInfo.numNodes, cur.getNodes().size()); diff --git a/src/test/java/org/opensearch/security/InitializationIntegrationTests.java b/src/test/java/org/opensearch/security/InitializationIntegrationTests.java index d6306e7f5d..fc6544aa4f 100644 --- a/src/test/java/org/opensearch/security/InitializationIntegrationTests.java +++ b/src/test/java/org/opensearch/security/InitializationIntegrationTests.java @@ -52,6 +52,7 @@ import org.opensearch.security.action.configupdate.ConfigUpdateAction; import org.opensearch.security.action.configupdate.ConfigUpdateRequest; import org.opensearch.security.action.configupdate.ConfigUpdateResponse; +import org.opensearch.security.securityconf.impl.CType; import org.opensearch.security.ssl.util.SSLConfigConstants; import org.opensearch.security.support.ConfigConstants; import org.opensearch.security.test.DynamicSecurityConfig; @@ -218,7 +219,10 @@ public void testConfigHotReload() throws Exception { ).actionGet(); ConfigUpdateResponse cur = tc.execute( ConfigUpdateAction.INSTANCE, - new ConfigUpdateRequest(new String[] { "config", "roles", "rolesmapping", "internalusers", "actiongroups" }) + new ConfigUpdateRequest( + new CType[] { CType.CONFIG, CType.ROLES, CType.ROLESMAPPING, CType.INTERNALUSERS, CType.ACTIONGROUPS }, + null + ) ).actionGet(); Assert.assertEquals(clusterInfo.numNodes, cur.getNodes().size()); } @@ -250,7 +254,7 @@ public void testConfigHotReload() throws Exception { .id("config") .source("config", FileHelper.readYamlContent("config_anon.yml")) ).actionGet(); - ConfigUpdateResponse cur = tc.execute(ConfigUpdateAction.INSTANCE, new ConfigUpdateRequest(new String[] { "config" })) + ConfigUpdateResponse cur = tc.execute(ConfigUpdateAction.INSTANCE, new ConfigUpdateRequest(new CType[] { CType.CONFIG }, null)) .actionGet(); Assert.assertEquals(clusterInfo.numNodes, cur.getNodes().size()); } diff --git a/src/test/java/org/opensearch/security/IntegrationTests.java b/src/test/java/org/opensearch/security/IntegrationTests.java index 4c1086ef4f..0d33a9be39 100644 --- a/src/test/java/org/opensearch/security/IntegrationTests.java +++ b/src/test/java/org/opensearch/security/IntegrationTests.java @@ -49,6 +49,7 @@ import org.opensearch.security.action.configupdate.ConfigUpdateRequest; import org.opensearch.security.action.configupdate.ConfigUpdateResponse; import org.opensearch.security.http.HTTPClientCertAuthenticator; +import org.opensearch.security.securityconf.impl.CType; import org.opensearch.security.ssl.util.SSLConfigConstants; import org.opensearch.security.support.ConfigConstants; import org.opensearch.security.test.DynamicSecurityConfig; @@ -308,7 +309,10 @@ public void testSingle() throws Exception { ConfigUpdateResponse cur = tc.execute( ConfigUpdateAction.INSTANCE, - new ConfigUpdateRequest(new String[] { "config", "roles", "rolesmapping", "internalusers", "actiongroups" }) + new ConfigUpdateRequest( + new CType[] { CType.CONFIG, CType.ROLES, CType.ROLESMAPPING, CType.INTERNALUSERS, CType.ACTIONGROUPS }, + null + ) ).actionGet(); Assert.assertEquals(clusterInfo.numNodes, cur.getNodes().size()); } @@ -423,7 +427,7 @@ public void testMultiRoleSpan() throws Exception { .source("config", FileHelper.readYamlContent("config_multirolespan.yml")) ).actionGet(); - ConfigUpdateResponse cur = tc.execute(ConfigUpdateAction.INSTANCE, new ConfigUpdateRequest(new String[] { "config" })) + ConfigUpdateResponse cur = tc.execute(ConfigUpdateAction.INSTANCE, new ConfigUpdateRequest(new CType[] { CType.CONFIG }, null)) .actionGet(); Assert.assertEquals(clusterInfo.numNodes, cur.getNodes().size()); } diff --git a/src/test/java/org/opensearch/security/SnapshotRestoreTests.java b/src/test/java/org/opensearch/security/SnapshotRestoreTests.java index 1c884a8e5d..c8dc58f11e 100644 --- a/src/test/java/org/opensearch/security/SnapshotRestoreTests.java +++ b/src/test/java/org/opensearch/security/SnapshotRestoreTests.java @@ -45,6 +45,7 @@ import org.opensearch.security.action.configupdate.ConfigUpdateAction; import org.opensearch.security.action.configupdate.ConfigUpdateRequest; import org.opensearch.security.action.configupdate.ConfigUpdateResponse; +import org.opensearch.security.securityconf.impl.CType; import org.opensearch.security.test.DynamicSecurityConfig; import org.opensearch.security.test.SingleClusterTest; import org.opensearch.security.test.helper.cluster.ClusterConfiguration; @@ -492,7 +493,10 @@ public void testSnapshotCheckWritePrivileges() throws Exception { ConfigUpdateResponse cur = tc.execute( ConfigUpdateAction.INSTANCE, - new ConfigUpdateRequest(new String[] { "config", "roles", "rolesmapping", "internalusers", "actiongroups" }) + new ConfigUpdateRequest( + new CType[] { CType.CONFIG, CType.ROLES, CType.ROLESMAPPING, CType.INTERNALUSERS, CType.ACTIONGROUPS }, + null + ) ).actionGet(); Assert.assertFalse(cur.hasFailures()); Assert.assertEquals(currentClusterConfig.getNodes(), cur.getNodes().size()); diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/AbstractApiActionValidationTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/AbstractApiActionValidationTest.java index 5336b5e8de..beb7add73c 100644 --- a/src/test/java/org/opensearch/security/dlic/rest/api/AbstractApiActionValidationTest.java +++ b/src/test/java/org/opensearch/security/dlic/rest/api/AbstractApiActionValidationTest.java @@ -31,10 +31,9 @@ import java.io.IOException; import java.util.List; -import java.util.Map; import static org.junit.Assert.assertEquals; -import static org.mockito.Mockito.when; +import static org.mockito.Mockito.doReturn; @RunWith(MockitoJUnitRunner.class) public abstract class AbstractApiActionValidationTest { @@ -87,9 +86,7 @@ void setupRolesConfiguration() throws IOException { config.set("regular_role", objectMapper.createObjectNode().set("cluster_permissions", objectMapper.createArrayNode().add("*"))); rolesConfiguration = SecurityDynamicConfiguration.fromJson(objectMapper.writeValueAsString(config), CType.ROLES, 2, 1, 1); - when(configurationRepository.getConfigurationsFromIndex(List.of(CType.ROLES), false)).thenReturn( - Map.of(CType.ROLES, rolesConfiguration) - ); + doReturn(rolesConfiguration).when(configurationRepository).getConfigurationFromIndex(CType.ROLES, false); } @Test diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/RolesMappingApiActionValidationTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/RolesMappingApiActionValidationTest.java index bbcc5021ff..c70aca6ffe 100644 --- a/src/test/java/org/opensearch/security/dlic/rest/api/RolesMappingApiActionValidationTest.java +++ b/src/test/java/org/opensearch/security/dlic/rest/api/RolesMappingApiActionValidationTest.java @@ -16,14 +16,12 @@ import org.opensearch.core.rest.RestStatus; import org.opensearch.security.securityconf.impl.CType; -import java.util.List; -import java.util.Map; - import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.when; +import static org.mockito.Mockito.doReturn; public class RolesMappingApiActionValidationTest extends AbstractApiActionValidationTest { @@ -62,8 +60,7 @@ public void isNotAllowedNoRightsToChangeRoleEntity() throws Exception { public void onConfigChangeShouldCheckRoles() throws Exception { when(restApiAdminPrivilegesEvaluator.isCurrentUserAdminFor(Endpoint.ROLESMAPPING)).thenReturn(false); when(restApiAdminPrivilegesEvaluator.containsRestApiAdminPermissions(any(Object.class))).thenCallRealMethod(); - when(configurationRepository.getConfigurationsFromIndex(List.of(CType.ROLES), false)) - .thenReturn(Map.of(CType.ROLES, rolesConfiguration)); + doReturn(rolesConfiguration).when(configurationRepository).getConfigurationFromIndex(CType.ROLES, false); final var rolesApiActionEndpointValidator = new RolesMappingApiAction(clusterService, threadPool, securityApiDependencies).createEndpointValidator(); diff --git a/src/test/java/org/opensearch/security/test/AbstractSecurityUnitTest.java b/src/test/java/org/opensearch/security/test/AbstractSecurityUnitTest.java index 6ca6c65d91..a9492a0d02 100644 --- a/src/test/java/org/opensearch/security/test/AbstractSecurityUnitTest.java +++ b/src/test/java/org/opensearch/security/test/AbstractSecurityUnitTest.java @@ -278,10 +278,7 @@ protected void initialize(ClusterHelper clusterHelper, ClusterInfo clusterInfo, tc.index(ir).actionGet(); } - ConfigUpdateResponse cur = tc.execute( - ConfigUpdateAction.INSTANCE, - new ConfigUpdateRequest(CType.lcStringValues().toArray(new String[0])) - ).actionGet(); + ConfigUpdateResponse cur = tc.execute(ConfigUpdateAction.INSTANCE, new ConfigUpdateRequest(CType.values(), null)).actionGet(); Assert.assertFalse(cur.failures().toString(), cur.hasFailures()); Assert.assertEquals(clusterInfo.numNodes, cur.getNodes().size());