Skip to content

Commit

Permalink
Improve message about insecure S3 settings (#116955)
Browse files Browse the repository at this point in the history
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 `8.16`
  • Loading branch information
DaveCTurner authored Nov 18, 2024
1 parent dab09e2 commit a8e616f
Show file tree
Hide file tree
Showing 6 changed files with 19 additions and 15 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 @@ -288,8 +288,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 @@ -306,6 +305,11 @@ class S3Repository extends MeteredBlobStoreRepository {
);
}

static final String INSECURE_CREDENTIALS_DEPRECATION_WARNING = Strings.format("""
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. See [%s] for more information.\
""", ReferenceDocs.SECURE_SETTINGS);

private static Map<String, String> buildLocation(RepositoryMetadata metadata) {
return Map.of("base_path", BASE_PATH_SETTING.get(metadata.settings()), "bucket", BUCKET_SETTING.get(metadata.settings()));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,10 +107,9 @@ public void testRepositoryCredentialsOverrideSecureCredentials() {
assertThat(credentials.getAWSSecretKey(), is("insecure_aws_secret"));

assertCriticalWarnings(
"[access_key] setting was deprecated in Elasticsearch and will be removed in a future release.",
"[secret_key] setting was deprecated in Elasticsearch and will be removed in a future release.",
"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."
S3Repository.INSECURE_CREDENTIALS_DEPRECATION_WARNING
);
}

Expand Down Expand Up @@ -194,10 +193,9 @@ public void testReinitSecureCredentials() {

if (hasInsecureSettings) {
assertCriticalWarnings(
"[access_key] setting was deprecated in Elasticsearch and will be removed in a future release.",
"[secret_key] setting was deprecated in Elasticsearch and will be removed in a future release.",
"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."
S3Repository.INSECURE_CREDENTIALS_DEPRECATION_WARNING
);
}
}
Expand Down Expand Up @@ -238,10 +236,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 @@ -81,6 +81,7 @@ public enum ReferenceDocs {
X_OPAQUE_ID,
FORMING_SINGLE_NODE_CLUSTERS,
JDK_LOCALE_DIFFERENCES,
SECURE_SETTINGS,
// this comment keeps the ';' on the next line so every entry above has a trailing ',' which makes the diff for adding new links cleaner
;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -192,9 +192,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
Original file line number Diff line number Diff line change
Expand Up @@ -43,3 +43,4 @@ FLOOD_STAGE_WATERMARK fix-watermark-er
X_OPAQUE_ID api-conventions.html#x-opaque-id
FORMING_SINGLE_NODE_CLUSTERS modules-discovery-bootstrap-cluster.html#modules-discovery-bootstrap-cluster-joining
JDK_LOCALE_DIFFERENCES mapping-date-format.html#custom-date-format-locales
SECURE_SETTINGS secure-settings.html

0 comments on commit a8e616f

Please sign in to comment.