Make secretmanager primary storage for keyring (#1124)

* Make secretmanager primary storage for keyring

Also removed the migrate_kms_keyring command.

core/src/main/java/google/registry/keyring/kms/KmsKeyring.java

@@ -21,9 +21,6 @@ import static google.registry.model.common.EntityGroupRoot.getCrossTldKey;
 import static google.registry.model.ofy.ObjectifyService.ofy;
 import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
 
-import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.Streams;
 import com.google.common.flogger.FluentLogger;
 import com.googlecode.objectify.Key;
 import google.registry.config.RegistryConfig.Config;
@@ -37,7 +34,6 @@ import google.registry.privileges.secretmanager.KeyringSecretStore;
 import java.io.IOException;
 import java.util.Arrays;
 import java.util.Optional;
-import java.util.stream.Stream;
 import javax.inject.Inject;
 import org.bouncycastle.openpgp.PGPException;
 import org.bouncycastle.openpgp.PGPKeyPair;
@@ -51,6 +47,7 @@ import org.bouncycastle.openpgp.PGPPublicKey;
  * @see <a href="https://cloud.google.com/kms/docs/">Google Cloud Key Management Service
  *     Documentation</a>
  */
+// TODO(2021-06-01): rename this class to SecretManagerKeyring and delete KmsSecretRevision
 public class KmsKeyring implements Keyring {
 
   private static final FluentLogger logger = FluentLogger.forEnclosingClass();
@@ -221,8 +218,7 @@ public class KmsKeyring implements Keyring {
     try {
       return kmsConnection.decrypt(keyName, encryptedData);
     } catch (Exception e) {
-      throw new KeyringException(
+      return new byte[0];
-          String.format("CloudKMS decrypt operation failed for secret %s", keyName), e);
     }
   }
 
@@ -230,7 +226,7 @@ public class KmsKeyring implements Keyring {
     try {
       return secretStore.getSecret(keyName);
     } catch (Exception e) {
-      return new byte[0];
+      throw new KeyringException("Failed to retrieve secret for " + keyName, e);
     }
   }
 
@@ -243,44 +239,6 @@ public class KmsKeyring implements Keyring {
       return secretStoreData;
     }
     logger.atWarning().log("Values for %s in Datastore and Secret Manager do not match.", keyName);
-    return dsData;
+    return secretStoreData;
-  }
-
-  /**
-   * Generates the tasks to migrate secrets from Datastore to Secret Manager.
-   *
-   * <p>The keys in the returned {@link ImmutableMap} are the names of the secrets that need
-   * migration. The values in the map are {@link Runnable Runnables} that copy secret data from
-   * Datastore to Secret Manager for their corresponding keys. Only secrets that are absent in
-   * Secret Manager or have inconsistent values are included in the returned map.
-   */
-  public ImmutableMap<String, Runnable> migrationPlan() {
-
-    ImmutableMap.Builder<String, Runnable> tasks = new ImmutableMap.Builder<>();
-
-    ImmutableList<String> labels =
-        Streams.concat(
-                Stream.of(PrivateKeyLabel.values()).map(PrivateKeyLabel::getLabel),
-                Stream.of(PublicKeyLabel.values()).map(PublicKeyLabel::getLabel),
-                Stream.of(StringKeyLabel.values()).map(StringKeyLabel::getLabel))
-            .collect(ImmutableList.toImmutableList());
-
-    for (String keyName : labels) {
-      byte[] dsData;
-      try {
-        dsData = getDecryptedDataFromDatastore(keyName);
-      } catch (IllegalStateException e) {
-        logger.atWarning().log("Cannot load %s from Datastore. Skipping...", keyName);
-        continue;
-      }
-      byte[] secretStoreData = getDataFromSecretStore(keyName);
-      if (Arrays.equals(dsData, secretStoreData)) {
-        logger.atInfo().log("%s is already up to date.\n", keyName);
-        continue;
-      }
-      logger.atInfo().log("%s needs to be migrated.\n", keyName);
-      tasks.put(keyName, () -> secretStore.createOrUpdateSecret(keyName, dsData));
-    }
-    return tasks.build();
   }
 }

core/src/main/java/google/registry/keyring/kms/KmsUpdater.java

@@ -36,6 +36,7 @@ import static google.registry.persistence.transaction.TransactionManagerFactory.
 import static google.registry.util.PreconditionsUtils.checkArgumentNotNull;
 
 import com.google.common.collect.ImmutableMap;
+import com.google.common.flogger.FluentLogger;
 import google.registry.config.RegistryConfig.Config;
 import google.registry.keyring.api.KeySerializer;
 import google.registry.keyring.kms.KmsKeyring.PrivateKeyLabel;
@@ -57,7 +58,9 @@ import org.bouncycastle.openpgp.PGPPublicKey;
  * The {@link KmsUpdater} accumulates updates to a {@link KmsKeyring} and persists them to KMS and
  * Datastore when closed.
  */
+// TODO(2021-06-01): rename this class to SecretManagerKeyringUpdater
 public final class KmsUpdater {
+  private static final FluentLogger logger = FluentLogger.forEnclosingClass();
 
   private final KmsConnection kmsConnection;
   private final KeyringSecretStore secretStore;
@@ -126,29 +129,30 @@ public final class KmsUpdater {
   }
 
   /**
-   * Generates new encryption keys in KMS, encrypts the updated secrets with them, and persists the
+   * Persists the secrets in the Secret Manager (primary) and the Datastore (secondary).
-   * encrypted secrets to Datastore.
    *
-   * <p>The operations in this method are organized so that existing {@link KmsSecretRevision}
+   * <p>Updates to the Secret Manager are not transactional. If an error happens, the successful
-   * entities remain primary and decryptable if a failure occurs.
+   * updates are not reverted; unwritten updates are aborted. This is not a problem right now, since
+   * this class is only used by the {@code UpdateKmsKeyringCommand}, which is invoked manually and
+   * only updates one secret at a time.
    */
   public void update() {
     checkState(!secretValues.isEmpty(), "At least one Keyring value must be persisted");
 
-    persistEncryptedValues(encryptValues(secretValues));
-
-    // Errors when writing to secret store can be thrown to the top, since writes are always
-    // executed by a human user using the UpdateKmsKeyringCommand.
     try {
-      secretValues
+      for (Map.Entry<String, byte[]> e : secretValues.entrySet()) {
-          .entrySet()
+        secretStore.createOrUpdateSecret(e.getKey(), e.getValue());
-          .forEach(e -> secretStore.createOrUpdateSecret(e.getKey(), e.getValue()));
+        logger.atInfo().log("Secret %s updated.", e.getKey());
+      }
     } catch (RuntimeException e) {
       throw new RuntimeException(
           "Failed to persist secrets to Secret Manager. "
               + "Please check the status of Secret Manager and re-run the command.",
           e);
     }
+
+    // TODO(2021-06-01): remove the writes to Datastore
+    persistEncryptedValues(encryptValues(secretValues));
   }
 
   /**

core/src/main/java/google/registry/tools/MigrateKmsKeyringCommand.java

@@ -1,78 +0,0 @@
-// Copyright 2021 The Nomulus Authors. All Rights Reserved.
-//
-// 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 google.registry.tools;
-
-import static com.google.common.base.Preconditions.checkState;
-
-import com.beust.jcommander.Parameters;
-import google.registry.keyring.api.Keyring;
-import google.registry.keyring.kms.KmsKeyring;
-import google.registry.privileges.secretmanager.KeyringSecretStore;
-import java.util.Map;
-import javax.inject.Inject;
-
-/** Migrates secrets from the KMS keyring to the Secret Manager. */
-@Parameters(
-    separators = " =",
-    commandDescription = "Migrate values of secrets in KmsKeyring to Secret Manager.")
-public class MigrateKmsKeyringCommand extends ConfirmingCommand implements CommandWithRemoteApi {
-
-  @Inject Keyring keyring;
-
-  @Inject KeyringSecretStore secretStore;
-
-  Map<String, Runnable> migrationTasks;
-
-  @Inject
-  MigrateKmsKeyringCommand() {}
-
-  @Override
-  protected void init() {
-
-    checkState(
-        keyring instanceof KmsKeyring,
-        "Expecting KmsKeyring, found %s",
-        keyring.getClass().getSimpleName());
-
-    migrationTasks = ((KmsKeyring) keyring).migrationPlan();
-  }
-
-  @Override
-  protected boolean dontRunCommand() {
-    return migrationTasks.isEmpty();
-  }
-
-  @Override
-  protected String prompt() {
-    if (migrationTasks.isEmpty()) {
-      return "All keys are up to date.";
-    }
-    return String.format("Migrate %s keys?", migrationTasks.size());
-  }
-
-  @Override
-  protected String execute() {
-    int errors = 0;
-    for (Map.Entry<String, Runnable> entry : migrationTasks.entrySet()) {
-      try {
-        entry.getValue().run();
-      } catch (Exception e) {
-        System.err.printf("Failed to migrate %s: %s", entry.getKey(), e.getMessage());
-        errors++;
-      }
-    }
-    return errors == 0 ? "Success!" : "Failed to migrate " + errors + "keys.";
-  }
-}

core/src/main/java/google/registry/tools/RegistryTool.java

@@ -103,7 +103,6 @@ public final class RegistryTool {
           .put("lock_domain", LockDomainCommand.class)
           .put("login", LoginCommand.class)
           .put("logout", LogoutCommand.class)
-          .put("migrate_kms_keyring", MigrateKmsKeyringCommand.class)
           .put("pending_escrow", PendingEscrowCommand.class)
           .put("populate_null_registrar_fields", PopulateNullRegistrarFieldsCommand.class)
           .put("registrar_contact", RegistrarContactCommand.class)

core/src/main/java/google/registry/tools/RegistryToolComponent.java

@@ -135,8 +135,6 @@ interface RegistryToolComponent {
 
   void inject(LogoutCommand command);
 
-  void inject(MigrateKmsKeyringCommand command);
-
   void inject(PendingEscrowCommand command);
 
   void inject(RenewDomainCommand command);

core/src/test/java/google/registry/keyring/kms/KmsKeyringTest.java

@@ -44,12 +44,12 @@ class KmsKeyringTest {
       AppEngineExtension.builder().withDatastoreAndCloudSql().build();
 
   private KmsKeyring keyring;
+  private KeyringSecretStore fakeSecretStore =
+      new KeyringSecretStore(new FakeSecretManagerClient());
 
   @BeforeEach
   void beforeEach() {
-    keyring =
+    keyring = new KmsKeyring(new FakeKmsConnection(), fakeSecretStore);
-        new KmsKeyring(
-            new FakeKmsConnection(), new KeyringSecretStore(new FakeSecretManagerClient()));
   }
 
   @TestOfyAndSql
@@ -154,7 +154,7 @@ class KmsKeyringTest {
     assertThat(jsonCredential).isEqualTo("json-credential-stringmoo");
   }
 
-  private static void persistSecret(String secretName, byte[] secretValue) {
+  private void persistSecret(String secretName, byte[] secretValue) {
     KmsConnection kmsConnection = new FakeKmsConnection();
 
     KmsSecretRevision secretRevision =
@@ -165,22 +165,22 @@ class KmsKeyringTest {
             .build();
     KmsSecret secret = KmsSecret.create(secretName, secretRevision);
     tm().transact(() -> tm().putAll(secretRevision, secret));
+    fakeSecretStore.createOrUpdateSecret(secretName, secretValue);
   }
 
-  private static void saveCleartextSecret(String secretName) {
+  private void saveCleartextSecret(String secretName) {
     persistSecret(secretName, KeySerializer.serializeString(secretName + "moo"));
   }
 
-  private static void savePublicKeySecret(String publicKeyName) throws Exception {
+  private void savePublicKeySecret(String publicKeyName) throws Exception {
     persistSecret(publicKeyName, KeySerializer.serializePublicKey(KmsTestHelper.getPublicKey()));
   }
 
-  private static void savePrivateKeySecret(String privateKeyName) throws Exception {
+  private void savePrivateKeySecret(String privateKeyName) throws Exception {
     persistSecret(privateKeyName, KeySerializer.serializeKeyPair(KmsTestHelper.getKeyPair()));
   }
 
-  private static void saveKeyPairSecret(String publicKeyName, String privateKeyName)
+  private void saveKeyPairSecret(String publicKeyName, String privateKeyName) throws Exception {
-      throws Exception {
     savePublicKeySecret(publicKeyName);
     savePrivateKeySecret(privateKeyName);
   }