From 3c02507cc53407d7d8891f8a31179352285e336d Mon Sep 17 00:00:00 2001 From: sahusanket Date: Mon, 17 Aug 2026 17:00:40 +0530 Subject: [PATCH 1/3] CDAP-21261 : Lease or Locking support for Secure Store - RTR Oauth --- .../api/security/store/SecureStoreLease.java | 86 ++++++++ .../security/store/SecureStoreManager.java | 35 ++++ .../internal/app/runtime/DefaultAdmin.java | 18 ++ .../CloudSecretManagerClient.java | 27 +++ .../cloudsecretmanager/GcpSecretManager.java | 90 +++++++++ .../gcp/cloudsecretmanager/WrappedSecret.java | 51 ++++- .../GcpSecretManagerTest.java | 188 ++++++++++++++++++ .../cdap/securestore/spi/SecretLease.java | 86 ++++++++ .../cdap/securestore/spi/SecretManager.java | 35 ++++ .../store/DefaultSecureStoreService.java | 23 +++ .../security/store/SecureStoreHandler.java | 43 ++++ .../store/client/RemoteSecureStore.java | 32 +++ .../SecretManagerSecureStoreService.java | 33 +++ .../secretmanager/MockSecretManager.java | 22 ++ .../SecretManagerSecureStoreServiceTest.java | 18 ++ 15 files changed, 785 insertions(+), 2 deletions(-) create mode 100644 cdap-api/src/main/java/io/cdap/cdap/api/security/store/SecureStoreLease.java create mode 100644 cdap-securestore-spi/src/main/java/io/cdap/cdap/securestore/spi/SecretLease.java diff --git a/cdap-api/src/main/java/io/cdap/cdap/api/security/store/SecureStoreLease.java b/cdap-api/src/main/java/io/cdap/cdap/api/security/store/SecureStoreLease.java new file mode 100644 index 000000000000..bb34aa9c642a --- /dev/null +++ b/cdap-api/src/main/java/io/cdap/cdap/api/security/store/SecureStoreLease.java @@ -0,0 +1,86 @@ +/* + * Copyright © 2026 Cask Data, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ + +package io.cdap.cdap.api.security.store; + +import java.util.Objects; + +/** + * API model representing a distributed lease lock on a secret or credential update in SecureStore. + *

+ * Note: This is the caller-facing API counterpart to {@code io.cdap.cdap.securestore.spi.lease.SecretLease} + * in the SPI layer, mirroring the {@link io.cdap.cdap.api.security.store.SecureStoreMetadata} vs {@code SecretMetadata} + * architectural pattern in CDAP. + */ +public class SecureStoreLease { + private final boolean acquired; + private final String lockTimestamp; + private final String lockHolder; + + private SecureStoreLease(boolean acquired, String lockTimestamp, String lockHolder) { + this.acquired = acquired; + this.lockTimestamp = lockTimestamp; + this.lockHolder = lockHolder; + } + + public static SecureStoreLease acquired(String lockTimestamp, String lockHolder) { + return new SecureStoreLease(true, lockTimestamp, lockHolder); + } + + public static SecureStoreLease failed() { + return new SecureStoreLease(false, null, null); + } + + public boolean isAcquired() { + return acquired; + } + + public String getLockTimestamp() { + return lockTimestamp; + } + + public String getLockHolder() { + return lockHolder; + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + SecureStoreLease that = (SecureStoreLease) o; + return acquired == that.acquired + && Objects.equals(lockTimestamp, that.lockTimestamp) + && Objects.equals(lockHolder, that.lockHolder); + } + + @Override + public int hashCode() { + return Objects.hash(acquired, lockTimestamp, lockHolder); + } + + @Override + public String toString() { + return "SecureStoreLease{" + + "acquired=" + acquired + + ", lockTimestamp='" + lockTimestamp + '\'' + + ", lockHolder='" + lockHolder + '\'' + + '}'; + } +} diff --git a/cdap-api/src/main/java/io/cdap/cdap/api/security/store/SecureStoreManager.java b/cdap-api/src/main/java/io/cdap/cdap/api/security/store/SecureStoreManager.java index 401232634e81..8d5cfd51ba84 100644 --- a/cdap-api/src/main/java/io/cdap/cdap/api/security/store/SecureStoreManager.java +++ b/cdap-api/src/main/java/io/cdap/cdap/api/security/store/SecureStoreManager.java @@ -67,4 +67,39 @@ default void put(String namespace, String name, String data, @Nullable String de * @throws Exception If the specified namespace or name does not exist */ void delete(String namespace, String name) throws Exception; + + /** + * Checks if the underlying secure store implementation supports distributed lease locking. + * + * @return true if lease locking is supported, false otherwise. + */ + default boolean isLeaseSupported() throws IOException { + return false; + } + + /** + * Attempts to acquire a lease lock on a secret resource. + * + * @param namespace The namespace that this key belongs to + * @param name Name of the secure key + * @param timeoutMs Lock timeout in milliseconds before lease is considered expired + * @return {@link SecureStoreLease} indicating acquisition success and lock details + * @throws Exception If lock acquisition fails due to underlying storage errors + */ + default SecureStoreLease acquireLease(String namespace, String name, long timeoutMs, + String lockHolder) throws Exception { + throw new UnsupportedOperationException("Distributed leases are not supported by this SecureStore implementation."); + } + + /** + * Releases an acquired lease lock on a secret resource. + * + * @param namespace The namespace that this key belongs to + * @param name Name of the secure key + * @param lease {@link SecureStoreLease} to release + * @throws Exception If lock release fails due to underlying storage errors + */ + default void releaseLease(String namespace, String name, SecureStoreLease lease) throws Exception { + throw new UnsupportedOperationException("Distributed leases are not supported by this SecureStore implementation."); + } } diff --git a/cdap-app-fabric/src/main/java/io/cdap/cdap/internal/app/runtime/DefaultAdmin.java b/cdap-app-fabric/src/main/java/io/cdap/cdap/internal/app/runtime/DefaultAdmin.java index d325135ef0f0..6d9f86a38a12 100644 --- a/cdap-app-fabric/src/main/java/io/cdap/cdap/internal/app/runtime/DefaultAdmin.java +++ b/cdap-app-fabric/src/main/java/io/cdap/cdap/internal/app/runtime/DefaultAdmin.java @@ -22,6 +22,7 @@ import io.cdap.cdap.api.messaging.TopicAlreadyExistsException; import io.cdap.cdap.api.messaging.TopicNotFoundException; import io.cdap.cdap.api.security.store.SecureStoreManager; +import io.cdap.cdap.api.security.store.SecureStoreLease; import io.cdap.cdap.common.NamespaceNotFoundException; import io.cdap.cdap.common.namespace.NamespaceQueryAdmin; import io.cdap.cdap.common.service.Retries; @@ -91,6 +92,23 @@ public void delete(String namespace, String name) throws Exception { Retries.runWithRetries(() -> secureStoreManager.delete(namespace, name), retryStrategy); } + @Override + public boolean isLeaseSupported() { + return secureStoreManager.isLeaseSupported(); + } + + @Override + public SecureStoreLease acquireLease(String namespace, String name, long timeoutMs, + String lockHolder) throws Exception { + return Retries.callWithRetries( + () -> secureStoreManager.acquireLease(namespace, name, timeoutMs, lockHolder), retryStrategy); + } + + @Override + public void releaseLease(String namespace, String name, SecureStoreLease lease) throws Exception { + Retries.runWithRetries(() -> secureStoreManager.releaseLease(namespace, name, lease), retryStrategy); + } + @Override public void createTopic(final String topic) throws TopicAlreadyExistsException, IOException { if (messagingAdmin == null) { diff --git a/cdap-securestore-ext-gcp-secretstore/src/main/java/io/cdap/cdap/securestore/gcp/cloudsecretmanager/CloudSecretManagerClient.java b/cdap-securestore-ext-gcp-secretstore/src/main/java/io/cdap/cdap/securestore/gcp/cloudsecretmanager/CloudSecretManagerClient.java index f9e23dcce5cc..42d47cbd4f0a 100644 --- a/cdap-securestore-ext-gcp-secretstore/src/main/java/io/cdap/cdap/securestore/gcp/cloudsecretmanager/CloudSecretManagerClient.java +++ b/cdap-securestore-ext-gcp-secretstore/src/main/java/io/cdap/cdap/securestore/gcp/cloudsecretmanager/CloudSecretManagerClient.java @@ -19,6 +19,7 @@ import com.google.api.gax.core.CredentialsProvider; import com.google.api.gax.core.FixedCredentialsProvider; import com.google.api.gax.rpc.ApiException; +import com.google.api.gax.rpc.StatusCode; import com.google.auth.oauth2.GoogleCredentials; import com.google.cloud.ServiceOptions; import com.google.cloud.secretmanager.v1.AddSecretVersionRequest; @@ -175,6 +176,32 @@ public void updateSecret(WrappedSecret wrappedSecret, long ttlInSeconds) { fieldMask.build()); } + /** + * Conditionally updates annotations on the specified secret using an ETag for optimistic concurrency control. + * + * @param namespace CDAP secret namespace + * @param name CDAP secret name + * @param annotationsToUpdate Map of annotations to update + * @param etag The expected ETag of the secret + */ + public void updateSecretAnnotations(String namespace, + String name, + Map annotationsToUpdate, + String etag) { + String resourceName = getSecretResourceName(namespace, name); + Secret.Builder secretBuilder = Secret.newBuilder() + .setName(resourceName) + .setEtag(etag); + + for (Map.Entry entry : annotationsToUpdate.entrySet()) { + secretBuilder.putAnnotations(entry.getKey(), entry.getValue()); + } + + secretManager.updateSecret( + secretBuilder.build(), + FieldMask.newBuilder().addPaths("annotations").build()); + } + /** * Deletes the specified secret. * diff --git a/cdap-securestore-ext-gcp-secretstore/src/main/java/io/cdap/cdap/securestore/gcp/cloudsecretmanager/GcpSecretManager.java b/cdap-securestore-ext-gcp-secretstore/src/main/java/io/cdap/cdap/securestore/gcp/cloudsecretmanager/GcpSecretManager.java index d73e8d411432..6b9bef7408c9 100644 --- a/cdap-securestore-ext-gcp-secretstore/src/main/java/io/cdap/cdap/securestore/gcp/cloudsecretmanager/GcpSecretManager.java +++ b/cdap-securestore-ext-gcp-secretstore/src/main/java/io/cdap/cdap/securestore/gcp/cloudsecretmanager/GcpSecretManager.java @@ -19,6 +19,7 @@ import com.google.api.gax.rpc.ApiException; import com.google.api.gax.rpc.StatusCode; import com.google.common.annotations.VisibleForTesting; +import io.cdap.cdap.securestore.spi.SecretLease; import io.cdap.cdap.securestore.spi.SecretManager; import io.cdap.cdap.securestore.spi.SecretManagerContext; import io.cdap.cdap.securestore.spi.SecretNotFoundException; @@ -27,7 +28,11 @@ import java.io.IOException; import java.util.Arrays; import java.util.Collection; +import java.util.HashMap; +import java.util.Map; import java.util.stream.Collectors; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * SecretManager implementation backed by GCP's service also called "Secret Manager" @@ -42,10 +47,18 @@ * the total size of metadata associated with any given secret cannot exceed 16 KiB. */ public class GcpSecretManager implements SecretManager { + private static final Logger LOG = LoggerFactory.getLogger(GcpSecretManager.class); private static final String PROVIDER_NAME = "gcp-secretmanager"; private CloudSecretManagerClient client; + private static final String ANNOTATION_LEASE_STATE = "lease_state"; + private static final String ANNOTATION_LEASE_ACQUIRED_TIME_MS = "lease_acquired_time_ms"; + private static final String ANNOTATION_LEASE_HOLDER = "lease_holder"; + + private static final String STATE_IDLE = "idle"; + private static final String STATE_REFRESHING = "refreshing"; + @Override public String getName() { return PROVIDER_NAME; @@ -148,4 +161,81 @@ public void delete(String namespace, String name) throws SecretNotFoundException public void destroy(SecretManagerContext context) { client.destroy(); } + + @Override + public boolean isLeaseSupported() { + return true; + } + + @Override + public SecretLease acquireLease(String namespace, String key, long timeoutMs, String lockHolder) throws IOException { + try { + WrappedSecret refreshSecret = client.getSecret(namespace, key); + String currentEtag = refreshSecret.getEtag() == null ? "" : refreshSecret.getEtag(); + + String state = refreshSecret.getAnnotation(ANNOTATION_LEASE_STATE, STATE_IDLE); + long lockTimestamp = 0L; + try { + lockTimestamp = Long.parseLong(refreshSecret.getAnnotation(ANNOTATION_LEASE_ACQUIRED_TIME_MS, "0")); + } catch (NumberFormatException e) { + // ignore invalid timestamp + } + + String currentLockHolder = refreshSecret.getAnnotation(ANNOTATION_LEASE_HOLDER, ""); + long now = System.currentTimeMillis(); + boolean isExpired = (now - lockTimestamp) > timeoutMs; + + if (STATE_REFRESHING.equalsIgnoreCase(state) && !isExpired && !lockHolder.equals(currentLockHolder)) { + return SecretLease.failed(); + } + + Map annotationsToUpdate = new HashMap<>(refreshSecret.getAnnotations()); + annotationsToUpdate.put(ANNOTATION_LEASE_STATE, STATE_REFRESHING); + annotationsToUpdate.put(ANNOTATION_LEASE_ACQUIRED_TIME_MS, String.valueOf(now)); + annotationsToUpdate.put(ANNOTATION_LEASE_HOLDER, lockHolder); + + client.updateSecretAnnotations(namespace, key, annotationsToUpdate, currentEtag); + return SecretLease.acquired(String.valueOf(now), lockHolder); + + } catch (ApiException e) { + if (e.getStatusCode().getCode() == StatusCode.Code.FAILED_PRECONDITION) { + LOG.debug("Lease acquire failure (ETag mismatch) for secret {} in namespace {}", key, namespace); + return SecretLease.failed(); + } + throw new IOException("Failed to acquire lease lock on secret " + key, e); + } catch (InvalidSecretException e) { + throw new IOException("Failed to parse secret", e); + } + } + + @Override + public void releaseLease(String namespace, String key, SecretLease lease) throws IOException { + if (lease == null || !lease.isAcquired()) { + return; + } + try { + WrappedSecret refreshSecret = client.getSecret(namespace, key); + String currentEtag = refreshSecret.getEtag() == null ? "" : refreshSecret.getEtag(); + Map currentAnnotations = refreshSecret.getAnnotations(); + String currentLockHolder = currentAnnotations.get(ANNOTATION_LEASE_HOLDER); + + if (STATE_IDLE.equalsIgnoreCase(currentAnnotations.get(ANNOTATION_LEASE_STATE)) + || currentLockHolder == null || currentLockHolder.isEmpty()) { + return; + } + + if (!lease.getLockHolder().equals(currentLockHolder)) { + throw new IOException(String.format("Cannot release lease for %s: lock held by %s.", key, currentLockHolder)); + } + + Map annotationsToUpdate = new HashMap<>(currentAnnotations); + annotationsToUpdate.put(ANNOTATION_LEASE_STATE, STATE_IDLE); + annotationsToUpdate.put(ANNOTATION_LEASE_ACQUIRED_TIME_MS, "0"); + annotationsToUpdate.put(ANNOTATION_LEASE_HOLDER, ""); + + client.updateSecretAnnotations(namespace, key, annotationsToUpdate, currentEtag); + } catch (ApiException | InvalidSecretException e) { + throw new IOException("Failed to release lease lock on secret " + key, e); + } + } } diff --git a/cdap-securestore-ext-gcp-secretstore/src/main/java/io/cdap/cdap/securestore/gcp/cloudsecretmanager/WrappedSecret.java b/cdap-securestore-ext-gcp-secretstore/src/main/java/io/cdap/cdap/securestore/gcp/cloudsecretmanager/WrappedSecret.java index d650ba36eccd..1e4b2ca12681 100644 --- a/cdap-securestore-ext-gcp-secretstore/src/main/java/io/cdap/cdap/securestore/gcp/cloudsecretmanager/WrappedSecret.java +++ b/cdap-securestore-ext-gcp-secretstore/src/main/java/io/cdap/cdap/securestore/gcp/cloudsecretmanager/WrappedSecret.java @@ -25,8 +25,11 @@ import com.google.protobuf.Duration; import com.google.protobuf.util.Timestamps; import io.cdap.cdap.securestore.spi.secret.SecretMetadata; +import javax.annotation.Nullable; import java.lang.reflect.Type; +import java.util.Collections; +import java.util.HashMap; import java.util.Map; import java.util.Optional; @@ -43,10 +46,22 @@ public final class WrappedSecret { private final String namespace; private final SecretMetadata secretMetadata; + @Nullable + private final String etag; + private final Map annotations; private WrappedSecret(String namespace, SecretMetadata secretMetadata) { + this(namespace, secretMetadata, null, Collections.emptyMap()); + } + + private WrappedSecret(String namespace, + SecretMetadata secretMetadata, + @Nullable String etag, + Map annotations) { this.namespace = namespace; this.secretMetadata = secretMetadata; + this.etag = etag; + this.annotations = annotations == null ? Collections.emptyMap() : annotations; } /** Constructs a new WrappedSecret from a CDAP Secret. */ @@ -61,7 +76,7 @@ public static WrappedSecret fromMetadata(String namespace, SecretMetadata metada public static WrappedSecret fromGcpSecret(Secret secret) throws InvalidSecretException { String namespace = getNamespace(secret); SecretMetadata metadata = toSecretMetadata(secret); - return new WrappedSecret(namespace, metadata); + return new WrappedSecret(namespace, metadata, secret.getEtag(), secret.getAnnotationsMap()); } public String getNamespace() { @@ -72,12 +87,34 @@ public SecretMetadata getCdapSecretMetadata() { return secretMetadata; } + @Nullable + public String getEtag() { + return etag; + } + + public Map getAnnotations() { + return annotations; + } + + public String getAnnotation(String key, String defaultValue) { + return annotations.getOrDefault(key, defaultValue); + } + /** * Returns a new GCP {@link Secret} representing the underlying SecretMetadata. * * @param resourceName Value to set for the "name" field needed for update operations. */ public Secret getGcpSecret(String resourceName, long ttlInSeconds) { + return getGcpSecret(resourceName, ttlInSeconds, null); + } + + /** + * Returns a new GCP {@link Secret} representing the underlying SecretMetadata including etag + * and additional annotations. + */ + public Secret getGcpSecret(String resourceName, long ttlInSeconds, + @Nullable Map additionalAnnotations) { Secret.Builder builder = Secret.newBuilder() // Set replication policy to automatic (as opposed to user-managed) and do not specify a // CMEK (use google-managed key). @@ -91,15 +128,25 @@ public Secret getGcpSecret(String resourceName, long ttlInSeconds) { if (ttlInSeconds > 0) { builder.setTtl(Duration.newBuilder().setSeconds(ttlInSeconds).build()); } + + if (etag != null && !etag.isEmpty()) { + builder.setEtag(etag); + } + if (additionalAnnotations != null) { + for (Map.Entry entry : additionalAnnotations.entrySet()) { + builder.putAnnotations(entry.getKey(), entry.getValue()); + } + } return builder.build(); } private static SecretMetadata toSecretMetadata(Secret secret) throws InvalidSecretException { + Map props = new HashMap<>(deserializeProps(secret.getAnnotationsOrDefault("cdap_props", "{}"))); return new SecretMetadata( secret.getAnnotationsOrDefault("cdap_secret_name", ""), secret.getAnnotationsOrDefault("cdap_description", ""), Timestamps.toMillis(secret.getCreateTime()), - deserializeProps(secret.getAnnotationsOrDefault("cdap_props", "{}"))); + props); } private static String getNamespace(Secret secret) { diff --git a/cdap-securestore-ext-gcp-secretstore/src/test/java/io/cdap/cdap/securestore/gcp/cloudsecretmanager/GcpSecretManagerTest.java b/cdap-securestore-ext-gcp-secretstore/src/test/java/io/cdap/cdap/securestore/gcp/cloudsecretmanager/GcpSecretManagerTest.java index 47664288f46b..5086b939bf89 100644 --- a/cdap-securestore-ext-gcp-secretstore/src/test/java/io/cdap/cdap/securestore/gcp/cloudsecretmanager/GcpSecretManagerTest.java +++ b/cdap-securestore-ext-gcp-secretstore/src/test/java/io/cdap/cdap/securestore/gcp/cloudsecretmanager/GcpSecretManagerTest.java @@ -21,6 +21,7 @@ import com.google.api.gax.rpc.StatusCode.Code; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import io.cdap.cdap.securestore.spi.SecretLease; import io.cdap.cdap.securestore.spi.SecretNotFoundException; import io.cdap.cdap.securestore.spi.secret.Secret; import io.cdap.cdap.securestore.spi.secret.SecretMetadata; @@ -32,11 +33,14 @@ import org.mockito.junit.MockitoJUnit; import org.mockito.junit.MockitoRule; +import java.lang.reflect.Field; import java.io.IOException; import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.eq; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -202,6 +206,40 @@ public void delete_wrapsApiExceptions() throws Exception { assertThrows(IOException.class, () -> secretManager.delete(NAMESPACE, "example")); } + @Test + public void testAcquireGcpLeaseSuccess() throws Exception { + SecretMetadata metadata = createMetadata("salesforce"); + WrappedSecret wrappedSecret = WrappedSecret.fromMetadata(NAMESPACE, metadata); + when(client.getSecret(eq(NAMESPACE), eq("salesforce"))).thenReturn(wrappedSecret); + + + + SecretLease lease = + secretManager.acquireLease(NAMESPACE, "salesforce", 30000L, "test-lock-holder"); + assertTrue(lease.isAcquired()); + + Field field = WrappedSecret.class.getDeclaredField("annotations"); + field.setAccessible(true); + field.set(wrappedSecret, ImmutableMap.of("lease_holder", "test-lock-holder")); + + secretManager.releaseLease(NAMESPACE, "salesforce", lease); + } + + @Test + public void testAcquireGcpLeaseEtagMismatchFailure() throws Exception { + SecretMetadata metadata = createMetadata("salesforce"); + WrappedSecret wrappedSecret = WrappedSecret.fromMetadata(NAMESPACE, metadata); + when(client.getSecret(eq(NAMESPACE), eq("salesforce"))).thenReturn(wrappedSecret); + + doThrow(createApiException(Code.FAILED_PRECONDITION)) + .when(client).updateSecretAnnotations( + eq(NAMESPACE), eq("salesforce"), ArgumentMatchers.any(), ArgumentMatchers.any()); + + SecretLease lease = + secretManager.acquireLease(NAMESPACE, "salesforce", 30000L, "test-lock-holder"); + assertFalse(lease.isAcquired()); + } + private static Secret createSecret(String name) { return new Secret(name.getBytes(), createMetadata(name)); } @@ -210,6 +248,156 @@ private static SecretMetadata createMetadata(String name) { return new SecretMetadata(name, "Fake description", 0, ImmutableMap.of()); } + @Test + public void testAcquireLeaseSameHolder() throws Exception { + SecretMetadata metadata = createMetadata("salesforce"); + WrappedSecret wrappedSecret = WrappedSecret.fromMetadata(NAMESPACE, metadata); + Field field = WrappedSecret.class.getDeclaredField("annotations"); + field.setAccessible(true); + field.set(wrappedSecret, ImmutableMap.of( + "lease_state", "refreshing", + "lease_acquired_time_ms", String.valueOf(System.currentTimeMillis()), + "lease_holder", "test-lock-holder" + )); + when(client.getSecret(eq(NAMESPACE), eq("salesforce"))).thenReturn(wrappedSecret); + + + + SecretLease lease = + secretManager.acquireLease(NAMESPACE, "salesforce", 30000L, "test-lock-holder"); + assertTrue(lease.isAcquired()); + } + + @Test + public void testAcquireLeaseAlreadyLocked() throws Exception { + SecretMetadata metadata = createMetadata("salesforce"); + WrappedSecret wrappedSecret = WrappedSecret.fromMetadata(NAMESPACE, metadata); + Field field = WrappedSecret.class.getDeclaredField("annotations"); + field.setAccessible(true); + field.set(wrappedSecret, ImmutableMap.of( + "lease_state", "refreshing", + "lease_acquired_time_ms", String.valueOf(System.currentTimeMillis()), + "lease_holder", "other-holder" + )); + when(client.getSecret(eq(NAMESPACE), eq("salesforce"))).thenReturn(wrappedSecret); + + + SecretLease lease = + secretManager.acquireLease(NAMESPACE, "salesforce", 30000L, "test-lock-holder"); + assertFalse(lease.isAcquired()); + } + + @Test + public void testAcquireLeaseLockExpired() throws Exception { + SecretMetadata metadata = createMetadata("salesforce"); + WrappedSecret wrappedSecret = WrappedSecret.fromMetadata(NAMESPACE, metadata); + Field field = WrappedSecret.class.getDeclaredField("annotations"); + field.setAccessible(true); + field.set(wrappedSecret, ImmutableMap.of( + "lease_state", "refreshing", + "lease_acquired_time_ms", String.valueOf(System.currentTimeMillis() - 60000L), + "lease_holder", "other-holder" + )); + when(client.getSecret(eq(NAMESPACE), eq("salesforce"))).thenReturn(wrappedSecret); + + + + SecretLease lease = + secretManager.acquireLease(NAMESPACE, "salesforce", 30000L, "test-lock-holder"); + assertTrue(lease.isAcquired()); + } + + @Test + public void testReleaseLeaseSuccess() throws Exception { + SecretMetadata metadata = createMetadata("salesforce"); + WrappedSecret wrappedSecret = WrappedSecret.fromMetadata(NAMESPACE, metadata); + Field field = WrappedSecret.class.getDeclaredField("annotations"); + field.setAccessible(true); + field.set(wrappedSecret, ImmutableMap.of( + "lease_state", "refreshing", + "lease_acquired_time_ms", String.valueOf(System.currentTimeMillis()), + "lease_holder", "test-lock-holder" + )); + when(client.getSecret(eq(NAMESPACE), eq("salesforce"))).thenReturn(wrappedSecret); + + + SecretLease lease = + SecretLease.acquired("123", "test-lock-holder"); + secretManager.releaseLease(NAMESPACE, "salesforce", lease); + + verify(client).updateSecretAnnotations(eq(NAMESPACE), eq("salesforce"), + ArgumentMatchers.any(), ArgumentMatchers.any()); + } + + @Test + public void testReleaseLeaseAlreadyIdle() throws Exception { + SecretMetadata metadata = createMetadata("salesforce"); + WrappedSecret wrappedSecret = WrappedSecret.fromMetadata(NAMESPACE, metadata); + Field field = WrappedSecret.class.getDeclaredField("annotations"); + field.setAccessible(true); + field.set(wrappedSecret, ImmutableMap.of( + "lease_state", "idle", + "lease_acquired_time_ms", "0", + "lease_holder", "" + )); + when(client.getSecret(eq(NAMESPACE), eq("salesforce"))).thenReturn(wrappedSecret); + + + SecretLease lease = + SecretLease.acquired("test-timestamp", "test-lock-holder"); + + secretManager.releaseLease(NAMESPACE, "salesforce", lease); + } + + @Test + public void testReleaseLeaseEtagMismatchFailure() throws Exception { + SecretMetadata metadata = createMetadata("salesforce"); + WrappedSecret wrappedSecret = WrappedSecret.fromMetadata(NAMESPACE, metadata); + Field field = WrappedSecret.class.getDeclaredField("annotations"); + field.setAccessible(true); + field.set(wrappedSecret, ImmutableMap.of( + "lease_state", "refreshing", + "lease_acquired_time_ms", String.valueOf(System.currentTimeMillis()), + "lease_holder", "test-lock-holder" + )); + when(client.getSecret(eq(NAMESPACE), eq("salesforce"))).thenReturn(wrappedSecret); + + doThrow(createApiException(Code.FAILED_PRECONDITION)) + .when(client).updateSecretAnnotations(eq(NAMESPACE), eq("salesforce"), + ArgumentMatchers.anyMap(), + ArgumentMatchers.anyString()); + + SecretLease lease = + SecretLease.acquired("test-timestamp", "test-lock-holder"); + + assertThrows(IOException.class, () -> + secretManager.releaseLease(NAMESPACE, "salesforce", lease) + ); + } + + @Test + public void testReleaseLeaseLockHolderMismatch() throws Exception { + SecretMetadata metadata = createMetadata("salesforce"); + WrappedSecret wrappedSecret = WrappedSecret.fromMetadata(NAMESPACE, metadata); + Field field = WrappedSecret.class.getDeclaredField("annotations"); + field.setAccessible(true); + field.set(wrappedSecret, ImmutableMap.of( + "lease_state", "refreshing", + "lease_acquired_time_ms", String.valueOf(System.currentTimeMillis()), + "lease_holder", "someone-else" + )); + when(client.getSecret(eq(NAMESPACE), eq("salesforce"))).thenReturn(wrappedSecret); + + + SecretLease lease = + SecretLease.acquired("123", "test-lock-holder"); + + IOException exception = assertThrows(IOException.class, + () -> secretManager.releaseLease(NAMESPACE, "salesforce", lease)); + System.out.println("EXCEPTION MESSAGE: " + exception.getMessage()); + assertTrue(exception.getMessage().contains("lock held by")); + } + private static ApiException createApiException(Code code) { return new ApiException(new RuntimeException("Fake Exception"), createStatusCode(code), true); } diff --git a/cdap-securestore-spi/src/main/java/io/cdap/cdap/securestore/spi/SecretLease.java b/cdap-securestore-spi/src/main/java/io/cdap/cdap/securestore/spi/SecretLease.java new file mode 100644 index 000000000000..76e954d97395 --- /dev/null +++ b/cdap-securestore-spi/src/main/java/io/cdap/cdap/securestore/spi/SecretLease.java @@ -0,0 +1,86 @@ +/* + * Copyright © 2026 Cask Data, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ + +package io.cdap.cdap.securestore.spi; + +import java.util.Objects; + +/** + * SPI model representing a distributed lease lock on a secret in the underlying storage plugin. + * + *

Note: This is the plugin-facing SPI counterpart to {@code io.cdap.cdap.api.security.store.lease.SecureStoreLease} + * in the API layer, mirroring the {@link io.cdap.cdap.securestore.spi.secret.SecretMetadata} vs + * {@code SecureStoreMetadata} architectural pattern in CDAP. + */ +public class SecretLease { + private final boolean acquired; + private final String lockTimestamp; + private final String lockHolder; + + private SecretLease(boolean acquired, String lockTimestamp, String lockHolder) { + this.acquired = acquired; + this.lockTimestamp = lockTimestamp; + this.lockHolder = lockHolder; + } + + public static SecretLease acquired(String lockTimestamp, String lockHolder) { + return new SecretLease(true, lockTimestamp, lockHolder); + } + + public static SecretLease failed() { + return new SecretLease(false, null, null); + } + + public boolean isAcquired() { + return acquired; + } + + public String getLockTimestamp() { + return lockTimestamp; + } + + public String getLockHolder() { + return lockHolder; + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + SecretLease that = (SecretLease) o; + return acquired == that.acquired + && Objects.equals(lockTimestamp, that.lockTimestamp) + && Objects.equals(lockHolder, that.lockHolder); + } + + @Override + public int hashCode() { + return Objects.hash(acquired, lockTimestamp, lockHolder); + } + + @Override + public String toString() { + return "SecretLease{" + + "acquired=" + acquired + + ", lockTimestamp='" + lockTimestamp + '\'' + + ", lockHolder='" + lockHolder + '\'' + + '}'; + } +} diff --git a/cdap-securestore-spi/src/main/java/io/cdap/cdap/securestore/spi/SecretManager.java b/cdap-securestore-spi/src/main/java/io/cdap/cdap/securestore/spi/SecretManager.java index 2bf2713fdce0..b0b7b9694fb9 100644 --- a/cdap-securestore-spi/src/main/java/io/cdap/cdap/securestore/spi/SecretManager.java +++ b/cdap-securestore-spi/src/main/java/io/cdap/cdap/securestore/spi/SecretManager.java @@ -134,4 +134,39 @@ default SecretMetadata getMetadata(String namespace, String name) * @param context secret manager context */ void destroy(SecretManagerContext context); + + /** + * Returns whether this secret manager supports distributed locking/leasing. + * By default, this returns false. + * + * @return true if leases are supported, false otherwise + */ + default boolean isLeaseSupported() throws IOException { + return false; + } + + /** + * Attempts to acquire a distributed lease on a secret. + * + * @param namespace the namespace that this secret belongs to + * @param key the name of the secret + * @param timeoutMs the lease expiration timeout in milliseconds + * @return a SecretLease object detailing whether it was acquired and its lock metadata + * @throws IOException if unable to acquire lease due to I/O error + */ + default SecretLease acquireLease(String namespace, String key, long timeoutMs, String lockHolder) throws IOException { + throw new UnsupportedOperationException("Distributed leases are not supported by this SecureStore implementation."); + } + + /** + * Releases an acquired lease on a secret. + * + * @param namespace the namespace that this secret belongs to + * @param key the name of the secret + * @param lease the previously acquired lease + * @throws IOException if unable to release lease due to I/O error + */ + default void releaseLease(String namespace, String key, SecretLease lease) throws IOException { + throw new UnsupportedOperationException("Distributed leases are not supported by this SecureStore implementation."); + } } diff --git a/cdap-security/src/main/java/io/cdap/cdap/security/store/DefaultSecureStoreService.java b/cdap-security/src/main/java/io/cdap/cdap/security/store/DefaultSecureStoreService.java index 137bbcc3615e..1f533aa73b89 100644 --- a/cdap-security/src/main/java/io/cdap/cdap/security/store/DefaultSecureStoreService.java +++ b/cdap-security/src/main/java/io/cdap/cdap/security/store/DefaultSecureStoreService.java @@ -32,6 +32,7 @@ import io.cdap.cdap.security.spi.authentication.AuthenticationContext; import io.cdap.cdap.security.spi.authorization.AccessEnforcer; import io.cdap.cdap.security.spi.authorization.UnauthorizedException; +import io.cdap.cdap.api.security.store.SecureStoreLease; import java.io.IOException; import java.util.List; import java.util.Map; @@ -164,4 +165,26 @@ protected void startUp() throws Exception { protected void shutDown() throws Exception { secureStoreService.stopAndWait(); } + + @Override + public boolean isLeaseSupported() { + return secureStoreService.isLeaseSupported(); + } + + @Override + public SecureStoreLease acquireLease(String namespace, String name, long timeoutMs, + String lockHolder) throws Exception { + Principal principal = authenticationContext.getPrincipal(); + SecureKeyId secureKeyId = new NamespaceId(namespace).secureKey(name); + accessEnforcer.enforce(secureKeyId, principal, StandardPermission.UPDATE); + return secureStoreService.acquireLease(namespace, name, timeoutMs, lockHolder); + } + + @Override + public void releaseLease(String namespace, String name, SecureStoreLease lease) throws Exception { + Principal principal = authenticationContext.getPrincipal(); + SecureKeyId secureKeyId = new NamespaceId(namespace).secureKey(name); + accessEnforcer.enforce(secureKeyId, principal, StandardPermission.UPDATE); + secureStoreService.releaseLease(namespace, name, lease); + } } diff --git a/cdap-security/src/main/java/io/cdap/cdap/security/store/SecureStoreHandler.java b/cdap-security/src/main/java/io/cdap/cdap/security/store/SecureStoreHandler.java index 2759140ab279..a93fc72e9a05 100644 --- a/cdap-security/src/main/java/io/cdap/cdap/security/store/SecureStoreHandler.java +++ b/cdap-security/src/main/java/io/cdap/cdap/security/store/SecureStoreHandler.java @@ -24,6 +24,7 @@ import io.cdap.cdap.api.security.store.SecureStore; import io.cdap.cdap.api.security.store.SecureStoreManager; import io.cdap.cdap.api.security.store.SecureStoreMetadata; +import io.cdap.cdap.api.security.store.SecureStoreLease; import io.cdap.cdap.common.BadRequestException; import io.cdap.cdap.common.conf.Constants; import io.cdap.cdap.common.security.AuditDetail; @@ -46,9 +47,11 @@ import java.nio.charset.StandardCharsets; import javax.ws.rs.DELETE; import javax.ws.rs.GET; +import javax.ws.rs.POST; import javax.ws.rs.PUT; import javax.ws.rs.Path; import javax.ws.rs.PathParam; +import javax.ws.rs.QueryParam; /** * Exposes REST APIs for {@link SecureStore} and {@link SecureStoreManager}. @@ -128,6 +131,46 @@ public void getMetadata(HttpRequest httpRequest, HttpResponder httpResponder, httpResponder.sendJson(HttpResponseStatus.OK, GSON.toJson(metadata)); } + @Path("/lease/supported") + @GET + public void isLeaseSupported(HttpRequest httpRequest, HttpResponder httpResponder, + @PathParam("namespace-id") String namespace) throws Exception { + httpResponder.sendString(HttpResponseStatus.OK, String.valueOf(secureStoreManager.isLeaseSupported())); + } + + @Path("/{key-name}/lease") + @POST + public void acquireLease(HttpRequest httpRequest, HttpResponder httpResponder, + @PathParam("namespace-id") String namespace, + @PathParam("key-name") String name, + @QueryParam("timeoutMs") long timeoutMs, + @QueryParam("lockHolder") String lockHolder) throws Exception { + SecureStoreLease lease = secureStoreManager.acquireLease(namespace, name, timeoutMs, lockHolder); + httpResponder.sendJson(HttpResponseStatus.OK, GSON.toJson(lease)); + } + + @Path("/{key-name}/lease") + @DELETE + public void releaseLease(FullHttpRequest httpRequest, HttpResponder httpResponder, + @PathParam("namespace-id") String namespace, + @PathParam("key-name") String name) throws Exception { + SecureStoreLease lease = parseLeaseBody(httpRequest); + secureStoreManager.releaseLease(namespace, name, lease); + httpResponder.sendStatus(HttpResponseStatus.OK); + } + + private SecureStoreLease parseLeaseBody(FullHttpRequest request) + throws IOException, BadRequestException { + ByteBuf content = request.content(); + if (!content.isReadable()) { + throw new BadRequestException("Lease body is missing or empty"); + } + try (Reader reader = new InputStreamReader(new ByteBufInputStream(content), + StandardCharsets.UTF_8)) { + return GSON.fromJson(reader, SecureStoreLease.class); + } + } + @Path("/") @GET public void list(HttpRequest httpRequest, HttpResponder httpResponder, diff --git a/cdap-security/src/main/java/io/cdap/cdap/security/store/client/RemoteSecureStore.java b/cdap-security/src/main/java/io/cdap/cdap/security/store/client/RemoteSecureStore.java index 7fc91f10907a..37f1040b342a 100644 --- a/cdap-security/src/main/java/io/cdap/cdap/security/store/client/RemoteSecureStore.java +++ b/cdap-security/src/main/java/io/cdap/cdap/security/store/client/RemoteSecureStore.java @@ -24,7 +24,9 @@ import io.cdap.cdap.api.security.store.SecureStore; import io.cdap.cdap.api.security.store.SecureStoreData; import io.cdap.cdap.api.security.store.SecureStoreManager; +import io.cdap.cdap.proto.id.NamespaceId; import io.cdap.cdap.api.security.store.SecureStoreMetadata; +import io.cdap.cdap.api.security.store.SecureStoreLease; import io.cdap.cdap.common.SecureKeyAlreadyExistsException; import io.cdap.cdap.common.SecureKeyNotFoundException; import io.cdap.cdap.common.conf.Constants; @@ -131,6 +133,36 @@ public void delete(String namespace, String name) throws Exception { namespace, name)); } + @Override + public boolean isLeaseSupported() throws IOException { + String path = String.format("%s/securekeys/lease/supported", NamespaceId.SYSTEM.getNamespace()); + HttpRequest request = remoteClient.requestBuilder(HttpMethod.GET, path).build(); + HttpResponse response = remoteClient.execute(request, Idempotency.IDEMPOTENT); + return Boolean.parseBoolean(response.getResponseBodyAsString()); + } + + @Override + public SecureStoreLease acquireLease(final String namespace, final String name, + final long timeoutMs, final String lockHolder) throws Exception { + String path = createPath(namespace, name) + "/lease?timeoutMs=" + timeoutMs + "&lockHolder=" + lockHolder; + HttpRequest request = remoteClient.requestBuilder(HttpMethod.POST, path).build(); + HttpResponse response = remoteClient.execute(request, Idempotency.NONE); + handleResponse(response, namespace, name, + String.format("Error occurred while acquiring lease for key %s:%s", namespace, name)); + return GSON.fromJson(response.getResponseBodyAsString(), SecureStoreLease.class); + } + + @Override + public void releaseLease(final String namespace, final String name, + final SecureStoreLease lease) throws Exception { + HttpRequest request = remoteClient.requestBuilder(HttpMethod.DELETE, + createPath(namespace, name) + "/lease") + .withBody(GSON.toJson(lease)).build(); + HttpResponse response = remoteClient.execute(request, Idempotency.NONE); + handleResponse(response, namespace, name, + String.format("Error occurred while releasing lease for key %s:%s", namespace, name)); + } + /** * Handles error based on response code. */ diff --git a/cdap-security/src/main/java/io/cdap/cdap/security/store/secretmanager/SecretManagerSecureStoreService.java b/cdap-security/src/main/java/io/cdap/cdap/security/store/secretmanager/SecretManagerSecureStoreService.java index 956107bd3b1b..709002d1c5d3 100644 --- a/cdap-security/src/main/java/io/cdap/cdap/security/store/secretmanager/SecretManagerSecureStoreService.java +++ b/cdap-security/src/main/java/io/cdap/cdap/security/store/secretmanager/SecretManagerSecureStoreService.java @@ -22,6 +22,7 @@ import com.google.inject.Inject; import io.cdap.cdap.api.security.store.SecureStoreData; import io.cdap.cdap.api.security.store.SecureStoreMetadata; +import io.cdap.cdap.api.security.store.SecureStoreLease; import io.cdap.cdap.common.NamespaceNotFoundException; import io.cdap.cdap.common.SecureKeyNotFoundException; import io.cdap.cdap.common.conf.CConfiguration; @@ -33,6 +34,7 @@ import io.cdap.cdap.securestore.spi.SecretManagerContext; import io.cdap.cdap.securestore.spi.SecretNotFoundException; import io.cdap.cdap.securestore.spi.SecretStore; +import io.cdap.cdap.securestore.spi.SecretLease; import io.cdap.cdap.securestore.spi.secret.Secret; import io.cdap.cdap.securestore.spi.secret.SecretMetadata; import io.cdap.cdap.security.store.SecureStoreService; @@ -212,4 +214,35 @@ private void destroySecretManager() { LOG.warn("Error occurred while stopping {}.", getClass().getSimpleName(), e); } } + + @Override + public boolean isLeaseSupported() throws IOException { + if (secretManager == null) { + throw new RuntimeException("Secret manager is either not initialized or not loaded. "); + } + return this.secretManager.isLeaseSupported(); + } + + @Override + public SecureStoreLease acquireLease(String namespace, String name, long timeoutMs, + String lockHolder) throws IOException { + SecretLease secretLease = this.secretManager.acquireLease(namespace, name, timeoutMs, lockHolder); + if (secretLease != null && secretLease.isAcquired()) { + LOG.debug("Successfully acquired lease for namespace '{}' name '{}' (holder: {}, timestamp: {})", + namespace, name, secretLease.getLockHolder(), secretLease.getLockTimestamp()); + return SecureStoreLease.acquired(secretLease.getLockTimestamp(), secretLease.getLockHolder()); + } + LOG.debug("Lease acquisition returned unacquired/held lease for namespace '{}' name '{}'", + namespace, name); + return SecureStoreLease.failed(); + } + + @Override + public void releaseLease(String namespace, String name, SecureStoreLease lease) throws IOException { + if (lease != null && lease.isAcquired()) { + SecretLease spiLease = + SecretLease.acquired(lease.getLockTimestamp(), lease.getLockHolder()); + this.secretManager.releaseLease(namespace, name, spiLease); + } + } } diff --git a/cdap-security/src/test/java/io/cdap/cdap/security/store/secretmanager/MockSecretManager.java b/cdap-security/src/test/java/io/cdap/cdap/security/store/secretmanager/MockSecretManager.java index 19d0d5fc349c..45a0f3257b42 100644 --- a/cdap-security/src/test/java/io/cdap/cdap/security/store/secretmanager/MockSecretManager.java +++ b/cdap-security/src/test/java/io/cdap/cdap/security/store/secretmanager/MockSecretManager.java @@ -16,6 +16,7 @@ package io.cdap.cdap.security.store.secretmanager; +import io.cdap.cdap.securestore.spi.SecretLease; import io.cdap.cdap.securestore.spi.SecretManager; import io.cdap.cdap.securestore.spi.SecretManagerContext; import io.cdap.cdap.securestore.spi.SecretNotFoundException; @@ -87,6 +88,27 @@ public void destroy(SecretManagerContext context) { map.clear(); } + @Override + public boolean isLeaseSupported() { + return true; + } + + @Override + public SecretLease acquireLease(String namespace, String key, long timeoutMs, String lockHolder) + throws IOException { + String fullKey = getKey(namespace, key); + if (!map.containsKey(fullKey)) { + throw new IOException("Not found"); + } + // simple mock: always return acquired for testing + return SecretLease.acquired(String.valueOf(System.currentTimeMillis()), lockHolder); + } + + @Override + public void releaseLease(String namespace, String key, SecretLease lease) throws IOException { + // simple mock: do nothing + } + private String getKey(String namespace, String name) { return namespace + SEPARATOR + name; } diff --git a/cdap-security/src/test/java/io/cdap/cdap/security/store/secretmanager/SecretManagerSecureStoreServiceTest.java b/cdap-security/src/test/java/io/cdap/cdap/security/store/secretmanager/SecretManagerSecureStoreServiceTest.java index b6d2cb829b02..c097be23a092 100644 --- a/cdap-security/src/test/java/io/cdap/cdap/security/store/secretmanager/SecretManagerSecureStoreServiceTest.java +++ b/cdap-security/src/test/java/io/cdap/cdap/security/store/secretmanager/SecretManagerSecureStoreServiceTest.java @@ -18,6 +18,7 @@ import io.cdap.cdap.api.security.store.SecureStoreData; import io.cdap.cdap.api.security.store.SecureStoreMetadata; +import io.cdap.cdap.api.security.store.SecureStoreLease; import io.cdap.cdap.common.SecureKeyNotFoundException; import io.cdap.cdap.common.namespace.InMemoryNamespaceAdmin; import io.cdap.cdap.proto.NamespaceMeta; @@ -103,6 +104,23 @@ public void testSecureStoreService() throws Exception { Assert.assertEquals(0, secureStoreService.list(NAMESPACE1).size()); } + @Test + public void testLeaseOperations() throws Exception { + String key = "leasekey"; + secureStoreService.put(NAMESPACE1, key, "value", "desc", new HashMap<>()); + + Assert.assertTrue(secureStoreService.isLeaseSupported()); + + SecureStoreLease lease = secureStoreService.acquireLease(NAMESPACE1, key, 1000L, "holder1"); + + Assert.assertNotNull(lease); + Assert.assertTrue(lease.isAcquired()); + Assert.assertEquals("holder1", lease.getLockHolder()); + + secureStoreService.releaseLease(NAMESPACE1, key, lease); + secureStoreService.delete(NAMESPACE1, key); + } + @Test(expected = SecureKeyNotFoundException.class) public void testKeyNotFound() throws Exception { secureStoreService.get(NAMESPACE1, "nonexistingkey"); From db3ab22c55c1713f140a80e518afdbd5488db225 Mon Sep 17 00:00:00 2001 From: sahusanket Date: Mon, 17 Aug 2026 22:29:15 +0530 Subject: [PATCH 2/3] address comment 1 --- .../security/store/SecureStoreManager.java | 4 +-- .../cloudsecretmanager/GcpSecretManager.java | 25 ++++++++++----- .../gcp/cloudsecretmanager/WrappedSecret.java | 31 ++++++++---------- .../GcpSecretManagerTest.java | 20 +++--------- .../cdap/securestore/spi/SecretManager.java | 4 +-- .../security/store/SecureStoreHandler.java | 32 +++++++------------ .../store/client/RemoteSecureStore.java | 9 ++++-- .../SecretManagerSecureStoreService.java | 4 +-- .../secretmanager/MockSecretManager.java | 15 +++++++-- 9 files changed, 71 insertions(+), 73 deletions(-) diff --git a/cdap-api/src/main/java/io/cdap/cdap/api/security/store/SecureStoreManager.java b/cdap-api/src/main/java/io/cdap/cdap/api/security/store/SecureStoreManager.java index 8d5cfd51ba84..fcb8cbef7a61 100644 --- a/cdap-api/src/main/java/io/cdap/cdap/api/security/store/SecureStoreManager.java +++ b/cdap-api/src/main/java/io/cdap/cdap/api/security/store/SecureStoreManager.java @@ -88,7 +88,7 @@ default boolean isLeaseSupported() throws IOException { */ default SecureStoreLease acquireLease(String namespace, String name, long timeoutMs, String lockHolder) throws Exception { - throw new UnsupportedOperationException("Distributed leases are not supported by this SecureStore implementation."); + throw new UnsupportedOperationException("Leases are not supported by this SecureStore implementation."); } /** @@ -100,6 +100,6 @@ default SecureStoreLease acquireLease(String namespace, String name, long timeou * @throws Exception If lock release fails due to underlying storage errors */ default void releaseLease(String namespace, String name, SecureStoreLease lease) throws Exception { - throw new UnsupportedOperationException("Distributed leases are not supported by this SecureStore implementation."); + throw new UnsupportedOperationException("Leases are not supported by this SecureStore implementation."); } } diff --git a/cdap-securestore-ext-gcp-secretstore/src/main/java/io/cdap/cdap/securestore/gcp/cloudsecretmanager/GcpSecretManager.java b/cdap-securestore-ext-gcp-secretstore/src/main/java/io/cdap/cdap/securestore/gcp/cloudsecretmanager/GcpSecretManager.java index 6b9bef7408c9..58eb13d0b1d3 100644 --- a/cdap-securestore-ext-gcp-secretstore/src/main/java/io/cdap/cdap/securestore/gcp/cloudsecretmanager/GcpSecretManager.java +++ b/cdap-securestore-ext-gcp-secretstore/src/main/java/io/cdap/cdap/securestore/gcp/cloudsecretmanager/GcpSecretManager.java @@ -81,18 +81,23 @@ public void store(String namespace, Secret secret) throws IOException { @Override public void store(String namespace, Secret secret, long ttlInSeconds) throws IOException { - WrappedSecret wrappedSecret = WrappedSecret.fromMetadata(namespace, secret.getMetadata()); - + WrappedSecret wrappedSecret; try { - Secret existingSecret = get(namespace, secret.getMetadata().getName()); + WrappedSecret existingWrappedSecret = getWrappedSecret(namespace, secret.getMetadata().getName()); + byte[] existingData = getData(namespace, secret.getMetadata().getName()); + + wrappedSecret = WrappedSecret.fromMetadata( + namespace, secret.getMetadata(), existingWrappedSecret.getAnnotations()); + // 'update' only if 'get' request succeeds, otherwise 'create'. client.updateSecret(wrappedSecret, ttlInSeconds); // Add a new secret version only if the secret payload has changed. - if (!Arrays.equals(existingSecret.getData(), secret.getData())) { + if (!Arrays.equals(existingData, secret.getData())) { client.addSecretVersion(wrappedSecret, secret.getData()); } } catch (SecretNotFoundException unused) { + wrappedSecret = WrappedSecret.fromMetadata(namespace, secret.getMetadata()); try { client.createSecret(wrappedSecret, ttlInSeconds); client.addSecretVersion(wrappedSecret, secret.getData()); @@ -125,15 +130,19 @@ public byte[] getData(String namespace, String name) throws SecretNotFoundExcept @Override public SecretMetadata getMetadata(String namespace, String name) throws SecretNotFoundException, IOException { + return getWrappedSecret(namespace, name).getCdapSecretMetadata(); + } + + private WrappedSecret getWrappedSecret(String namespace, String name) throws SecretNotFoundException, IOException { try { - return client.getSecret(namespace, name).getCdapSecretMetadata(); + return client.getSecret(namespace, name); } catch (ApiException e) { if (e.getStatusCode().getCode() == StatusCode.Code.NOT_FOUND) { throw new SecretNotFoundException(namespace, name); } throw new IOException("Secret Manager get API call failed", e); } catch (InvalidSecretException e) { - throw new IOException("Failed to parse secret", e); + throw new IOException("Secret Manager get API call failed: existing secret is invalid", e); } } @@ -178,7 +187,7 @@ public SecretLease acquireLease(String namespace, String key, long timeoutMs, St try { lockTimestamp = Long.parseLong(refreshSecret.getAnnotation(ANNOTATION_LEASE_ACQUIRED_TIME_MS, "0")); } catch (NumberFormatException e) { - // ignore invalid timestamp + LOG.debug("Invalid lease timestamp found for secret {}, treating as expired.", key); } String currentLockHolder = refreshSecret.getAnnotation(ANNOTATION_LEASE_HOLDER, ""); @@ -225,7 +234,7 @@ public void releaseLease(String namespace, String key, SecretLease lease) throws } if (!lease.getLockHolder().equals(currentLockHolder)) { - throw new IOException(String.format("Cannot release lease for %s: lock held by %s.", key, currentLockHolder)); + return; } Map annotationsToUpdate = new HashMap<>(currentAnnotations); diff --git a/cdap-securestore-ext-gcp-secretstore/src/main/java/io/cdap/cdap/securestore/gcp/cloudsecretmanager/WrappedSecret.java b/cdap-securestore-ext-gcp-secretstore/src/main/java/io/cdap/cdap/securestore/gcp/cloudsecretmanager/WrappedSecret.java index 1e4b2ca12681..630a39b6d5d6 100644 --- a/cdap-securestore-ext-gcp-secretstore/src/main/java/io/cdap/cdap/securestore/gcp/cloudsecretmanager/WrappedSecret.java +++ b/cdap-securestore-ext-gcp-secretstore/src/main/java/io/cdap/cdap/securestore/gcp/cloudsecretmanager/WrappedSecret.java @@ -19,6 +19,7 @@ import com.google.cloud.secretmanager.v1.Replication; import com.google.cloud.secretmanager.v1.Replication.Automatic; import com.google.cloud.secretmanager.v1.Secret; +import com.google.common.base.Strings; import com.google.common.reflect.TypeToken; import com.google.gson.Gson; import com.google.gson.JsonSyntaxException; @@ -69,6 +70,12 @@ public static WrappedSecret fromMetadata(String namespace, SecretMetadata metada return new WrappedSecret(namespace, metadata); } + /** Constructs a new WrappedSecret from a CDAP Secret and existing annotations. */ + public static WrappedSecret fromMetadata(String namespace, SecretMetadata metadata, + Map existingAnnotations) { + return new WrappedSecret(namespace, metadata, null, existingAnnotations); + } + /** * Constructs a new WrappedSecret from a GCP Secret Manager secret, throwing an * {@link InvalidSecretException} if the secret cannot be parsed. @@ -106,21 +113,16 @@ public String getAnnotation(String key, String defaultValue) { * @param resourceName Value to set for the "name" field needed for update operations. */ public Secret getGcpSecret(String resourceName, long ttlInSeconds) { - return getGcpSecret(resourceName, ttlInSeconds, null); - } - - /** - * Returns a new GCP {@link Secret} representing the underlying SecretMetadata including etag - * and additional annotations. - */ - public Secret getGcpSecret(String resourceName, long ttlInSeconds, - @Nullable Map additionalAnnotations) { Secret.Builder builder = Secret.newBuilder() // Set replication policy to automatic (as opposed to user-managed) and do not specify a // CMEK (use google-managed key). .setReplication(Replication.newBuilder().setAutomatic(Automatic.getDefaultInstance())) - .setName(resourceName) - .putAnnotations("cdap_namespace", namespace) + .setName(resourceName); + + // Add existing annotations to preserve them during updates + builder.putAllAnnotations(annotations); + + builder.putAnnotations("cdap_namespace", namespace) .putAnnotations("cdap_secret_name", secretMetadata.getName()) .putAnnotations("cdap_description", Optional.ofNullable(secretMetadata.getDescription()).orElse("")) .putAnnotations("cdap_props", serializeProps(secretMetadata.getProperties())); @@ -129,14 +131,9 @@ public Secret getGcpSecret(String resourceName, long ttlInSeconds, builder.setTtl(Duration.newBuilder().setSeconds(ttlInSeconds).build()); } - if (etag != null && !etag.isEmpty()) { + if (!Strings.isNullOrEmpty(etag)) { builder.setEtag(etag); } - if (additionalAnnotations != null) { - for (Map.Entry entry : additionalAnnotations.entrySet()) { - builder.putAnnotations(entry.getKey(), entry.getValue()); - } - } return builder.build(); } diff --git a/cdap-securestore-ext-gcp-secretstore/src/test/java/io/cdap/cdap/securestore/gcp/cloudsecretmanager/GcpSecretManagerTest.java b/cdap-securestore-ext-gcp-secretstore/src/test/java/io/cdap/cdap/securestore/gcp/cloudsecretmanager/GcpSecretManagerTest.java index 5086b939bf89..053cdbf4733a 100644 --- a/cdap-securestore-ext-gcp-secretstore/src/test/java/io/cdap/cdap/securestore/gcp/cloudsecretmanager/GcpSecretManagerTest.java +++ b/cdap-securestore-ext-gcp-secretstore/src/test/java/io/cdap/cdap/securestore/gcp/cloudsecretmanager/GcpSecretManagerTest.java @@ -45,6 +45,7 @@ import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doThrow; public class GcpSecretManagerTest { @@ -212,8 +213,6 @@ public void testAcquireGcpLeaseSuccess() throws Exception { WrappedSecret wrappedSecret = WrappedSecret.fromMetadata(NAMESPACE, metadata); when(client.getSecret(eq(NAMESPACE), eq("salesforce"))).thenReturn(wrappedSecret); - - SecretLease lease = secretManager.acquireLease(NAMESPACE, "salesforce", 30000L, "test-lock-holder"); assertTrue(lease.isAcquired()); @@ -260,9 +259,6 @@ public void testAcquireLeaseSameHolder() throws Exception { "lease_holder", "test-lock-holder" )); when(client.getSecret(eq(NAMESPACE), eq("salesforce"))).thenReturn(wrappedSecret); - - - SecretLease lease = secretManager.acquireLease(NAMESPACE, "salesforce", 30000L, "test-lock-holder"); assertTrue(lease.isAcquired()); @@ -280,8 +276,6 @@ public void testAcquireLeaseAlreadyLocked() throws Exception { "lease_holder", "other-holder" )); when(client.getSecret(eq(NAMESPACE), eq("salesforce"))).thenReturn(wrappedSecret); - - SecretLease lease = secretManager.acquireLease(NAMESPACE, "salesforce", 30000L, "test-lock-holder"); assertFalse(lease.isAcquired()); @@ -300,8 +294,6 @@ public void testAcquireLeaseLockExpired() throws Exception { )); when(client.getSecret(eq(NAMESPACE), eq("salesforce"))).thenReturn(wrappedSecret); - - SecretLease lease = secretManager.acquireLease(NAMESPACE, "salesforce", 30000L, "test-lock-holder"); assertTrue(lease.isAcquired()); @@ -320,7 +312,6 @@ public void testReleaseLeaseSuccess() throws Exception { )); when(client.getSecret(eq(NAMESPACE), eq("salesforce"))).thenReturn(wrappedSecret); - SecretLease lease = SecretLease.acquired("123", "test-lock-holder"); secretManager.releaseLease(NAMESPACE, "salesforce", lease); @@ -342,7 +333,6 @@ public void testReleaseLeaseAlreadyIdle() throws Exception { )); when(client.getSecret(eq(NAMESPACE), eq("salesforce"))).thenReturn(wrappedSecret); - SecretLease lease = SecretLease.acquired("test-timestamp", "test-lock-holder"); @@ -388,14 +378,12 @@ public void testReleaseLeaseLockHolderMismatch() throws Exception { )); when(client.getSecret(eq(NAMESPACE), eq("salesforce"))).thenReturn(wrappedSecret); - SecretLease lease = SecretLease.acquired("123", "test-lock-holder"); - IOException exception = assertThrows(IOException.class, - () -> secretManager.releaseLease(NAMESPACE, "salesforce", lease)); - System.out.println("EXCEPTION MESSAGE: " + exception.getMessage()); - assertTrue(exception.getMessage().contains("lock held by")); + secretManager.releaseLease(NAMESPACE, "salesforce", lease); + + verify(client, times(0)).updateSecretAnnotations(any(), any(), any(), any()); } private static ApiException createApiException(Code code) { diff --git a/cdap-securestore-spi/src/main/java/io/cdap/cdap/securestore/spi/SecretManager.java b/cdap-securestore-spi/src/main/java/io/cdap/cdap/securestore/spi/SecretManager.java index b0b7b9694fb9..1fd308afc0bc 100644 --- a/cdap-securestore-spi/src/main/java/io/cdap/cdap/securestore/spi/SecretManager.java +++ b/cdap-securestore-spi/src/main/java/io/cdap/cdap/securestore/spi/SecretManager.java @@ -155,7 +155,7 @@ default boolean isLeaseSupported() throws IOException { * @throws IOException if unable to acquire lease due to I/O error */ default SecretLease acquireLease(String namespace, String key, long timeoutMs, String lockHolder) throws IOException { - throw new UnsupportedOperationException("Distributed leases are not supported by this SecureStore implementation."); + throw new UnsupportedOperationException("Leases are not supported by this SecureStore implementation."); } /** @@ -167,6 +167,6 @@ default SecretLease acquireLease(String namespace, String key, long timeoutMs, S * @throws IOException if unable to release lease due to I/O error */ default void releaseLease(String namespace, String key, SecretLease lease) throws IOException { - throw new UnsupportedOperationException("Distributed leases are not supported by this SecureStore implementation."); + throw new UnsupportedOperationException("Leases are not supported by this SecureStore implementation."); } } diff --git a/cdap-security/src/main/java/io/cdap/cdap/security/store/SecureStoreHandler.java b/cdap-security/src/main/java/io/cdap/cdap/security/store/SecureStoreHandler.java index a93fc72e9a05..5c99d996ef77 100644 --- a/cdap-security/src/main/java/io/cdap/cdap/security/store/SecureStoreHandler.java +++ b/cdap-security/src/main/java/io/cdap/cdap/security/store/SecureStoreHandler.java @@ -81,8 +81,8 @@ public void create(FullHttpRequest httpRequest, HttpResponder httpResponder, SecureKeyId secureKeyId = new SecureKeyId(namespace, name); SecureKeyCreateRequest secureKeyCreateRequest; try { - secureKeyCreateRequest = parseBody(httpRequest); - } catch (IOException e) { + secureKeyCreateRequest = parseBody(httpRequest, REQUEST_TYPE); + } catch (IOException | BadRequestException e) { SecureKeyCreateRequest dummy = new SecureKeyCreateRequest("", "", ImmutableMap.of("key", "value"), 3600L); throw new BadRequestException( @@ -138,7 +138,7 @@ public void isLeaseSupported(HttpRequest httpRequest, HttpResponder httpResponde httpResponder.sendString(HttpResponseStatus.OK, String.valueOf(secureStoreManager.isLeaseSupported())); } - @Path("/{key-name}/lease") + @Path("/{key-name}/acquireLease") @POST public void acquireLease(HttpRequest httpRequest, HttpResponder httpResponder, @PathParam("namespace-id") String namespace, @@ -149,27 +149,17 @@ public void acquireLease(HttpRequest httpRequest, HttpResponder httpResponder, httpResponder.sendJson(HttpResponseStatus.OK, GSON.toJson(lease)); } - @Path("/{key-name}/lease") - @DELETE + @Path("/{key-name}/releaseLease") + @POST public void releaseLease(FullHttpRequest httpRequest, HttpResponder httpResponder, @PathParam("namespace-id") String namespace, @PathParam("key-name") String name) throws Exception { - SecureStoreLease lease = parseLeaseBody(httpRequest); + SecureStoreLease lease = parseBody(httpRequest, SecureStoreLease.class); secureStoreManager.releaseLease(namespace, name, lease); httpResponder.sendStatus(HttpResponseStatus.OK); } - private SecureStoreLease parseLeaseBody(FullHttpRequest request) - throws IOException, BadRequestException { - ByteBuf content = request.content(); - if (!content.isReadable()) { - throw new BadRequestException("Lease body is missing or empty"); - } - try (Reader reader = new InputStreamReader(new ByteBufInputStream(content), - StandardCharsets.UTF_8)) { - return GSON.fromJson(reader, SecureStoreLease.class); - } - } + @Path("/") @GET @@ -178,15 +168,15 @@ public void list(HttpRequest httpRequest, HttpResponder httpResponder, httpResponder.sendJson(HttpResponseStatus.OK, GSON.toJson(secureStore.list(namespace))); } - private SecureKeyCreateRequest parseBody(FullHttpRequest request) throws IOException { + private T parseBody(FullHttpRequest request, Type typeOfT) + throws IOException, BadRequestException { ByteBuf content = request.content(); if (!content.isReadable()) { - throw new IOException("Unable to read contents of the request."); + throw new BadRequestException("Request body is missing or empty"); } - try (Reader reader = new InputStreamReader(new ByteBufInputStream(content), StandardCharsets.UTF_8)) { - return GSON.fromJson(reader, REQUEST_TYPE); + return GSON.fromJson(reader, typeOfT); } } } diff --git a/cdap-security/src/main/java/io/cdap/cdap/security/store/client/RemoteSecureStore.java b/cdap-security/src/main/java/io/cdap/cdap/security/store/client/RemoteSecureStore.java index 37f1040b342a..585ea794926a 100644 --- a/cdap-security/src/main/java/io/cdap/cdap/security/store/client/RemoteSecureStore.java +++ b/cdap-security/src/main/java/io/cdap/cdap/security/store/client/RemoteSecureStore.java @@ -144,7 +144,10 @@ public boolean isLeaseSupported() throws IOException { @Override public SecureStoreLease acquireLease(final String namespace, final String name, final long timeoutMs, final String lockHolder) throws Exception { - String path = createPath(namespace, name) + "/lease?timeoutMs=" + timeoutMs + "&lockHolder=" + lockHolder; + String path = new StringBuilder(createPath(namespace, name)) + .append("/acquireLease?timeoutMs=").append(timeoutMs) + .append("&lockHolder=").append(lockHolder) + .toString(); HttpRequest request = remoteClient.requestBuilder(HttpMethod.POST, path).build(); HttpResponse response = remoteClient.execute(request, Idempotency.NONE); handleResponse(response, namespace, name, @@ -155,8 +158,8 @@ public SecureStoreLease acquireLease(final String namespace, final String name, @Override public void releaseLease(final String namespace, final String name, final SecureStoreLease lease) throws Exception { - HttpRequest request = remoteClient.requestBuilder(HttpMethod.DELETE, - createPath(namespace, name) + "/lease") + HttpRequest request = remoteClient.requestBuilder(HttpMethod.POST, + createPath(namespace, name) + "/releaseLease") .withBody(GSON.toJson(lease)).build(); HttpResponse response = remoteClient.execute(request, Idempotency.NONE); handleResponse(response, namespace, name, diff --git a/cdap-security/src/main/java/io/cdap/cdap/security/store/secretmanager/SecretManagerSecureStoreService.java b/cdap-security/src/main/java/io/cdap/cdap/security/store/secretmanager/SecretManagerSecureStoreService.java index 709002d1c5d3..e9fe61057479 100644 --- a/cdap-security/src/main/java/io/cdap/cdap/security/store/secretmanager/SecretManagerSecureStoreService.java +++ b/cdap-security/src/main/java/io/cdap/cdap/security/store/secretmanager/SecretManagerSecureStoreService.java @@ -240,9 +240,9 @@ public SecureStoreLease acquireLease(String namespace, String name, long timeout @Override public void releaseLease(String namespace, String name, SecureStoreLease lease) throws IOException { if (lease != null && lease.isAcquired()) { - SecretLease spiLease = + SecretLease secretLease = SecretLease.acquired(lease.getLockTimestamp(), lease.getLockHolder()); - this.secretManager.releaseLease(namespace, name, spiLease); + this.secretManager.releaseLease(namespace, name, secretLease); } } } diff --git a/cdap-security/src/test/java/io/cdap/cdap/security/store/secretmanager/MockSecretManager.java b/cdap-security/src/test/java/io/cdap/cdap/security/store/secretmanager/MockSecretManager.java index 45a0f3257b42..faff69510635 100644 --- a/cdap-security/src/test/java/io/cdap/cdap/security/store/secretmanager/MockSecretManager.java +++ b/cdap-security/src/test/java/io/cdap/cdap/security/store/secretmanager/MockSecretManager.java @@ -26,8 +26,10 @@ import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; /** * Mock Secret Manager for unit tests. @@ -36,6 +38,7 @@ public class MockSecretManager implements SecretManager { private static final String MOCK_SECRET_MANAGER = "mock_secret_mananger"; private static final String SEPARATOR = ":"; private Map map; + private Set leasedKeys; @Override public String getName() { @@ -45,6 +48,7 @@ public String getName() { @Override public void initialize(SecretManagerContext context) throws IOException { map = new HashMap<>(); + leasedKeys = new HashSet<>(); } @Override @@ -86,6 +90,7 @@ public void delete(String namespace, String name) throws SecretNotFoundException @Override public void destroy(SecretManagerContext context) { map.clear(); + leasedKeys.clear(); } @Override @@ -100,13 +105,19 @@ public SecretLease acquireLease(String namespace, String key, long timeoutMs, St if (!map.containsKey(fullKey)) { throw new IOException("Not found"); } - // simple mock: always return acquired for testing + if (!leasedKeys.add(fullKey)) { + return SecretLease.failed(); + } return SecretLease.acquired(String.valueOf(System.currentTimeMillis()), lockHolder); } @Override public void releaseLease(String namespace, String key, SecretLease lease) throws IOException { - // simple mock: do nothing + String fullKey = getKey(namespace, key); + if (!map.containsKey(fullKey)) { + throw new IOException("Not found"); + } + leasedKeys.remove(fullKey); } private String getKey(String namespace, String name) { From d1a2bcd62dc63d0cb2878ca803f3338072274272 Mon Sep 17 00:00:00 2001 From: sahusanket Date: Tue, 18 Aug 2026 02:18:02 +0530 Subject: [PATCH 3/3] Handling Support of LEASE / capabilities --- .../cdap/api/security/store/SecureStore.java | 12 ++++++++++ .../security/store/SecureStoreCapability.java | 24 +++++++++++++++++++ .../security/store/SecureStoreManager.java | 8 ------- .../internal/app/runtime/AbstractContext.java | 7 ++++++ .../internal/app/runtime/DefaultAdmin.java | 6 +---- .../cloudsecretmanager/GcpSecretManager.java | 7 ++++-- .../cdap/securestore/spi/SecretManager.java | 6 +++-- .../spi/SecretManagerCapability.java | 24 +++++++++++++++++++ .../store/DefaultSecureStoreService.java | 8 ++++--- .../security/store/SecureStoreHandler.java | 8 +++---- .../store/client/RemoteSecureStore.java | 20 ++++++++++++---- .../SecretManagerSecureStoreService.java | 21 ++++++++++++---- .../secretmanager/MockSecretManager.java | 6 +++-- .../SecretManagerSecureStoreServiceTest.java | 5 ++-- 14 files changed, 125 insertions(+), 37 deletions(-) create mode 100644 cdap-api/src/main/java/io/cdap/cdap/api/security/store/SecureStoreCapability.java create mode 100644 cdap-securestore-spi/src/main/java/io/cdap/cdap/securestore/spi/SecretManagerCapability.java diff --git a/cdap-api/src/main/java/io/cdap/cdap/api/security/store/SecureStore.java b/cdap-api/src/main/java/io/cdap/cdap/api/security/store/SecureStore.java index 0ab8b3cbb37d..57a9d7e246b1 100644 --- a/cdap-api/src/main/java/io/cdap/cdap/api/security/store/SecureStore.java +++ b/cdap-api/src/main/java/io/cdap/cdap/api/security/store/SecureStore.java @@ -18,7 +18,10 @@ import io.cdap.cdap.api.annotation.Beta; import java.io.IOException; +import java.util.Collections; import java.util.List; +import java.util.Set; + /** * Provides read access to the secure store. For write access use {@link SecureStoreManager}. @@ -73,4 +76,13 @@ default SecureStoreMetadata getMetadata(String namespace, String name) throws Ex default byte[] getData(String namespace, String name) throws Exception { return get(namespace, name).get(); } + + /** + * Returns the capabilities supported by this secure store implementation. + * + * @return A set of supported capabilities + */ + default Set getCapabilities() throws IOException { + return Collections.emptySet(); + } } diff --git a/cdap-api/src/main/java/io/cdap/cdap/api/security/store/SecureStoreCapability.java b/cdap-api/src/main/java/io/cdap/cdap/api/security/store/SecureStoreCapability.java new file mode 100644 index 000000000000..75e6b4189aac --- /dev/null +++ b/cdap-api/src/main/java/io/cdap/cdap/api/security/store/SecureStoreCapability.java @@ -0,0 +1,24 @@ +/* + * Copyright © 2026 Cask Data, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ + +package io.cdap.cdap.api.security.store; + +/** + * Represents the capabilities of the Secure Store. + */ +public enum SecureStoreCapability { + LEASE +} diff --git a/cdap-api/src/main/java/io/cdap/cdap/api/security/store/SecureStoreManager.java b/cdap-api/src/main/java/io/cdap/cdap/api/security/store/SecureStoreManager.java index fcb8cbef7a61..e3c8b4fcab49 100644 --- a/cdap-api/src/main/java/io/cdap/cdap/api/security/store/SecureStoreManager.java +++ b/cdap-api/src/main/java/io/cdap/cdap/api/security/store/SecureStoreManager.java @@ -68,14 +68,6 @@ default void put(String namespace, String name, String data, @Nullable String de */ void delete(String namespace, String name) throws Exception; - /** - * Checks if the underlying secure store implementation supports distributed lease locking. - * - * @return true if lease locking is supported, false otherwise. - */ - default boolean isLeaseSupported() throws IOException { - return false; - } /** * Attempts to acquire a lease lock on a secret resource. diff --git a/cdap-app-fabric/src/main/java/io/cdap/cdap/internal/app/runtime/AbstractContext.java b/cdap-app-fabric/src/main/java/io/cdap/cdap/internal/app/runtime/AbstractContext.java index de28480620d7..4a0be3f30114 100644 --- a/cdap-app-fabric/src/main/java/io/cdap/cdap/internal/app/runtime/AbstractContext.java +++ b/cdap-app-fabric/src/main/java/io/cdap/cdap/internal/app/runtime/AbstractContext.java @@ -59,6 +59,7 @@ import io.cdap.cdap.api.schedule.TriggeringScheduleInfo; import io.cdap.cdap.api.security.AccessException; import io.cdap.cdap.api.security.store.SecureStore; +import io.cdap.cdap.api.security.store.SecureStoreCapability; import io.cdap.cdap.api.security.store.SecureStoreData; import io.cdap.cdap.api.security.store.SecureStoreManager; import io.cdap.cdap.api.security.store.SecureStoreMetadata; @@ -535,6 +536,12 @@ public byte[] getData(String namespace, String name) throws Exception { return Retries.callWithRetries(() -> secureStore.getData(namespace, name), retryStrategy); } + + @Override + public Set getCapabilities() throws java.io.IOException { + return secureStore.getCapabilities(); + } + @Override public void execute(final TxRunnable runnable) throws TransactionFailureException { execute(runnable, false); diff --git a/cdap-app-fabric/src/main/java/io/cdap/cdap/internal/app/runtime/DefaultAdmin.java b/cdap-app-fabric/src/main/java/io/cdap/cdap/internal/app/runtime/DefaultAdmin.java index 6d9f86a38a12..dd534b0103d1 100644 --- a/cdap-app-fabric/src/main/java/io/cdap/cdap/internal/app/runtime/DefaultAdmin.java +++ b/cdap-app-fabric/src/main/java/io/cdap/cdap/internal/app/runtime/DefaultAdmin.java @@ -21,8 +21,8 @@ import io.cdap.cdap.api.messaging.MessagingAdmin; import io.cdap.cdap.api.messaging.TopicAlreadyExistsException; import io.cdap.cdap.api.messaging.TopicNotFoundException; -import io.cdap.cdap.api.security.store.SecureStoreManager; import io.cdap.cdap.api.security.store.SecureStoreLease; +import io.cdap.cdap.api.security.store.SecureStoreManager; import io.cdap.cdap.common.NamespaceNotFoundException; import io.cdap.cdap.common.namespace.NamespaceQueryAdmin; import io.cdap.cdap.common.service.Retries; @@ -92,10 +92,6 @@ public void delete(String namespace, String name) throws Exception { Retries.runWithRetries(() -> secureStoreManager.delete(namespace, name), retryStrategy); } - @Override - public boolean isLeaseSupported() { - return secureStoreManager.isLeaseSupported(); - } @Override public SecureStoreLease acquireLease(String namespace, String name, long timeoutMs, diff --git a/cdap-securestore-ext-gcp-secretstore/src/main/java/io/cdap/cdap/securestore/gcp/cloudsecretmanager/GcpSecretManager.java b/cdap-securestore-ext-gcp-secretstore/src/main/java/io/cdap/cdap/securestore/gcp/cloudsecretmanager/GcpSecretManager.java index 58eb13d0b1d3..2b01c3120964 100644 --- a/cdap-securestore-ext-gcp-secretstore/src/main/java/io/cdap/cdap/securestore/gcp/cloudsecretmanager/GcpSecretManager.java +++ b/cdap-securestore-ext-gcp-secretstore/src/main/java/io/cdap/cdap/securestore/gcp/cloudsecretmanager/GcpSecretManager.java @@ -21,6 +21,7 @@ import com.google.common.annotations.VisibleForTesting; import io.cdap.cdap.securestore.spi.SecretLease; import io.cdap.cdap.securestore.spi.SecretManager; +import io.cdap.cdap.securestore.spi.SecretManagerCapability; import io.cdap.cdap.securestore.spi.SecretManagerContext; import io.cdap.cdap.securestore.spi.SecretNotFoundException; import io.cdap.cdap.securestore.spi.secret.Secret; @@ -28,8 +29,10 @@ import java.io.IOException; import java.util.Arrays; import java.util.Collection; +import java.util.EnumSet; import java.util.HashMap; import java.util.Map; +import java.util.Set; import java.util.stream.Collectors; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -172,8 +175,8 @@ public void destroy(SecretManagerContext context) { } @Override - public boolean isLeaseSupported() { - return true; + public Set getCapabilities() { + return EnumSet.of(SecretManagerCapability.LEASE); } @Override diff --git a/cdap-securestore-spi/src/main/java/io/cdap/cdap/securestore/spi/SecretManager.java b/cdap-securestore-spi/src/main/java/io/cdap/cdap/securestore/spi/SecretManager.java index 1fd308afc0bc..06949cafd57b 100644 --- a/cdap-securestore-spi/src/main/java/io/cdap/cdap/securestore/spi/SecretManager.java +++ b/cdap-securestore-spi/src/main/java/io/cdap/cdap/securestore/spi/SecretManager.java @@ -20,6 +20,8 @@ import io.cdap.cdap.securestore.spi.secret.SecretMetadata; import java.io.IOException; import java.util.Collection; +import java.util.Collections; +import java.util.Set; /** * Secrets Manager interface to store secrets securely and retrieve them when needed. Secrets are @@ -141,8 +143,8 @@ default SecretMetadata getMetadata(String namespace, String name) * * @return true if leases are supported, false otherwise */ - default boolean isLeaseSupported() throws IOException { - return false; + default Set getCapabilities() throws IOException { + return Collections.emptySet(); } /** diff --git a/cdap-securestore-spi/src/main/java/io/cdap/cdap/securestore/spi/SecretManagerCapability.java b/cdap-securestore-spi/src/main/java/io/cdap/cdap/securestore/spi/SecretManagerCapability.java new file mode 100644 index 000000000000..7675b3b9756a --- /dev/null +++ b/cdap-securestore-spi/src/main/java/io/cdap/cdap/securestore/spi/SecretManagerCapability.java @@ -0,0 +1,24 @@ +/* + * Copyright © 2026 Cask Data, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ + +package io.cdap.cdap.securestore.spi; + +/** + * Represents the capabilities of the Secret Manager. + */ +public enum SecretManagerCapability { + LEASE +} diff --git a/cdap-security/src/main/java/io/cdap/cdap/security/store/DefaultSecureStoreService.java b/cdap-security/src/main/java/io/cdap/cdap/security/store/DefaultSecureStoreService.java index 1f533aa73b89..264d5082d7cd 100644 --- a/cdap-security/src/main/java/io/cdap/cdap/security/store/DefaultSecureStoreService.java +++ b/cdap-security/src/main/java/io/cdap/cdap/security/store/DefaultSecureStoreService.java @@ -19,7 +19,9 @@ import com.google.common.util.concurrent.AbstractIdleService; import com.google.inject.Inject; import com.google.inject.name.Named; +import io.cdap.cdap.api.security.store.SecureStoreCapability; import io.cdap.cdap.api.security.store.SecureStoreData; +import io.cdap.cdap.api.security.store.SecureStoreLease; import io.cdap.cdap.api.security.store.SecureStoreMetadata; import io.cdap.cdap.common.NamespaceNotFoundException; import io.cdap.cdap.common.NotFoundException; @@ -32,10 +34,10 @@ import io.cdap.cdap.security.spi.authentication.AuthenticationContext; import io.cdap.cdap.security.spi.authorization.AccessEnforcer; import io.cdap.cdap.security.spi.authorization.UnauthorizedException; -import io.cdap.cdap.api.security.store.SecureStoreLease; import java.io.IOException; import java.util.List; import java.util.Map; +import java.util.Set; import javax.annotation.Nullable; /** @@ -167,8 +169,8 @@ protected void shutDown() throws Exception { } @Override - public boolean isLeaseSupported() { - return secureStoreService.isLeaseSupported(); + public Set getCapabilities() throws IOException { + return secureStoreService.getCapabilities(); } @Override diff --git a/cdap-security/src/main/java/io/cdap/cdap/security/store/SecureStoreHandler.java b/cdap-security/src/main/java/io/cdap/cdap/security/store/SecureStoreHandler.java index 5c99d996ef77..34078729565e 100644 --- a/cdap-security/src/main/java/io/cdap/cdap/security/store/SecureStoreHandler.java +++ b/cdap-security/src/main/java/io/cdap/cdap/security/store/SecureStoreHandler.java @@ -22,9 +22,9 @@ import com.google.gson.reflect.TypeToken; import com.google.inject.Inject; import io.cdap.cdap.api.security.store.SecureStore; +import io.cdap.cdap.api.security.store.SecureStoreLease; import io.cdap.cdap.api.security.store.SecureStoreManager; import io.cdap.cdap.api.security.store.SecureStoreMetadata; -import io.cdap.cdap.api.security.store.SecureStoreLease; import io.cdap.cdap.common.BadRequestException; import io.cdap.cdap.common.conf.Constants; import io.cdap.cdap.common.security.AuditDetail; @@ -131,11 +131,11 @@ public void getMetadata(HttpRequest httpRequest, HttpResponder httpResponder, httpResponder.sendJson(HttpResponseStatus.OK, GSON.toJson(metadata)); } - @Path("/lease/supported") + @Path("/capabilities") @GET - public void isLeaseSupported(HttpRequest httpRequest, HttpResponder httpResponder, + public void getCapabilities(HttpRequest httpRequest, HttpResponder httpResponder, @PathParam("namespace-id") String namespace) throws Exception { - httpResponder.sendString(HttpResponseStatus.OK, String.valueOf(secureStoreManager.isLeaseSupported())); + httpResponder.sendJson(HttpResponseStatus.OK, GSON.toJson(secureStore.getCapabilities())); } @Path("/{key-name}/acquireLease") diff --git a/cdap-security/src/main/java/io/cdap/cdap/security/store/client/RemoteSecureStore.java b/cdap-security/src/main/java/io/cdap/cdap/security/store/client/RemoteSecureStore.java index 585ea794926a..61d4eaf3bb16 100644 --- a/cdap-security/src/main/java/io/cdap/cdap/security/store/client/RemoteSecureStore.java +++ b/cdap-security/src/main/java/io/cdap/cdap/security/store/client/RemoteSecureStore.java @@ -22,17 +22,18 @@ import com.google.inject.Inject; import io.cdap.cdap.api.retry.Idempotency; import io.cdap.cdap.api.security.store.SecureStore; +import io.cdap.cdap.api.security.store.SecureStoreCapability; import io.cdap.cdap.api.security.store.SecureStoreData; +import io.cdap.cdap.api.security.store.SecureStoreLease; import io.cdap.cdap.api.security.store.SecureStoreManager; -import io.cdap.cdap.proto.id.NamespaceId; import io.cdap.cdap.api.security.store.SecureStoreMetadata; -import io.cdap.cdap.api.security.store.SecureStoreLease; import io.cdap.cdap.common.SecureKeyAlreadyExistsException; import io.cdap.cdap.common.SecureKeyNotFoundException; import io.cdap.cdap.common.conf.Constants; import io.cdap.cdap.common.http.DefaultHttpRequestConfig; import io.cdap.cdap.common.internal.remote.RemoteClient; import io.cdap.cdap.common.internal.remote.RemoteClientFactory; +import io.cdap.cdap.proto.id.NamespaceId; import io.cdap.cdap.proto.id.SecureKeyId; import io.cdap.cdap.proto.security.SecureKeyCreateRequest; import io.cdap.common.http.HttpMethod; @@ -43,6 +44,7 @@ import java.net.HttpURLConnection; import java.util.List; import java.util.Map; +import java.util.Set; import javax.annotation.Nullable; /** @@ -55,6 +57,9 @@ public class RemoteSecureStore implements SecureStoreManager, SecureStore { private static final Type LIST_TYPE = new TypeToken>() { }.getType(); private static final Gson GSON = new Gson(); + private static final Type CAPABILITIES_TYPE = new TypeToken>() { + }.getType(); + private volatile Set capabilities; private final RemoteClient remoteClient; @VisibleForTesting @@ -134,11 +139,16 @@ public void delete(String namespace, String name) throws Exception { } @Override - public boolean isLeaseSupported() throws IOException { - String path = String.format("%s/securekeys/lease/supported", NamespaceId.SYSTEM.getNamespace()); + public Set getCapabilities() throws IOException { + if (capabilities != null){ + return capabilities; + } + + String path = String.format("%s/securekeys/capabilities", NamespaceId.SYSTEM.getNamespace()); HttpRequest request = remoteClient.requestBuilder(HttpMethod.GET, path).build(); HttpResponse response = remoteClient.execute(request, Idempotency.IDEMPOTENT); - return Boolean.parseBoolean(response.getResponseBodyAsString()); + capabilities = GSON.fromJson(response.getResponseBodyAsString(), CAPABILITIES_TYPE); + return capabilities; } @Override diff --git a/cdap-security/src/main/java/io/cdap/cdap/security/store/secretmanager/SecretManagerSecureStoreService.java b/cdap-security/src/main/java/io/cdap/cdap/security/store/secretmanager/SecretManagerSecureStoreService.java index e9fe61057479..0b2ee4cc7f18 100644 --- a/cdap-security/src/main/java/io/cdap/cdap/security/store/secretmanager/SecretManagerSecureStoreService.java +++ b/cdap-security/src/main/java/io/cdap/cdap/security/store/secretmanager/SecretManagerSecureStoreService.java @@ -17,12 +17,15 @@ package io.cdap.cdap.security.store.secretmanager; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Enums; +import com.google.common.base.Optional; import com.google.common.collect.ImmutableMap; import com.google.common.util.concurrent.AbstractIdleService; import com.google.inject.Inject; +import io.cdap.cdap.api.security.store.SecureStoreCapability; import io.cdap.cdap.api.security.store.SecureStoreData; -import io.cdap.cdap.api.security.store.SecureStoreMetadata; import io.cdap.cdap.api.security.store.SecureStoreLease; +import io.cdap.cdap.api.security.store.SecureStoreMetadata; import io.cdap.cdap.common.NamespaceNotFoundException; import io.cdap.cdap.common.SecureKeyNotFoundException; import io.cdap.cdap.common.conf.CConfiguration; @@ -30,19 +33,22 @@ import io.cdap.cdap.common.namespace.NamespaceQueryAdmin; import io.cdap.cdap.proto.id.NamespaceId; import io.cdap.cdap.proto.id.SecureKeyId; +import io.cdap.cdap.securestore.spi.SecretLease; import io.cdap.cdap.securestore.spi.SecretManager; +import io.cdap.cdap.securestore.spi.SecretManagerCapability; import io.cdap.cdap.securestore.spi.SecretManagerContext; import io.cdap.cdap.securestore.spi.SecretNotFoundException; import io.cdap.cdap.securestore.spi.SecretStore; -import io.cdap.cdap.securestore.spi.SecretLease; import io.cdap.cdap.securestore.spi.secret.Secret; import io.cdap.cdap.securestore.spi.secret.SecretMetadata; import io.cdap.cdap.security.store.SecureStoreService; import java.io.IOException; import java.nio.charset.StandardCharsets; import java.util.ArrayList; +import java.util.EnumSet; import java.util.List; import java.util.Map; +import java.util.Set; import javax.annotation.Nullable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -216,11 +222,18 @@ private void destroySecretManager() { } @Override - public boolean isLeaseSupported() throws IOException { + public Set getCapabilities() throws IOException { if (secretManager == null) { throw new RuntimeException("Secret manager is either not initialized or not loaded. "); } - return this.secretManager.isLeaseSupported(); + Set caps = EnumSet.noneOf(SecureStoreCapability.class); + for (SecretManagerCapability cap : this.secretManager.getCapabilities()) { + Optional mapped = Enums.getIfPresent(SecureStoreCapability.class, cap.name()); + if (mapped.isPresent()) { + caps.add(mapped.get()); + } + } + return caps; } @Override diff --git a/cdap-security/src/test/java/io/cdap/cdap/security/store/secretmanager/MockSecretManager.java b/cdap-security/src/test/java/io/cdap/cdap/security/store/secretmanager/MockSecretManager.java index faff69510635..afd879b2dc20 100644 --- a/cdap-security/src/test/java/io/cdap/cdap/security/store/secretmanager/MockSecretManager.java +++ b/cdap-security/src/test/java/io/cdap/cdap/security/store/secretmanager/MockSecretManager.java @@ -18,6 +18,7 @@ import io.cdap.cdap.securestore.spi.SecretLease; import io.cdap.cdap.securestore.spi.SecretManager; +import io.cdap.cdap.securestore.spi.SecretManagerCapability; import io.cdap.cdap.securestore.spi.SecretManagerContext; import io.cdap.cdap.securestore.spi.SecretNotFoundException; import io.cdap.cdap.securestore.spi.secret.Secret; @@ -25,6 +26,7 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Collection; +import java.util.EnumSet; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -94,8 +96,8 @@ public void destroy(SecretManagerContext context) { } @Override - public boolean isLeaseSupported() { - return true; + public Set getCapabilities() { + return EnumSet.of(SecretManagerCapability.LEASE); } @Override diff --git a/cdap-security/src/test/java/io/cdap/cdap/security/store/secretmanager/SecretManagerSecureStoreServiceTest.java b/cdap-security/src/test/java/io/cdap/cdap/security/store/secretmanager/SecretManagerSecureStoreServiceTest.java index c097be23a092..472deda99a50 100644 --- a/cdap-security/src/test/java/io/cdap/cdap/security/store/secretmanager/SecretManagerSecureStoreServiceTest.java +++ b/cdap-security/src/test/java/io/cdap/cdap/security/store/secretmanager/SecretManagerSecureStoreServiceTest.java @@ -16,9 +16,10 @@ package io.cdap.cdap.security.store.secretmanager; +import io.cdap.cdap.api.security.store.SecureStoreCapability; import io.cdap.cdap.api.security.store.SecureStoreData; -import io.cdap.cdap.api.security.store.SecureStoreMetadata; import io.cdap.cdap.api.security.store.SecureStoreLease; +import io.cdap.cdap.api.security.store.SecureStoreMetadata; import io.cdap.cdap.common.SecureKeyNotFoundException; import io.cdap.cdap.common.namespace.InMemoryNamespaceAdmin; import io.cdap.cdap.proto.NamespaceMeta; @@ -109,7 +110,7 @@ public void testLeaseOperations() throws Exception { String key = "leasekey"; secureStoreService.put(NAMESPACE1, key, "value", "desc", new HashMap<>()); - Assert.assertTrue(secureStoreService.isLeaseSupported()); + Assert.assertTrue(secureStoreService.getCapabilities().contains(SecureStoreCapability.LEASE)); SecureStoreLease lease = secureStoreService.acquireLease(NAMESPACE1, key, 1000L, "holder1");