mchades commented on code in PR #11149:
URL: https://github.com/apache/gravitino/pull/11149#discussion_r3286385010
##########
docs/gravitino-server-config.md:
##########
@@ -143,8 +143,9 @@ Gravitino server uses tree lock to ensure the consistency
of the data. The tree
| Configuration item | Description
| Default value | Required | Since version |
|----------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------|----------|---------------|
-| `gravitino.catalog.cache.evictionIntervalMs` | The interval in milliseconds
to evict the catalog cache; default 3600000ms(1h).
| `3600000` | No | 0.1.0 |
-| `gravitino.catalog.classloader.isolated` | Whether to use an isolated
classloader for catalog. If `true`, an isolated classloader loads all
catalog-related libraries and configurations, not the AppClassLoader. The
default value is `true`. | `true` | No | 0.1.0 |
+| `gravitino.catalog.cache.evictionIntervalMs` | The interval in
milliseconds to evict the catalog cache; default 3600000ms(1h).
| `3600000` | No |
0.1.0 |
+| `gravitino.catalog.classloader.isolated` | Whether to use an
isolated classloader for catalog. If `true`, an isolated classloader loads all
catalog-related libraries and configurations, not the AppClassLoader. The
default value is `true`.
| `true` | No
| 0.1.0 |
+| `gravitino.catalog.credential.backfillToProperties` | For backward
compatibility only: if `true`, the server exposes hidden catalog credentials
(such as jdbc-user and jdbc-password) in the catalog properties response so
that connectors that do not support credential vending can still read them.
**Enabling this is a security risk** — credentials are visible to anyone who
can read catalog properties. Disable once all connectors are upgraded. |
`false` | No | 1.3.0 |
Review Comment:
Will this configuration affect all catalogs or only the JDBC catalog?
##########
core/src/main/java/org/apache/gravitino/Configs.java:
##########
@@ -521,6 +521,18 @@ private Configs() {}
+ " and must not contain '.' or the internal physical
separator (\\u0001)")
.createWithDefault(":");
+ public static final ConfigEntry<Boolean>
CATALOG_CREDENTIAL_BACKFILL_TO_PROPERTIES =
+ new ConfigBuilder("gravitino.catalog.credential.backfillToProperties")
+ .doc(
+ "If true, the server exposes hidden catalog credentials (such as
jdbc-user and "
+ + "jdbc-password) in the catalog properties response. Enable
only during a "
+ + "rolling upgrade while old connectors that do not support
credential vending "
+ + "are still in use. Enabling this is a security risk
because credentials "
+ + "become visible to anyone who can read catalog
properties.")
+ .version(ConfigConstants.VERSION_0_9_0)
Review Comment:
wrong version?
##########
catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalog.java:
##########
@@ -123,10 +126,44 @@ public PropertiesMetadata tablePropertiesMetadata()
throws UnsupportedOperationE
@Override
@Evolving
public Map<String, String> propertiesWithCredentialProviders() {
- Map<String, String> properties =
Maps.newHashMap(super.propertiesWithCredentialProviders());
+ // Use raw entity properties so that hidden credentials
(jdbc-user/jdbc-password) are visible
+ // to the credential manager even after they are marked hidden in the
properties metadata.
+ Map<String, String> properties = Maps.newHashMap(entity().getProperties());
return applyDefaultCredentialProviders(properties);
}
+ /**
+ * Returns catalog properties, optionally re-adding hidden JDBC credentials
for backward
+ * compatibility with connectors that do not support credential vending. The
backfill behavior is
+ * controlled by the server-level config {@code
+ * gravitino.catalog.credential.backfillToProperties}; it is disabled by
default and should only
+ * be enabled during rolling upgrades.
+ *
+ * @return the catalog properties map, with credentials backfilled if the
server config is set
+ */
+ @Override
+ public Map<String, String> properties() {
+ Map<String, String> props = super.properties();
+ Config serverConfig = GravitinoEnv.getInstance().config();
+ if (serverConfig == null
+ ||
!serverConfig.get(Configs.CATALOG_CREDENTIAL_BACKFILL_TO_PROPERTIES)) {
Review Comment:
It's better to use a method to do this, such as `boolean
shouldBackfillCredential()`
--
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]
