From 2bcfe1a2d82d52d7d0b74071610875cdaf418f76 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Thu, 29 Aug 2019 21:25:20 -0400 Subject: [PATCH] Remove insecure settings (#46147) This commit removes the oxymoron of insecure secure settings from the code base. In particular, we remove the ability to set the access_key and secret_key for S3 repositories inside the repository definition (in the cluster state). Instead, these settings now must be in the keystore. Thus, it also removes some leniency where these settings could be placed in the elasticsearch.yml, would not be rejected there, but would not be consumed for any purpose. --- plugins/repository-s3/build.gradle | 9 +- .../repositories/s3/S3ClientSettings.java | 33 +---- .../repositories/s3/S3Repository.java | 16 --- .../repositories/s3/S3RepositoryPlugin.java | 4 +- .../plugin-metadata/plugin-security.policy | 4 +- .../s3/RepositoryCredentialsTests.java | 135 +----------------- .../common/settings/SecureSetting.java | 34 +---- 7 files changed, 13 insertions(+), 222 deletions(-) diff --git a/plugins/repository-s3/build.gradle b/plugins/repository-s3/build.gradle index 4b996e8ec504e..dad3aecbc10e6 100644 --- a/plugins/repository-s3/build.gradle +++ b/plugins/repository-s3/build.gradle @@ -65,15 +65,8 @@ bundlePlugin { } } -task testRepositoryCreds(type: Test) { - include '**/RepositoryCredentialsTests.class' - systemProperty 'es.allow_insecure_settings', 'true' -} -check.dependsOn(testRepositoryCreds) - test { - // these are tested explicitly in separate test tasks - exclude '**/RepositoryCredentialsTests.class' + // this is tested explicitly in separate test tasks exclude '**/S3RepositoryThirdPartyTests.class' } diff --git a/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3ClientSettings.java b/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3ClientSettings.java index fee00786a2ab3..843208078b0e1 100644 --- a/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3ClientSettings.java +++ b/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3ClientSettings.java @@ -182,20 +182,14 @@ S3ClientSettings refine(RepositoryMetaData metadata) { final boolean usePathStyleAccess = getRepoSettingOrDefault(USE_PATH_STYLE_ACCESS, normalizedSettings, pathStyleAccess); final boolean newDisableChunkedEncoding = getRepoSettingOrDefault( DISABLE_CHUNKED_ENCODING, normalizedSettings, disableChunkedEncoding); - final S3BasicCredentials newCredentials; - if (checkDeprecatedCredentials(repoSettings)) { - newCredentials = loadDeprecatedCredentials(repoSettings); - } else { - newCredentials = credentials; - } if (Objects.equals(endpoint, newEndpoint) && protocol == newProtocol && Objects.equals(proxyHost, newProxyHost) && proxyPort == newProxyPort && newReadTimeoutMillis == readTimeoutMillis && maxRetries == newMaxRetries - && newThrottleRetries == throttleRetries && Objects.equals(credentials, newCredentials) + && newThrottleRetries == throttleRetries && newDisableChunkedEncoding == disableChunkedEncoding) { return this; } return new S3ClientSettings( - newCredentials, + credentials, newEndpoint, newProtocol, newProxyHost, @@ -229,29 +223,6 @@ static Map load(Settings settings) { return Collections.unmodifiableMap(clients); } - static boolean checkDeprecatedCredentials(Settings repositorySettings) { - if (S3Repository.ACCESS_KEY_SETTING.exists(repositorySettings)) { - if (S3Repository.SECRET_KEY_SETTING.exists(repositorySettings) == false) { - throw new IllegalArgumentException("Repository setting [" + S3Repository.ACCESS_KEY_SETTING.getKey() - + " must be accompanied by setting [" + S3Repository.SECRET_KEY_SETTING.getKey() + "]"); - } - return true; - } else if (S3Repository.SECRET_KEY_SETTING.exists(repositorySettings)) { - throw new IllegalArgumentException("Repository setting [" + S3Repository.SECRET_KEY_SETTING.getKey() - + " must be accompanied by setting [" + S3Repository.ACCESS_KEY_SETTING.getKey() + "]"); - } - return false; - } - - // backcompat for reading keys out of repository settings (clusterState) - private static S3BasicCredentials loadDeprecatedCredentials(Settings repositorySettings) { - assert checkDeprecatedCredentials(repositorySettings); - try (SecureString key = S3Repository.ACCESS_KEY_SETTING.get(repositorySettings); - SecureString secret = S3Repository.SECRET_KEY_SETTING.get(repositorySettings)) { - return new S3BasicCredentials(key.toString(), secret.toString()); - } - } - private static S3BasicCredentials loadCredentials(Settings settings, String clientName) { try (SecureString accessKey = getConfigValue(settings, clientName, ACCESS_KEY_SETTING); SecureString secretKey = getConfigValue(settings, clientName, SECRET_KEY_SETTING); diff --git a/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Repository.java b/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Repository.java index 72ff277ba4161..a164900b635e7 100644 --- a/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Repository.java +++ b/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Repository.java @@ -25,9 +25,6 @@ import org.elasticsearch.common.Strings; import org.elasticsearch.common.blobstore.BlobPath; import org.elasticsearch.common.blobstore.BlobStore; -import org.elasticsearch.common.logging.DeprecationLogger; -import org.elasticsearch.common.settings.SecureSetting; -import org.elasticsearch.common.settings.SecureString; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.unit.ByteSizeUnit; import org.elasticsearch.common.unit.ByteSizeValue; @@ -54,16 +51,9 @@ */ class S3Repository extends BlobStoreRepository { private static final Logger logger = LogManager.getLogger(S3Repository.class); - private static final DeprecationLogger deprecationLogger = new DeprecationLogger(logger); static final String TYPE = "s3"; - /** The access key to authenticate with s3. This setting is insecure because cluster settings are stored in cluster state */ - static final Setting ACCESS_KEY_SETTING = SecureSetting.insecureString("access_key"); - - /** The secret key to authenticate with s3. This setting is insecure because cluster settings are stored in cluster state */ - static final Setting SECRET_KEY_SETTING = SecureSetting.insecureString("secret_key"); - /** * Default is to use 100MB (S3 defaults) for heaps above 2GB and 5% of * the available memory for smaller heaps. @@ -186,12 +176,6 @@ class S3Repository extends BlobStoreRepository { this.storageClass = STORAGE_CLASS_SETTING.get(metadata.settings()); this.cannedACL = CANNED_ACL_SETTING.get(metadata.settings()); - if (S3ClientSettings.checkDeprecatedCredentials(metadata.settings())) { - // provided repository settings - deprecationLogger.deprecated("Using s3 access/secret key from repository settings. Instead " - + "store these in named clients and the elasticsearch keystore for secure settings."); - } - logger.debug( "using bucket [{}], chunk_size [{}], server_side_encryption [{}], buffer_size [{}], cannedACL [{}], storageClass [{}]", bucket, diff --git a/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3RepositoryPlugin.java b/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3RepositoryPlugin.java index 4bdbf482aeb6c..461eb9e9592cf 100644 --- a/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3RepositoryPlugin.java +++ b/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3RepositoryPlugin.java @@ -105,9 +105,7 @@ public List> getSettings() { S3ClientSettings.READ_TIMEOUT_SETTING, S3ClientSettings.MAX_RETRIES_SETTING, S3ClientSettings.USE_THROTTLE_RETRIES_SETTING, - S3ClientSettings.USE_PATH_STYLE_ACCESS, - S3Repository.ACCESS_KEY_SETTING, - S3Repository.SECRET_KEY_SETTING); + S3ClientSettings.USE_PATH_STYLE_ACCESS); } @Override diff --git a/plugins/repository-s3/src/main/plugin-metadata/plugin-security.policy b/plugins/repository-s3/src/main/plugin-metadata/plugin-security.policy index 5fd69b4c2fc3f..206335431f897 100644 --- a/plugins/repository-s3/src/main/plugin-metadata/plugin-security.policy +++ b/plugins/repository-s3/src/main/plugin-metadata/plugin-security.policy @@ -18,6 +18,7 @@ */ grant { + // needed because of problems in ClientConfiguration // TODO: get these fixed in aws sdk permission java.lang.RuntimePermission "accessDeclaredMembers"; @@ -38,6 +39,5 @@ grant { // s3 client opens socket connections for to access repository permission java.net.SocketPermission "*", "connect"; - // only for tests : org.elasticsearch.repositories.s3.S3RepositoryPlugin - permission java.util.PropertyPermission "es.allow_insecure_settings", "read,write"; + }; diff --git a/plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/RepositoryCredentialsTests.java b/plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/RepositoryCredentialsTests.java index 6695d3f2d61b3..f7d5ba021e25c 100644 --- a/plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/RepositoryCredentialsTests.java +++ b/plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/RepositoryCredentialsTests.java @@ -24,53 +24,28 @@ import com.amazonaws.services.s3.AmazonS3; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; -import org.elasticsearch.client.node.NodeClient; import org.elasticsearch.cluster.metadata.RepositoryMetaData; -import org.elasticsearch.common.SuppressForbidden; import org.elasticsearch.common.settings.MockSecureSettings; import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.common.settings.SettingsFilter; import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.plugins.Plugin; import org.elasticsearch.plugins.PluginsService; import org.elasticsearch.repositories.RepositoriesService; -import org.elasticsearch.rest.AbstractRestChannel; -import org.elasticsearch.rest.RestController; -import org.elasticsearch.rest.RestRequest; -import org.elasticsearch.rest.RestResponse; -import org.elasticsearch.rest.action.admin.cluster.RestGetRepositoriesAction; import org.elasticsearch.test.ESSingleNodeTestCase; -import org.elasticsearch.test.rest.FakeRestRequest; import org.elasticsearch.threadpool.ThreadPool; -import java.security.AccessController; -import java.security.PrivilegedAction; import java.util.Collection; import java.util.List; -import java.util.concurrent.CountDownLatch; -import java.util.concurrent.atomic.AtomicReference; import static org.elasticsearch.repositories.s3.S3ClientSettings.ACCESS_KEY_SETTING; import static org.elasticsearch.repositories.s3.S3ClientSettings.SECRET_KEY_SETTING; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; -import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; -import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.notNullValue; -import static org.mockito.Mockito.mock; -@SuppressForbidden(reason = "test requires to set a System property to allow insecure settings when running in IDE") public class RepositoryCredentialsTests extends ESSingleNodeTestCase { - static { - AccessController.doPrivileged((PrivilegedAction) () -> { - // required for client settings overwriting when running in IDE - System.setProperty("es.allow_insecure_settings", "true"); - return null; - }); - } - @Override protected Collection> getPlugins() { return List.of(ProxyS3RepositoryPlugin.class); @@ -95,52 +70,11 @@ protected Settings nodeSettings() { .build(); } - public void testRepositoryCredentialsOverrideSecureCredentials() { - final String repositoryName = "repo-creds-override"; - final Settings.Builder repositorySettings = Settings.builder() - // repository settings for credentials override node secure settings - .put(S3Repository.ACCESS_KEY_SETTING.getKey(), "insecure_aws_key") - .put(S3Repository.SECRET_KEY_SETTING.getKey(), "insecure_aws_secret"); - - final String clientName = randomFrom("default", "other", null); - if (clientName != null) { - repositorySettings.put(S3Repository.CLIENT_NAME.getKey(), clientName); - } - createRepository(repositoryName, repositorySettings.build()); - - final RepositoriesService repositories = getInstanceFromNode(RepositoriesService.class); - assertThat(repositories.repository(repositoryName), notNullValue()); - assertThat(repositories.repository(repositoryName), instanceOf(S3Repository.class)); - - final S3Repository repository = (S3Repository) repositories.repository(repositoryName); - final AmazonS3 client = repository.createBlobStore().clientReference().client(); - assertThat(client, instanceOf(ProxyS3RepositoryPlugin.ClientAndCredentials.class)); - - final AWSCredentials credentials = ((ProxyS3RepositoryPlugin.ClientAndCredentials) client).credentials.getCredentials(); - assertThat(credentials.getAWSAccessKeyId(), is("insecure_aws_key")); - assertThat(credentials.getAWSSecretKey(), is("insecure_aws_secret")); - - assertWarnings( - "[secret_key] setting was deprecated in Elasticsearch and will be removed in a future release!" - + " See the breaking changes documentation for the next major version.", - "Using s3 access/secret key from repository settings. Instead store these in named clients and" - + " the elasticsearch keystore for secure settings.", - "[access_key] setting was deprecated in Elasticsearch and will be removed in a future release!" - + " See the breaking changes documentation for the next major version."); - } - public void testReinitSecureCredentials() { final String clientName = randomFrom("default", "other"); final Settings.Builder repositorySettings = Settings.builder(); - final boolean hasInsecureSettings = randomBoolean(); - if (hasInsecureSettings) { - // repository settings for credentials override node secure settings - repositorySettings.put(S3Repository.ACCESS_KEY_SETTING.getKey(), "insecure_aws_key"); - repositorySettings.put(S3Repository.SECRET_KEY_SETTING.getKey(), "insecure_aws_secret"); - } else { - repositorySettings.put(S3Repository.CLIENT_NAME.getKey(), clientName); - } + repositorySettings.put(S3Repository.CLIENT_NAME.getKey(), clientName); final String repositoryName = "repo-reinit-creds"; createRepository(repositoryName, repositorySettings.build()); @@ -155,10 +89,7 @@ public void testReinitSecureCredentials() { assertThat(client, instanceOf(ProxyS3RepositoryPlugin.ClientAndCredentials.class)); final AWSCredentials credentials = ((ProxyS3RepositoryPlugin.ClientAndCredentials) client).credentials.getCredentials(); - if (hasInsecureSettings) { - assertThat(credentials.getAWSAccessKeyId(), is("insecure_aws_key")); - assertThat(credentials.getAWSSecretKey(), is("insecure_aws_secret")); - } else if ("other".equals(clientName)) { + if ("other".equals(clientName)) { assertThat(credentials.getAWSAccessKeyId(), is("secure_other_key")); assertThat(credentials.getAWSSecretKey(), is("secure_other_secret")); } else { @@ -177,10 +108,7 @@ public void testReinitSecureCredentials() { plugin.reload(newSettings); // check the not-yet-closed client reference still has the same credentials - if (hasInsecureSettings) { - assertThat(credentials.getAWSAccessKeyId(), is("insecure_aws_key")); - assertThat(credentials.getAWSSecretKey(), is("insecure_aws_secret")); - } else if ("other".equals(clientName)) { + if ("other".equals(clientName)) { assertThat(credentials.getAWSAccessKeyId(), is("secure_other_key")); assertThat(credentials.getAWSSecretKey(), is("secure_other_secret")); } else { @@ -195,64 +123,11 @@ public void testReinitSecureCredentials() { assertThat(client, instanceOf(ProxyS3RepositoryPlugin.ClientAndCredentials.class)); final AWSCredentials newCredentials = ((ProxyS3RepositoryPlugin.ClientAndCredentials) client).credentials.getCredentials(); - if (hasInsecureSettings) { - assertThat(newCredentials.getAWSAccessKeyId(), is("insecure_aws_key")); - assertThat(newCredentials.getAWSSecretKey(), is("insecure_aws_secret")); - } else { - assertThat(newCredentials.getAWSAccessKeyId(), is("new_secret_aws_key")); - assertThat(newCredentials.getAWSSecretKey(), is("new_secret_aws_secret")); - } - } - - if (hasInsecureSettings) { - assertWarnings( - "[secret_key] setting was deprecated in Elasticsearch and will be removed in a future release!" - + " See the breaking changes documentation for the next major version.", - "Using s3 access/secret key from repository settings. Instead store these in named clients and" - + " the elasticsearch keystore for secure settings.", - "[access_key] setting was deprecated in Elasticsearch and will be removed in a future release!" - + " See the breaking changes documentation for the next major version."); + assertThat(newCredentials.getAWSAccessKeyId(), is("new_secret_aws_key")); + assertThat(newCredentials.getAWSSecretKey(), is("new_secret_aws_secret")); } } - public void testInsecureRepositoryCredentials() throws Exception { - final String repositoryName = "repo-insecure-creds"; - createRepository(repositoryName, Settings.builder() - .put(S3Repository.ACCESS_KEY_SETTING.getKey(), "insecure_aws_key") - .put(S3Repository.SECRET_KEY_SETTING.getKey(), "insecure_aws_secret") - .build()); - - final RestRequest fakeRestRequest = new FakeRestRequest(); - fakeRestRequest.params().put("repository", repositoryName); - final RestGetRepositoriesAction action = - new RestGetRepositoriesAction(mock(RestController.class), getInstanceFromNode(SettingsFilter.class)); - - final CountDownLatch latch = new CountDownLatch(1); - final AtomicReference error = new AtomicReference<>(); - action.handleRequest(fakeRestRequest, new AbstractRestChannel(fakeRestRequest, true) { - @Override - public void sendResponse(RestResponse response) { - try { - String responseAsString = response.content().utf8ToString(); - assertThat(responseAsString, containsString(repositoryName)); - assertThat(responseAsString, not(containsString("insecure_"))); - } catch (final AssertionError ex) { - error.set(ex); - } - latch.countDown(); - } - }, getInstanceFromNode(NodeClient.class)); - - latch.await(); - if (error.get() != null) { - throw error.get(); - } - - assertWarnings( - "Using s3 access/secret key from repository settings. Instead store these in named clients and" - + " the elasticsearch keystore for secure settings."); - } - private void createRepository(final String name, final Settings repositorySettings) { assertAcked(client().admin().cluster().preparePutRepository(name) .setType(S3Repository.TYPE) diff --git a/server/src/main/java/org/elasticsearch/common/settings/SecureSetting.java b/server/src/main/java/org/elasticsearch/common/settings/SecureSetting.java index e022e4e3760a5..9f902febc7730 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/SecureSetting.java +++ b/server/src/main/java/org/elasticsearch/common/settings/SecureSetting.java @@ -19,14 +19,13 @@ package org.elasticsearch.common.settings; +import org.elasticsearch.common.util.ArrayUtils; + import java.io.InputStream; import java.security.GeneralSecurityException; import java.util.EnumSet; import java.util.Set; -import org.elasticsearch.common.Booleans; -import org.elasticsearch.common.util.ArrayUtils; - /** * A secure setting. * @@ -34,9 +33,6 @@ */ public abstract class SecureSetting extends Setting { - /** Determines whether legacy settings with sensitive values should be allowed. */ - private static final boolean ALLOW_INSECURE_SETTINGS = Booleans.parseBoolean(System.getProperty("es.allow_insecure_settings", "false")); - private static final Set ALLOWED_PROPERTIES = EnumSet.of(Property.Deprecated, Property.Consistent); private static final Property[] FIXED_PROPERTIES = { @@ -139,14 +135,6 @@ public static Setting secureString(String name, Setting insecureString(String name) { - return new InsecureStringSetting(name); - } - /** * A setting which contains a file. Reading the setting opens an input stream to the file. * @@ -179,24 +167,6 @@ SecureString getFallback(Settings settings) { } } - private static class InsecureStringSetting extends Setting { - private final String name; - - private InsecureStringSetting(String name) { - super(name, "", SecureString::new, Property.Deprecated, Property.Filtered, Property.NodeScope); - this.name = name; - } - - @Override - public SecureString get(Settings settings) { - if (ALLOW_INSECURE_SETTINGS == false && exists(settings)) { - throw new IllegalArgumentException("Setting [" + name + "] is insecure, " + - "but property [allow_insecure_settings] is not set"); - } - return super.get(settings); - } - } - private static class SecureFileSetting extends SecureSetting { private final Setting fallback;