From f8e0557be5e8177382c9d1e36bf8dffa979e5446 Mon Sep 17 00:00:00 2001 From: David Pilato Date: Sat, 23 Jul 2016 01:17:23 +0200 Subject: [PATCH] Extract AWS Key from KeyChain instead of using potential null value While I was working on #18703, I discovered a bad behavior when people don't provide AWS key/secret as part as their `elasticsearch.yml` but rely on SysProps or env. variables... In [`InternalAwsS3Service#getClient(...)`](https://github.com/elastic/elasticsearch/blob/d4366f8493ac8d2f7091404ffd346e4f3c0f9af9/plugins/repository-s3/src/main/java/org/elasticsearch/cloud/aws/InternalAwsS3Service.java#L76-L141), we have: ```java Tuple clientDescriptor = new Tuple<>(endpoint, account); AmazonS3Client client = clients.get(clientDescriptor); ``` But if people don't provide credentials, `account` is `null`. Even if it actually could work, I think that we should use the `AWSCredentialsProvider` we create later on and extract from it the `account` (AWS KEY actually) and then use it as the second value of the tuple. Closes #19557. --- .../elasticsearch/cloud/aws/AwsS3Service.java | 3 ++- .../cloud/aws/InternalAwsS3Service.java | 19 ++++++++++++---- .../repositories/s3/S3Repository.java | 5 +---- .../cloud/aws/AwsS3ServiceImplTests.java | 9 ++------ .../cloud/aws/TestAwsS3Service.java | 6 ++--- .../s3/AbstractS3SnapshotRestoreTest.java | 22 +++++++++---------- .../repositories/s3/S3RepositoryTests.java | 2 +- 7 files changed, 34 insertions(+), 32 deletions(-) diff --git a/plugins/repository-s3/src/main/java/org/elasticsearch/cloud/aws/AwsS3Service.java b/plugins/repository-s3/src/main/java/org/elasticsearch/cloud/aws/AwsS3Service.java index 43de8a3ba27..59c3d3445a2 100644 --- a/plugins/repository-s3/src/main/java/org/elasticsearch/cloud/aws/AwsS3Service.java +++ b/plugins/repository-s3/src/main/java/org/elasticsearch/cloud/aws/AwsS3Service.java @@ -24,6 +24,7 @@ import com.amazonaws.services.s3.AmazonS3; import org.elasticsearch.common.component.LifecycleComponent; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Setting.Property; +import org.elasticsearch.common.settings.Settings; import java.util.Locale; import java.util.function.Function; @@ -154,6 +155,6 @@ public interface AwsS3Service extends LifecycleComponent { Setting ENDPOINT_SETTING = Setting.simpleString("cloud.aws.s3.endpoint", Property.NodeScope); } - AmazonS3 client(String endpoint, Protocol protocol, String region, String account, String key, Integer maxRetries, + AmazonS3 client(Settings repositorySettings, String endpoint, Protocol protocol, String region, Integer maxRetries, boolean useThrottleRetries, Boolean pathStyleAccess); } diff --git a/plugins/repository-s3/src/main/java/org/elasticsearch/cloud/aws/InternalAwsS3Service.java b/plugins/repository-s3/src/main/java/org/elasticsearch/cloud/aws/InternalAwsS3Service.java index 27053379db1..c4d8a63adc6 100644 --- a/plugins/repository-s3/src/main/java/org/elasticsearch/cloud/aws/InternalAwsS3Service.java +++ b/plugins/repository-s3/src/main/java/org/elasticsearch/cloud/aws/InternalAwsS3Service.java @@ -35,10 +35,13 @@ import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.component.AbstractLifecycleComponent; import org.elasticsearch.common.logging.ESLogger; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.repositories.s3.S3Repository; import java.util.HashMap; import java.util.Map; +import static org.elasticsearch.repositories.s3.S3Repository.getValue; + /** * */ @@ -54,17 +57,20 @@ public class InternalAwsS3Service extends AbstractLifecycleComponent implements } @Override - public synchronized AmazonS3 client(String endpoint, Protocol protocol, String region, String key, String secret, Integer maxRetries, + public synchronized AmazonS3 client(Settings repositorySettings, String endpoint, Protocol protocol, String region, Integer maxRetries, boolean useThrottleRetries, Boolean pathStyleAccess) { String foundEndpoint = findEndpoint(logger, settings, endpoint, region); - Tuple clientDescriptor = new Tuple<>(foundEndpoint, key); + + AWSCredentialsProvider credentials = buildCredentials(logger, settings, repositorySettings); + + Tuple clientDescriptor = new Tuple<>(foundEndpoint, credentials.getCredentials().getAWSAccessKeyId()); AmazonS3Client client = clients.get(clientDescriptor); if (client != null) { return client; } client = new AmazonS3Client( - buildCredentials(logger, key, secret), + credentials, buildConfiguration(logger, settings, protocol, maxRetries, foundEndpoint, useThrottleRetries)); if (pathStyleAccess != null) { @@ -116,8 +122,13 @@ public class InternalAwsS3Service extends AbstractLifecycleComponent implements return clientConfiguration; } - public static AWSCredentialsProvider buildCredentials(ESLogger logger, String key, String secret) { + public static AWSCredentialsProvider buildCredentials(ESLogger logger, Settings settings, Settings repositorySettings) { AWSCredentialsProvider credentials; + String key = getValue(repositorySettings, settings, + S3Repository.Repository.KEY_SETTING, S3Repository.Repositories.KEY_SETTING); + String secret = getValue(repositorySettings, settings, + S3Repository.Repository.SECRET_SETTING, S3Repository.Repositories.SECRET_SETTING); + if (key.isEmpty() && secret.isEmpty()) { logger.debug("Using either environment variables, system properties or instance profile credentials"); credentials = new DefaultAWSCredentialsProviderChain(); 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 56d05b65711..b5abb361be8 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 @@ -301,11 +301,8 @@ public class S3Repository extends BlobStoreRepository { bucket, region, endpoint, protocol, chunkSize, serverSideEncryption, bufferSize, maxRetries, useThrottleRetries, cannedACL, storageClass, pathStyleAccess); - String key = getValue(metadata.settings(), settings, Repository.KEY_SETTING, Repositories.KEY_SETTING); - String secret = getValue(metadata.settings(), settings, Repository.SECRET_SETTING, Repositories.SECRET_SETTING); - blobStore = new S3BlobStore(settings, - s3Service.client(endpoint, protocol, region, key, secret, maxRetries, useThrottleRetries, pathStyleAccess), + s3Service.client(metadata.settings(), endpoint, protocol, region, maxRetries, useThrottleRetries, pathStyleAccess), bucket, region, serverSideEncryption, bufferSize, maxRetries, cannedACL, storageClass); String basePath = getValue(metadata.settings(), settings, Repository.BASE_PATH_SETTING, Repositories.BASE_PATH_SETTING); diff --git a/plugins/repository-s3/src/test/java/org/elasticsearch/cloud/aws/AwsS3ServiceImplTests.java b/plugins/repository-s3/src/test/java/org/elasticsearch/cloud/aws/AwsS3ServiceImplTests.java index 777bb5ff358..3f92a511190 100644 --- a/plugins/repository-s3/src/test/java/org/elasticsearch/cloud/aws/AwsS3ServiceImplTests.java +++ b/plugins/repository-s3/src/test/java/org/elasticsearch/cloud/aws/AwsS3ServiceImplTests.java @@ -35,7 +35,7 @@ import static org.hamcrest.Matchers.is; public class AwsS3ServiceImplTests extends ESTestCase { public void testAWSCredentialsWithSystemProviders() { - AWSCredentialsProvider credentialsProvider = InternalAwsS3Service.buildCredentials(logger, "", ""); + AWSCredentialsProvider credentialsProvider = InternalAwsS3Service.buildCredentials(logger, Settings.EMPTY, Settings.EMPTY); assertThat(credentialsProvider, instanceOf(DefaultAWSCredentialsProviderChain.class)); } @@ -136,12 +136,7 @@ public class AwsS3ServiceImplTests extends ESTestCase { protected void launchAWSCredentialsWithElasticsearchSettingsTest(Settings singleRepositorySettings, Settings settings, String expectedKey, String expectedSecret) { - String key = S3Repository.getValue(singleRepositorySettings, settings, - S3Repository.Repository.KEY_SETTING, S3Repository.Repositories.KEY_SETTING); - String secret = S3Repository.getValue(singleRepositorySettings, settings, - S3Repository.Repository.SECRET_SETTING, S3Repository.Repositories.SECRET_SETTING); - - AWSCredentials credentials = InternalAwsS3Service.buildCredentials(logger, key, secret).getCredentials(); + AWSCredentials credentials = InternalAwsS3Service.buildCredentials(logger, settings, singleRepositorySettings).getCredentials(); assertThat(credentials.getAWSAccessKeyId(), is(expectedKey)); assertThat(credentials.getAWSSecretKey(), is(expectedSecret)); } diff --git a/plugins/repository-s3/src/test/java/org/elasticsearch/cloud/aws/TestAwsS3Service.java b/plugins/repository-s3/src/test/java/org/elasticsearch/cloud/aws/TestAwsS3Service.java index d7c706822a8..e7a57958a89 100644 --- a/plugins/repository-s3/src/test/java/org/elasticsearch/cloud/aws/TestAwsS3Service.java +++ b/plugins/repository-s3/src/test/java/org/elasticsearch/cloud/aws/TestAwsS3Service.java @@ -21,10 +21,8 @@ package org.elasticsearch.cloud.aws; import com.amazonaws.Protocol; import com.amazonaws.services.s3.AmazonS3; import org.elasticsearch.ElasticsearchException; -import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.plugin.repository.s3.S3RepositoryPlugin; -import org.elasticsearch.plugins.Plugin; import java.util.IdentityHashMap; @@ -44,9 +42,9 @@ public class TestAwsS3Service extends InternalAwsS3Service { @Override - public synchronized AmazonS3 client(String endpoint, Protocol protocol, String region, String account, String key, Integer maxRetries, + public synchronized AmazonS3 client(Settings repositorySettings, String endpoint, Protocol protocol, String region, Integer maxRetries, boolean useThrottleRetries, Boolean pathStyleAccess) { - return cachedWrapper(super.client(endpoint, protocol, region, account, key, maxRetries, useThrottleRetries, pathStyleAccess)); + return cachedWrapper(super.client(repositorySettings, endpoint, protocol, region, maxRetries, useThrottleRetries, pathStyleAccess)); } private AmazonS3 cachedWrapper(AmazonS3 client) { diff --git a/plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/AbstractS3SnapshotRestoreTest.java b/plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/AbstractS3SnapshotRestoreTest.java index a3671b42ee4..d9d15ce0b3b 100644 --- a/plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/AbstractS3SnapshotRestoreTest.java +++ b/plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/AbstractS3SnapshotRestoreTest.java @@ -161,12 +161,15 @@ public abstract class AbstractS3SnapshotRestoreTest extends AbstractAwsTestCase public void testEncryption() { Client client = client(); logger.info("--> creating s3 repository with bucket[{}] and path [{}]", internalCluster().getInstance(Settings.class).get("repositories.s3.bucket"), basePath); + + Settings repositorySettings = Settings.builder() + .put(S3Repository.Repository.BASE_PATH_SETTING.getKey(), basePath) + .put(S3Repository.Repository.CHUNK_SIZE_SETTING.getKey(), randomIntBetween(1000, 10000)) + .put(S3Repository.Repository.SERVER_SIDE_ENCRYPTION_SETTING.getKey(), true) + .build(); + PutRepositoryResponse putRepositoryResponse = client.admin().cluster().preparePutRepository("test-repo") - .setType("s3").setSettings(Settings.builder() - .put(S3Repository.Repository.BASE_PATH_SETTING.getKey(), basePath) - .put(S3Repository.Repository.CHUNK_SIZE_SETTING.getKey(), randomIntBetween(1000, 10000)) - .put(S3Repository.Repository.SERVER_SIDE_ENCRYPTION_SETTING.getKey(), true) - ).get(); + .setType("s3").setSettings(repositorySettings).get(); assertThat(putRepositoryResponse.isAcknowledged(), equalTo(true)); createIndex("test-idx-1", "test-idx-2", "test-idx-3"); @@ -193,11 +196,10 @@ public abstract class AbstractS3SnapshotRestoreTest extends AbstractAwsTestCase Settings settings = internalCluster().getInstance(Settings.class); Settings bucket = settings.getByPrefix("repositories.s3."); AmazonS3 s3Client = internalCluster().getInstance(AwsS3Service.class).client( + repositorySettings, null, S3Repository.Repositories.PROTOCOL_SETTING.get(settings), S3Repository.Repositories.REGION_SETTING.get(settings), - S3Repository.Repositories.KEY_SETTING.get(settings), - S3Repository.Repositories.SECRET_SETTING.get(settings), null, randomBoolean(), null); String bucketName = bucket.get("bucket"); @@ -466,15 +468,13 @@ public abstract class AbstractS3SnapshotRestoreTest extends AbstractAwsTestCase String endpoint = bucket.get("endpoint", S3Repository.Repositories.ENDPOINT_SETTING.get(settings)); Protocol protocol = S3Repository.Repositories.PROTOCOL_SETTING.get(settings); String region = bucket.get("region", S3Repository.Repositories.REGION_SETTING.get(settings)); - String accessKey = bucket.get("access_key", S3Repository.Repositories.KEY_SETTING.get(settings)); - String secretKey = bucket.get("secret_key", S3Repository.Repositories.SECRET_SETTING.get(settings)); String bucketName = bucket.get("bucket"); // We check that settings has been set in elasticsearch.yml integration test file // as described in README assertThat("Your settings in elasticsearch.yml are incorrects. Check README file.", bucketName, notNullValue()); - AmazonS3 client = internalCluster().getInstance(AwsS3Service.class).client(endpoint, protocol, region, accessKey, secretKey, - null, randomBoolean(), null); + AmazonS3 client = internalCluster().getInstance(AwsS3Service.class).client(Settings.EMPTY, endpoint, protocol, region, null, + randomBoolean(), null); try { ObjectListing prevListing = null; //From http://docs.amazonwebservices.com/AmazonS3/latest/dev/DeletingMultipleObjectsUsingJava.html diff --git a/plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3RepositoryTests.java b/plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3RepositoryTests.java index d6cca5d70d6..f8940c6158c 100644 --- a/plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3RepositoryTests.java +++ b/plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3RepositoryTests.java @@ -58,7 +58,7 @@ public class S3RepositoryTests extends ESTestCase { @Override protected void doClose() {} @Override - public AmazonS3 client(String endpoint, Protocol protocol, String region, String account, String key, Integer maxRetries, + public AmazonS3 client(Settings repositorySettings, String endpoint, Protocol protocol, String region, Integer maxRetries, boolean useThrottleRetries, Boolean pathStyleAccess) { return new DummyS3Client(); }