Avoid putting secret in the exported JSON object (#11296)

The OSS deployment allows to do an export and import of the workspace configuration. If the users is not using a secret manager in their deployment, this will return API key and password in plain text. This is a serious security issue, especially since it has been presented by @timroes that some instance are exposed on the public internet. In order to avoid returning password or other values that should be in a secret we need to sanitize the export. This is what this PR is doing.
airbytehq · Mar 23, 2022 · f2cb12c · f2cb12c
1 parent 721cb37
commit f2cb12c
Show file tree

Hide file tree

Showing 15 changed files with 180 additions and 27 deletions.
diff --git a/.env b/.env
@@ -92,3 +92,4 @@ MAX_DISCOVER_WORKERS=5
 ### FEATURE FLAGS ###
 NEW_SCHEDULER=false
 AUTO_DISABLE_FAILING_CONNECTIONS=false
+EXPOSE_SECRETS_IN_EXPORT=false
diff --git a/airbyte-bootloader/src/main/java/io/airbyte/bootloader/BootloaderApp.java b/airbyte-bootloader/src/main/java/io/airbyte/bootloader/BootloaderApp.java
@@ -16,6 +16,7 @@
 import io.airbyte.config.persistence.ConfigPersistence;
 import io.airbyte.config.persistence.ConfigRepository;
 import io.airbyte.config.persistence.DatabaseConfigPersistence;
+import io.airbyte.config.persistence.split_secrets.JsonSecretsProcessor;
 import io.airbyte.db.Database;
 import io.airbyte.db.instance.DatabaseMigrator;
 import io.airbyte.db.instance.configs.ConfigsDatabaseInstance;
@@ -79,19 +80,21 @@ public BootloaderApp(final Configs configs, final Runnable postLoadExecution, fi
 
   public BootloaderApp() {
     configs = new EnvConfigs();
+    featureFlags = new EnvVariableFeatureFlags();
     postLoadExecution = () -> {
       try {
         final Database configDatabase =
             new ConfigsDatabaseInstance(configs.getConfigDatabaseUser(), configs.getConfigDatabasePassword(), configs.getConfigDatabaseUrl())
                 .getAndInitialize();
-        final ConfigPersistence configPersistence = DatabaseConfigPersistence.createWithValidation(configDatabase);
+        final JsonSecretsProcessor jsonSecretsProcessor = new JsonSecretsProcessor();
+        final ConfigPersistence configPersistence =
+            DatabaseConfigPersistence.createWithValidation(configDatabase, jsonSecretsProcessor, featureFlags);
         configPersistence.loadData(YamlSeedConfigPersistence.getDefault());
         LOGGER.info("Loaded seed data..");
       } catch (final IOException e) {
         e.printStackTrace();
       }
     };
-    featureFlags = new EnvVariableFeatureFlags();
   }
 
   public void load() throws Exception {
@@ -112,7 +115,8 @@ public void load() throws Exception {
       runFlywayMigration(configs, configDatabase, jobDatabase);
       LOGGER.info("Ran Flyway migrations...");
 
-      final ConfigPersistence configPersistence = DatabaseConfigPersistence.createWithValidation(configDatabase);
+      final JsonSecretsProcessor jsonSecretsProcessor = new JsonSecretsProcessor();
+      final ConfigPersistence configPersistence = DatabaseConfigPersistence.createWithValidation(configDatabase, jsonSecretsProcessor, featureFlags);
       final ConfigRepository configRepository =
           new ConfigRepository(configPersistence, configDatabase);
 

diff --git a/airbyte-commons/src/main/java/io/airbyte/commons/features/EnvVariableFeatureFlags.java b/airbyte-commons/src/main/java/io/airbyte/commons/features/EnvVariableFeatureFlags.java
@@ -23,4 +23,9 @@ public boolean autoDisablesFailingConnections() {
     return Boolean.parseBoolean(System.getenv("AUTO_DISABLE_FAILING_CONNECTIONS"));
   }
 
+  @Override
+  public boolean exposeSecretsInExport() {
+    return Boolean.parseBoolean(System.getenv("EXPOSE_SECRETS_IN_EXPORT"));
+  }
+
 }
diff --git a/airbyte-commons/src/main/java/io/airbyte/commons/features/FeatureFlags.java b/airbyte-commons/src/main/java/io/airbyte/commons/features/FeatureFlags.java
@@ -14,4 +14,6 @@ public interface FeatureFlags {
 
   boolean autoDisablesFailingConnections();
 
+  boolean exposeSecretsInExport();
+
 }
diff --git a/...ig/persistence/src/main/java/io/airbyte/config/persistence/DatabaseConfigPersistence.java b/...ig/persistence/src/main/java/io/airbyte/config/persistence/DatabaseConfigPersistence.java
@@ -22,6 +22,7 @@
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.Sets;
 import io.airbyte.commons.enums.Enums;
+import io.airbyte.commons.features.FeatureFlags;
 import io.airbyte.commons.json.Jsons;
 import io.airbyte.commons.util.MoreIterators;
 import io.airbyte.commons.version.AirbyteVersion;
@@ -46,6 +47,7 @@
 import io.airbyte.config.StandardSyncState;
 import io.airbyte.config.StandardWorkspace;
 import io.airbyte.config.State;
+import io.airbyte.config.persistence.split_secrets.JsonSecretsProcessor;
 import io.airbyte.db.Database;
 import io.airbyte.db.ExceptionWrappingDatabase;
 import io.airbyte.db.instance.configs.jooq.enums.ActorType;
@@ -80,14 +82,20 @@
 public class DatabaseConfigPersistence implements ConfigPersistence {
 
   private final ExceptionWrappingDatabase database;
+  private final JsonSecretsProcessor jsonSecretsProcessor;
+  private final FeatureFlags featureFlags;
   private static final Logger LOGGER = LoggerFactory.getLogger(DatabaseConfigPersistence.class);
 
-  public static ConfigPersistence createWithValidation(final Database database) {
-    return new ValidatingConfigPersistence(new DatabaseConfigPersistence(database));
+  public static ConfigPersistence createWithValidation(final Database database,
+                                                       final JsonSecretsProcessor jsonSecretsProcessor,
+                                                       final FeatureFlags featureFlags) {
+    return new ValidatingConfigPersistence(new DatabaseConfigPersistence(database, jsonSecretsProcessor, featureFlags));
   }
 
-  public DatabaseConfigPersistence(final Database database) {
+  public DatabaseConfigPersistence(final Database database, final JsonSecretsProcessor jsonSecretsProcessor, final FeatureFlags featureFlags) {
     this.database = new ExceptionWrappingDatabase(database);
+    this.jsonSecretsProcessor = jsonSecretsProcessor;
+    this.featureFlags = featureFlags;
   }
 
   @Override
@@ -1592,16 +1600,49 @@ public Map<String, Stream<JsonNode>> dumpConfigs() throws IOException {
       result.put(ConfigSchema.SOURCE_CONNECTION.name(),
           sourceConnectionWithMetadata
               .stream()
-              .map(ConfigWithMetadata::getConfig)
-              .map(Jsons::jsonNode));
+              .map(configWithMetadata -> {
+                if (featureFlags.exposeSecretsInExport()) {
+                  return Jsons.jsonNode(configWithMetadata.getConfig());
+                }
+
+                try {
+                  final UUID sourceDefinitionId = configWithMetadata.getConfig().getSourceDefinitionId();
+                  final StandardSourceDefinition standardSourceDefinition = getConfig(
+                      ConfigSchema.STANDARD_SOURCE_DEFINITION,
+                      sourceDefinitionId.toString(),
+                      StandardSourceDefinition.class);
+                  final JsonNode connectionSpecs = standardSourceDefinition.getSpec().getConnectionSpecification();
+                  final JsonNode sanitizedConfig = jsonSecretsProcessor.maskSecrets(Jsons.jsonNode(configWithMetadata.getConfig()), connectionSpecs);
+                  return sanitizedConfig;
+                } catch (final ConfigNotFoundException | JsonValidationException | IOException e) {
+                  throw new RuntimeException(e);
+                }
+              }));
     }
     final List<ConfigWithMetadata<DestinationConnection>> destinationConnectionWithMetadata = listDestinationConnectionWithMetadata();
     if (!destinationConnectionWithMetadata.isEmpty()) {
-      result.put(ConfigSchema.DESTINATION_CONNECTION.name(),
-          destinationConnectionWithMetadata
-              .stream()
-              .map(ConfigWithMetadata::getConfig)
-              .map(Jsons::jsonNode));
+      final Stream<JsonNode> jsonNodeStream = destinationConnectionWithMetadata
+          .stream()
+          .map(configWithMetadata -> {
+            if (featureFlags.exposeSecretsInExport()) {
+              return Jsons.jsonNode(configWithMetadata.getConfig());
+            }
+
+            try {
+              final UUID destinationDefinition = configWithMetadata.getConfig().getDestinationDefinitionId();
+              final StandardDestinationDefinition standardDestinationDefinition = getConfig(
+                  ConfigSchema.STANDARD_DESTINATION_DEFINITION,
+                  destinationDefinition.toString(),
+                  StandardDestinationDefinition.class);
+              final JsonNode connectionSpec = standardDestinationDefinition.getSpec().getConnectionSpecification();
+              final JsonNode sanitizedConfig = jsonSecretsProcessor.maskSecrets(Jsons.jsonNode(configWithMetadata.getConfig()), connectionSpec);
+              return sanitizedConfig;
+            } catch (final ConfigNotFoundException | JsonValidationException | IOException e) {
+              throw new RuntimeException(e);
+            }
+          });
+      result.put(ConfigSchema.DESTINATION_CONNECTION.name(), jsonNodeStream);
+
     }
     final List<ConfigWithMetadata<SourceOAuthParameter>> sourceOauthParamWithMetadata = listSourceOauthParamWithMetadata();
     if (!sourceOauthParamWithMetadata.isEmpty()) {

diff --git a/...stence/src/test/java/io/airbyte/config/persistence/BaseDatabaseConfigPersistenceTest.java b/...stence/src/test/java/io/airbyte/config/persistence/BaseDatabaseConfigPersistenceTest.java
@@ -6,14 +6,19 @@
 
 import static org.jooq.impl.DSL.asterisk;
 import static org.jooq.impl.DSL.count;
-import static org.jooq.impl.DSL.table;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.Mockito.mock;
 
 import com.fasterxml.jackson.databind.JsonNode;
+import io.airbyte.commons.features.FeatureFlags;
 import io.airbyte.config.ConfigSchema;
+import io.airbyte.config.DestinationConnection;
+import io.airbyte.config.SourceConnection;
 import io.airbyte.config.StandardDestinationDefinition;
 import io.airbyte.config.StandardSourceDefinition;
 import io.airbyte.config.StandardSourceDefinition.SourceType;
+import io.airbyte.config.StandardWorkspace;
+import io.airbyte.config.persistence.split_secrets.JsonSecretsProcessor;
 import io.airbyte.db.Database;
 import java.sql.SQLException;
 import java.util.List;
@@ -39,6 +44,8 @@ public abstract class BaseDatabaseConfigPersistenceTest {
   protected static PostgreSQLContainer<?> container;
   protected static Database database;
   protected static DatabaseConfigPersistence configPersistence;
+  protected static JsonSecretsProcessor jsonSecretsProcessor;
+  protected static FeatureFlags featureFlags;
 
   @BeforeAll
   public static void dbSetup() {
@@ -47,6 +54,8 @@ public static void dbSetup() {
         .withUsername("docker")
         .withPassword("docker");
     container.start();
+    jsonSecretsProcessor = mock(JsonSecretsProcessor.class);
+    featureFlags = mock(FeatureFlags.class);
   }
 
   @AfterAll
@@ -97,11 +106,52 @@ protected static void writeSource(final ConfigPersistence configPersistence, fin
     configPersistence.writeConfig(ConfigSchema.STANDARD_SOURCE_DEFINITION, source.getSourceDefinitionId().toString(), source);
   }
 
+  protected static void writeSourceWithSourceConnection(final ConfigPersistence configPersistence, final StandardSourceDefinition source)
+      throws Exception {
+    configPersistence.writeConfig(ConfigSchema.STANDARD_SOURCE_DEFINITION, source.getSourceDefinitionId().toString(), source);
+    final UUID connectionId = UUID.randomUUID();
+    final UUID workspaceId = UUID.randomUUID();
+    final StandardWorkspace workspace = new StandardWorkspace()
+        .withWorkspaceId(workspaceId)
+        .withName("can not be null")
+        .withSlug("can not be null")
+        .withInitialSetupComplete(true);
+    configPersistence.writeConfig(ConfigSchema.STANDARD_WORKSPACE, workspaceId.toString(), workspace);
+
+    final SourceConnection sourceConnection = new SourceConnection()
+        .withSourceId(connectionId)
+        .withWorkspaceId(workspaceId)
+        .withName("can not be null")
+        .withSourceDefinitionId(source.getSourceDefinitionId());
+    configPersistence.writeConfig(ConfigSchema.SOURCE_CONNECTION, connectionId.toString(), sourceConnection);
+  }
+
   protected static void writeDestination(final ConfigPersistence configPersistence, final StandardDestinationDefinition destination)
       throws Exception {
     configPersistence.writeConfig(ConfigSchema.STANDARD_DESTINATION_DEFINITION, destination.getDestinationDefinitionId().toString(), destination);
   }
 
+  protected static void writeDestinationWithDestinationConnection(final ConfigPersistence configPersistence,
+                                                                  final StandardDestinationDefinition destination)
+      throws Exception {
+    configPersistence.writeConfig(ConfigSchema.STANDARD_DESTINATION_DEFINITION, destination.getDestinationDefinitionId().toString(), destination);
+    final UUID connectionId = UUID.randomUUID();
+    final UUID workspaceId = UUID.randomUUID();
+    final StandardWorkspace workspace = new StandardWorkspace()
+        .withWorkspaceId(workspaceId)
+        .withName("can not be null")
+        .withSlug("can not be null")
+        .withInitialSetupComplete(true);
+    configPersistence.writeConfig(ConfigSchema.STANDARD_WORKSPACE, workspaceId.toString(), workspace);
+
+    final DestinationConnection destinationConnection = new DestinationConnection()
+        .withDestinationId(connectionId)
+        .withWorkspaceId(workspaceId)
+        .withName("can not be null")
+        .withDestinationDefinitionId(destination.getDestinationDefinitionId());
+    configPersistence.writeConfig(ConfigSchema.DESTINATION_CONNECTION, connectionId.toString(), destinationConnection);
+  }
+
   protected static void writeDestinations(final ConfigPersistence configPersistence, final List<StandardDestinationDefinition> destinations)
       throws Exception {
     final Map<String, StandardDestinationDefinition> destinationsByID = destinations.stream()

diff --git a/...istence/src/test/java/io/airbyte/config/persistence/ConfigRepositoryE2EReadWriteTest.java b/...istence/src/test/java/io/airbyte/config/persistence/ConfigRepositoryE2EReadWriteTest.java
@@ -10,8 +10,10 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.spy;
 
+import io.airbyte.commons.features.FeatureFlags;
 import io.airbyte.commons.json.Jsons;
 import io.airbyte.config.DestinationConnection;
 import io.airbyte.config.SourceConnection;
@@ -21,6 +23,7 @@
 import io.airbyte.config.StandardSync;
 import io.airbyte.config.StandardSyncOperation;
 import io.airbyte.config.StandardWorkspace;
+import io.airbyte.config.persistence.split_secrets.JsonSecretsProcessor;
 import io.airbyte.db.Database;
 import io.airbyte.db.instance.configs.ConfigsDatabaseInstance;
 import io.airbyte.db.instance.configs.ConfigsDatabaseMigrator;
@@ -51,6 +54,8 @@ public class ConfigRepositoryE2EReadWriteTest {
   private Database database;
   private ConfigRepository configRepository;
   private DatabaseConfigPersistence configPersistence;
+  private JsonSecretsProcessor jsonSecretsProcessor;
+  private FeatureFlags featureFlags;
 
   @BeforeAll
   public static void dbSetup() {
@@ -64,7 +69,9 @@ public static void dbSetup() {
   @BeforeEach
   void setup() throws IOException, JsonValidationException {
     database = new ConfigsDatabaseInstance(container.getUsername(), container.getPassword(), container.getJdbcUrl()).getAndInitialize();
-    configPersistence = spy(new DatabaseConfigPersistence(database));
+    jsonSecretsProcessor = mock(JsonSecretsProcessor.class);
+    featureFlags = mock(FeatureFlags.class);
+    configPersistence = spy(new DatabaseConfigPersistence(database, jsonSecretsProcessor, featureFlags));
     configRepository = spy(new ConfigRepository(configPersistence, database));
     final ConfigsDatabaseMigrator configsDatabaseMigrator =
         new ConfigsDatabaseMigrator(database, DatabaseConfigPersistenceLoadDataTest.class.getName());

diff --git a/...rc/test/java/io/airbyte/config/persistence/DatabaseConfigPersistenceE2EReadWriteTest.java b/...rc/test/java/io/airbyte/config/persistence/DatabaseConfigPersistenceE2EReadWriteTest.java
@@ -5,7 +5,7 @@
 package io.airbyte.config.persistence;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.junit.jupiter.api.Assertions.*;
+import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.Mockito.spy;
@@ -39,7 +39,8 @@ public class DatabaseConfigPersistenceE2EReadWriteTest extends BaseDatabaseConfi
   @BeforeEach
   public void setup() throws Exception {
     database = new ConfigsDatabaseInstance(container.getUsername(), container.getPassword(), container.getJdbcUrl()).getAndInitialize();
-    configPersistence = spy(new DatabaseConfigPersistence(database));
+
+    configPersistence = spy(new DatabaseConfigPersistence(database, jsonSecretsProcessor, featureFlags));
     final ConfigsDatabaseMigrator configsDatabaseMigrator =
         new ConfigsDatabaseMigrator(database, DatabaseConfigPersistenceLoadDataTest.class.getName());
     final DevDatabaseMigrator devDatabaseMigrator = new DevDatabaseMigrator(configsDatabaseMigrator);

diff --git a/...ce/src/test/java/io/airbyte/config/persistence/DatabaseConfigPersistenceLoadDataTest.java b/...ce/src/test/java/io/airbyte/config/persistence/DatabaseConfigPersistenceLoadDataTest.java
@@ -48,7 +48,7 @@ public class DatabaseConfigPersistenceLoadDataTest extends BaseDatabaseConfigPer
   @BeforeAll
   public static void setup() throws Exception {
     database = new ConfigsDatabaseInstance(container.getUsername(), container.getPassword(), container.getJdbcUrl()).getAndInitialize();
-    configPersistence = spy(new DatabaseConfigPersistence(database));
+    configPersistence = spy(new DatabaseConfigPersistence(database, jsonSecretsProcessor, featureFlags));
     final ConfigsDatabaseMigrator configsDatabaseMigrator =
         new ConfigsDatabaseMigrator(database, DatabaseConfigPersistenceLoadDataTest.class.getName());
     final DevDatabaseMigrator devDatabaseMigrator = new DevDatabaseMigrator(configsDatabaseMigrator);

diff --git a/...ersistence/src/test/java/io/airbyte/config/persistence/DatabaseConfigPersistenceTest.java b/...ersistence/src/test/java/io/airbyte/config/persistence/DatabaseConfigPersistenceTest.java
@@ -13,7 +13,13 @@
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.eq;
 import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
 
 import com.fasterxml.jackson.databind.JsonNode;
 import com.google.common.collect.Lists;
@@ -29,6 +35,7 @@
 import io.airbyte.db.instance.configs.ConfigsDatabaseMigrator;
 import io.airbyte.db.instance.development.DevDatabaseMigrator;
 import io.airbyte.db.instance.development.MigrationDevHelper;
+import io.airbyte.protocol.models.ConnectorSpecification;
 import java.time.Duration;
 import java.time.Instant;
 import java.time.LocalDate;
@@ -37,6 +44,7 @@
 import java.util.Map;
 import java.util.Set;
 import java.util.UUID;
+import java.util.stream.Collectors;
 import java.util.stream.Stream;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
@@ -51,7 +59,7 @@ public class DatabaseConfigPersistenceTest extends BaseDatabaseConfigPersistence
   @BeforeEach
   public void setup() throws Exception {
     database = new ConfigsDatabaseInstance(container.getUsername(), container.getPassword(), container.getJdbcUrl()).getAndInitialize();
-    configPersistence = spy(new DatabaseConfigPersistence(database));
+    configPersistence = spy(new DatabaseConfigPersistence(database, jsonSecretsProcessor, featureFlags));
     final ConfigsDatabaseMigrator configsDatabaseMigrator =
         new ConfigsDatabaseMigrator(database, DatabaseConfigPersistenceLoadDataTest.class.getName());
     final DevDatabaseMigrator devDatabaseMigrator = new DevDatabaseMigrator(configsDatabaseMigrator);
@@ -170,6 +178,27 @@ public void testDumpConfigs() throws Exception {
     assertSameConfigDump(expected, actual);
   }
 
+  @Test
+  public void testDumpConfigsWithoutSecret() throws Exception {
+    final ConnectorSpecification mockedConnectorSpec = new ConnectorSpecification()
+        .withConnectionSpecification(
+            Jsons.emptyObject());
+    doReturn(new StandardDestinationDefinition()
+        .withSpec(mockedConnectorSpec)).when(configPersistence).getConfig(eq(ConfigSchema.STANDARD_DESTINATION_DEFINITION), any(), any());
+    doReturn(new StandardSourceDefinition()
+        .withSpec(mockedConnectorSpec)).when(configPersistence).getConfig(eq(ConfigSchema.STANDARD_SOURCE_DEFINITION), any(), any());
+
+    when(featureFlags.exposeSecretsInExport()).thenReturn(false);
+    writeSourceWithSourceConnection(configPersistence, SOURCE_GITHUB);
+    writeSourceWithSourceConnection(configPersistence, SOURCE_POSTGRES);
+    writeDestinationWithDestinationConnection(configPersistence, DESTINATION_S3);
+    final Map<String, Stream<JsonNode>> result = configPersistence.dumpConfigs();
+    result.values().forEach(stream -> {
+      stream.collect(Collectors.toList());
+    });
+    verify(jsonSecretsProcessor, times(3)).maskSecrets(any(), any());
+  }
+
   @Test
   public void testGetConnectorRepositoryToInfoMap() throws Exception {
     final String connectorRepository = "airbyte/duplicated-connector";