Skip to content

Commit 839a677

Browse files
jasontedorjasontedor
authored
Do not override named S3 client credentials (#33793)
In cases when mixed secure S3 client credentials and insecure S3 client credentials were used (that is, those defined on the repository), we were overriding the credentials from the repository using insecure settings to all the repositories. This commit fixes this by not mixing up repositories that use insecure settings with those that use secure settings.
1 parent 26c4f1f commit 839a677

File tree

5 files changed

+95
-59
lines changed

5 files changed

+95
-59
lines changed

‎plugins/repository-s3/build.gradle‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,12 +70,14 @@ bundlePlugin {
7070

7171
additionalTest('testRepositoryCreds'){
7272
include '**/RepositoryCredentialsTests.class'
73+
include '**/S3BlobStoreRepositoryTests.class'
7374
systemProperty 'es.allow_insecure_settings', 'true'
7475
}
7576

7677
test {
7778
// these are tested explicitly in separate test tasks
7879
exclude '**/*CredentialsTests.class'
80+
exclude '**/S3BlobStoreRepositoryTests.class'
7981
}
8082

8183
boolean useFixture = false

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

Lines changed: 30 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -19,25 +19,25 @@
1919

2020
package org.elasticsearch.repositories.s3;
2121

22-
import java.util.Collections;
23-
import java.util.HashMap;
24-
import java.util.Locale;
25-
import java.util.Map;
26-
import java.util.Set;
2722
import com.amazonaws.ClientConfiguration;
2823
import com.amazonaws.Protocol;
2924
import com.amazonaws.auth.AWSCredentials;
3025
import com.amazonaws.auth.BasicAWSCredentials;
31-
3226
import com.amazonaws.auth.BasicSessionCredentials;
33-
import org.elasticsearch.common.collect.MapBuilder;
27+
import org.elasticsearch.cluster.metadata.RepositoryMetaData;
3428
import org.elasticsearch.common.settings.SecureSetting;
3529
import org.elasticsearch.common.settings.SecureString;
3630
import org.elasticsearch.common.settings.Setting;
3731
import org.elasticsearch.common.settings.Setting.Property;
3832
import org.elasticsearch.common.settings.Settings;
3933
import org.elasticsearch.common.unit.TimeValue;
4034

35+
import java.util.Collections;
36+
import java.util.HashMap;
37+
import java.util.Locale;
38+
import java.util.Map;
39+
import java.util.Set;
40+
4141
/**
4242
* A container for settings used to create an S3 client.
4343
*/
@@ -160,19 +160,6 @@ static Map<String, S3ClientSettings> load(Settings settings) {
160160
return Collections.unmodifiableMap(clients);
161161
}
162162

163-
static Map<String, S3ClientSettings> overrideCredentials(Map<String, S3ClientSettings> clientsSettings,
164-
BasicAWSCredentials credentials) {
165-
final MapBuilder<String, S3ClientSettings> mapBuilder = new MapBuilder<>();
166-
for (final Map.Entry<String, S3ClientSettings> entry : clientsSettings.entrySet()) {
167-
final S3ClientSettings s3ClientSettings = new S3ClientSettings(credentials, entry.getValue().endpoint,
168-
entry.getValue().protocol, entry.getValue().proxyHost, entry.getValue().proxyPort, entry.getValue().proxyUsername,
169-
entry.getValue().proxyPassword, entry.getValue().readTimeoutMillis, entry.getValue().maxRetries,
170-
entry.getValue().throttleRetries);
171-
mapBuilder.put(entry.getKey(), s3ClientSettings);
172-
}
173-
return mapBuilder.immutableMap();
174-
}
175-
176163
static boolean checkDeprecatedCredentials(Settings repositorySettings) {
177164
if (S3Repository.ACCESS_KEY_SETTING.exists(repositorySettings)) {
178165
if (S3Repository.SECRET_KEY_SETTING.exists(repositorySettings) == false) {
@@ -224,25 +211,37 @@ static AWSCredentials loadCredentials(Settings settings, String clientName) {
224211

225212
// pkg private for tests
226213
/** Parse settings for a single client. */
227-
static S3ClientSettings getClientSettings(Settings settings, String clientName) {
214+
static S3ClientSettings getClientSettings(final Settings settings, final String clientName) {
228215
final AWSCredentials credentials = S3ClientSettings.loadCredentials(settings, clientName);
216+
return getClientSettings(settings, clientName, credentials);
217+
}
218+
219+
static S3ClientSettings getClientSettings(final Settings settings, final String clientName, final AWSCredentials credentials) {
229220
try (SecureString proxyUsername = getConfigValue(settings, clientName, PROXY_USERNAME_SETTING);
230221
SecureString proxyPassword = getConfigValue(settings, clientName, PROXY_PASSWORD_SETTING)) {
231222
return new S3ClientSettings(
232-
credentials,
233-
getConfigValue(settings, clientName, ENDPOINT_SETTING),
234-
getConfigValue(settings, clientName, PROTOCOL_SETTING),
235-
getConfigValue(settings, clientName, PROXY_HOST_SETTING),
236-
getConfigValue(settings, clientName, PROXY_PORT_SETTING),
237-
proxyUsername.toString(),
238-
proxyPassword.toString(),
239-
(int)getConfigValue(settings, clientName, READ_TIMEOUT_SETTING).millis(),
240-
getConfigValue(settings, clientName, MAX_RETRIES_SETTING),
241-
getConfigValue(settings, clientName, USE_THROTTLE_RETRIES_SETTING)
223+
credentials,
224+
getConfigValue(settings, clientName, ENDPOINT_SETTING),
225+
getConfigValue(settings, clientName, PROTOCOL_SETTING),
226+
getConfigValue(settings, clientName, PROXY_HOST_SETTING),
227+
getConfigValue(settings, clientName, PROXY_PORT_SETTING),
228+
proxyUsername.toString(),
229+
proxyPassword.toString(),
230+
Math.toIntExact(getConfigValue(settings, clientName, READ_TIMEOUT_SETTING).millis()),
231+
getConfigValue(settings, clientName, MAX_RETRIES_SETTING),
232+
getConfigValue(settings, clientName, USE_THROTTLE_RETRIES_SETTING)
242233
);
243234
}
244235
}
245236

237+
static S3ClientSettings getClientSettings(final RepositoryMetaData metadata, final AWSCredentials credentials) {
238+
final Settings.Builder builder = Settings.builder();
239+
for (final String key : metadata.settings().keySet()) {
240+
builder.put(PREFIX + "provided" + "." + key, metadata.settings().get(key));
241+
}
242+
return getClientSettings(builder.build(), "provided", credentials);
243+
}
244+
246245
private static <T> T getConfigValue(Settings settings, String clientName,
247246
Setting.AffixSetting<T> clientSetting) {
248247
final Setting<T> concreteSetting = clientSetting.getConcreteSettingForNamespace(clientName);

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

Lines changed: 49 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,6 @@
3535
import org.elasticsearch.repositories.RepositoryException;
3636
import org.elasticsearch.repositories.blobstore.BlobStoreRepository;
3737

38-
import java.util.Map;
3938
import java.util.function.Function;
4039

4140
/**
@@ -163,6 +162,8 @@ class S3Repository extends BlobStoreRepository {
163162

164163
private final String clientName;
165164

165+
private final AmazonS3Reference reference;
166+
166167
/**
167168
* Constructs an s3 backed repository
168169
*/
@@ -200,21 +201,54 @@ class S3Repository extends BlobStoreRepository {
200201

201202
this.storageClass = STORAGE_CLASS_SETTING.get(metadata.settings());
202203
this.cannedACL = CANNED_ACL_SETTING.get(metadata.settings());
204+
203205
this.clientName = CLIENT_NAME.get(metadata.settings());
204206

205-
logger.debug("using bucket [{}], chunk_size [{}], server_side_encryption [{}], " +
206-
"buffer_size [{}], cannedACL [{}], storageClass [{}]",
207-
bucket, chunkSize, serverSideEncryption, bufferSize, cannedACL, storageClass);
207+
if (CLIENT_NAME.exists(metadata.settings()) && S3ClientSettings.checkDeprecatedCredentials(metadata.settings())) {
208+
logger.warn(
209+
"ignoring use of named client [{}] for repository [{}] as insecure credentials were specified",
210+
clientName,
211+
metadata.name());
212+
}
208213

209-
// (repository settings)
210214
if (S3ClientSettings.checkDeprecatedCredentials(metadata.settings())) {
211-
overrideCredentialsFromClusterState(service);
215+
// provided repository settings
216+
deprecationLogger.deprecated("Using s3 access/secret key from repository settings. Instead "
217+
+ "store these in named clients and the elasticsearch keystore for secure settings.");
218+
final BasicAWSCredentials insecureCredentials = S3ClientSettings.loadDeprecatedCredentials(metadata.settings());
219+
final S3ClientSettings s3ClientSettings = S3ClientSettings.getClientSettings(metadata, insecureCredentials);
220+
this.reference = new AmazonS3Reference(service.buildClient(s3ClientSettings));
221+
} else {
222+
reference = null;
212223
}
224+
225+
logger.debug(
226+
"using bucket [{}], chunk_size [{}], server_side_encryption [{}], buffer_size [{}], cannedACL [{}], storageClass [{}]",
227+
bucket,
228+
chunkSize,
229+
serverSideEncryption,
230+
bufferSize,
231+
cannedACL,
232+
storageClass);
213233
}
214234

215235
@Override
216236
protected S3BlobStore createBlobStore() {
217-
return new S3BlobStore(settings, service, clientName, bucket, serverSideEncryption, bufferSize, cannedACL, storageClass);
237+
if (reference != null) {
238+
assert S3ClientSettings.checkDeprecatedCredentials(metadata.settings()) : metadata.name();
239+
return new S3BlobStore(settings, service, clientName, bucket, serverSideEncryption, bufferSize, cannedACL, storageClass) {
240+
@Override
241+
public AmazonS3Reference clientReference() {
242+
if (reference.tryIncRef()) {
243+
return reference;
244+
} else {
245+
throw new IllegalStateException("S3 client is closed");
246+
}
247+
}
248+
};
249+
} else {
250+
return new S3BlobStore(settings, service, clientName, bucket, serverSideEncryption, bufferSize, cannedACL, storageClass);
251+
}
218252
}
219253

220254
// only use for testing
@@ -244,13 +278,13 @@ protected ByteSizeValue chunkSize() {
244278
return chunkSize;
245279
}
246280

247-
void overrideCredentialsFromClusterState(final S3Service s3Service) {
248-
deprecationLogger.deprecated("Using s3 access/secret key from repository settings. Instead "
249-
+ "store these in named clients and the elasticsearch keystore for secure settings.");
250-
final BasicAWSCredentials insecureCredentials = S3ClientSettings.loadDeprecatedCredentials(metadata.settings());
251-
// hack, but that's ok because the whole if branch should be axed
252-
final Map<String, S3ClientSettings> prevSettings = s3Service.refreshAndClearCache(S3ClientSettings.load(Settings.EMPTY));
253-
final Map<String, S3ClientSettings> newSettings = S3ClientSettings.overrideCredentials(prevSettings, insecureCredentials);
254-
s3Service.refreshAndClearCache(newSettings);
281+
@Override
282+
protected void doClose() {
283+
if (reference != null) {
284+
assert S3ClientSettings.checkDeprecatedCredentials(metadata.settings()) : metadata.name();
285+
reference.decRef();
286+
}
287+
super.doClose();
255288
}
289+
256290
}

‎plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/RepositoryCredentialsTests.java‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,6 @@ public void testRepositoryCredentialsOverrideSecureCredentials() throws IOExcept
107107
final Settings settings = Settings.builder().setSecureSettings(secureSettings).build();
108108
// repository settings for credentials override node secure settings
109109
final RepositoryMetaData metadata = new RepositoryMetaData("dummy-repo", "mock", Settings.builder()
110-
.put(S3Repository.CLIENT_NAME.getKey(), randomFrom(clientNames))
111110
.put(S3Repository.ACCESS_KEY_SETTING.getKey(), "insecure_aws_key")
112111
.put(S3Repository.SECRET_KEY_SETTING.getKey(), "insecure_aws_secret").build());
113112
try (S3RepositoryPlugin s3Plugin = new ProxyS3RepositoryPlugin(settings);
@@ -163,11 +162,13 @@ public void testReinitSecureCredentials() throws IOException {
163162
secureSettings.setString("s3.client." + clientName + ".secret_key", "secure_aws_secret");
164163
final Settings settings = Settings.builder().setSecureSettings(secureSettings).build();
165164
// repository settings
166-
final Settings.Builder builder = Settings.builder().put(S3Repository.CLIENT_NAME.getKey(), clientName);
165+
final Settings.Builder builder = Settings.builder();
167166
final boolean repositorySettings = randomBoolean();
168167
if (repositorySettings) {
169168
builder.put(S3Repository.ACCESS_KEY_SETTING.getKey(), "insecure_aws_key");
170169
builder.put(S3Repository.SECRET_KEY_SETTING.getKey(), "insecure_aws_secret");
170+
} else {
171+
builder.put(S3Repository.CLIENT_NAME.getKey(), clientName);
171172
}
172173
final RepositoryMetaData metadata = new RepositoryMetaData("dummy-repo", "mock", builder.build());
173174
try (S3RepositoryPlugin s3Plugin = new ProxyS3RepositoryPlugin(settings);
@@ -202,8 +203,13 @@ public void testReinitSecureCredentials() throws IOException {
202203
try (AmazonS3Reference s3Ref = ((S3BlobStore) s3repo.blobStore()).clientReference()) {
203204
final AWSCredentials newCredentials = ((ProxyS3RepositoryPlugin.ClientAndCredentials) s3Ref.client()).credentials
204205
.getCredentials();
205-
assertThat(newCredentials.getAWSAccessKeyId(), is("new_secret_aws_key"));
206-
assertThat(newCredentials.getAWSSecretKey(), is("new_secret_aws_secret"));
206+
if (repositorySettings) {
207+
assertThat(newCredentials.getAWSAccessKeyId(), is("insecure_aws_key"));
208+
assertThat(newCredentials.getAWSSecretKey(), is("insecure_aws_secret"));
209+
} else {
210+
assertThat(newCredentials.getAWSAccessKeyId(), is("new_secret_aws_key"));
211+
assertThat(newCredentials.getAWSSecretKey(), is("new_secret_aws_secret"));
212+
}
207213
}
208214
}
209215
if (repositorySettings) {

‎plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3BlobStoreRepositoryTests.java‎

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,9 @@
1818
*/
1919
package org.elasticsearch.repositories.s3;
2020

21+
import com.amazonaws.services.s3.AmazonS3;
2122
import com.amazonaws.services.s3.model.CannedAccessControlList;
2223
import com.amazonaws.services.s3.model.StorageClass;
23-
2424
import org.elasticsearch.client.node.NodeClient;
2525
import org.elasticsearch.common.settings.Settings;
2626
import org.elasticsearch.common.settings.SettingsFilter;
@@ -91,7 +91,6 @@ protected void createTestRepository(final String name, boolean verify) {
9191
.setVerify(verify)
9292
.setSettings(Settings.builder()
9393
.put(S3Repository.BUCKET_SETTING.getKey(), bucket)
94-
.put(S3Repository.CLIENT_NAME.getKey(), client)
9594
.put(S3Repository.BUFFER_SIZE_SETTING.getKey(), bufferSize)
9695
.put(S3Repository.SERVER_SIDE_ENCRYPTION_SETTING.getKey(), serverSideEncryption)
9796
.put(S3Repository.CANNED_ACL_SETTING.getKey(), cannedACL)
@@ -121,14 +120,10 @@ public Map<String, Repository.Factory> getRepositories(final Environment env, fi
121120
return Collections.singletonMap(S3Repository.TYPE,
122121
(metadata) -> new S3Repository(metadata, env.settings(), registry, new S3Service(env.settings()) {
123122
@Override
124-
public synchronized AmazonS3Reference client(String clientName) {
125-
return new AmazonS3Reference(new MockAmazonS3(blobs, bucket, serverSideEncryption, cannedACL, storageClass));
126-
}
127-
}) {
128-
@Override
129-
void overrideCredentialsFromClusterState(S3Service awsService) {
123+
AmazonS3 buildClient(S3ClientSettings clientSettings) {
124+
return new MockAmazonS3(blobs, bucket, serverSideEncryption, cannedACL, storageClass);
130125
}
131-
});
126+
}));
132127
}
133128
}
134129

0 commit comments

Comments
 (0)