Remove insecure settings (#46147)

jasontedor · web-flow · commit 2bcfe1a2d82d · 2019-08-29T21:25:20.000-04:00
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.
diff --git 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
@@ -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);
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
@@ -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,
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
@@ -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
diff --git 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
@@ -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)
diff --git a/server/src/main/java/org/elasticsearch/common/settings/SecureSetting.java b/server/src/main/java/org/elasticsearch/common/settings/SecureSetting.java

Original file line number	Diff line number	Diff line change
`@@ -65,15 +65,8 @@ bundlePlugin {`
`65`	`65`	`}`
`66`	`66`	`}`
`67`	`67`
`68`		`-task testRepositoryCreds(type: Test) {`
`69`		`- include '**/RepositoryCredentialsTests.class'`
`70`		`- systemProperty 'es.allow_insecure_settings', 'true'`
`71`		`-}`
`72`		`-check.dependsOn(testRepositoryCreds)`
`73`		`-`
`74`	`68`	`test {`
`75`		`- // these are tested explicitly in separate test tasks`
`76`		`- exclude '**/RepositoryCredentialsTests.class'`
	`69`	`+ // this is tested explicitly in separate test tasks`
`77`	`70`	`exclude '**/S3RepositoryThirdPartyTests.class'`
`78`	`71`	`}`
`79`	`72`