added changes to consumer grants and source details in crypto-kms plugin. #20114
Conversation
Signed-off-by: raghav-bhutani <[email protected]>
WalkthroughThe changes introduce grant tokens and source context support to the KMS encryption infrastructure. A new interface method Changes
Sequence DiagramsequenceDiagram
participant Config as Config/Settings
participant Service as KmsService
participant Settings as KmsClientSettings
participant Provider as KmsMasterKeyProvider
participant KMSClient as KMS Client
participant HTTP as HTTP Layer
Config->>Service: Parse GRANT_TOKENS_SETTING & SOURCE_CONTEXT_SETTING
Service->>Settings: createClientSettings(sourceContext)
Settings->>HTTP: buildClient() with sourceContext
HTTP->>HTTP: buildOverrideConfiguration(sourceContext)
Note over HTTP: Install ExecutionInterceptor<br/>to inject source_context header
Service->>Provider: createMasterKeyProvider(grantTokens, clientRef)
Provider->>Provider: Store grantTokens
Provider->>KMSClient: GenerateDataKeyRequest (with grantTokens if present)
KMSClient->>HTTP: Execute with source_context header
HTTP->>KMSClient: Response
Provider->>KMSClient: DecryptRequest (with grantTokens if present)
KMSClient->>HTTP: Execute with source_context header
HTTP->>KMSClient: Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
libs/common/src/main/java/org/opensearch/common/crypto/MasterKeyProvider.java (1)
49-53: KMS-specific terminology in a generic interface.The Javadoc mentions "KMS operations" but
MasterKeyProvideris a generic interface inlibs/common. Consider using more generic terminology like "cryptographic operations" to maintain abstraction and support potential non-KMS implementations./** - * Returns grant tokens for KMS operations. + * Returns grant tokens for cryptographic operations. * @return list of grant tokens or empty list if not configured */ List<String> getGrantTokens();plugins/crypto-kms/src/main/java/org/opensearch/crypto/kms/KmsMasterKeyProvider.java (4)
71-73: Redundant null check forgrantTokens.Since
grantTokensis always initialized (either toCollections.emptyList()in the first constructor or via null-safe assignment in the second), thegrantTokens != nullcheck is redundant. Only the emptiness check is necessary.- if (grantTokens != null && !grantTokens.isEmpty()) { + if (!grantTokens.isEmpty()) { requestBuilder.grantTokens(grantTokens); }
89-91: Same redundant null check pattern.Same observation as above—
grantTokenscan never be null due to constructor initialization.- if (grantTokens != null && !grantTokens.isEmpty()) { + if (!grantTokens.isEmpty()) { requestBuilder.grantTokens(grantTokens); }
111-114: Consider returning an unmodifiable view ofgrantTokens.Returning the internal list directly allows callers to mutate it. Since
grantTokensmay be the caller-supplied list (not defensively copied in the constructor), returning it directly could lead to unexpected behavior if modified externally.@Override public List<String> getGrantTokens() { - return grantTokens; + return Collections.unmodifiableList(grantTokens); }Alternatively, wrap the list in an unmodifiable view in the constructor at line 56.
54-56: Defensive copy recommended forgrantTokensparameter.The constructor stores the caller-provided list directly. If the caller later modifies that list, the internal state of this provider changes unexpectedly.
- this.grantTokens = grantTokens != null ? grantTokens : Collections.emptyList(); + this.grantTokens = grantTokens != null ? List.copyOf(grantTokens) : Collections.emptyList();
List.copyOf()(Java 10+) creates an immutable copy. Alternatively, useCollections.unmodifiableList(new ArrayList<>(grantTokens))for older Java versions.plugins/crypto-kms/src/main/java/org/opensearch/crypto/kms/KmsClientSettings.java (2)
67-68: MissingProperty.NodeScopeonSOURCE_CONTEXT_SETTING.Other similar settings like
ENC_CTX_SETTINGandKEY_ARN_SETTINGinKmsServiceincludeSetting.Property.NodeScope. For consistency and proper settings behavior, consider adding it here./** Source context for grant-based access. */ - static final Setting<String> SOURCE_CONTEXT_SETTING = Setting.simpleString("kms.source_context"); + static final Setting<String> SOURCE_CONTEXT_SETTING = Setting.simpleString("kms.source_context", Property.NodeScope);
107-148: Consider using constructor chaining to reduce duplication.The two constructors share significant code. You could have the shorter constructor delegate to the longer one.
protected KmsClientSettings( AwsCredentials credentials, String endpoint, String region, String proxyHost, int proxyPort, String proxyUsername, String proxyPassword, int readTimeoutMillis ) { - this.credentials = credentials; - this.endpoint = endpoint; - this.region = region; - this.proxyHost = proxyHost; - this.proxyPort = proxyPort; - this.proxyUsername = proxyUsername; - this.proxyPassword = proxyPassword; - this.readTimeoutMillis = readTimeoutMillis; - this.sourceContext = ""; + this(credentials, endpoint, region, proxyHost, proxyPort, proxyUsername, proxyPassword, readTimeoutMillis, ""); }plugins/crypto-kms/src/main/java/org/opensearch/crypto/kms/KmsService.java (2)
153-153: Lambda can be simplified to method reference.Minor style improvement.
- sourceHeaders.forEach((key, value) -> { requestBuilder.putHeader(key, value); }); + sourceHeaders.forEach(requestBuilder::putHeader);
144-160: Consider logging when source context headers are added.For debugging and audit purposes, it would be helpful to log when custom headers are being injected into KMS requests, similar to how other configuration is logged (e.g., lines 116-117, 121-122).
if (Strings.hasText(clientSettings.sourceContext)) { Map<String, String> sourceHeaders = parseKeyValuePairs(clientSettings.sourceContext, "source_context"); + logger.debug("adding source context headers to KMS requests: {}", sourceHeaders.keySet()); builder.addExecutionInterceptor(new ExecutionInterceptor() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libs/common/src/main/java/org/opensearch/common/crypto/MasterKeyProvider.java(2 hunks)plugins/crypto-kms/src/main/java/org/opensearch/crypto/kms/KmsClientSettings.java(7 hunks)plugins/crypto-kms/src/main/java/org/opensearch/crypto/kms/KmsMasterKeyProvider.java(3 hunks)plugins/crypto-kms/src/main/java/org/opensearch/crypto/kms/KmsService.java(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/crypto-kms/src/main/java/org/opensearch/crypto/kms/KmsMasterKeyProvider.java (1)
libs/agent-sm/agent-policy/src/main/java/org/opensearch/secure_sm/AccessController.java (1)
AccessController(44-144)
⏰ 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). (1)
- GitHub Check: gradle-check
🔇 Additional comments (1)
plugins/crypto-kms/src/main/java/org/opensearch/crypto/kms/KmsService.java (1)
149-156: Verify cache key composition in KmsService.Unable to access the repository to confirm whether
ClientOverrideConfigurationis part of the cache key identity. The concern that anonymousExecutionInterceptoraffects client caching is theoretically valid only ifClientOverrideConfiguration(or an object containing it) is used as a cache key. AWS SDK v2'sClientOverrideConfigurationdoes not overrideequals()/hashCode(), so it relies on object identity.However, if
clientsCachekeys are based on a separateKmsClientSettingsobject that does not include the configuration instance itself, the anonymous class would not affect cache behavior. Clarification needed: Verify whetherbuildOverrideConfiguration()is called fresh on each invocation and confirm ifClientOverrideConfigurationparticipates in the cache key logic.
| // Extract grant details | ||
| String grantTokensStr = GRANT_TOKENS_SETTING.get(cryptoSettings); | ||
| List<String> grantTokens = Strings.hasText(grantTokensStr) ? Arrays.asList(grantTokensStr.split(",")) : Collections.emptyList(); |
There was a problem hiding this comment.
Grant tokens are not trimmed after splitting.
Unlike parseKeyValuePairs which trims whitespace, splitting grant tokens by comma retains any surrounding whitespace (e.g., "grant1, grant2" yields [" grant2"]). The testing logs show spaces: [grant1, grant2, grant3].
- List<String> grantTokens = Strings.hasText(grantTokensStr) ? Arrays.asList(grantTokensStr.split(",")) : Collections.emptyList();
+ List<String> grantTokens = Strings.hasText(grantTokensStr)
+ ? Arrays.stream(grantTokensStr.split(",")).map(String::trim).collect(Collectors.toList())
+ : Collections.emptyList();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Extract grant details | |
| String grantTokensStr = GRANT_TOKENS_SETTING.get(cryptoSettings); | |
| List<String> grantTokens = Strings.hasText(grantTokensStr) ? Arrays.asList(grantTokensStr.split(",")) : Collections.emptyList(); | |
| // Extract grant details | |
| String grantTokensStr = GRANT_TOKENS_SETTING.get(cryptoSettings); | |
| List<String> grantTokens = Strings.hasText(grantTokensStr) | |
| ? Arrays.stream(grantTokensStr.split(",")).map(String::trim).collect(Collectors.toList()) | |
| : Collections.emptyList(); |
🤖 Prompt for AI Agents
In plugins/crypto-kms/src/main/java/org/opensearch/crypto/kms/KmsService.java
around lines 286 to 288, the grant token string is split on commas but
individual tokens are not trimmed so tokens can retain surrounding whitespace;
after splitting, trim each token and filter out empty tokens (e.g., using a
stream or loop to map String::trim and discard zero-length entries) so the
resulting List<String> contains clean tokens like "grant2" rather than "
grant2".
| private static Map<String, String> parseKeyValuePairs(String input, String settingName) { | ||
| try { | ||
| return Arrays.stream(input.split(",")).map(s -> s.split("=")).collect(Collectors.toMap(e -> e[0].trim(), e -> e[1].trim())); | ||
| } catch (Exception ex) { | ||
| throw new IllegalArgumentException(settingName + " Format should be: Name1=Value1, Name2=Value2"); | ||
| } | ||
| } |
There was a problem hiding this comment.
parseKeyValuePairs will throw ArrayIndexOutOfBoundsException for malformed input.
If the input contains an entry without an = (e.g., "key1=value1,invalidkey"), accessing e[1] will throw ArrayIndexOutOfBoundsException. The generic Exception catch then wraps it, losing the specific error context. Consider explicit validation.
private static Map<String, String> parseKeyValuePairs(String input, String settingName) {
- try {
- return Arrays.stream(input.split(",")).map(s -> s.split("=")).collect(Collectors.toMap(e -> e[0].trim(), e -> e[1].trim()));
- } catch (Exception ex) {
- throw new IllegalArgumentException(settingName + " Format should be: Name1=Value1, Name2=Value2");
- }
+ Map<String, String> result = new HashMap<>();
+ for (String pair : input.split(",")) {
+ String[] keyValue = pair.split("=", 2);
+ if (keyValue.length != 2) {
+ throw new IllegalArgumentException(
+ settingName + " contains invalid entry '" + pair.trim() + "'. Format should be: Name1=Value1,Name2=Value2"
+ );
+ }
+ result.put(keyValue[0].trim(), keyValue[1].trim());
+ }
+ return result;
}Using split("=", 2) also handles values containing = characters.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static Map<String, String> parseKeyValuePairs(String input, String settingName) { | |
| try { | |
| return Arrays.stream(input.split(",")).map(s -> s.split("=")).collect(Collectors.toMap(e -> e[0].trim(), e -> e[1].trim())); | |
| } catch (Exception ex) { | |
| throw new IllegalArgumentException(settingName + " Format should be: Name1=Value1, Name2=Value2"); | |
| } | |
| } | |
| private static Map<String, String> parseKeyValuePairs(String input, String settingName) { | |
| Map<String, String> result = new HashMap<>(); | |
| for (String pair : input.split(",")) { | |
| String[] keyValue = pair.split("=", 2); | |
| if (keyValue.length != 2) { | |
| throw new IllegalArgumentException( | |
| settingName + " contains invalid entry '" + pair.trim() + "'. Format should be: Name1=Value1,Name2=Value2" | |
| ); | |
| } | |
| result.put(keyValue[0].trim(), keyValue[1].trim()); | |
| } | |
| return result; | |
| } |
🤖 Prompt for AI Agents
In plugins/crypto-kms/src/main/java/org/opensearch/crypto/kms/KmsService.java
around lines 298 to 304, the current parsing can throw
ArrayIndexOutOfBoundsException for malformed entries and the generic catch hides
context; change the method to split each entry with split("=", 2), trim both
sides, validate that the result has exactly 2 non-empty parts, and if any entry
is invalid throw an IllegalArgumentException including settingName and the
offending entry; remove the broad Exception catch (or catch only expected
parsing exceptions) so real errors are not masked and support values containing
"=" by using the limit=2 split.
|
❌ Gradle check result for cb89d94: 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? |
1 participant
Description
Added support for AWS KMS grant tokens and source context to enable more flexible access control for KMS operations. This change extends the MasterKeyProvider interface and KMS implementation to pass grant tokens during encryption/decryption operations and adds source context headers to KMS requests.
Related Issues
Testing
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
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.