Add connector SPI for returning redactable properties #24562
Conversation
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/ConfigPropertyMetadata.java
Outdated
Show resolved
Hide resolved
plugin/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlPlugin.java
Outdated
Show resolved
Hide resolved
core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorFactory.java
Outdated
Show resolved
Hide resolved
| { | ||
| checkArgument(!isNullOrEmpty(name), "name is null or empty"); | ||
| this.name = name; | ||
| this.module = requireNonNull(module, "module is null"); | ||
| Set<Class<?>> configClasses = ImmutableSet.<Class<?>>builder() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@piotrrzysko Can we scan the classpath to find all configuration classes annotated with @config ?
There was a problem hiding this comment.
I do this in this test:
I wanted to avoid scanning the classpath at runtime to minimize the time required for plugin loading.There was a problem hiding this comment.
@piotrrzysko we could generate an index while building a project, in the trino-maven-plugin
There was a problem hiding this comment.
Classpath scanning won't work becasue you can mount configuration classes under a prefix... like we do for HTTP clients.
piotrrzysko
force-pushed
the
redactable-properties-spi
branch
from
8018b45 to
a4f5809
Compare
|
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). |
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.
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.
piotrrzysko
force-pushed
the
redactable-properties-spi
branch
from
a4f5809 to
47db44b
Compare
github-actions
bot
added
hudi
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:
This PR includes an implementation of the new SPI for the PostgreSQL connector. 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: