Add connector SPI for returning redactable properties #24562

piotrrzysko · 2024-12-23T11:35:07Z

Description

An alternative approach to #23103. The main difference is that in this approach, the properties requiring redaction are selected from those provided by the user, rather than always returning a static set of predefined security-sensitive properties. The benefits are as follows:

By default (if a connector doesn't implement the SPI), all properties are masked.
Unknown (potentially misspelled) properties can also be treated as redactable.

This PR includes an implementation of the new SPI for the PostgreSQL, Hive, Iceberg, Delta, and Hudi connectors. Once we confirm that the approach is correct, we will apply it to the remaining connectors.

Here is a PR demonstrating how the new SPI could be used to mask security-sensitive properties in queries related to creating catalogs: #24563.

Additional context and related issues

Resolves #22887.

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text:

## SPI
* Add connector SPI for returning security-sensitive properties ({issue}`22887`)

core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorFactory.java

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

plugin/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlPlugin.java

core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorFactory.java

dain · 2025-01-02T19:46:37Z

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

    {
        checkArgument(!isNullOrEmpty(name), "name is null or empty");
        this.name = name;
        this.module = requireNonNull(module, "module is null");
+        Set<Class<?>> configClasses = ImmutableSet.<Class<?>>builder()


Instead of attempting to list every configuration class, I think we should modify ConfigurationFactory in Airlift to extract the properties for us. I'm thinking (just thoughts after a brief look) that we have a method to extract all properties from a set of modules, and classify them into used, unused, and unknown. With used and unused having sub classification for secure or unsecure.

I like the idea of getting properties from Airlift. However, I’m wondering if this is feasible, given that some modules are bound conditionally. Additionally, in some cases, we use constructs like: https://github.com/trinodb/trino/blob/master/plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeSecurityModule.java#L43-L49, which means Airlift isn’t even aware that multiple modules can be bound.

I’m assuming we want to have a static list of possible properties, rather than bootstrapping a connector when getSecuritySensitivePropertyNames is called. Is this a wrong assumption?

@piotrrzysko Can we scan the classpath to find all configuration classes annotated with @config ?

I do this in this test:

trino/plugin/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlPlugin.java

Line 97 in 8018b45

Set<ConfigPropertyMetadata> propertiesFoundInClasspath = findPropertiesInRuntimeClasspath(excludedClasses);

I wanted to avoid scanning the classpath at runtime to minimize the time required for plugin loading.

@piotrrzysko we could generate an index while building a project, in the trino-maven-plugin

Classpath scanning won't work becasue you can mount configuration classes under a prefix... like we do for HTTP clients.

Update

I implemented the idea of retrieving properties from Airlift. See the recent changes in this PR and the corresponding PR in Airlift: airlift/airlift#1354.

To verify its functionality, I applied it to the following connectors:

PostgreSQL (effectively all JDBC connectors, as they share the same returning sensitive properties)

Hive, Iceberg, Delta, and Hudi – These are unique because they create a separate Bootstrap instance when loading trino-hdfs.

One concern I had was whether traversing the module tree might cause side effects. Specifically, I wasn’t sure what operations are allowed in the Module.configure method. For example, can you open a database connection in this method?

After some thought, I believe we’re fine because a similar situation can occur during catalog creation. Here’s an example:

A user issues a query with invalid configuration.

Connector bootstrapping fails after the configuration stage.

If any resource was opened during the process, it would remain unclosed.

Based on this, I assume operations with side effects, such as opening database connections, shouldn't be performed in Module.configure.

Unresolved Issue

I encountered a problem while updating the PR with masking (#24563).
Since we now need to evaluate catalog properties for redaction right after parsing (reference code), more complex expressions than string literals can cause failures. For example, evaluating the following properties fails because the session doesn’t yet contain a transactionId:

CREATE CATALOG catalog USING jdbc WITH ( "connection-url" = 'jdbc:h2:mem:config', "connection-user" = lower('BOB'), "connection-password" = '1234' )

To address this, I tried delaying redaction as much as possible by moving it to just before the query state machine is created (reference code). This didn’t resolve the issue. The query failed because it hadn’t been registered yet in the LanguageFunctionManager (reference code). The registration only happens after the query machine is created (reference code).

I’m not yet sure if I can delay redaction further, as the query machine references the query text, which is used to build QueryInfo objects that are exposed via the REST API. For now, when a query contains more complex expressions I mask all the properties.

ksobolew · 2025-01-07T07:33:59Z

I think this will be helpful in getting #22669 done as well (the main stumbling block there is that by making catalog properties available in all catalog manager implementations, we may expose some security-sensitive properties).

piotrrzysko · 2025-01-10T14:52:35Z

Update:

It seems the issue I described here: #24562 (comment) is solvable (thanks, @dain, for the help!). I’ve applied the necessary changes to #24563 and added tests to confirm that the evaluation of complex expressions works as expected.

This PR now depends on: airlift/airlift#1354

piotrrzysko · 2025-01-29T12:28:52Z

@dain @hashhar could you please review this PR again?

hashhar

Looks good % comment

core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorFactory.java

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/config/ConfigUtils.java

The SPI will be used by the engine to redact security-sensitive information in statements that manage catalogs. It has been added at the connector factory level, rather than the connector level, to allow more flexibility in retrieving properties. In some cases, we want to perform redacting before a connector is initiated. For example, when we create a new catalog by issuing the CREATE CATALOG statement.

wendigo · 2025-02-05T15:51:35Z

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/config/ConfigUtils.java

+public final class ConfigUtils
+{
+    private ConfigUtils() {}
+
+    public static Set<String> getSecuritySensitivePropertyNames(Map<String, String> config, Set<ConfigPropertyMetadata> usedProperties)
+    {
+        Set<String> sensitivePropertyNames = new HashSet<>(config.keySet());
+
+        for (ConfigPropertyMetadata propertyMetadata : usedProperties) {
+            if (!propertyMetadata.securitySensitive()) {
+                sensitivePropertyNames.remove(propertyMetadata.name());
+            }
+        }
+
+        return ImmutableSet.copyOf(sensitivePropertyNames);
+    }
+}


Suggested change

public final class ConfigUtils

{

private ConfigUtils() {}

public static Set<String> getSecuritySensitivePropertyNames(Map<String, String> config, Set<ConfigPropertyMetadata> usedProperties)

{

Set<String> sensitivePropertyNames = new HashSet<>(config.keySet());

for (ConfigPropertyMetadata propertyMetadata : usedProperties) {

if (!propertyMetadata.securitySensitive()) {

sensitivePropertyNames.remove(propertyMetadata.name());

}

}

return ImmutableSet.copyOf(sensitivePropertyNames);

}

}

public final class ConfigUtils

{

private ConfigUtils() {}

public static Set<String> getSecuritySensitivePropertyNames(Map<String, String> config, Set<ConfigPropertyMetadata> usedProperties)

{

Set<String> safePropertyNames = usedProperties.stream()

.filter(metadata -> !metadata.securitySensitive())

.map(ConfigPropertyMetadata::name)

.collect(toImmutableSet());

return Sets.difference(config.keySet(), safePropertyNames);

}

}

@hashhar why oh why :)

wendigo · 2025-02-05T15:54:11Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveConnectorFactory.java

+    @Override
+    public Set<String> getSecuritySensitivePropertyNames(String catalogName, Map<String, String> config, ConnectorContext context)
+    {
+        ClassLoader classLoader = HiveConnectorFactory.class.getClassLoader();


you can inline class loader

wendigo · 2025-02-05T15:54:43Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveConnectorFactory.java

+
+        if (quietBootstrap) {
+            app.quiet()
+                    .skipErrorReporting();


move to the left

wendigo · 2025-02-05T15:55:30Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveConnectorFactory.java

+                module);
+
+        if (quietBootstrap) {
+            app.quiet()


it would be better to have a single method that accepts boolean flag as these are always called together in the if

this will allow for the chained call without the need to assign to a variable

let's add withQuiet(boolean flag) and withSkipErrorReporting(boolean flag)

Are you suggesting adding these methods to Airlift? If so, that would be inconsistent with other methods in Bootstrap. Currently, boolean switches like loadSecretsPlugins and doNotInitializeLogging do not take arguments.

Yes. Inconsistent? Maybe, but more readable in the callsite.

For consistency we can add all methods with boolean flags

I don't have a strong opinion on this. However, the construct with conditionally applying quiet and skipErrorReporting is currently used in five places, and I don't think it will be needed for other connectors. In fact, I can replace four of these five usages with a different approach - it will be a chained call.

It seems to me that introducing a new convention only for this case might be overkill, especially considering that Bootstrap is used extensively elsewhere.

plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHivePlugin.java

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.

This preparatory commit enables bootstrapping HDFS to retrieve its security-sensitive properties.

cla-bot bot added the cla-signed label Dec 23, 2024

This was referenced Dec 23, 2024

Redact sensitive information in catalog queries #24563

Draft

Add connector SPI for returning security-sensitive properties #23103

Closed

hashhar reviewed Jan 2, 2025

View reviewed changes

core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorFactory.java Outdated Show resolved Hide resolved

hashhar reviewed Jan 2, 2025

View reviewed changes

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/config/ConfigPropertyMetadata.java Outdated Show resolved Hide resolved

hashhar reviewed Jan 2, 2025

View reviewed changes

plugin/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlPlugin.java Outdated Show resolved Hide resolved

hashhar approved these changes Jan 2, 2025

View reviewed changes

piotrrzysko mentioned this pull request Jan 2, 2025

Extend syntax for Dynamic Catalogs #22188

Open

1 task

dain reviewed Jan 2, 2025

View reviewed changes

piotrrzysko force-pushed the redactable-properties-spi branch from 8018b45 to a4f5809 Compare January 6, 2025 15:55

piotrrzysko force-pushed the redactable-properties-spi branch from a4f5809 to 47db44b Compare January 9, 2025 17:51

github-actions bot added hudi Hudi connector iceberg Iceberg connector delta-lake Delta Lake connector hive Hive connector labels Jan 9, 2025

piotrrzysko force-pushed the redactable-properties-spi branch 2 times, most recently from c4a3d9f to 4edb088 Compare January 13, 2025 15:44

piotrrzysko marked this pull request as ready for review January 13, 2025 19:43

piotrrzysko requested a review from dain January 13, 2025 19:43

piotrrzysko force-pushed the redactable-properties-spi branch from 4edb088 to e32ef86 Compare January 20, 2025 08:49

hashhar reviewed Feb 3, 2025

View reviewed changes

core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorFactory.java Outdated Show resolved Hide resolved

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/config/ConfigUtils.java Show resolved Hide resolved

piotrrzysko force-pushed the redactable-properties-spi branch from e32ef86 to 6f81c49 Compare February 5, 2025 12:20

github-actions bot added the postgresql PostgreSQL connector label Feb 5, 2025

wendigo reviewed Feb 5, 2025

View reviewed changes

plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHivePlugin.java Outdated Show resolved Hide resolved

piotrrzysko added 6 commits February 6, 2025 13:08

Expose security-sensitive properties for HDFS

e438164

This preparatory commit enables bootstrapping HDFS to retrieve its security-sensitive properties.

Expose security-sensitive properties for Hive connector

b0016ae

Expose security-sensitive properties for Iceberg connector

0fb4f58

Expose security-sensitive properties for Delta Lake connector

ca44a46

Expose security-sensitive properties for Hudi connector

9c71c79

piotrrzysko force-pushed the redactable-properties-spi branch from 6f81c49 to 9c71c79 Compare February 6, 2025 12:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add connector SPI for returning redactable properties #24562

Add connector SPI for returning redactable properties #24562

piotrrzysko commented Dec 23, 2024 •

edited

Loading

dain Jan 2, 2025

piotrrzysko Jan 5, 2025 •

edited

Loading

wendigo Jan 5, 2025

piotrrzysko Jan 6, 2025

wendigo Jan 6, 2025

dain Jan 6, 2025

piotrrzysko Jan 9, 2025 •

edited

Loading

ksobolew commented Jan 7, 2025

piotrrzysko commented Jan 10, 2025

piotrrzysko commented Jan 29, 2025

hashhar left a comment

wendigo Feb 5, 2025

wendigo Feb 6, 2025

wendigo Feb 5, 2025

wendigo Feb 5, 2025

wendigo Feb 5, 2025

wendigo Feb 5, 2025

wendigo Feb 5, 2025

piotrrzysko Feb 6, 2025

wendigo Feb 6, 2025

wendigo Feb 6, 2025

piotrrzysko Feb 6, 2025

Add connector SPI for returning redactable properties #24562

Are you sure you want to change the base?

Add connector SPI for returning redactable properties #24562

Conversation

piotrrzysko commented Dec 23, 2024 • edited Loading

Description

Additional context and related issues

Release notes

Choose a reason for hiding this comment

piotrrzysko Jan 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piotrrzysko Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Update

Unresolved Issue

ksobolew commented Jan 7, 2025

piotrrzysko commented Jan 10, 2025

piotrrzysko commented Jan 29, 2025

hashhar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piotrrzysko commented Dec 23, 2024 •

edited

Loading

piotrrzysko Jan 5, 2025 •

edited

Loading

piotrrzysko Jan 9, 2025 •

edited

Loading