CalvinKirs opened a new pull request, #68129:
URL: https://github.com/apache/doris/pull/68129
### What problem does this PR solve?
Related PR: #66483 (an earlier take on the same problem via a new
connector-SPI method; this PR supersedes it with a smaller, holder-based
approach that leaves the plugin API untouched)
Problem Summary:
`driver_url` on a jdbc-flavored catalog names a jar that the FE loads into
its own JVM
(`URLClassLoader` + `Class.forName(name, true, loader)`). Doris guards it in
two layers:
1. a mandatory, non-configurable rule — no `..` path segment, and a bare
file name must match
`[A-Za-z0-9._-]+\.jar`;
2. an operator-configurable gate from fe.conf — `jdbc_driver_secure_path` /
`jdbc_driver_url_white_list`.
Two gaps:
**The iceberg-jdbc and paimon-jdbc catalogs never ran the mandatory rule.**
`iceberg.catalog.type=jdbc`
and `paimon.catalog.type=jdbc` reach the same class-loading sink through
`iceberg.jdbc.driver_url` /
`jdbc.driver_url`. Their `preCreateValidation` routes the value through the
fe.conf gate at CREATE, but
with the default `jdbc_driver_secure_path=*` that gate accepts everything —
nothing forbids a `..`
traversal segment, and the connector-side resolver
(`JdbcDriverSupport.resolveDriverUrl`) happily
resolves `../` against the drivers directory.
**None of the three catalogs ran either check on `ALTER CATALOG`.** ALTER
validates through
`PluginDrivenExternalCatalog.validatePropertiesBeforeUpdate` and never
reaches
`Connector.preCreateValidation`, which is where CREATE applies both layers;
`resetToUninitialized`
then makes the new value effective on the next metadata access. So an
operator who narrowed
`jdbc_driver_secure_path` got the restriction enforced at `CREATE CATALOG`
and silently bypassed by a
follow-up `ALTER CATALOG ... SET ("driver_url" = ...)`. (The paimon
connector's own javadoc records
this as a known gap "shared by all plugin connectors".)
What this PR does — one unified check, declared where the property is
declared:
- Extracts the mandatory rule into `JdbcDriverUrlSecurity` (fe-foundation,
`foundation.security` —
the one module every property holder depends on), deleting the jdbc-local
copy
(`JdbcDorisConnector.checkDriverUrlSecurityRule`). The rule is called from
the property holders'
statement-time validation, right next to the `driver_url` field it guards:
`JdbcCatalogProperties.checkCreateTimeOnlyRules` and the iceberg/paimon
JDBC metastore holders'
`validate()`. Those hooks run on CREATE **and** on ALTER
(`checkCreateTimeOnlyRules` →
`bindForType`) and never on replay or a catalog rebuild, so existing
catalogs and follower startup
are unaffected; the flavor gating comes for free because only the jdbc
flavor selects the jdbc
metastore backend. A dedicated test pins that `of()` keeps tolerating a
pre-rule `driver_url`.
- Applies the operator's fe.conf gate on ALTER from
`PluginDrivenExternalCatalog.checkDriverUrlsAgainstOperatorGate`, driven
by a small key table of
the three catalogs' driver-url property names (user-facing, wire-stable
keys with their documented
aliases, flavor-gated for iceberg/paimon). The fe.conf policy is the
engine's to apply while the
keys belong to the connectors; spelling out three constants avoids
widening the connector plugin
SPI, so the plugin API version and surface baseline are untouched.
- `AGENTS.md`: no new code path may fetch an artifact from a user-supplied
URL and load it into a
Doris process; the existing `driver_url` paths are grandfathered, not a
precedent.
### Release note
Fixed `driver_url` validation for jdbc-flavored catalogs: the Iceberg and
Paimon JDBC catalogs now
apply the same mandatory driver-jar rules as the JDBC catalog, and `ALTER
CATALOG` now applies
`jdbc_driver_secure_path` / `jdbc_driver_url_white_list` instead of only
`CREATE CATALOG` doing so.
### Check List (For Author)
- Test
- [x] Unit Test
New: `JdbcDriverUrlSecurityTest` (fe-foundation; rule semantics, moved
with the class),
`IcebergJdbcDriverUrlSecurityTest` / `PaimonJdbcDriverUrlSecurityTest`
(the provider's CREATE and
ALTER hooks both reach the rule; non-jdbc flavors skipped; paimon alias
covered),
`JdbcCatalogPropertiesTest` gained a pair pinning the rule on the
statement side and `of()`'s
tolerance of pre-rule values on the rebuild side,
`PluginDrivenExternalCatalogDriverUrlGateTest` (ALTER honours the operator
allow-list for all
three types; a non-jdbc iceberg flavor is untouched).
Each new production line was mutation-checked (removing it turns the
corresponding tests red).
Full runs on the affected modules (fe-foundation, fe-connector
spi/jdbc/metastore-iceberg/
metastore-paimon/iceberg/paimon): all green except
`IcebergWritePlanProviderTest#planMergePreservesExplicitlyEmptyReadAcrossConcurrentFirstAppend`,
which fails identically on unmodified master in the same environment
(pre-existing, unrelated).
fe-core plugin/gate tests: 42 passing, 0 Checkstyle violations.
- Behavior changed:
- [x] Yes.
A `driver_url` containing a `..` segment, or a bare file name outside
`[A-Za-z0-9._-]+\.jar`, is
now rejected on the Iceberg/Paimon JDBC catalogs as it already was on the
JDBC catalog, and on
ALTER as well as CREATE. `jdbc_driver_secure_path` /
`jdbc_driver_url_white_list` now also apply
on ALTER. The default posture is unchanged: `jdbc_driver_secure_path`
still defaults to `*`, so a
remote driver jar is still accepted unless the operator narrows the config
— what changes is that
narrowing it now holds on every DDL path. Validation never runs on replay
or catalog rebuild, so
no existing catalog and no follower startup can be broken by this.
Note that ALTER validates the **merged** property candidate, not only the
keys being changed: once
an operator narrows `jdbc_driver_secure_path`, any ALTER on a catalog
whose stored `driver_url`
falls outside the new allow-list is rejected (fail-closed) until the
`driver_url` itself is fixed
in the same statement. The catalog keeps working for queries and across
restarts either way.
- Does this need documentation?
- [ ] No.
--
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]
