Remove insecure settings

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'
 }
 

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<String, S3ClientSettings> 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);

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<SecureString> 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<SecureString> 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,

plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3RepositoryPlugin.java

@@ -105,9 +105,7 @@ public List<Setting<?>> 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

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";
+
 };

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<Void>) () -> {
-            // required for client settings overwriting when running in IDE
-            System.setProperty("es.allow_insecure_settings", "true");
-            return null;
-        });
-    }
-
     @Override
     protected Collection<Class<? extends Plugin>> 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<AssertionError> 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)

server/src/main/java/org/elasticsearch/common/settings/SecureSetting.java

@@ -34,9 +34,6 @@
  */
 public abstract class SecureSetting<T> extends Setting<T> {
 
-    /** 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<Property> ALLOWED_PROPERTIES = EnumSet.of(Property.Deprecated, Property.Consistent);
 
     private static final Property[] FIXED_PROPERTIES = {
@@ -139,14 +136,6 @@ public static Setting<SecureString> secureString(String name, Setting<SecureStri
         return new SecureStringSetting(name, fallback, properties);
     }
 
-    /**
-     * A setting which contains a sensitive string, but which for legacy reasons must be found outside secure settings.
-     * @see #secureString(String, Setting, Property...)
-     */
-    public static Setting<SecureString> 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 +168,6 @@ SecureString getFallback(Settings settings) {
         }
     }
 
-    private static class InsecureStringSetting extends Setting<SecureString> {
-        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<InputStream> {
         private final Setting<InputStream> fallback;