Migrate Keyring secrets to Secret Manager (#1072)

* Migrate Keyring secrets to Secret Manager

Implented dual-read of Keyring secrets with Datastore as primary.

Implemented dual-write of keyring secrets with Datastore as primary.
Secret manager write failures are simply thrown. This is fine since all
keyring writes are manual, throught eh update_kms_keyring command.

Added a one-way migration command that copies all data to secret manager
(unencrypted).

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

@@ -21,6 +21,10 @@ 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;
 import google.registry.keyring.api.KeySerializer;
@@ -29,8 +33,11 @@ import google.registry.keyring.api.KeyringException;
 import google.registry.model.server.KmsSecret;
 import google.registry.model.server.KmsSecretRevision;
 import google.registry.model.server.KmsSecretRevisionSqlDao;
+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;
@@ -46,6 +53,8 @@ import org.bouncycastle.openpgp.PGPPublicKey;
  */
 public class KmsKeyring implements Keyring {
 
+  private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
   /** Key labels for private key secrets. */
   enum PrivateKeyLabel {
     BRDA_SIGNING_PRIVATE,
@@ -87,10 +96,13 @@ public class KmsKeyring implements Keyring {
   }
 
   private final KmsConnection kmsConnection;
+  private final KeyringSecretStore secretStore;
 
   @Inject
-  KmsKeyring(@Config("defaultKmsConnection") KmsConnection kmsConnection) {
+  KmsKeyring(
+      @Config("defaultKmsConnection") KmsConnection kmsConnection, KeyringSecretStore secretStore) {
     this.kmsConnection = kmsConnection;
+    this.secretStore = secretStore;
   }
 
   @Override
@@ -192,7 +204,7 @@ public class KmsKeyring implements Keyring {
     return getKeyPair(keyLabel).getPrivateKey();
   }
 
-  private byte[] getDecryptedData(String keyName) {
+  private byte[] getDecryptedDataFromDatastore(String keyName) {
     String encryptedData;
     if (tm().isOfy()) {
       KmsSecret secret =
@@ -213,4 +225,56 @@ public class KmsKeyring implements Keyring {
           String.format("CloudKMS decrypt operation failed for secret %s", keyName), e);
     }
   }
+
+  private byte[] getDataFromSecretStore(String keyName) {
+    try {
+      return secretStore.getSecret(keyName);
+    } catch (Exception e) {
+      return new byte[0];
+    }
+  }
+
+  private byte[] getDecryptedData(String keyName) {
+    byte[] dsData = getDecryptedDataFromDatastore(keyName);
+    byte[] secretStoreData = getDataFromSecretStore(keyName);
+
+    if (Arrays.equals(dsData, secretStoreData)) {
+      logger.atInfo().log("Values for %s in Datastore and Secret Manager match.", keyName);
+      return secretStoreData;
+    }
+    logger.atWarning().log("Values for %s in Datastore and Secret Manager do not match.", keyName);
+    return dsData;
+  }
+
+  /**
+   * 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 = getDecryptedDataFromDatastore(keyName);
+      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

@@ -43,6 +43,7 @@ import google.registry.keyring.kms.KmsKeyring.PublicKeyLabel;
 import google.registry.keyring.kms.KmsKeyring.StringKeyLabel;
 import google.registry.model.server.KmsSecret;
 import google.registry.model.server.KmsSecretRevision;
+import google.registry.privileges.secretmanager.KeyringSecretStore;
 import java.io.IOException;
 import java.util.HashMap;
 import java.util.LinkedHashMap;
@@ -59,11 +60,14 @@ import org.bouncycastle.openpgp.PGPPublicKey;
 public final class KmsUpdater {
 
   private final KmsConnection kmsConnection;
+  private final KeyringSecretStore secretStore;
   private final HashMap<String, byte[]> secretValues;
 
   @Inject
-  public KmsUpdater(@Config("defaultKmsConnection") KmsConnection kmsConnection) {
+  public KmsUpdater(
+      @Config("defaultKmsConnection") KmsConnection kmsConnection, KeyringSecretStore secretStore) {
     this.kmsConnection = kmsConnection;
+    this.secretStore = secretStore;
 
     // Use LinkedHashMap to preserve insertion order on update() to simplify testing and debugging
     this.secretValues = new LinkedHashMap<>();
@@ -132,6 +136,19 @@ public final class KmsUpdater {
     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
+          .entrySet()
+          .forEach(e -> secretStore.createOrUpdateSecret(e.getKey(), e.getValue()));
+    } 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);
+    }
   }
 
   /**

core/src/main/java/google/registry/module/frontend/FrontendComponent.java

@@ -30,6 +30,7 @@ import google.registry.keyring.api.KeyModule;
 import google.registry.keyring.kms.KmsModule;
 import google.registry.module.frontend.FrontendRequestComponent.FrontendRequestComponentModule;
 import google.registry.monitoring.whitebox.StackdriverModule;
+import google.registry.privileges.secretmanager.SecretManagerModule;
 import google.registry.request.Modules.Jackson2Module;
 import google.registry.request.Modules.NetHttpTransportModule;
 import google.registry.request.Modules.UrlFetchTransportModule;
@@ -58,6 +59,7 @@ import javax.inject.Singleton;
       KeyringModule.class,
       KmsModule.class,
       NetHttpTransportModule.class,
+      SecretManagerModule.class,
       ServerTridProviderModule.class,
       StackdriverModule.class,
       UrlFetchTransportModule.class,

core/src/main/java/google/registry/module/pubapi/PubApiComponent.java

@@ -30,6 +30,7 @@ import google.registry.keyring.api.KeyModule;
 import google.registry.keyring.kms.KmsModule;
 import google.registry.module.pubapi.PubApiRequestComponent.PubApiRequestComponentModule;
 import google.registry.monitoring.whitebox.StackdriverModule;
+import google.registry.privileges.secretmanager.SecretManagerModule;
 import google.registry.request.Modules.Jackson2Module;
 import google.registry.request.Modules.NetHttpTransportModule;
 import google.registry.request.Modules.UrlFetchTransportModule;
@@ -56,6 +57,7 @@ import javax.inject.Singleton;
       KmsModule.class,
       NetHttpTransportModule.class,
       PubApiRequestComponentModule.class,
+      SecretManagerModule.class,
       ServerTridProviderModule.class,
       StackdriverModule.class,
       UrlFetchTransportModule.class,

core/src/main/java/google/registry/module/tools/ToolsComponent.java

@@ -32,6 +32,7 @@ import google.registry.keyring.api.KeyModule;
 import google.registry.keyring.kms.KmsModule;
 import google.registry.module.tools.ToolsRequestComponent.ToolsRequestComponentModule;
 import google.registry.monitoring.whitebox.StackdriverModule;
+import google.registry.privileges.secretmanager.SecretManagerModule;
 import google.registry.request.Modules.DatastoreServiceModule;
 import google.registry.request.Modules.Jackson2Module;
 import google.registry.request.Modules.NetHttpTransportModule;
@@ -61,6 +62,7 @@ import javax.inject.Singleton;
       KeyringModule.class,
       KmsModule.class,
       NetHttpTransportModule.class,
+      SecretManagerModule.class,
       ServerTridProviderModule.class,
       StackdriverModule.class,
       ToolsRequestComponentModule.class,

core/src/main/java/google/registry/privileges/secretmanager/KeyringSecretStore.java

@@ -0,0 +1,60 @@
+// 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.privileges.secretmanager;
+
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Strings.isNullOrEmpty;
+
+import java.nio.charset.StandardCharsets;
+import java.util.Optional;
+import javax.inject.Inject;
+
+/**
+ * Storage for 'keyring' secrets, backed by the Secret Manager.
+ *
+ * <p>This store is for secrets and credentials that must be set up manually and/or do not require
+ * non-disruptive password changes, e.g., passwords to regulatory reporting websites, which are used
+ * by cron jobs.
+ *
+ * <p>In contrast, the {@link SqlCredentialStore} is designed to support non-disruptive credential
+ * changes with Cloud SQL.
+ */
+public class KeyringSecretStore {
+  static final String SECRET_NAME_PREFIX = "keyring-";
+
+  private final SecretManagerClient csmClient;
+
+  @Inject
+  public KeyringSecretStore(SecretManagerClient csmClient) {
+    this.csmClient = csmClient;
+  }
+
+  public void createOrUpdateSecret(String label, byte[] data) {
+    String secretId = decorateLabel(label);
+    csmClient.createSecretIfAbsent(secretId);
+    csmClient.addSecretVersion(secretId, new String(data, StandardCharsets.UTF_8));
+  }
+
+  public byte[] getSecret(String label) {
+    return csmClient
+        .getSecretData(decorateLabel(label), Optional.empty())
+        .getBytes(StandardCharsets.UTF_8);
+  }
+
+  static String decorateLabel(String label) {
+    checkArgument(!isNullOrEmpty(label), "null or empty label");
+    return SECRET_NAME_PREFIX + label;
+  }
+}

core/src/main/java/google/registry/privileges/secretmanager/SecretManagerClient.java

@@ -44,6 +44,15 @@ public interface SecretManagerClient {
   /** Returns the {@link SecretVersionState} of all secrets with {@code secretId}. */
   Iterable<SecretVersionState> listSecretVersions(String secretId);
 
+  /** Creates a secret if it does not already exists. */
+  default void createSecretIfAbsent(String secretId) {
+    try {
+      createSecret(secretId);
+    } catch (SecretAlreadyExistsException ignore) {
+      // Not a problem.
+    }
+  }
+
   /**
    * Returns the version strings of all secrets in the given {@code state} with {@code secretId}.
    */

core/src/main/java/google/registry/privileges/secretmanager/SqlCredentialStore.java

@@ -17,7 +17,6 @@ package google.registry.privileges.secretmanager;
 import com.google.cloud.secretmanager.v1.SecretVersionName;
 import google.registry.config.RegistryConfig.Config;
 import google.registry.privileges.secretmanager.SecretManagerClient.NoSuchSecretResourceException;
-import google.registry.privileges.secretmanager.SecretManagerClient.SecretAlreadyExistsException;
 import java.util.Optional;
 import javax.inject.Inject;
 
@@ -74,17 +73,9 @@ public class SqlCredentialStore {
     }
   }
 
-  private void createSecretIfAbsent(String secretId) {
-    try {
-      csmClient.createSecret(secretId);
-    } catch (SecretAlreadyExistsException ignore) {
-      // Not a problem.
-    }
-  }
-
   private SecretVersionName saveCredentialData(SqlUser user, String password) {
     String credentialDataSecretId = getCredentialDataSecretId(user, dbInstance);
-    createSecretIfAbsent(credentialDataSecretId);
+    csmClient.createSecretIfAbsent(credentialDataSecretId);
     String credentialVersion =
         csmClient.addSecretVersion(
             credentialDataSecretId,
@@ -94,7 +85,7 @@ public class SqlCredentialStore {
 
   private void saveLiveLabel(SqlUser user, SecretVersionName dataVersionName) {
     String liveLabelSecretId = getLiveLabelSecretId(user, dbInstance);
-    createSecretIfAbsent(liveLabelSecretId);
+    csmClient.createSecretIfAbsent(liveLabelSecretId);
     csmClient.addSecretVersion(liveLabelSecretId, dataVersionName.toString());
   }
 

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

@@ -0,0 +1,78 @@
+// 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

@@ -104,6 +104,7 @@ 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

@@ -141,6 +141,8 @@ 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

@@ -20,6 +20,8 @@ import static google.registry.persistence.transaction.TransactionManagerFactory.
 import google.registry.keyring.api.KeySerializer;
 import google.registry.model.server.KmsSecret;
 import google.registry.model.server.KmsSecretRevision;
+import google.registry.privileges.secretmanager.FakeSecretManagerClient;
+import google.registry.privileges.secretmanager.KeyringSecretStore;
 import google.registry.testing.AppEngineExtension;
 import google.registry.testing.BouncyCastleProviderExtension;
 import google.registry.testing.DualDatabaseTest;
@@ -45,7 +47,9 @@ class KmsKeyringTest {
 
   @BeforeEach
   void beforeEach() {
-    keyring = new KmsKeyring(new FakeKmsConnection());
+    keyring =
+        new KmsKeyring(
+            new FakeKmsConnection(), new KeyringSecretStore(new FakeSecretManagerClient()));
   }
 
   @TestOfyAndSql

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

@@ -24,6 +24,8 @@ import google.registry.keyring.api.KeySerializer;
 import google.registry.model.server.KmsSecret;
 import google.registry.model.server.KmsSecretRevision;
 import google.registry.model.server.KmsSecretRevisionSqlDao;
+import google.registry.privileges.secretmanager.FakeSecretManagerClient;
+import google.registry.privileges.secretmanager.KeyringSecretStore;
 import google.registry.testing.AppEngineExtension;
 import google.registry.testing.BouncyCastleProviderExtension;
 import google.registry.testing.DualDatabaseTest;
@@ -49,7 +51,9 @@ public class KmsUpdaterTest {
 
   @BeforeEach
   void beforeEach() {
-    updater = new KmsUpdater(new FakeKmsConnection());
+    updater =
+        new KmsUpdater(
+            new FakeKmsConnection(), new KeyringSecretStore(new FakeSecretManagerClient()));
   }
 
   @TestOfyAndSql

core/src/test/java/google/registry/privileges/secretmanager/FakeSecretManagerClient.java

@@ -30,7 +30,7 @@ public class FakeSecretManagerClient implements SecretManagerClient {
   private final HashMap<String, SecretEntry> secrets = new HashMap<>();
 
   @Inject
-  FakeSecretManagerClient() {}
+  public FakeSecretManagerClient() {}
 
   @Override
   public String getProject() {

core/src/test/java/google/registry/privileges/secretmanager/KeyringSecretStoreTest.java

@@ -0,0 +1,53 @@
+// 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.privileges.secretmanager;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import java.nio.charset.StandardCharsets;
+import java.util.Optional;
+import org.junit.jupiter.api.Test;
+
+/** Unit tests for {@link KeyringSecretStore}. */
+public class KeyringSecretStoreTest {
+
+  private final SecretManagerClient csmClient = new FakeSecretManagerClient();
+  private final KeyringSecretStore secretStore = new KeyringSecretStore(csmClient);
+  private final byte[] data = {1, 2, 3, 0};
+
+  @Test
+  void createSecret() {
+    secretStore.createOrUpdateSecret("a", data);
+    byte[] persistedData = secretStore.getSecret("a");
+    assertThat(persistedData).isEqualTo(data);
+  }
+
+  @Test
+  void createSecret_underTheHood() {
+    secretStore.createOrUpdateSecret("a", data);
+    byte[] persistedData =
+        csmClient.getSecretData("keyring-a", Optional.empty()).getBytes(StandardCharsets.UTF_8);
+    assertThat(persistedData).isEqualTo(data);
+  }
+
+  @Test
+  void updateSecret() {
+    secretStore.createOrUpdateSecret("a", data);
+    byte[] newData = {0, 1, 2, 3};
+    secretStore.createOrUpdateSecret("a", newData);
+    byte[] persistedData = secretStore.getSecret("a");
+    assertThat(persistedData).isEqualTo(newData);
+  }
+}