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 24ea2a347..3b5c4c970 100644 --- a/core/src/main/java/google/registry/keyring/kms/KmsKeyring.java +++ b/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. + * + *

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 = 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(); + } } 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 fb7e2690f..942e75594 100644 --- a/core/src/main/java/google/registry/keyring/kms/KmsUpdater.java +++ b/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 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); + } } /** diff --git a/core/src/main/java/google/registry/module/frontend/FrontendComponent.java b/core/src/main/java/google/registry/module/frontend/FrontendComponent.java index 56e867886..54af5d0ad 100644 --- a/core/src/main/java/google/registry/module/frontend/FrontendComponent.java +++ b/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, diff --git a/core/src/main/java/google/registry/module/pubapi/PubApiComponent.java b/core/src/main/java/google/registry/module/pubapi/PubApiComponent.java index 93e2b0c1a..727d92b18 100644 --- a/core/src/main/java/google/registry/module/pubapi/PubApiComponent.java +++ b/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, diff --git a/core/src/main/java/google/registry/module/tools/ToolsComponent.java b/core/src/main/java/google/registry/module/tools/ToolsComponent.java index 83eb02163..bfa012ef0 100644 --- a/core/src/main/java/google/registry/module/tools/ToolsComponent.java +++ b/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, diff --git a/core/src/main/java/google/registry/privileges/secretmanager/KeyringSecretStore.java b/core/src/main/java/google/registry/privileges/secretmanager/KeyringSecretStore.java new file mode 100644 index 000000000..1054b3014 --- /dev/null +++ b/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. + * + *

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. + * + *

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; + } +} diff --git a/core/src/main/java/google/registry/privileges/secretmanager/SecretManagerClient.java b/core/src/main/java/google/registry/privileges/secretmanager/SecretManagerClient.java index fdf062f60..7b05ee45b 100644 --- a/core/src/main/java/google/registry/privileges/secretmanager/SecretManagerClient.java +++ b/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 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}. */ diff --git a/core/src/main/java/google/registry/privileges/secretmanager/SqlCredentialStore.java b/core/src/main/java/google/registry/privileges/secretmanager/SqlCredentialStore.java index 078fc91f7..d72ba2f7d 100644 --- a/core/src/main/java/google/registry/privileges/secretmanager/SqlCredentialStore.java +++ b/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()); } diff --git a/core/src/main/java/google/registry/tools/MigrateKmsKeyringCommand.java b/core/src/main/java/google/registry/tools/MigrateKmsKeyringCommand.java new file mode 100644 index 000000000..80891c4b5 --- /dev/null +++ b/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 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 1007f7275..b6803e5c5 100644 --- a/core/src/main/java/google/registry/tools/RegistryTool.java +++ b/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) diff --git a/core/src/main/java/google/registry/tools/RegistryToolComponent.java b/core/src/main/java/google/registry/tools/RegistryToolComponent.java index 429516ae3..bb895b3b7 100644 --- a/core/src/main/java/google/registry/tools/RegistryToolComponent.java +++ b/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); 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 cc2005643..718ceeb85 100644 --- a/core/src/test/java/google/registry/keyring/kms/KmsKeyringTest.java +++ b/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 diff --git a/core/src/test/java/google/registry/keyring/kms/KmsUpdaterTest.java b/core/src/test/java/google/registry/keyring/kms/KmsUpdaterTest.java index 760026cad..929b26708 100644 --- a/core/src/test/java/google/registry/keyring/kms/KmsUpdaterTest.java +++ b/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 diff --git a/core/src/test/java/google/registry/privileges/secretmanager/FakeSecretManagerClient.java b/core/src/test/java/google/registry/privileges/secretmanager/FakeSecretManagerClient.java index 46c56534c..6880236eb 100644 --- a/core/src/test/java/google/registry/privileges/secretmanager/FakeSecretManagerClient.java +++ b/core/src/test/java/google/registry/privileges/secretmanager/FakeSecretManagerClient.java @@ -30,7 +30,7 @@ public class FakeSecretManagerClient implements SecretManagerClient { private final HashMap secrets = new HashMap<>(); @Inject - FakeSecretManagerClient() {} + public FakeSecretManagerClient() {} @Override public String getProject() { diff --git a/core/src/test/java/google/registry/privileges/secretmanager/KeyringSecretStoreTest.java b/core/src/test/java/google/registry/privileges/secretmanager/KeyringSecretStoreTest.java new file mode 100644 index 000000000..099b93d13 --- /dev/null +++ b/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); + } +}