diff --git a/core/src/main/java/google/registry/keyring/kms/KmsKeyring.java b/core/src/main/java/google/registry/keyring/kms/KmsKeyring.java index cd615a104..2c7978b03 100644 --- a/core/src/main/java/google/registry/keyring/kms/KmsKeyring.java +++ b/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 Google Cloud Key Management Service * Documentation */ +// 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( - String.format("CloudKMS decrypt operation failed for secret %s", keyName), e); + return new byte[0]; } } @@ -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; - } - - /** - * Generates the tasks to migrate secrets from Datastore to Secret Manager. - * - *

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 migrationPlan() { - - ImmutableMap.Builder tasks = new ImmutableMap.Builder<>(); - - ImmutableList 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(); + return secretStoreData; } } diff --git a/core/src/main/java/google/registry/keyring/kms/KmsUpdater.java b/core/src/main/java/google/registry/keyring/kms/KmsUpdater.java index 942e75594..816a68e55 100644 --- a/core/src/main/java/google/registry/keyring/kms/KmsUpdater.java +++ b/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 - * encrypted secrets to Datastore. + * Persists the secrets in the Secret Manager (primary) and the Datastore (secondary). * - *

The operations in this method are organized so that existing {@link KmsSecretRevision} - * entities remain primary and decryptable if a failure occurs. + *

Updates to the Secret Manager are not transactional. If an error happens, the successful + * 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 - .entrySet() - .forEach(e -> secretStore.createOrUpdateSecret(e.getKey(), e.getValue())); + for (Map.Entry e : secretValues.entrySet()) { + 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)); } /** diff --git a/core/src/main/java/google/registry/tools/MigrateKmsKeyringCommand.java b/core/src/main/java/google/registry/tools/MigrateKmsKeyringCommand.java deleted file mode 100644 index 80891c4b5..000000000 --- a/core/src/main/java/google/registry/tools/MigrateKmsKeyringCommand.java +++ /dev/null @@ -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 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 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."; - } -} diff --git a/core/src/main/java/google/registry/tools/RegistryTool.java b/core/src/main/java/google/registry/tools/RegistryTool.java index 30b9bda34..c09aaa925 100644 --- a/core/src/main/java/google/registry/tools/RegistryTool.java +++ b/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) diff --git a/core/src/main/java/google/registry/tools/RegistryToolComponent.java b/core/src/main/java/google/registry/tools/RegistryToolComponent.java index c655ff120..06dcf9249 100644 --- a/core/src/main/java/google/registry/tools/RegistryToolComponent.java +++ b/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); diff --git a/core/src/test/java/google/registry/keyring/kms/KmsKeyringTest.java b/core/src/test/java/google/registry/keyring/kms/KmsKeyringTest.java index 718ceeb85..b00ba18fc 100644 --- a/core/src/test/java/google/registry/keyring/kms/KmsKeyringTest.java +++ b/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 = - new KmsKeyring( - new FakeKmsConnection(), new KeyringSecretStore(new FakeSecretManagerClient())); + keyring = new KmsKeyring(new FakeKmsConnection(), fakeSecretStore); } @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) - throws Exception { + private void saveKeyPairSecret(String publicKeyName, String privateKeyName) throws Exception { savePublicKeySecret(publicKeyName); savePrivateKeySecret(privateKeyName); }