Validate SQL credentials in Secret Manager (#907)

* Validate SQL credentials in Secret Manager

Load SQL credentials from the SecretManager and compare them with the
ones currently in use in Nomulus server, beam pipeline, and the registry
tool. Normal operations are not affected by failures related to the
SecretManager, be it IOException, insufficient permission , or wrong or
missing credential.

The appengine and compute engine default service accounts must be
granted the permission to access the secret data. In the short term, we
will grant the secretmanager.secretAccessor role to these accounts. In
the long term, with the proposed privilege service, access will be granted
on per-secret basis.

config/presubmits.py

@@ -65,7 +65,7 @@ class PresubmitCheck:
     for pattern in self.skipped_patterns:
       if pattern in file:
         return False
-    with open(file, "r") as f:
+    with open(file, "r", encoding='utf8') as f:
       file_content = f.read()
       matches = re.match(self.regex, file_content, re.DOTALL)
       if self.regex_type == FORBIDDEN:
@@ -241,7 +241,7 @@ def verify_flyway_index():
 
   # Remove the sequence numbers and compare against the index file contents.
   files = [filename[1] for filename in sorted(files)]
-  with open('db/src/main/resources/sql/flyway.txt') as index:
+  with open('db/src/main/resources/sql/flyway.txt', encoding='utf8') as index:
     indexed_files = index.read().splitlines()
   if files != indexed_files:
     unindexed = set(files) - set(indexed_files)

core/src/main/java/google/registry/beam/initsql/BeamJpaModule.java

@@ -31,6 +31,7 @@ import google.registry.persistence.PersistenceModule;
 import google.registry.persistence.PersistenceModule.JdbcJpaTm;
 import google.registry.persistence.PersistenceModule.SocketFactoryJpaTm;
 import google.registry.persistence.transaction.JpaTransactionManager;
+import google.registry.privileges.secretmanager.SecretManagerModule;
 import google.registry.util.UtilsModule;
 import java.io.BufferedReader;
 import java.io.IOException;
@@ -168,6 +169,7 @@ public class BeamJpaModule {
         BeamJpaModule.class,
         KmsModule.class,
         PersistenceModule.class,
+        SecretManagerModule.class,
         UtilsModule.class
       })
   public interface JpaTransactionManagerComponent {

core/src/main/java/google/registry/model/DatabaseMigrationUtils.java

@@ -0,0 +1,38 @@
+// Copyright 2020 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.model;
+
+import com.google.common.flogger.FluentLogger;
+import google.registry.config.RegistryEnvironment;
+
+/** Utility methods related to migrating dual-read/dual-write entities. */
+public class DatabaseMigrationUtils {
+
+  private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+  /** Throws exceptions only in unit tests, otherwise only logs exceptions. */
+  public static void suppressExceptionUnlessInTest(Runnable work, String message) {
+    try {
+      work.run();
+    } catch (Exception e) {
+      if (RegistryEnvironment.get().equals(RegistryEnvironment.UNITTEST)) {
+        throw e;
+      }
+      logger.atWarning().withCause(e).log(message);
+    }
+  }
+
+  private DatabaseMigrationUtils() {}
+}

core/src/main/java/google/registry/model/smd/SignedMarkRevocationList.java

@@ -19,6 +19,7 @@ import static com.google.common.base.Preconditions.checkState;
 import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.common.collect.Iterables.isEmpty;
 import static google.registry.model.CacheUtils.memoizeWithShortExpiration;
+import static google.registry.model.DatabaseMigrationUtils.suppressExceptionUnlessInTest;
 import static google.registry.model.common.EntityGroupRoot.getCrossTldKey;
 import static google.registry.model.ofy.ObjectifyService.allocateId;
 import static google.registry.model.ofy.ObjectifyService.ofy;
@@ -32,7 +33,6 @@ import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.MapDifference;
 import com.google.common.collect.Maps;
-import com.google.common.flogger.FluentLogger;
 import com.googlecode.objectify.Key;
 import com.googlecode.objectify.annotation.EmbedMap;
 import com.googlecode.objectify.annotation.Entity;
@@ -82,8 +82,6 @@ import org.joda.time.DateTime;
 @NotBackedUp(reason = Reason.EXTERNALLY_SOURCED)
 public class SignedMarkRevocationList extends ImmutableObject implements NonReplicatedEntity {
 
-  private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
   @VisibleForTesting static final int SHARD_SIZE = 10000;
 
   /** Common ancestor for queries. */
@@ -121,12 +119,11 @@ public class SignedMarkRevocationList extends ImmutableObject implements NonRepl
       memoizeWithShortExpiration(
           () -> {
             SignedMarkRevocationList datastoreList = loadFromDatastore();
-            // Also load the list from Cloud SQL, compare the two lists, and log if different.
-            try {
-              loadAndCompareCloudSqlList(datastoreList);
-            } catch (Throwable t) {
-              logger.atSevere().withCause(t).log("Error comparing signed mark revocation lists.");
-            }
+            suppressExceptionUnlessInTest(
+                () -> {
+                  loadAndCompareCloudSqlList(datastoreList);
+                },
+                "Error comparing signed mark revocation lists.");
             return datastoreList;
           });
 
@@ -229,11 +226,12 @@ public class SignedMarkRevocationList extends ImmutableObject implements NonRepl
           Maps.difference(datastoreList.revokes, cloudSqlList.revokes);
       if (!diff.areEqual()) {
         if (diff.entriesDiffering().size() > 10) {
-          logger.atWarning().log(
+          String message =
               String.format(
                   "Unequal SM revocation lists detected, Cloud SQL list with revision id %d has %d"
                       + " different records than the current Datastore list.",
-                  cloudSqlList.revisionId, diff.entriesDiffering().size()));
+                  cloudSqlList.revisionId, diff.entriesDiffering().size());
+          throw new RuntimeException(message);
         } else {
           StringBuilder diffMessage = new StringBuilder("Unequal SM revocation lists detected:\n");
           diff.entriesDiffering()
@@ -243,11 +241,13 @@ public class SignedMarkRevocationList extends ImmutableObject implements NonRepl
                           String.format(
                               "SMD %s has key %s in Datastore and key %s in Cloud SQL.\n",
                               label, valueDiff.leftValue(), valueDiff.rightValue())));
-          logger.atWarning().log(diffMessage.toString());
+          throw new RuntimeException(diffMessage.toString());
         }
       }
     } else {
-      logger.atWarning().log("Signed mark revocation list in Cloud SQL is empty.");
+      if (datastoreList.size() != 0) {
+        throw new RuntimeException("Signed mark revocation list in Cloud SQL is empty.");
+      }
     }
   }
 

core/src/main/java/google/registry/model/smd/SignedMarkRevocationListDao.java

@@ -15,6 +15,7 @@
 package google.registry.model.smd;
 
 import static google.registry.model.CacheUtils.memoizeWithShortExpiration;
+import static google.registry.model.DatabaseMigrationUtils.suppressExceptionUnlessInTest;
 import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
 
 import com.google.common.base.Supplier;
@@ -60,14 +61,14 @@ public class SignedMarkRevocationListDao {
    * the authoritative database.
    */
   static void trySave(SignedMarkRevocationList signedMarkRevocationList) {
-    try {
-      SignedMarkRevocationListDao.save(signedMarkRevocationList);
-      logger.atInfo().log(
-          "Inserted %,d signed mark revocations into Cloud SQL",
-          signedMarkRevocationList.revokes.size());
-    } catch (Throwable e) {
-      logger.atSevere().withCause(e).log("Error inserting signed mark revocations into Cloud SQL");
-    }
+    suppressExceptionUnlessInTest(
+        () -> {
+          SignedMarkRevocationListDao.save(signedMarkRevocationList);
+          logger.atInfo().log(
+              "Inserted %,d signed mark revocations into Cloud SQL.",
+              signedMarkRevocationList.revokes.size());
+        },
+        "Error inserting signed mark revocations into Cloud SQL.");
   }
 
   private static void save(SignedMarkRevocationList signedMarkRevocationList) {

core/src/main/java/google/registry/persistence/PersistenceComponent.java

@@ -20,6 +20,7 @@ import google.registry.config.RegistryConfig.ConfigModule;
 import google.registry.keyring.kms.KmsModule;
 import google.registry.persistence.PersistenceModule.AppEngineJpaTm;
 import google.registry.persistence.transaction.JpaTransactionManager;
+import google.registry.privileges.secretmanager.SecretManagerModule;
 import google.registry.util.UtilsModule;
 import javax.inject.Singleton;
 import javax.persistence.EntityManagerFactory;
@@ -32,6 +33,7 @@ import javax.persistence.EntityManagerFactory;
       CredentialModule.class,
       KmsModule.class,
       PersistenceModule.class,
+      SecretManagerModule.class,
       UtilsModule.class
     })
 public interface PersistenceComponent {

core/src/main/java/google/registry/persistence/PersistenceModule.java

@@ -26,6 +26,7 @@ import com.google.api.client.auth.oauth2.Credential;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Maps;
+import com.google.common.flogger.FluentLogger;
 import dagger.Module;
 import dagger.Provides;
 import google.registry.config.RegistryConfig.Config;
@@ -33,6 +34,11 @@ import google.registry.keyring.kms.KmsKeyring;
 import google.registry.persistence.transaction.CloudSqlCredentialSupplier;
 import google.registry.persistence.transaction.JpaTransactionManager;
 import google.registry.persistence.transaction.JpaTransactionManagerImpl;
+import google.registry.privileges.secretmanager.SqlCredential;
+import google.registry.privileges.secretmanager.SqlCredentialStore;
+import google.registry.privileges.secretmanager.SqlUser;
+import google.registry.privileges.secretmanager.SqlUser.RobotId;
+import google.registry.privileges.secretmanager.SqlUser.RobotUser;
 import google.registry.tools.AuthModule.CloudSqlClientCredential;
 import google.registry.util.Clock;
 import java.lang.annotation.Documented;
@@ -47,6 +53,8 @@ import org.hibernate.cfg.Environment;
 /** Dagger module class for the persistence layer. */
 @Module
 public class PersistenceModule {
+  private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
   // This name must be the same as the one defined in persistence.xml.
   public static final String PERSISTENCE_UNIT_NAME = "nomulus";
   public static final String HIKARI_CONNECTION_TIMEOUT = "hibernate.hikari.connectionTimeout";
@@ -122,11 +130,17 @@ public class PersistenceModule {
   static JpaTransactionManager provideAppEngineJpaTm(
       @Config("cloudSqlUsername") String username,
       KmsKeyring kmsKeyring,
+      SqlCredentialStore credentialStore,
       @PartialCloudSqlConfigs ImmutableMap<String, String> cloudSqlConfigs,
       Clock clock) {
     HashMap<String, String> overrides = Maps.newHashMap(cloudSqlConfigs);
     overrides.put(Environment.USER, username);
     overrides.put(Environment.PASS, kmsKeyring.getCloudSqlPassword());
+    validateCredentialStore(
+        credentialStore,
+        new RobotUser(RobotId.NOMULUS),
+        overrides.get(Environment.USER),
+        overrides.get(Environment.PASS));
     return new JpaTransactionManagerImpl(create(overrides), clock);
   }
 
@@ -136,6 +150,7 @@ public class PersistenceModule {
   static JpaTransactionManager provideNomulusToolJpaTm(
       @Config("toolsCloudSqlUsername") String username,
       KmsKeyring kmsKeyring,
+      SqlCredentialStore credentialStore,
       @PartialCloudSqlConfigs ImmutableMap<String, String> cloudSqlConfigs,
       @CloudSqlClientCredential Credential credential,
       Clock clock) {
@@ -143,6 +158,11 @@ public class PersistenceModule {
     HashMap<String, String> overrides = Maps.newHashMap(cloudSqlConfigs);
     overrides.put(Environment.USER, username);
     overrides.put(Environment.PASS, kmsKeyring.getToolsCloudSqlPassword());
+    validateCredentialStore(
+        credentialStore,
+        new RobotUser(RobotId.TOOL),
+        overrides.get(Environment.USER),
+        overrides.get(Environment.PASS));
     return new JpaTransactionManagerImpl(create(overrides), clock);
   }
 
@@ -150,6 +170,7 @@ public class PersistenceModule {
   @Singleton
   @SocketFactoryJpaTm
   static JpaTransactionManager provideSocketFactoryJpaTm(
+      SqlCredentialStore credentialStore,
       @Config("beamCloudSqlUsername") String username,
       @Config("beamCloudSqlPassword") String password,
       @Config("beamHibernateHikariMaximumPoolSize") int hikariMaximumPoolSize,
@@ -159,6 +180,12 @@ public class PersistenceModule {
     overrides.put(Environment.USER, username);
     overrides.put(Environment.PASS, password);
     overrides.put(HIKARI_MAXIMUM_POOL_SIZE, String.valueOf(hikariMaximumPoolSize));
+    // TODO(b/175700623): consider assigning different logins to pipelines
+    validateCredentialStore(
+        credentialStore,
+        new RobotUser(RobotId.NOMULUS),
+        overrides.get(Environment.USER),
+        overrides.get(Environment.PASS));
     return new JpaTransactionManagerImpl(create(overrides), clock);
   }
 
@@ -203,6 +230,30 @@ public class PersistenceModule {
     return emf;
   }
 
+  /** Verifies that the credential from the Secret Manager matches the one currently in use.
+   *
+   * <p>This is a helper for the transition to the Secret Manager, and will be removed once data
+   * and permissions are properly set up for all projects.
+   **/
+  private static void validateCredentialStore(
+      SqlCredentialStore credentialStore, SqlUser sqlUser, String login, String password) {
+    try {
+      SqlCredential credential = credentialStore.getCredential(sqlUser);
+      if (!credential.login().equals(login)) {
+        logger.atWarning().log(
+            "Wrong login for %s. Expecting %s, found %s.",
+            sqlUser.geUserName(), login, credential.login());
+        return;
+      }
+      if (!credential.password().equals(password)) {
+        logger.atWarning().log("Wrong password for %s.", sqlUser.geUserName());
+      }
+      logger.atWarning().log("Credentials in the kerying and the secret manager match.");
+    } catch (Throwable e) {
+      logger.atWarning().log(e.getMessage());
+    }
+  }
+
   /** Dagger qualifier for {@link JpaTransactionManager} used for App Engine application. */
   @Qualifier
   @Documented

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

@@ -16,13 +16,13 @@ package google.registry.privileges.secretmanager;
 
 
 import com.google.cloud.secretmanager.v1.SecretManagerServiceClient;
-import dagger.Component;
+import com.google.cloud.secretmanager.v1.SecretManagerServiceSettings;
 import dagger.Module;
 import dagger.Provides;
+import google.registry.config.CredentialModule.DefaultCredential;
 import google.registry.config.RegistryConfig.Config;
-import google.registry.config.RegistryConfig.ConfigModule;
+import google.registry.util.GoogleCredentialsBundle;
 import google.registry.util.Retrier;
-import google.registry.util.UtilsModule;
 import java.io.IOException;
 import javax.inject.Singleton;
 
@@ -32,20 +32,29 @@ public abstract class SecretManagerModule {
 
   @Provides
   @Singleton
-  static SecretManagerClient provideSecretManagerClient(
-      @Config("projectId") String project, Retrier retrier) {
+  static SecretManagerServiceSettings provideSecretManagerSetting(
+      @DefaultCredential GoogleCredentialsBundle credentialsBundle) {
     try {
-      SecretManagerServiceClient stub = SecretManagerServiceClient.create();
+      return SecretManagerServiceSettings.newBuilder()
+          .setCredentialsProvider(() -> credentialsBundle.getGoogleCredentials())
+          .build();
+    } catch (IOException e) {
+      throw new RuntimeException(e);
+    }
+  }
+
+  @Provides
+  @Singleton
+  static SecretManagerClient provideSecretManagerClient(
+      SecretManagerServiceSettings serviceSettings,
+      @Config("projectId") String project,
+      Retrier retrier) {
+    try {
+      SecretManagerServiceClient stub = SecretManagerServiceClient.create(serviceSettings);
       Runtime.getRuntime().addShutdownHook(new Thread(stub::close));
       return new SecretManagerClientImpl(project, stub, retrier);
     } catch (IOException e) {
       throw new RuntimeException(e);
     }
   }
-
-  @Singleton
-  @Component(modules = {ConfigModule.class, SecretManagerModule.class, UtilsModule.class})
-  public interface SecretManagerComponent {
-    SecretManagerClient secretManagerClient();
-  }
 }

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

@@ -48,7 +48,12 @@ public abstract class SqlUser {
 
   /** Enumerates the {@link RobotUser RobotUsers} in the system. */
   public enum RobotId {
-    NOMULUS;
+    NOMULUS,
+    /**
+     * Credential for RegistryTool. This is temporary, and will be removed when tool users are
+     * assigned their personal credentials.
+     */
+    TOOL;
   }
 
   /** Information of a RobotUser for privilege management purposes. */

core/src/test/java/google/registry/model/smd/SignedMarkRevocationListDaoTest.java

@@ -50,21 +50,6 @@ public class SignedMarkRevocationListDaoTest {
     assertAboutImmutableObjects().that(fromDb).isEqualExceptFields(list);
   }
 
-  @Test
-  void trySave_failureIsSwallowed() {
-    SignedMarkRevocationList list =
-        SignedMarkRevocationList.create(
-            fakeClock.nowUtc(), ImmutableMap.of("mark", fakeClock.nowUtc().minusHours(1)));
-    SignedMarkRevocationListDao.trySave(list);
-    SignedMarkRevocationList fromDb = SignedMarkRevocationListDao.getLatestRevision().get();
-    assertAboutImmutableObjects().that(fromDb).isEqualExceptFields(list);
-
-    // This should throw an exception, which is swallowed and nothing changed
-    SignedMarkRevocationListDao.trySave(list);
-    SignedMarkRevocationList secondFromDb = SignedMarkRevocationListDao.getLatestRevision().get();
-    assertAboutImmutableObjects().that(secondFromDb).isEqualExceptFields(fromDb);
-  }
-
   @Test
   void testRetrieval_notPresent() {
     assertThat(SignedMarkRevocationListDao.getLatestRevision().isPresent()).isFalse();

core/src/test/java/google/registry/model/smd/SignedMarkRevocationListTest.java

@@ -18,6 +18,7 @@ import static com.google.common.truth.Truth.assertThat;
 import static google.registry.model.ImmutableObjectSubject.assertAboutImmutableObjects;
 import static google.registry.model.ofy.ObjectifyService.ofy;
 import static google.registry.model.smd.SignedMarkRevocationList.SHARD_SIZE;
+import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
 import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
 import static google.registry.util.DateTimeUtils.START_OF_TIME;
 import static org.joda.time.Duration.standardDays;
@@ -138,6 +139,33 @@ public class SignedMarkRevocationListTest {
         .isEqualTo(DateTime.parse("2000-01-01T00:00:00Z"));
   }
 
+  @Test
+  void test_getCreationTime_missingInCloudSQL() {
+    clock.setTo(DateTime.parse("2000-01-01T00:00:00Z"));
+    createSaveGetHelper(1);
+    jpaTm().transact(() -> jpaTm().delete(SignedMarkRevocationListDao.getLatestRevision().get()));
+    RuntimeException thrown =
+        assertThrows(RuntimeException.class, () -> SignedMarkRevocationList.get());
+    assertThat(thrown)
+        .hasMessageThat()
+        .isEqualTo("Signed mark revocation list in Cloud SQL is empty.");
+  }
+
+  @Test
+  void test_getCreationTime_unequalListsInDatabases() {
+    clock.setTo(DateTime.parse("2000-01-01T00:00:00Z"));
+    createSaveGetHelper(1);
+    ImmutableMap.Builder<String, DateTime> revokes = new ImmutableMap.Builder<>();
+    for (int i = 0; i < 3; i++) {
+      revokes.put(Integer.toString(i), clock.nowUtc());
+    }
+    SignedMarkRevocationListDao.trySave(
+        SignedMarkRevocationList.create(clock.nowUtc(), revokes.build()));
+    RuntimeException thrown =
+        assertThrows(RuntimeException.class, () -> SignedMarkRevocationList.get());
+    assertThat(thrown).hasMessageThat().contains("Unequal SM revocation lists detected:");
+  }
+
   @Test
   void test_isSmdRevoked_present() {
     final int rows = SHARD_SIZE + 1;

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

@@ -17,6 +17,8 @@ package google.registry.privileges.secretmanager;
 import static com.google.common.truth.Truth.assertThat;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 
+import com.google.auth.oauth2.GoogleCredentials;
+import com.google.cloud.secretmanager.v1.SecretManagerServiceSettings;
 import com.google.cloud.secretmanager.v1.SecretVersion.State;
 import google.registry.privileges.secretmanager.SecretManagerClient.NoSuchSecretResourceException;
 import google.registry.privileges.secretmanager.SecretManagerClient.SecretAlreadyExistsException;
@@ -54,11 +56,14 @@ public class SecretManagerClientTest {
   private String secretId;
 
   @BeforeAll
-  static void beforeAll() {
+  static void beforeAll() throws IOException {
     String environmentName = System.getProperty("test.gcp_integration.env");
     if (environmentName != null) {
       secretManagerClient =
           SecretManagerModule.provideSecretManagerClient(
+              SecretManagerServiceSettings.newBuilder()
+                  .setCredentialsProvider(() -> GoogleCredentials.getApplicationDefault())
+                  .build(),
               String.format("domain-registry-%s", environmentName),
               new Retrier(new SystemSleeper(), 1));
       isUnitTest = false;

java_common.gradle

@@ -74,10 +74,12 @@ test {
   useJUnitPlatform()
 }
 
-// Sets up integration test with a registry environment. The target environment is
-// passed by the 'test.gcp_integration.env' property. Test runner must have been
-// authorized to access the corresponding GCP project, e.g., by running 'gcloud auth'
-// or placing a credential file at a well known place.
+// Sets up integration test with a registry environment. The target environment
+// is passed by the 'test.gcp_integration.env' property. Test runner must have
+// been authorized to access the corresponding GCP project, e.g., by running
+// 'gcloud auth application-default login' or by downloading a credential file
+// and assign the path to it to the GOOGLE_APPLICATION_CREDENTIALS environment
+// variable.
 //
 // A typical use case is to run tests from desktop that accesses Cloud resources. See
 // core/src/test/java/google/registry/beam/initsql/BeamJpaModuleTest.java for an example.