paultanay commented on issue #12689:
URL: https://github.com/apache/gravitino/issues/12689#issuecomment-5451257548
> Hi, I'd like to work on this issue.
>
> I traced the current `main` path: `GravitinoConfig.toCatalogConfig()`
propagates the `gravitino.client.*` properties into dynamically created
catalogs, while the Starburst compatibility hook marks property names ending in
`credential`, `password`, `secret`, or `token` as security-sensitive. This
matches the reported behavior where the static connector on the coordinator can
initialize, but a dynamic connector on a worker no longer receives the OAuth
credential.
>
> Before starting a patch, could you confirm the following contract?
>
> 1. Keep `getSecuritySensitivePropertyNames()` unchanged so raw secrets
remain
> redacted from dynamic catalog storage and distribution.
> 2. Add an explicit dynamic-catalog environment-variable mapping for
> `gravitino.client.oauth2.credential`. The generated catalog
configuration
> would contain a `${ENV:...}` reference plus non-sensitive mapping
metadata,
> never the literal credential.
> 3. Resolve that mapping locally on each Trino node before
> `GravitinoAuthProvider` builds the OAuth client.
> 4. If the environment variable is missing, fail with an error that names
both
> `gravitino.client.oauth2.credential` and the expected environment
variable.
> 5. Keep `trino.jdbc.*` coordinator-only; those properties should not be
restored
> on workers.
>
> I would add focused tests proving that the generated catalog config
contains no raw secret, a dynamic worker config can recover the credential from
the mapping, a missing variable produces the actionable error, and
non-sensitive properties are unchanged. I would also cover the routed IRC
configuration path if it shares the same redaction behavior, and document that
the environment variable must be set on the coordinator and every worker.
>
> For the first PR, I would keep the scope limited to the OAuth credential
unless you prefer a generic mapping mechanism for all security-sensitive
`gravitino.client.*` properties.
Hi @jjj-n, thanks for looking into this. I traced the dynamic catalog
configuration path and noticed one thing worth checking with the proposed
environment-variable approach.
In `GravitinoConnectorFactory.java`, security-sensitive properties are
identified from the **property name** (including the `credential` suffix),
rather than from the value. So if `gravitino.client.oauth2.credential` contains
a `${ENV:...}` reference, would it still be removed before the dynamic catalog
configuration reaches the worker?
Also, are you introducing `gravitino.dynamic-catalog.environment-variable.*`
as a new mapping mechanism, or is this relying on an existing Trino/Starburst
mechanism?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
