Expose security-sensitive properties for PostgreSQL connector

Exposed properties fall into one of the following categories: they are
either explicitly marked as security-sensitive or are unknown. The
connector assumes that unknown properties might be misspelled
security-sensitive properties.

The purpose of the included test is to identify security-sensitive
properties that may be used by the connector. It uses the output
generated by the maven-dependency-plugin, configured in the connector's
pom.xml file. This output contains the connector's runtime classpath,
which is then scanned to identify all property names annotated with
@config. Scanning the classpath ensures that all configuration classes
are included, even those used conditionally or contributed by other
modules.

Author: piotrrzysko

Diff for: lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/config/ConfigPropertyMetadata.java

@@ -0,0 +1,39 @@
+/*
+ * 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 io.trino.plugin.base.config;
+
+import io.airlift.configuration.ConfigurationMetadata;
+
+import java.util.Set;
+
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
+import static io.airlift.configuration.ConfigurationMetadata.getConfigurationMetadata;
+import static java.util.Objects.requireNonNull;
+
+public record ConfigPropertyMetadata(String name, boolean sensitive)
+{
+    public ConfigPropertyMetadata
+    {
+        requireNonNull(name, "name is null");
+    }
+
+    public static Set<ConfigPropertyMetadata> getConfigProperties(Class<?> configClass)
+    {
+        ConfigurationMetadata<?> configurationMetadata = getConfigurationMetadata(configClass);
+        return configurationMetadata.getAttributes().values().stream()
+                .map(attribute -> new ConfigPropertyMetadata(attribute.getInjectionPoint().getProperty(), attribute.isSecuritySensitive()))
+                .collect(toImmutableSet());
+    }
+}

Diff for: plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcConnectorFactory.java

@@ -13,10 +13,21 @@
  */
 package io.trino.plugin.jdbc;
 
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
 import com.google.inject.Injector;
 import com.google.inject.Module;
 import io.airlift.bootstrap.Bootstrap;
+import io.airlift.bootstrap.BootstrapConfig;
 import io.opentelemetry.api.OpenTelemetry;
+import io.trino.plugin.base.config.ConfigPropertyMetadata;
+import io.trino.plugin.base.mapping.MappingConfig;
+import io.trino.plugin.jdbc.credential.CredentialConfig;
+import io.trino.plugin.jdbc.credential.CredentialProviderTypeConfig;
+import io.trino.plugin.jdbc.credential.ExtraCredentialConfig;
+import io.trino.plugin.jdbc.credential.file.ConfigFileBasedCredentialProviderConfig;
+import io.trino.plugin.jdbc.credential.keystore.KeyStoreBasedCredentialProviderConfig;
+import io.trino.plugin.jdbc.logging.FormatBasedRemoteQueryModifierConfig;
 import io.trino.spi.NodeManager;
 import io.trino.spi.VersionEmbedder;
 import io.trino.spi.catalog.CatalogName;
@@ -26,24 +37,55 @@
 import io.trino.spi.type.TypeManager;
 
 import java.util.Map;
+import java.util.Set;
 import java.util.function.Supplier;
+import java.util.stream.Stream;
 
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Strings.isNullOrEmpty;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static io.trino.plugin.base.Versions.checkStrictSpiVersionMatch;
+import static io.trino.plugin.base.config.ConfigPropertyMetadata.getConfigProperties;
 import static java.util.Objects.requireNonNull;
 
 public class JdbcConnectorFactory
         implements ConnectorFactory
 {
     private final String name;
     private final Supplier<Module> module;
+    private final Set<String> nonSecuritySensitivePropertyNames;
 
-    public JdbcConnectorFactory(String name, Supplier<Module> module)
+    public JdbcConnectorFactory(String name, Supplier<Module> module, Set<ConfigPropertyMetadata> additionalProperties)
     {
         checkArgument(!isNullOrEmpty(name), "name is null or empty");
         this.name = name;
         this.module = requireNonNull(module, "module is null");
+        Set<Class<?>> configClasses = ImmutableSet.<Class<?>>builder()
+                .add(BaseJdbcConfig.class)
+                .add(CredentialConfig.class)
+                .add(JdbcStatisticsConfig.class)
+                .add(JdbcWriteConfig.class)
+                .add(QueryConfig.class)
+                .add(RemoteQueryCancellationConfig.class)
+                .add(TypeHandlingJdbcConfig.class)
+                .add(JdbcMetadataConfig.class)
+                .add(JdbcJoinPushdownConfig.class)
+                .add(DecimalConfig.class)
+                .add(JdbcDynamicFilteringConfig.class)
+                .add(KeyStoreBasedCredentialProviderConfig.class)
+                .add(FormatBasedRemoteQueryModifierConfig.class)
+                .add(ConfigFileBasedCredentialProviderConfig.class)
+                .add(CredentialProviderTypeConfig.class)
+                .add(ExtraCredentialConfig.class)
+                .add(MappingConfig.class)
+                .add(BootstrapConfig.class)
+                .build();
+        this.nonSecuritySensitivePropertyNames = Stream.concat(
+                        configClasses.stream().flatMap(clazz -> getConfigProperties(clazz).stream()),
+                        requireNonNull(additionalProperties, "additionalProperties is null").stream())
+                .filter(property -> !property.sensitive())
+                .map(ConfigPropertyMetadata::name)
+                .collect(toImmutableSet());
     }
 
     @Override
@@ -74,4 +116,10 @@ public Connector create(String catalogName, Map<String, String> requiredConfig,
 
         return injector.getInstance(JdbcConnector.class);
     }
+
+    @Override
+    public Set<String> getSecuritySensitivePropertyNames(Set<String> propertyNames)
+    {
+        return Sets.difference(propertyNames, nonSecuritySensitivePropertyNames);
+    }
 }

Diff for: plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcPlugin.java

@@ -14,11 +14,14 @@
 package io.trino.plugin.jdbc;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import com.google.inject.Module;
+import io.trino.plugin.base.config.ConfigPropertyMetadata;
 import io.trino.plugin.jdbc.credential.CredentialProviderModule;
 import io.trino.spi.Plugin;
 import io.trino.spi.connector.ConnectorFactory;
 
+import java.util.Set;
 import java.util.function.Supplier;
 
 import static com.google.common.base.Preconditions.checkArgument;
@@ -31,12 +34,19 @@ public class JdbcPlugin
 {
     private final String name;
     private final Supplier<Module> module;
+    private final Set<ConfigPropertyMetadata> additionalProperties;
 
     public JdbcPlugin(String name, Supplier<Module> module)
+    {
+        this(name, module, ImmutableSet.of());
+    }
+
+    public JdbcPlugin(String name, Supplier<Module> module, Set<ConfigPropertyMetadata> additionalProperties)
     {
         checkArgument(!isNullOrEmpty(name), "name is null or empty");
         this.name = name;
         this.module = requireNonNull(module, "module is null");
+        this.additionalProperties = ImmutableSet.copyOf(requireNonNull(additionalProperties, "additionalProperties is null"));
     }
 
     @Override
@@ -47,6 +57,7 @@ public Iterable<ConnectorFactory> getConnectorFactories()
                 () -> combine(
                         new CredentialProviderModule(),
                         new ExtraCredentialsBasedIdentityCacheMappingModule(),
-                        module.get())));
+                        module.get()),
+                additionalProperties));
     }
 }

Diff for: plugin/trino-postgresql/pom.xml

@@ -158,6 +158,13 @@
             <scope>test</scope>
         </dependency>
 
+        <dependency>
+            <groupId>io.github.classgraph</groupId>
+            <artifactId>classgraph</artifactId>
+            <version>4.8.174</version>
+            <scope>test</scope>
+        </dependency>
+
         <dependency>
             <groupId>io.trino</groupId>
             <artifactId>trino-base-jdbc</artifactId>
@@ -276,4 +283,27 @@
             <scope>test</scope>
         </dependency>
     </dependencies>
+
+    <build>
+        <plugins>
+            <plugin>
+                <groupId>org.apache.maven.plugins</groupId>
+                <artifactId>maven-dependency-plugin</artifactId>
+                <executions>
+                    <execution>
+                        <id>print-runtime-dependencies</id>
+                        <goals>
+                            <goal>build-classpath</goal>
+                        </goals>
+                        <phase>generate-sources</phase>
+                        <configuration>
+                            <skip>false</skip>
+                            <includeScope>runtime</includeScope>
+                            <outputFile>${project.build.testOutputDirectory}/runtime-dependencies.txt</outputFile>
+                        </configuration>
+                    </execution>
+                </executions>
+            </plugin>
+        </plugins>
+    </build>
 </project>

Diff for: plugin/trino-postgresql/src/main/java/io/trino/plugin/postgresql/PostgreSqlPlugin.java

@@ -16,12 +16,13 @@
 import io.trino.plugin.jdbc.JdbcPlugin;
 
 import static io.airlift.configuration.ConfigurationAwareModule.combine;
+import static io.trino.plugin.base.config.ConfigPropertyMetadata.getConfigProperties;
 
 public class PostgreSqlPlugin
         extends JdbcPlugin
 {
     public PostgreSqlPlugin()
     {
-        super("postgresql", () -> combine(new PostgreSqlClientModule(), new PostgreSqlConnectionFactoryModule()));
+        super("postgresql", () -> combine(new PostgreSqlClientModule(), new PostgreSqlConnectionFactoryModule()), getConfigProperties(PostgreSqlConfig.class));
     }
 }

Diff for: plugin/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlPlugin.java

@@ -14,11 +14,30 @@
 package io.trino.plugin.postgresql;
 
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import io.airlift.configuration.Config;
+import io.airlift.configuration.ConfigSecuritySensitive;
+import io.github.classgraph.AnnotationInfo;
+import io.github.classgraph.AnnotationParameterValueList;
+import io.github.classgraph.ClassGraph;
+import io.github.classgraph.ScanResult;
+import io.trino.plugin.base.config.ConfigPropertyMetadata;
 import io.trino.spi.Plugin;
 import io.trino.spi.connector.ConnectorFactory;
 import org.junit.jupiter.api.Test;
 
+import java.io.File;
+import java.io.IOException;
+import java.net.URISyntaxException;
+import java.net.URL;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.Set;
+
+import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.common.collect.Iterables.getOnlyElement;
+import static org.assertj.core.api.Assertions.assertThat;
 
 public class TestPostgreSqlPlugin
 {
@@ -34,4 +53,97 @@ public void testCreateConnector()
                         "bootstrap.quiet", "true"),
                 new TestingPostgreSqlConnectorContext()).shutdown();
     }
+
+    @Test
+    void testUnknownPropertiesAreSecuritySensitive()
+    {
+        Plugin plugin = new PostgreSqlPlugin();
+        ConnectorFactory connectorFactory = getOnlyElement(plugin.getConnectorFactories());
+        Set<String> unknownProperties = ImmutableSet.of("unknown");
+
+        Set<String> sensitiveProperties = connectorFactory.getSecuritySensitivePropertyNames(unknownProperties);
+
+        assertThat(sensitiveProperties).isEqualTo(unknownProperties);
+    }
+
+    @Test
+    void testSecuritySensitiveProperties()
+            throws Exception
+    {
+        // The purpose of this test is to help identify security-sensitive properties that
+        // may be used by the connector. These properties are detected by scanning the
+        // plugin's runtime classpath and collecting all property names annotated with
+        // @ConfigSecuritySensitive. The scan includes all configuration classes, whether
+        // they are always used, conditionally used, or never used. This approach has both
+        // advantages and disadvantages.
+        //
+        // One advantage is that we don't need to rely on the plugin's configuration to
+        // retrieve properties that are used conditionally. However, this method may also
+        // capture properties that are not used at all but are pulled into the classpath
+        // by dependencies. With that in mind, if this test fails, it likely indicates that
+        // either a property needs to be added to the connector's security-sensitive
+        // property names list, or it should be added to the excluded properties list below.
+        Set<String> excludedClasses = ImmutableSet.of(
+                "io.trino.plugin.base.ldap.LdapClientConfig",
+                "io.airlift.http.client.HttpClientConfig",
+                "io.airlift.node.NodeConfig",
+                "io.airlift.log.LoggingConfiguration",
+                "io.trino.plugin.base.security.FileBasedAccessControlConfig",
+                "io.airlift.configuration.secrets.SecretsPluginConfig",
+                "io.trino.plugin.base.jmx.ObjectNameGeneratorConfig");
+        Plugin plugin = new PostgreSqlPlugin();
+        ConnectorFactory connectorFactory = getOnlyElement(plugin.getConnectorFactories());
+
+        Set<ConfigPropertyMetadata> propertiesFoundInClasspath = findPropertiesInRuntimeClasspath(excludedClasses);
+        Set<String> allPropertyNames = propertiesFoundInClasspath.stream()
+                .map(ConfigPropertyMetadata::name)
+                .collect(toImmutableSet());
+        Set<String> expectedProperties = propertiesFoundInClasspath.stream()
+                .filter(ConfigPropertyMetadata::sensitive)
+                .map(ConfigPropertyMetadata::name)
+                .collect(toImmutableSet());
+
+        Set<String> sensitiveProperties = connectorFactory.getSecuritySensitivePropertyNames(allPropertyNames);
+
+        assertThat(sensitiveProperties).isEqualTo(expectedProperties);
+    }
+
+    private static Set<ConfigPropertyMetadata> findPropertiesInRuntimeClasspath(Set<String> excludedClassNames)
+            throws URISyntaxException, IOException
+    {
+        try (ScanResult scanResult = new ClassGraph()
+                .overrideClasspath(buildRuntimeClasspath())
+                .enableAllInfo()
+                .scan()) {
+            return scanResult.getClassesWithMethodAnnotation(Config.class).stream()
+                    .filter(classInfo -> !excludedClassNames.contains(classInfo.getName()))
+                    .flatMap(classInfo -> classInfo.getMethodInfo().stream())
+                    .filter(methodInfo -> methodInfo.hasAnnotation(Config.class))
+                    .map(methodInfo -> {
+                        boolean sensitive = methodInfo.hasAnnotation(ConfigSecuritySensitive.class);
+                        AnnotationInfo annotationInfo = methodInfo.getAnnotationInfo(Config.class);
+                        checkState(annotationInfo != null, "Missing @Config annotation for %s", methodInfo);
+                        AnnotationParameterValueList parameterValues = annotationInfo.getParameterValues();
+                        checkState(parameterValues.size() == 1, "Expected exactly one parameter for %s", annotationInfo);
+                        String propertyName = (String) parameterValues.getFirst().getValue();
+                        return new ConfigPropertyMetadata(propertyName, sensitive);
+                    })
+                    .collect(toImmutableSet());
+        }
+    }
+
+    private static String buildRuntimeClasspath()
+            throws URISyntaxException, IOException
+    {
+        // This file is generated by the maven-dependency-plugin, which is configured in the connector's pom.xml file.
+        String runtimeDependenciesFile = "runtime-dependencies.txt";
+        URL runtimeDependenciesUrl = TestPostgreSqlPlugin.class.getClassLoader().getResource(runtimeDependenciesFile);
+        checkState(runtimeDependenciesUrl != null, "Missing %s file", runtimeDependenciesUrl);
+        String runtimeDependenciesClasspath = Files.readString(Path.of(runtimeDependenciesUrl.toURI()));
+
+        File classDirectory = new File(new File(runtimeDependenciesUrl.toURI()).getParentFile().getParentFile(), "classes/");
+        checkState(classDirectory.exists(), "Missing %s directory", classDirectory.getAbsolutePath());
+
+        return "%s:%s".formatted(runtimeDependenciesClasspath, classDirectory.getAbsolutePath());
+    }
 }