Add new setting property 'Sensitive' for tiering dynamic settings

[cwperks]

Description

Companion Security PR: opensearch-project/security#6016

This PR adds a new Setting property called Sensitive which is intended to help tier settings for authorization for different callers.

Currently, the security repo has dynamic settings through the security index (see config.yml) and still have some static cluster settings in opensearch.yml. The security index has had some bwc issues (particularly if a new setting is added from one minor version to the next and its persisted in a mixed cluster during rolling upgrade) and some of these settings would be better suited for using the existing settings mechanism.

In order to use the existing settings mechanism, the security plugin needs a mechanism for distinguishing which settings should be toggleable by a securityadmin vs any user with cluster:admin/settings/put permission.

I propose adding a new setting property called Sensitive so that the security plugin can check a settings update payload to see if any sensitive settings are being updated. If the payload contains a sensitive setting, then only a security admin (all_access or security_manager roles) should be able to update, otherwise any user with cluster:admin/settings/put should be able to update.

Related Issues

Related to:

• [Feature] Add support for tiering dynamic cluster settings for authorization #20905
• [RFC] Fine grained settings permissions security#5219
• Make plugins.security.dfm_empty_overrides_all dynamically toggleable security#6002

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 5f0621e)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Incomplete Validation The validateNoTransientSensitiveSettings method only blocks sensitive settings from being set as transient, but does not prevent them from being cleared (removed) via transient settings. A caller could pass a transient settings object with a null/empty value for a sensitive key to effectively remove it. Whether transientSettings.keySet() includes keys being explicitly nulled out depends on the Settings builder usage, but this edge case should be verified and documented. private void validateNoTransientSensitiveSettings(final Settings transientSettings) { for (String key : transientSettings.keySet()) { if (clusterSettings.isSensitiveSetting(key)) { throw new IllegalArgumentException("sensitive setting [" + key + "] must be updated using persistent settings"); } } } Missing Cluster Scope Check The Sensitive property is documented as intended for cluster-level dynamic settings (to be controlled by security admin), but there is no validation preventing it from being applied to index-scoped settings (IndexScope). A plugin could accidentally mark an index setting as Sensitive, which would never be checked by the transient settings validator (which only operates on cluster settings). This could create a false sense of security. if (propertiesAsSet.contains(Property.Sensitive) && propertiesAsSet.contains(Property.Dynamic) == false) { throw new IllegalArgumentException("sensitive setting [" + key + "] must be dynamic"); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 5f0621e

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Reject invalid Sensitive and Final property combination A Sensitive setting that is also Final would be contradictory — Final settings cannot be changed at all, making the Sensitive (dynamic-only) property meaningless. Add a validation to reject the combination of Sensitive and Final properties, similar to how Dynamic and Final are already rejected together. server/src/main/java/org/opensearch/common/settings/Setting.java [228-230] if (propertiesAsSet.contains(Property.Sensitive) && propertiesAsSet.contains(Property.Dynamic) == false) { throw new IllegalArgumentException("sensitive setting [" + key + "] must be dynamic"); } +if (propertiesAsSet.contains(Property.Sensitive) && propertiesAsSet.contains(Property.Final)) { + throw new IllegalArgumentException("sensitive setting [" + key + "] cannot be final"); +} Suggestion importance[1-10]: 5 __ Why: This is a valid defensive check — since Sensitive requires Dynamic and Final conflicts with Dynamic, a Sensitive + Final combination would already be caught by the existing Dynamic + Final check (line 223-224) indirectly. However, adding an explicit check improves clarity and provides a more descriptive error message for this specific invalid combination.	Low
Possible issue	Handle grouped sensitive settings in validation The validation only checks top-level keys but settings can contain grouped/prefixed keys (e.g., cluster.routing.allocation.*). Consider also checking whether any key matches a sensitive setting via prefix/group matching, similar to how other validation methods in AbstractScopedSettings handle grouped settings, to avoid bypassing the restriction with grouped keys. server/src/main/java/org/opensearch/action/admin/cluster/settings/SettingsUpdater.java [216-222] private void validateNoTransientSensitiveSettings(final Settings transientSettings) { for (String key : transientSettings.keySet()) { if (clusterSettings.isSensitiveSetting(key)) { throw new IllegalArgumentException("sensitive setting [" + key + "] must be updated using persistent settings"); } } + // Also check grouped/prefixed settings + for (Setting<?> setting : clusterSettings.filter(Setting::isSensitive)) { + if (setting.exists(transientSettings)) { + throw new IllegalArgumentException("sensitive setting [" + setting.getKey() + "] must be updated using persistent settings"); + } + } } Suggestion importance[1-10]: 4 __ Why: The suggestion raises a valid concern about grouped/prefixed settings potentially bypassing the validation. However, the improved_code introduces a clusterSettings.filter(Setting::isSensitive) call that may not exist in the API, making the implementation speculative. The actual risk depends on whether sensitive settings would realistically use grouped keys, which is unclear from the PR context.	Low

---

Previous suggestions

Suggestions up to commit d50d348

Category	Suggestion	Impact
General	Reject conflicting Sensitive and Final properties The Sensitive property validation is placed after the Final and UnmodifiableOnRestore conflict checks, but a setting could theoretically be both Sensitive and Final (which is also non-dynamic). The Sensitive check should be placed before or alongside the other property conflict checks to ensure consistent ordering and to also explicitly reject Sensitive + Final combinations with a clear error message. server/src/main/java/org/opensearch/common/settings/Setting.java [227-229] if (propertiesAsSet.contains(Property.Sensitive) && propertiesAsSet.contains(Property.Dynamic) == false) { throw new IllegalArgumentException("sensitive setting [" + key + "] must be dynamic"); } +if (propertiesAsSet.contains(Property.Sensitive) && propertiesAsSet.contains(Property.Final)) { + throw new IllegalArgumentException("sensitive setting [" + key + "] cannot be final"); +} Suggestion importance[1-10]: 3 __ Why: The existing check propertiesAsSet.contains(Property.Sensitive) && propertiesAsSet.contains(Property.Dynamic) == false already covers the Final case since Final settings cannot be Dynamic. Adding an explicit Sensitive + Final check is redundant but could provide a clearer error message. The impact is minor since the existing validation already prevents this combination.	Low
	Validate grouped settings keys too The validation only checks top-level keys in transientSettings, but settings can have grouped/prefixed keys (e.g., wildcards or grouped settings). Consider also handling grouped settings by checking against the full key set including any prefix-matched or grouped setting keys to avoid bypassing the sensitive check. server/src/main/java/org/opensearch/action/admin/cluster/settings/SettingsUpdater.java [216-222] private void validateNoTransientSensitiveSettings(final Settings transientSettings) { for (String key : transientSettings.keySet()) { if (clusterSettings.isSensitiveSetting(key)) { throw new IllegalArgumentException("sensitive setting [" + key + "] must be updated using persistent settings"); } } + // Also check grouped settings prefixes + for (String key : transientSettings.getAsGroups().keySet()) { + if (clusterSettings.isSensitiveSetting(key)) { + throw new IllegalArgumentException("sensitive setting [" + key + "] must be updated using persistent settings"); + } + } } Suggestion importance[1-10]: 2 __ Why: The suggestion proposes checking getAsGroups() keys, but grouped settings keys are prefixes (like "transport.profiles"), not actual setting keys. The isSensitiveSetting method looks up settings by exact key, so this approach would not correctly identify sensitive settings. Additionally, the Sensitive property requires Dynamic which implies these are concrete settings with exact keys, making the grouped check unnecessary and potentially misleading.	Low

Suggestions up to commit b0ab01c

Category	Suggestion	Impact
General	Explicitly reject conflicting Sensitive and Final properties The Sensitive property validation is placed after the Final and UnmodifiableOnRestore conflict checks, but a setting could theoretically be marked both Sensitive and Final. Since Final settings cannot be dynamic, and Sensitive requires Dynamic, a setting with both Sensitive and Final would trigger the "final setting cannot be dynamic" error first, which is a confusing message. Consider adding an explicit check or ensuring the Sensitive+Final combination is clearly rejected. server/src/main/java/org/opensearch/common/settings/Setting.java [227-229] +if (propertiesAsSet.contains(Property.Dynamic) && propertiesAsSet.contains(Property.Final)) { + throw new IllegalArgumentException("final setting [" + key + "] cannot be dynamic"); +} else if (propertiesAsSet.contains(Property.UnmodifiableOnRestore) && propertiesAsSet.contains(Property.Dynamic)) { + throw new IllegalArgumentException("UnmodifiableOnRestore setting [" + key + "] cannot be dynamic"); +} else if (propertiesAsSet.contains(Property.Sensitive) && propertiesAsSet.contains(Property.Final)) { + throw new IllegalArgumentException("sensitive setting [" + key + "] cannot be final"); +} if (propertiesAsSet.contains(Property.Sensitive) && propertiesAsSet.contains(Property.Dynamic) == false) { throw new IllegalArgumentException("sensitive setting [" + key + "] must be dynamic"); } Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that combining Sensitive and Final would produce a confusing error message. However, the existing check for Sensitive without Dynamic already covers this case indirectly (since Final settings can't be Dynamic), making this an edge case improvement for clarity rather than a critical fix.	Low
	Handle grouped settings in sensitive validation The validation only checks top-level keys but settings can have grouped/prefixed keys (e.g., cluster.routing.allocation.*). Consider also checking whether the setting key matches any sensitive setting via prefix/group matching, similar to how other validation methods in AbstractScopedSettings handle grouped settings. Otherwise, sensitive grouped settings could bypass this check when applied transiently. server/src/main/java/org/opensearch/action/admin/cluster/settings/SettingsUpdater.java [216-222] private void validateNoTransientSensitiveSettings(final Settings transientSettings) { for (String key : transientSettings.keySet()) { if (clusterSettings.isSensitiveSetting(key)) { throw new IllegalArgumentException("sensitive setting [" + key + "] must be updated using persistent settings"); } } + // Also check grouped/prefixed settings + for (String key : transientSettings.keySet()) { + Setting<?> setting = clusterSettings.get(key); + if (setting == null) { + setting = clusterSettings.getConcreteSettingForNamespace(key); + } + if (setting != null && setting.isSensitive()) { + throw new IllegalArgumentException("sensitive setting [" + key + "] must be updated using persistent settings"); + } + } } Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid concern about grouped settings, but the improved_code is flawed — it iterates transientSettings.keySet() twice and the second loop duplicates the first (both check the same keys). The getConcreteSettingForNamespace method may not exist in AbstractScopedSettings, making this suggestion potentially incorrect. The actual gap (if any) would need a different approach.	Low

Suggestions up to commit 1ce627d

Category	Suggestion	Impact
General	Consistent validation structure for property checks The validation for Sensitive should be placed within the else block alongside the other property validations (e.g., Dynamic+Final, UnmodifiableOnRestore+Dynamic), using else if for consistency. Currently, it's placed outside the else if chain, which means it runs independently even when properties is null or empty (though propertiesAsSet would not be populated in that case). More importantly, it should be an else if to maintain consistent validation structure and avoid any unintended execution order issues. server/src/main/java/org/opensearch/common/settings/Setting.java [227-229] -if (propertiesAsSet.contains(Property.Sensitive) && propertiesAsSet.contains(Property.Dynamic) == false) { +} else if (propertiesAsSet.contains(Property.Sensitive) && propertiesAsSet.contains(Property.Dynamic) == false) { throw new IllegalArgumentException("sensitive setting [" + key + "] must be dynamic"); } Suggestion importance[1-10]: 4 __ Why: The suggestion to use else if for consistency is stylistically valid, but the current if statement is functionally correct since propertiesAsSet is always populated within the else block. The change would improve consistency with the existing validation pattern but has no functional impact.	Low

[github-actions]

Persistent review updated to latest commit 79c3855

[github-actions]

❌ Gradle check result for 79c3855: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[finnegancarroll]

If this property is reserved for security relevant settings what do you think about forcing sensitive settings to be persistant as well?

This might prevent users from having unexpected security relevant changes due to a cluster restart, blue green deployment, or a process crash on a single node cluster.

[cwperks]

If this property is reserved for security relevant settings what do you think about forcing sensitive settings to be persistant as well?

This might prevent users from having unexpected security relevant changes due to a cluster restart, blue green deployment, or a process crash on a single node cluster.

That's a good idea. Let me check what it would entail.

[github-actions]

Persistent review updated to latest commit b0ab01c

[github-actions]

❌ Gradle check result for b0ab01c: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for b0ab01c: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[cwperks]

@finnegancarroll I was able to block transient in this PR. Please take a look at the latest changes when you have some time.

[github-actions]

Persistent review updated to latest commit d50d348

[github-actions]

✅ Gradle check result for d50d348: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 83.01158% with 44 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.30%. Comparing base (9f5d0e1) to head (5f0621e).
⚠️ Report is 21 commits behind head on main.

Files with missing lines	Patch %	Lines
...rch/indices/pollingingest/DefaultStreamPoller.java	81.25%	7 Missing and 5 partials ⚠️
...e/internal/InternalDistributionBwcSetupPlugin.java	0.00%	9 Missing ⚠️
...a/org/opensearch/index/engine/IngestionEngine.java	33.33%	5 Missing and 1 partial ⚠️
...rg/opensearch/index/analysis/AnalysisRegistry.java	93.67%	1 Missing and 4 partials ⚠️
...in/java/org/opensearch/index/shard/IndexShard.java	58.33%	4 Missing and 1 partial ⚠️
...arch/index/store/remote/utils/TransferManager.java	66.66%	0 Missing and 2 partials ⚠️
...nalysis/common/SynonymGraphTokenFilterFactory.java	50.00%	1 Missing ⚠️
...g/opensearch/cluster/metadata/IngestionSource.java	94.73%	0 Missing and 1 partial ⚠️
...ensearch/common/util/concurrent/ThreadContext.java	93.33%	0 Missing and 1 partial ⚠️
...bucket/terms/SignificantTextAggregatorFactory.java	0.00%	1 Missing ⚠️
... and 1 more

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #20901      +/-   ##
============================================
- Coverage     73.30%   73.30%   -0.01%     
- Complexity    72484    72560      +76     
============================================
  Files          5819     5819              
  Lines        331155   331385     +230     
  Branches      47840    47885      +45     
============================================
+ Hits         242769   242934     +165     
- Misses        68876    68940      +64     
- Partials      19510    19511       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

Persistent review updated to latest commit 5f0621e

[github-actions]

✅ Gradle check result for 5f0621e: SUCCESS