Skip to content

added changes to consumer grants and source details in crypto-kms plugin. #20114

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
raghav-bhutani wants to merge 1 commit into
base: main
Choose a base branch
from
+134 −21
Open

added changes to consumer grants and source details in crypto-kms plugin. #20114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions libs/common/src/main/java/org/opensearch/common/crypto/MasterKeyProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import org.opensearch.common.annotation.ExperimentalApi;

import java.io.Closeable;
import java.util.List;
import java.util.Map;

/**
Expand Down Expand Up @@ -44,4 +45,10 @@ public interface MasterKeyProvider extends Closeable {
* @return encryption context associated with this master key.
*/
Map<String, String> getEncryptionContext();

/**
* Returns grant tokens for KMS operations.
* @return list of grant tokens or empty list if not configured
*/
List<String> getGrantTokens();
}
51 changes: 47 additions & 4 deletions plugins/crypto-kms/src/main/java/org/opensearch/crypto/kms/KmsClientSettings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ public class KmsClientSettings {
Property.NodeScope
);

/** Source context for grant-based access. */
static final Setting<String> SOURCE_CONTEXT_SETTING = Setting.simpleString("kms.source_context");

private static final Logger logger = LogManager.getLogger(KmsClientSettings.class);

/** Credentials to authenticate with kms. */
Expand Down Expand Up @@ -98,6 +101,9 @@ public class KmsClientSettings {
/** The read timeout for the kms client. */
final int readTimeoutMillis;

/** The source context for the kms client. */
final String sourceContext;

protected KmsClientSettings(
AwsCredentials credentials,
String endpoint,
Expand All @@ -116,6 +122,29 @@ protected KmsClientSettings(
this.proxyUsername = proxyUsername;
this.proxyPassword = proxyPassword;
this.readTimeoutMillis = readTimeoutMillis;
this.sourceContext = "";
}

protected KmsClientSettings(
AwsCredentials credentials,
String endpoint,
String region,
String proxyHost,
int proxyPort,
String proxyUsername,
String proxyPassword,
int readTimeoutMillis,
String sourceContext
) {
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 = sourceContext != null ? sourceContext : "";
}

static AwsCredentials loadCredentials(Settings settings) {
Expand Down Expand Up @@ -181,7 +210,8 @@ static KmsClientSettings getClientSettings(Settings settings) {
PROXY_PORT_SETTING.get(settings),
proxyUsername.toString(),
proxyPassword.toString(),
(int) READ_TIMEOUT_SETTING.get(settings).millis()
(int) READ_TIMEOUT_SETTING.get(settings).millis(),
SOURCE_CONTEXT_SETTING.get(settings)
);
}
}
Expand Down Expand Up @@ -212,6 +242,7 @@ KmsClientSettings getMetadataSettings(Settings settings) {
normalizedSettings,
TimeValue.timeValueMillis(this.readTimeoutMillis)
);
String newSourceContext = getCryptoMetadataSettingOrExisting(SOURCE_CONTEXT_SETTING, normalizedSettings, this.sourceContext);

return new KmsClientSettings(
newCredentials,
Expand All @@ -221,7 +252,8 @@ KmsClientSettings getMetadataSettings(Settings settings) {
newProxyPort,
newProxyUsername,
newProxyPassword,
(int) newReadTimeout.millis()
(int) newReadTimeout.millis(),
newSourceContext
);
}

Expand All @@ -248,11 +280,22 @@ public boolean equals(final Object o) {
&& Objects.equals(proxyHost, that.proxyHost)
&& Objects.equals(proxyPort, that.proxyPort)
&& Objects.equals(proxyUsername, that.proxyUsername)
&& Objects.equals(proxyPassword, that.proxyPassword);
&& Objects.equals(proxyPassword, that.proxyPassword)
&& Objects.equals(sourceContext, that.sourceContext);
}

@Override
public int hashCode() {
return Objects.hash(readTimeoutMillis, credentials, endpoint, region, proxyHost, proxyPort, proxyUsername, proxyPassword);
return Objects.hash(
readTimeoutMillis,
credentials,
endpoint,
region,
proxyHost,
proxyPort,
proxyUsername,
proxyPassword,
sourceContext
);
}
}
47 changes: 39 additions & 8 deletions plugins/crypto-kms/src/main/java/org/opensearch/crypto/kms/KmsMasterKeyProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,15 @@
import org.opensearch.common.crypto.MasterKeyProvider;
import org.opensearch.secure_sm.AccessController;

import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.function.Supplier;

public class KmsMasterKeyProvider implements MasterKeyProvider {
private final Map<String, String> encryptionContext;
private final String keyArn;
private final List<String> grantTokens;
private final Supplier<AmazonKmsClientReference> clientReferenceSupplier;

private static final Logger logger = LogManager.getLogger(KmsMasterKeyProvider.class);
Expand All @@ -38,33 +41,56 @@ public KmsMasterKeyProvider(
) {
this.encryptionContext = encryptionContext;
this.keyArn = keyArn;
this.grantTokens = Collections.emptyList();
this.clientReferenceSupplier = clientReferenceSupplier;
}

public KmsMasterKeyProvider(
Map<String, String> encryptionContext,
String keyArn,
List<String> grantTokens,
Supplier<AmazonKmsClientReference> clientReferenceSupplier
) {
this.encryptionContext = encryptionContext;
this.keyArn = keyArn;
this.grantTokens = grantTokens != null ? grantTokens : Collections.emptyList();
this.clientReferenceSupplier = clientReferenceSupplier;
}

@Override
public DataKeyPair generateDataPair() {
logger.info("Generating new data key pair");
try (AmazonKmsClientReference clientReference = clientReferenceSupplier.get()) {
GenerateDataKeyRequest request = GenerateDataKeyRequest.builder()
GenerateDataKeyRequest.Builder requestBuilder = GenerateDataKeyRequest.builder()
.encryptionContext(encryptionContext)
// Currently only 32 byte data key is supported. To add support for other key sizes add key providers
// in org.opensearch.encryption.CryptoManagerFactory.createCryptoProvider.
.keySpec(DataKeySpec.AES_256)
.keyId(keyArn)
.build();
GenerateDataKeyResponse dataKeyPair = AccessController.doPrivileged(() -> clientReference.get().generateDataKey(request));
.keyId(keyArn);

if (grantTokens != null && !grantTokens.isEmpty()) {
requestBuilder.grantTokens(grantTokens);
}

GenerateDataKeyResponse dataKeyPair = AccessController.doPrivileged(
() -> clientReference.get().generateDataKey(requestBuilder.build())
);
return new DataKeyPair(dataKeyPair.plaintext().asByteArray(), dataKeyPair.ciphertextBlob().asByteArray());
}
}

@Override
public byte[] decryptKey(byte[] encryptedKey) {
try (AmazonKmsClientReference clientReference = clientReferenceSupplier.get()) {
DecryptRequest decryptRequest = DecryptRequest.builder()
DecryptRequest.Builder requestBuilder = DecryptRequest.builder()
.ciphertextBlob(SdkBytes.fromByteArray(encryptedKey))
.encryptionContext(encryptionContext)
.build();
DecryptResponse decryptResponse = AccessController.doPrivileged(() -> clientReference.get().decrypt(decryptRequest));
.encryptionContext(encryptionContext);

if (grantTokens != null && !grantTokens.isEmpty()) {
requestBuilder.grantTokens(grantTokens);
}

DecryptResponse decryptResponse = AccessController.doPrivileged(() -> clientReference.get().decrypt(requestBuilder.build()));
return decryptResponse.plaintext().asByteArray();
}
}
Expand All @@ -81,4 +107,9 @@ public Map<String, String> getEncryptionContext() {

@Override
public void close() {}

@Override
public List<String> getGrantTokens() {
return grantTokens;
}
}
50 changes: 41 additions & 9 deletions plugins/crypto-kms/src/main/java/org/opensearch/crypto/kms/KmsService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,11 @@
import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider;
import software.amazon.awssdk.core.client.config.ClientOverrideConfiguration;
import software.amazon.awssdk.core.exception.SdkException;
import software.amazon.awssdk.core.interceptor.Context;
import software.amazon.awssdk.core.interceptor.ExecutionAttributes;
import software.amazon.awssdk.core.interceptor.ExecutionInterceptor;
import software.amazon.awssdk.core.retry.RetryPolicy;
import software.amazon.awssdk.http.SdkHttpRequest;
import software.amazon.awssdk.http.apache.ApacheHttpClient;
import software.amazon.awssdk.http.apache.ProxyConfiguration;
import software.amazon.awssdk.profiles.ProfileFileSystemSetting;
Expand All @@ -36,7 +40,9 @@
import java.net.URISyntaxException;
import java.time.Duration;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;

Expand All @@ -54,6 +60,8 @@ public class KmsService implements Closeable {

static final Setting<String> KEY_ARN_SETTING = Setting.simpleString("kms.key_arn", Setting.Property.NodeScope);

static final Setting<String> GRANT_TOKENS_SETTING = Setting.simpleString("kms.grant_tokens", Setting.Property.NodeScope);

private volatile Map<KmsClientSettings, AmazonKmsClientReference> clientsCache = emptyMap();

/**
Expand All @@ -73,7 +81,7 @@ public KmsService() {
private KmsClient buildClient(KmsClientSettings clientSettings) {
AccessController.doPrivileged(KmsService::setDefaultAwsProfilePath);
final AwsCredentialsProvider awsCredentialsProvider = buildCredentials(clientSettings);
final ClientOverrideConfiguration overrideConfiguration = buildOverrideConfiguration();
final ClientOverrideConfiguration overrideConfiguration = buildOverrideConfiguration(clientSettings);
final ProxyConfiguration proxyConfiguration = AccessController.doPrivileged(() -> buildProxyConfiguration(clientSettings));
return buildClient(
awsCredentialsProvider,
Expand Down Expand Up @@ -133,6 +141,24 @@ ProxyConfiguration buildProxyConfiguration(KmsClientSettings clientSettings) {
}
}

ClientOverrideConfiguration buildOverrideConfiguration(KmsClientSettings clientSettings) {
ClientOverrideConfiguration.Builder builder = ClientOverrideConfiguration.builder().retryPolicy(buildRetryPolicy());

if (Strings.hasText(clientSettings.sourceContext)) {
Map<String, String> sourceHeaders = parseKeyValuePairs(clientSettings.sourceContext, "source_context");
builder.addExecutionInterceptor(new ExecutionInterceptor() {
@Override
public SdkHttpRequest modifyHttpRequest(Context.ModifyHttpRequest context, ExecutionAttributes executionAttributes) {
SdkHttpRequest.Builder requestBuilder = context.httpRequest().toBuilder();
sourceHeaders.forEach((key, value) -> { requestBuilder.putHeader(key, value); });
return requestBuilder.build();
}
});
}

return builder.build();
}

ClientOverrideConfiguration buildOverrideConfiguration() {
return ClientOverrideConfiguration.builder().retryPolicy(buildRetryPolicy()).build();
}
Expand Down Expand Up @@ -252,22 +278,28 @@ public MasterKeyProvider createMasterKeyProvider(CryptoMetadata cryptoMetadata)
String kmsEncCtx = ENC_CTX_SETTING.get(cryptoSettings);
Map<String, String> encCtx;
if (Strings.hasText(kmsEncCtx)) {
try {
encCtx = Arrays.stream(kmsEncCtx.split(","))
.map(s -> s.split("="))
.collect(Collectors.toMap(e -> e[0].trim(), e -> e[1].trim()));
} catch (Exception ex) {
throw new IllegalArgumentException(ENC_CTX_SETTING.getKey() + " Format should be: Name1=Value1, Name2=Value2");
}
encCtx = parseKeyValuePairs(kmsEncCtx, ENC_CTX_SETTING.getKey());
} else {
encCtx = new HashMap<>();
}

// Extract grant details
String grantTokensStr = GRANT_TOKENS_SETTING.get(cryptoSettings);
List<String> grantTokens = Strings.hasText(grantTokensStr) ? Arrays.asList(grantTokensStr.split(",")) : Collections.emptyList();
Comment on lines +286 to +288
Copy link

@coderabbitai coderabbitai bot Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
// 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".


// Verify client creation is successful to early detect any failure.
try (AmazonKmsClientReference clientReference = client(cryptoMetadata)) {
clientReference.get();
}

return new KmsMasterKeyProvider(encCtx, keyArn, () -> client(cryptoMetadata));
return new KmsMasterKeyProvider(encCtx, keyArn, grantTokens, () -> client(cryptoMetadata));
}

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");
}
}
Comment on lines +298 to 304
Copy link

@coderabbitai coderabbitai bot Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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.

}
Loading