Skip to content

Commit

Permalink
Improve message about insecure S3 settings (#116956)
Browse files Browse the repository at this point in the history
* Improve message about insecure S3 settings

Clarifies that insecure settings are stored in plaintext and must not be
used. Also removes the mention of the (wrong) system property from the
error message if insecure settings are not permitted.

Backport of #116915 to `7.17`

* CI poke
  • Loading branch information
DaveCTurner authored Nov 18, 2024
1 parent 3fa1e0a commit db8aecf
Show file tree
Hide file tree
Showing 4 changed files with 18 additions and 17 deletions.
5 changes: 5 additions & 0 deletions docs/changelog/116915.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 116915
summary: Improve message about insecure S3 settings
area: Snapshot/Restore
type: enhancement
issues: []
Original file line number Diff line number Diff line change
Expand Up @@ -267,8 +267,7 @@ class S3Repository extends MeteredBlobStoreRepository {
deprecationLogger.critical(
DeprecationCategory.SECURITY,
"s3_repository_secret_settings",
"Using s3 access/secret key from repository settings. Instead "
+ "store these in named clients and the elasticsearch keystore for secure settings."
INSECURE_CREDENTIALS_DEPRECATION_WARNING
);
}

Expand All @@ -285,6 +284,10 @@ class S3Repository extends MeteredBlobStoreRepository {
);
}

static final String INSECURE_CREDENTIALS_DEPRECATION_WARNING =
"This repository's settings include a S3 access key and secret key, but repository settings are stored in plaintext and must not "
+ "be used for security-sensitive information. Instead, store all secure settings in the keystore.";

private static Map<String, String> buildLocation(RepositoryMetadata metadata) {
return org.elasticsearch.core.Map.of(
"base_path",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,12 +108,11 @@ public void testRepositoryCredentialsOverrideSecureCredentials() {
assertThat(credentials.getAWSSecretKey(), is("insecure_aws_secret"));

assertWarnings(
"[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.",
"[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."
S3Repository.INSECURE_CREDENTIALS_DEPRECATION_WARNING
);
}

Expand Down Expand Up @@ -194,12 +193,11 @@ public void testReinitSecureCredentials() {

if (hasInsecureSettings) {
assertWarnings(
"[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.",
"[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."
S3Repository.INSECURE_CREDENTIALS_DEPRECATION_WARNING
);
}
}
Expand Down Expand Up @@ -239,10 +237,7 @@ public void sendResponse(RestResponse response) {
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."
);
assertWarnings(S3Repository.INSECURE_CREDENTIALS_DEPRECATION_WARNING);
}

private void createRepository(final String name, final Settings repositorySettings) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -182,9 +182,7 @@ private InsecureStringSetting(String 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"
);
throw new IllegalArgumentException("Setting [" + name + "] is insecure, use the elasticsearch keystore instead");
}
return super.get(settings);
}
Expand Down

0 comments on commit db8aecf

Please sign in to comment.