Make crypto store settings immutable #20123
Conversation
WalkthroughAdds validation to prevent changes to crypto-related index.store settings and to forbid switching to or from the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Rajat Gupta <gptrajat@amazon.com>
RajatGupta02
force-pushed
the
ile_disallow_settings_update
branch
from
e9c8610 to
65126b1
Compare
|
|
||
| // Validate store type changes | ||
| String newStoreType = indexSettings.get("index.store.type"); | ||
| if ("cryptofs".equals(newStoreType)) { |
There was a problem hiding this comment.
and vice versa? if you have cryptofs as store type it cannot be modified.
There was a problem hiding this comment.
this could be included in restrictedCryptoSettings.
There was a problem hiding this comment.
yeah we should also prevent cryptofs -> non-cryptofs update. But it shouldn't be included in restrictedCryptoSettings as it will prevent prevent ALL store type changes for all indices.
|
❌ Gradle check result for 65126b1: 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? |
Signed-off-by: Rajat Gupta <gptrajat@amazon.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java (3)
145-145: Consider moving crypto validation inside the execute() method for consistency.The validation uses
clusterService.state(), which gets the cluster state at submission time. However,validateSearchReplicaCountSettings(line 296) performs similar index-specific validation inside theexecute()method using the execution-timecurrentState. MovingvalidateCryptoStoreSettingsinsideexecute()would ensure validation against the authoritative state and maintain consistency with the pattern used for other index-specific validations.While the bidirectional nature of the cryptofs checks (blocking both TO and FROM) provides inherent safety against race conditions, aligning with the existing pattern would be clearer.
594-608: LGTM! Clear validation logic for crypto setting immutability.The validation correctly enforces that crypto settings cannot be modified after index creation. The straightforward check against
indexSettings.keySet()is appropriate and efficient.Optional improvements:
- Extract the restricted settings array as a class-level constant for better maintainability:
+ private static final String[] RESTRICTED_CRYPTO_SETTINGS = { + "index.store.crypto.key_provider", + "index.store.crypto.kms.key_arn", + "index.store.crypto.kms.encryption_context" + }; + /** - * Validates crypto store settings are immutable after index creation. + * Validates that crypto-related store settings are immutable after index creation. + * Prevents updates to crypto configuration settings and validates store type changes + * to/from cryptofs are not permitted. */ public static void validateCryptoStoreSettings(Settings indexSettings, Index[] indices, ClusterState clusterState) { - final String[] restrictedCryptoSettings = { - "index.store.crypto.key_provider", - "index.store.crypto.kms.key_arn", - "index.store.crypto.kms.encryption_context" }; - // Crypto settings are completely immutable - reject any attempt to modify them - for (String settingKey : restrictedCryptoSettings) { + for (String settingKey : RESTRICTED_CRYPTO_SETTINGS) {
- Enhanced JavaDoc provides clearer documentation of the method's purpose.
610-628: LGTM! Bidirectional cryptofs validation correctly implemented.The logic properly prevents both changing TO cryptofs and changing FROM cryptofs, addressing the concern raised in past review comments. The approach of checking store type changes separately from the other crypto settings is appropriate, as it allows this validation to be granular (only affecting cryptofs) rather than blocking all store type changes.
Minor enhancement: The error messages could be more consistent in explaining the rationale:
// Prevent changing TO cryptofs if ("cryptofs".equals(newStoreType) && !"cryptofs".equals(currentStoreType)) { - throw new IllegalArgumentException("Cannot change store type to 'cryptofs' for index [" + index.getName() + "]"); + throw new IllegalArgumentException( + "Cannot change store type to 'cryptofs' for index [" + index.getName() + "] - cryptofs store type is immutable" + ); }Based on past review comments, the decision to handle store type separately from
restrictedCryptoSettingsis correct, as including it there would prevent all store type changes for all indices.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: gradle-check
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: Analyze (java)
- GitHub Check: detect-breaking-change
🔇 Additional comments (1)
server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java (1)
594-629: Well-structured validation method that successfully enforces crypto setting immutability.The implementation effectively achieves the PR objective by:
- ✅ Preventing updates to crypto configuration settings (key_provider, kms.key_arn, kms.encryption_context)
- ✅ Preventing store type changes to cryptofs from non-cryptofs
- ✅ Preventing store type changes from cryptofs to non-cryptofs
- ✅ Addressing the bidirectional requirement mentioned in past review comments
The validation logic is sound and the error messages are informative.
|
cc @cwperks , could you please review? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #20123 +/- ##
============================================
- Coverage 73.33% 73.32% -0.02%
+ Complexity 71679 71645 -34
============================================
Files 5790 5786 -4
Lines 327549 327769 +220
Branches 47181 47212 +31
============================================
+ Hits 240217 240329 +112
- Misses 68080 68151 +71
- Partials 19252 19289 +37 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| final String[] restrictedCryptoSettings = { | ||
| "index.store.crypto.key_provider", | ||
| "index.store.crypto.kms.key_arn", | ||
| "index.store.crypto.kms.encryption_context" }; |
There was a problem hiding this comment.
I think this can be done through properties like Property.Filtered or Property.InternalIndex
The last param in https://github.com/opensearch-project/OpenSearch/blob/main/plugins/crypto-kms/src/main/java/org/opensearch/crypto/kms/KmsService.java#L53 is a vararg list of properties.
There was a problem hiding this comment.
Oh, yes, this is neat, didn't know about this, thanks!
3 participants
Description
Opensearch-storage-encryption plugin introduces some index settings which should be immutable after index creation. This PR adds the validation to not allow these crypto settings update.
Check List
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.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.