[CDAP-20872] Addressed comments, renamed AeadCipher SPI to AeadCipher…

…Cryptor
cdapio · Nov 16, 2023 · b2f31c1 · b2f31c1
1 parent ecb7d92
commit b2f31c1
Show file tree

Hide file tree

Showing 28 changed files with 141 additions and 142 deletions.
diff --git a/...-app-fabric/src/main/java/io/cdap/cdap/internal/credential/CredentialIdentityManager.java b/...-app-fabric/src/main/java/io/cdap/cdap/internal/credential/CredentialIdentityManager.java
@@ -18,8 +18,6 @@
 
 import io.cdap.cdap.common.AlreadyExistsException;
 import io.cdap.cdap.common.NotFoundException;
-import io.cdap.cdap.internal.credential.store.CredentialIdentityStore;
-import io.cdap.cdap.internal.credential.store.CredentialProfileStore;
 import io.cdap.cdap.proto.credential.CredentialIdentity;
 import io.cdap.cdap.proto.id.CredentialIdentityId;
 import io.cdap.cdap.proto.id.CredentialProfileId;

diff --git a/...ential/store/CredentialIdentityStore.java → ...l/credential/CredentialIdentityStore.java b/...ential/store/CredentialIdentityStore.java → ...l/credential/CredentialIdentityStore.java
@@ -14,7 +14,7 @@
  * the License.
  */
 
-package io.cdap.cdap.internal.credential.store;
+package io.cdap.cdap.internal.credential;
 
 import com.google.gson.Gson;
 import io.cdap.cdap.api.dataset.lib.CloseableIterator;
@@ -50,14 +50,18 @@
  */
 public class CredentialIdentityStore {
 
-  private static final byte[] CREDENTIAL_IDENTITY_STORE_AD = CredentialIdentityStore.class
-      .getCanonicalName().getBytes(StandardCharsets.UTF_8);
+  /**
+   * DO NOT CHANGE THIS VALUE! CHANGING THIS VALUE IS BACKWARDS-INCOMPATIBLE. Values encrypted using
+   * a different value will not decrypt properly!
+   */
+  private static final byte[] CREDENTIAL_IDENTITY_STORE_AD = "CredentialIdentityStore"
+      .getBytes(StandardCharsets.UTF_8);
   private static final Gson GSON = new Gson();
 
   private final AeadCipher dataStorageCipher;
 
   @Inject
-  public CredentialIdentityStore(@Named(DataStorageAeadEncryptionModule.DATA_STORAGE_ENCRYPTION)
+  CredentialIdentityStore(@Named(DataStorageAeadEncryptionModule.DATA_STORAGE_ENCRYPTION)
       AeadCipher dataStorageCipher) {
     this.dataStorageCipher = dataStorageCipher;
   }
@@ -109,13 +113,7 @@ public Collection<CredentialIdentityId> listForProfile(StructuredTableContext co
    */
   public boolean exists(StructuredTableContext context, CredentialIdentityId id)
       throws IOException {
-    StructuredTable table = context.getTable(CredentialProviderStore.CREDENTIAL_IDENTITIES);
-    Collection<Field<?>> key = Arrays.asList(
-        Fields.stringField(CredentialProviderStore.NAMESPACE_FIELD,
-            id.getNamespace()),
-        Fields.stringField(CredentialProviderStore.IDENTITY_NAME_FIELD,
-            id.getName()));
-    return table.read(key).isPresent();
+    return readIdentity(context, id).isPresent();
   }
 
   /**
@@ -128,13 +126,7 @@ public boolean exists(StructuredTableContext context, CredentialIdentityId id)
    */
   public Optional<CredentialIdentity> get(StructuredTableContext context, CredentialIdentityId id)
       throws CipherException, IOException {
-    StructuredTable table = context.getTable(CredentialProviderStore.CREDENTIAL_IDENTITIES);
-    Collection<Field<?>> key = Arrays.asList(
-        Fields.stringField(CredentialProviderStore.NAMESPACE_FIELD,
-            id.getNamespace()),
-        Fields.stringField(CredentialProviderStore.IDENTITY_NAME_FIELD,
-            id.getName()));
-    Optional<StructuredRow> row = table.read(key);
+    Optional<StructuredRow> row = readIdentity(context, id);
     if (!row.isPresent()) {
       return Optional.empty();
     }
@@ -200,4 +192,15 @@ private static Collection<CredentialIdentityId> identitiesFromRowIterator(
   private static String toProfileIndex(String profileNamespace, String profileName) {
     return String.format("%s:%s", profileNamespace, profileName);
   }
+
+  private Optional<StructuredRow> readIdentity(StructuredTableContext context,
+      CredentialIdentityId id) throws IOException {
+    StructuredTable table = context.getTable(CredentialProviderStore.CREDENTIAL_IDENTITIES);
+    Collection<Field<?>> key = Arrays.asList(
+        Fields.stringField(CredentialProviderStore.NAMESPACE_FIELD,
+            id.getNamespace()),
+        Fields.stringField(CredentialProviderStore.IDENTITY_NAME_FIELD,
+            id.getName()));
+    return table.read(key);
+  }
 }
diff --git a/cdap-app-fabric/src/main/java/io/cdap/cdap/internal/credential/CredentialProfileManager.java b/cdap-app-fabric/src/main/java/io/cdap/cdap/internal/credential/CredentialProfileManager.java
@@ -21,8 +21,6 @@
 import io.cdap.cdap.common.BadRequestException;
 import io.cdap.cdap.common.ConflictException;
 import io.cdap.cdap.common.NotFoundException;
-import io.cdap.cdap.internal.credential.store.CredentialIdentityStore;
-import io.cdap.cdap.internal.credential.store.CredentialProfileStore;
 import io.cdap.cdap.proto.credential.CredentialProfile;
 import io.cdap.cdap.proto.id.CredentialIdentityId;
 import io.cdap.cdap.proto.id.CredentialProfileId;

diff --git a/...dential/store/CredentialProfileStore.java → ...al/credential/CredentialProfileStore.java b/...dential/store/CredentialProfileStore.java → ...al/credential/CredentialProfileStore.java
@@ -14,7 +14,7 @@
  * the License.
  */
 
-package io.cdap.cdap.internal.credential.store;
+package io.cdap.cdap.internal.credential;
 
 import com.google.gson.Gson;
 import io.cdap.cdap.api.dataset.lib.CloseableIterator;
@@ -50,14 +50,18 @@
  */
 public class CredentialProfileStore {
 
-  private static final byte[] CREDENTIAL_PROFILE_STORE_AD = CredentialProviderStore.class
-      .getCanonicalName().getBytes(StandardCharsets.UTF_8);
+  /**
+   * DO NOT CHANGE THIS VALUE! CHANGING THIS VALUE IS BACKWARDS-INCOMPATIBLE. Values encrypted using
+   * a different value will not decrypt properly!
+   */
+  private static final byte[] CREDENTIAL_PROFILE_STORE_AD = "CredentialProviderStore"
+      .getBytes(StandardCharsets.UTF_8);
   private static final Gson GSON = new Gson();
 
   private final AeadCipher dataStorageCipher;
 
   @Inject
-  public CredentialProfileStore(@Named(DataStorageAeadEncryptionModule.DATA_STORAGE_ENCRYPTION)
+  CredentialProfileStore(@Named(DataStorageAeadEncryptionModule.DATA_STORAGE_ENCRYPTION)
       AeadCipher dataStorageCipher) {
     this.dataStorageCipher = dataStorageCipher;
   }

diff --git a/...p-fabric/src/test/java/io/cdap/cdap/internal/credential/CredentialProfileManagerTest.java b/...p-fabric/src/test/java/io/cdap/cdap/internal/credential/CredentialProfileManagerTest.java
@@ -20,7 +20,6 @@
 import io.cdap.cdap.common.BadRequestException;
 import io.cdap.cdap.common.ConflictException;
 import io.cdap.cdap.common.NotFoundException;
-import io.cdap.cdap.internal.credential.store.CredentialProfileStore;
 import io.cdap.cdap.proto.credential.CredentialIdentity;
 import io.cdap.cdap.proto.credential.CredentialProfile;
 import io.cdap.cdap.proto.id.CredentialIdentityId;

diff --git a/...app-fabric/src/test/java/io/cdap/cdap/internal/credential/CredentialProviderTestBase.java b/...app-fabric/src/test/java/io/cdap/cdap/internal/credential/CredentialProviderTestBase.java
@@ -31,8 +31,6 @@
 import io.cdap.cdap.common.metrics.NoOpMetricsCollectionService;
 import io.cdap.cdap.data.runtime.StorageModule;
 import io.cdap.cdap.data.runtime.SystemDatasetRuntimeModule;
-import io.cdap.cdap.internal.credential.store.CredentialIdentityStore;
-import io.cdap.cdap.internal.credential.store.CredentialProfileStore;
 import io.cdap.cdap.proto.credential.CredentialProfile;
 import io.cdap.cdap.proto.credential.CredentialProvisioningException;
 import io.cdap.cdap.proto.credential.ProvisionedCredential;

diff --git a/cdap-encryption-ext-tink/pom.xml b/cdap-encryption-ext-tink/pom.xml
@@ -92,6 +92,12 @@
       <artifactId>google-cloud-kms</artifactId>
     </dependency>
 
+    <!-- Other GCP Dependencies -->
+    <dependency>
+      <groupId>com.google.cloud</groupId>
+      <artifactId>google-cloud-core</artifactId>
+    </dependency>
+
     <!-- Test Dependencies -->
     <dependency>
       <groupId>junit</groupId>

diff --git a/...cryption/tink/AbstractTinkAeadCipher.java → ...n/tink/AbstractTinkAeadCipherCryptor.java b/...cryption/tink/AbstractTinkAeadCipher.java → ...n/tink/AbstractTinkAeadCipherCryptor.java
@@ -17,16 +17,16 @@
 package io.cdap.cdap.encryption.tink;
 
 import com.google.crypto.tink.Aead;
-import io.cdap.cdap.security.spi.encryption.AeadCipher;
 import io.cdap.cdap.security.spi.encryption.AeadCipherContext;
+import io.cdap.cdap.security.spi.encryption.AeadCipherCryptor;
 import io.cdap.cdap.security.spi.encryption.CipherInitializationException;
 import io.cdap.cdap.security.spi.encryption.CipherOperationException;
 import java.security.GeneralSecurityException;
 
 /**
- * An {@link AeadCipher} backed by Tink.
+ * An {@link AeadCipherCryptor} backed by Tink.
  */
-public abstract class AbstractTinkAeadCipher implements AeadCipher {
+public abstract class AbstractTinkAeadCipherCryptor implements AeadCipherCryptor {
 
   /**
    * Tink AEAD (Authenticated Encryption with Associated Data) primitive.

diff --git a/.../encryption/tink/CleartextTinkCipher.java → ...tion/tink/CleartextTinkCipherCryptor.java b/.../encryption/tink/CleartextTinkCipher.java → ...tion/tink/CleartextTinkCipherCryptor.java
@@ -29,7 +29,7 @@
 /**
  * Tink cipher that allows encrypting and decrypting data using keyset provided via properties.
  */
-public class CleartextTinkCipher extends AbstractTinkAeadCipher {
+public class CleartextTinkCipherCryptor extends AbstractTinkAeadCipherCryptor {
 
   /**
    * References a keyset stored in secure properties.

diff --git a/cdap-encryption-ext-tink/src/main/java/io/cdap/cdap/encryption/tink/CloudKmsClient.java b/cdap-encryption-ext-tink/src/main/java/io/cdap/cdap/encryption/tink/CloudKmsClient.java
@@ -17,7 +17,8 @@
 package io.cdap.cdap.encryption.tink;
 
 import com.google.api.gax.rpc.ApiException;
-import com.google.api.gax.rpc.StatusCode;
+import com.google.api.gax.rpc.StatusCode.Code;
+import com.google.cloud.ServiceOptions;
 import com.google.cloud.kms.v1.CryptoKey;
 import com.google.cloud.kms.v1.CryptoKey.CryptoKeyPurpose;
 import com.google.cloud.kms.v1.CryptoKeyName;
@@ -27,17 +28,12 @@
 import com.google.cloud.kms.v1.KeyRing;
 import com.google.cloud.kms.v1.KeyRingName;
 import com.google.cloud.kms.v1.LocationName;
-import com.google.common.io.CharStreams;
 import com.google.protobuf.Duration;
 import com.google.protobuf.Timestamp;
 import io.cdap.cdap.security.spi.encryption.CipherInitializationException;
 import java.io.Closeable;
 import java.io.IOException;
-import java.io.InputStreamReader;
-import java.io.Reader;
-import java.net.HttpURLConnection;
-import java.net.URL;
-import java.nio.charset.StandardCharsets;
+import java.time.Instant;
 import java.time.temporal.ChronoUnit;
 import java.util.HashSet;
 import java.util.Set;
@@ -64,37 +60,21 @@ public class CloudKmsClient implements Closeable {
    * @throws IOException if cloud kms client can not be created
    */
   CloudKmsClient(String projectId, String location, String keyRingId) throws IOException {
-    this.projectId = projectId;
+    if (projectId != null) {
+      this.projectId = projectId;
+    } else {
+      this.projectId = ServiceOptions.getDefaultProjectId();
+    }
     this.location = location;
     this.keyRingId = keyRingId;
     this.client = KeyManagementServiceClient.create();
     this.knownCryptoKeys = new HashSet<>();
   }
 
   /**
-   * Get project id from the metadata server. Makes a request to the metadata server that lives on
-   * the VM, as described at https://cloud.google.com/compute/docs/storing-retrieving-metadata.
-   */
-  private String getSystemProjectId(String metadataServerApi) throws IOException {
-    URL url = new URL(metadataServerApi);
-    HttpURLConnection connection = (HttpURLConnection) url.openConnection();
-    try {
-      connection = (HttpURLConnection) url.openConnection();
-      connection.setRequestProperty("Metadata-Flavor", "Google");
-      connection.connect();
-      try (Reader reader = new InputStreamReader(connection.getInputStream(),
-          StandardCharsets.UTF_8)) {
-        return CharStreams.toString(reader);
-      }
-    } finally {
-      connection.disconnect();
-    }
-  }
-
-  /**
-   * Creates a new key ring with the given id.
+   * Creates a new key ring with the given ID.
    *
-   * @throws IOException if there's an error while creating the key ring
+   * @throws CipherInitializationException If there's an error while creating the key ring
    */
   void createKeyRingIfNotExists() throws CipherInitializationException {
     LOG.debug("Creating key ring with id {} in projects/{}/locations/{}.", keyRingId, projectId,
@@ -105,20 +85,27 @@ void createKeyRingIfNotExists() throws CipherInitializationException {
       // If we get here, the key ring was found, so we skip creating the key ring.
       return;
     } catch (ApiException e) {
-      if (!e.getStatusCode().getCode().equals(StatusCode.Code.NOT_FOUND)) {
+      if (!e.getStatusCode().getCode().equals(Code.NOT_FOUND)) {
         throw new CipherInitializationException("Failed to check if Cloud KMS key ring exists", e);
       }
     }
 
     LocationName locationName = LocationName.of(projectId, location);
-    client.createKeyRing(locationName, keyRingId, KeyRing.newBuilder().build());
+    try {
+      client.createKeyRing(locationName, keyRingId, KeyRing.newBuilder().build());
+    } catch (ApiException e) {
+      if (!e.getStatusCode().getCode().equals(Code.ALREADY_EXISTS)) {
+        throw new CipherInitializationException("Failed to create new Cloud KMS key ring", e);
+      }
+      LOG.info("Failed to create new Cloud KMS key ring, likely already exists.");
+    }
   }
 
   /**
-   * Creates a new crypto key on google cloud kms with the given id.
+   * Creates a new crypto key with the given ID.
    *
-   * @param cryptoKeyId crypto key id
-   * @throws IOException if there's an error creating crypto key
+   * @param cryptoKeyId The crypto key ID
+   * @throws CipherInitializationException If there's an error creating crypto key
    */
   void createCryptoKeyIfNotExists(String cryptoKeyId) throws CipherInitializationException {
     // If crypto key is already created, do not attempt to create it again.
@@ -131,14 +118,14 @@ void createCryptoKeyIfNotExists(String cryptoKeyId) throws CipherInitializationE
       // If we get here, the crypto key was found, so we skip creating the crypto key.
       return;
     } catch (ApiException e) {
-      if (!e.getStatusCode().getCode().equals(StatusCode.Code.NOT_FOUND)) {
+      if (!e.getStatusCode().getCode().equals(Code.NOT_FOUND)) {
         throw new CipherInitializationException("Failed to check if Cloud KMS crypto key exists",
             e);
       }
     }
 
     // Calculate the date 24 hours from now (this is used below).
-    long tomorrow = java.time.Instant.now().plus(24, ChronoUnit.HOURS).getEpochSecond();
+    long tomorrow = Instant.now().plus(24, ChronoUnit.HOURS).getEpochSecond();
 
     // Build the key to create with a rotation schedule.
     com.google.cloud.kms.v1.CryptoKey key =
@@ -163,10 +150,10 @@ void createCryptoKeyIfNotExists(String cryptoKeyId) throws CipherInitializationE
           .createCryptoKey(KeyRingName.of(projectId, location, keyRingId), cryptoKeyId, key);
       LOG.info("Created key with rotation schedule {}", createdKey.getName());
     } catch (ApiException e) {
-      if (!e.getStatusCode().getCode().equals(StatusCode.Code.ALREADY_EXISTS)) {
+      if (!e.getStatusCode().getCode().equals(Code.ALREADY_EXISTS)) {
         throw new CipherInitializationException("Failed to create crypto key", e);
       }
-      LOG.info("Failed to create crypto key, likely already exists.");
+      LOG.info("Failed to create Cloud KMS crypto key, likely already exists.");
     }
 
     // In-memory cache to keep list of crypto keys created so far.

diff --git a/...ncryption/tink/GcpEnvelopeTinkCipher.java → ...on/tink/GcpEnvelopeTinkCipherCryptor.java b/...ncryption/tink/GcpEnvelopeTinkCipher.java → ...on/tink/GcpEnvelopeTinkCipherCryptor.java
@@ -35,7 +35,7 @@
 /**
  * Tink cipher that allows encrypting and decrypting data using Cloud KMS.
  */
-public class GcpEnvelopeTinkCipher extends AbstractTinkAeadCipher {
+public class GcpEnvelopeTinkCipherCryptor extends AbstractTinkAeadCipherCryptor {
 
   /**
    * KEK URI for use by Tink. For details, see

diff --git a/...p.cdap.security.spi.encryption.AeadCipher → ...security.spi.encryption.AeadCipherCryptor b/...p.cdap.security.spi.encryption.AeadCipher → ...security.spi.encryption.AeadCipherCryptor
@@ -14,5 +14,5 @@
 # the License.
 #
 
-io.cdap.cdap.encryption.tink.CleartextTinkCipher
-io.cdap.cdap.encryption.tink.GcpEnvelopeTinkCipher
+io.cdap.cdap.encryption.tink.CleartextTinkCipherCryptor
+io.cdap.cdap.encryption.tink.GcpEnvelopeTinkCipherCryptor
diff --git a/...cryption-ext-tink/src/test/java/io/cdap/cdap/encryption/tink/CleartextTinkCipherTest.java b/...cryption-ext-tink/src/test/java/io/cdap/cdap/encryption/tink/CleartextTinkCipherTest.java
@@ -40,9 +40,9 @@ public void testEncryptionAndDecryption() throws CipherInitializationException,
       CipherOperationException, IOException, GeneralSecurityException {
     Map<String, String> properties = new HashMap<>();
     Map<String, String> secureProperties = new HashMap<>();
-    secureProperties.put(CleartextTinkCipher.TINK_CLEARTEXT_KEYSET_KEY, generateKeySet());
+    secureProperties.put(CleartextTinkCipherCryptor.TINK_CLEARTEXT_KEYSET_KEY, generateKeySet());
 
-    CleartextTinkCipher cipher = new CleartextTinkCipher();
+    CleartextTinkCipherCryptor cipher = new CleartextTinkCipherCryptor();
     cipher.initialize(new AeadCipherContext(properties, secureProperties));
 
     byte[] plainData = generateRandomBytes(2 * 1024);
@@ -56,9 +56,9 @@ public void testEncryptionAndDecryption() throws CipherInitializationException,
   public void testInitException() throws CipherInitializationException {
     Map<String, String> properties = new HashMap<>();
     Map<String, String> secureProperties = new HashMap<>();
-    secureProperties.put(CleartextTinkCipher.TINK_CLEARTEXT_KEYSET_KEY, "invalid-keyset");
+    secureProperties.put(CleartextTinkCipherCryptor.TINK_CLEARTEXT_KEYSET_KEY, "invalid-keyset");
 
-    CleartextTinkCipher cipher = new CleartextTinkCipher();
+    CleartextTinkCipherCryptor cipher = new CleartextTinkCipherCryptor();
     cipher.initialize(new AeadCipherContext(properties, secureProperties));
   }
 
@@ -67,9 +67,9 @@ public void testDecryptExceptionTagMismatch() throws CipherInitializationExcepti
       CipherOperationException, IOException, GeneralSecurityException {
     Map<String, String> properties = new HashMap<>();
     Map<String, String> secureProperties = new HashMap<>();
-    secureProperties.put(CleartextTinkCipher.TINK_CLEARTEXT_KEYSET_KEY, generateKeySet());
+    secureProperties.put(CleartextTinkCipherCryptor.TINK_CLEARTEXT_KEYSET_KEY, generateKeySet());
 
-    CleartextTinkCipher cipher = new CleartextTinkCipher();
+    CleartextTinkCipherCryptor cipher = new CleartextTinkCipherCryptor();
     cipher.initialize(new AeadCipherContext(properties, secureProperties));
 
     byte[] plainData = generateRandomBytes(128);
@@ -84,9 +84,9 @@ public void testDecryptExceptionCorruption() throws CipherInitializationExceptio
       CipherOperationException, IOException, GeneralSecurityException {
     Map<String, String> properties = new HashMap<>();
     Map<String, String> secureProperties = new HashMap<>();
-    secureProperties.put(CleartextTinkCipher.TINK_CLEARTEXT_KEYSET_KEY, generateKeySet());
+    secureProperties.put(CleartextTinkCipherCryptor.TINK_CLEARTEXT_KEYSET_KEY, generateKeySet());
 
-    CleartextTinkCipher cipher = new CleartextTinkCipher();
+    CleartextTinkCipherCryptor cipher = new CleartextTinkCipherCryptor();
     cipher.initialize(new AeadCipherContext(properties, secureProperties));
 
     byte[] plainData = generateRandomBytes(128);

diff --git a/cdap-gateway/src/main/java/io/cdap/cdap/gateway/router/handlers/AuthenticationHandler.java b/cdap-gateway/src/main/java/io/cdap/cdap/gateway/router/handlers/AuthenticationHandler.java
@@ -256,7 +256,7 @@ private Credential getUserCredential(UserIdentityPair userIdentityPair)
       return new Credential(userCredential, Credential.CredentialType.EXTERNAL);
     }
     String encryptedCredential = userCredentialAeadCipher
-        .encryptStringToBase64(userCredential,
+        .encryptToBase64(userCredential,
             Encryption.USER_CREDENTIAL_ENCRYPTION_ASSOCIATED_DATA.getBytes());
     return new Credential(encryptedCredential, Credential.CredentialType.EXTERNAL_ENCRYPTED);
   }