Repository navigation
[feature](fe) Gate catalog SSRF endpoints and driver_url via a connector security surface - #69588
Merged
Merged
Conversation
morningman
requested review from
924060929,
CalvinKirs and
gavinchou
as code owners
October 25, 2026 14:36
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
morningman
force-pushed
the
catalog-ssrf-gate
branch
from
October 25, 2026 15:41
7ba7432 to
b3a8671
Compare
morningman
changed the title
morningman
force-pushed
the
catalog-ssrf-gate
branch
from
October 25, 2026 23:30
b3a8671 to
37a8d9f
Compare
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Request changes for two verified validation defects: degraded catalogs can persist a driver URL without the mandatory/operator gate, and valid HMS URI suffixes can skip the configured endpoint probe. Both findings are inline. There were no existing inline threads to carry forward and no additional user-provided focus.
Reviewed the complete supplied diff at 37a8d9f and its upstream/downstream consumers. Review converged after two rounds; all candidates and risk focuses have explicit dispositions.
The invocation chains checked were:
CREATE -> provider construction -> preCreateValidation -> endpoint probe -> optional connectivity test -> property validation -> register/journal
ALTER -> detached candidate validation/gates -> lock and compare snapshot -> apply/journal -> lazy connector rebuild
REPLAY -> restore/apply properties -> lazy rebuild (interactive gates skipped)
Critical checkpoint conclusions:
- Goal and proof: provider-owned gate metadata and interactive endpoint probing are implemented, but the two inline cases defeat the intended validation. Added unit tests exercise synthetic branch behavior rather than these end-to-end cases.
- Scope and abstraction: the provider/SPI refactor is focused and avoids connector property tables in the engine. Missing-provider policy metadata must survive the abstraction change.
- Concurrency and locking: concurrent ALTER uses detached snapshots and rechecks under CatalogMgr's write lock. Normal probes run before that lock; no new lock-order/deadlock defect found. The volatile lazy singleton is safely published.
- Lifecycle/static initialization: sockets close with try-with-resources and hooks stop after probe I/O. Provider absence followed by restoration is the material lifecycle defect. No cross-TU initialization or ownership-cycle change.
- Configuration: no new settings; existing driver policy is read at validation, and checker selection retains its previous first-access singleton behavior. No new dynamic-update regression found.
- Compatibility: SPI major 20.0 -> 21.0, recorded plugin methods, inherited manifest version, and parent-first SPI identity agree. Old-major plugins require a matching rebuild; missing providers remain a supported degraded-catalog state. No storage-format change.
- Thrift/protobuf: no changes; field-ID and rolling wire compatibility checks are not applicable.
- Parallel paths: driver aliases/flavors preserve the old table when providers exist. HMS aliases, HA lists, CREATE with test_connection=false, ALTER, and replay were traced. Endpoint declarations currently cover type=hms only; Iceberg/Paimon HMS flavors and other connectors remain outside this new probe, as their explicit empty-endpoint tests indicate.
- Conditional checks/errors: disabled, untouched-key, dead-flavor, and plain connectivity-failure behavior is intentional. Hook initialization failure and policy rejection surface as DdlException. Parser-null and missing-provider skips account for the findings.
- Tests: reviewed all added/changed unit tests. Missing-provider tests now override securitySurface, masking the production lookup failure; URI tests omit query/fragment/userinfo. Operator SDK integration is absent. No builds or tests were run, per the static-only contract.
- Test results: no regression output files changed; no executed results are claimed.
- Observability: endpoint errors contain host/port, initialization failures retain causes, and ordinary probe connectivity failures use debug logging. No additional DDL-frequency metric requirement found.
- Persistence/failover: no new durable fields. Replay correctly avoids interactive network checks, but that also leaves newly accepted driver URLs unchecked after restoration (first finding).
- Writes/atomicity/crashes: detached validation does not publish tentative properties; existing apply/journal/reset flow remains. No row-data writes, tablet visibility, delete bitmaps, or data-transaction behavior changes.
- FE/BE propagation: no new transmitted variables or send paths.
- Performance: local declaration lookup is small and off query hot paths. Probes are sequential with a five-second TCP connect timeout per endpoint; DNS resolution and total HA-list duration are outside that single-connect bound. No normal global catalog lock is held during probes.
- Core invariants: BE memory accounting, committed-data visibility, partition versions, and MoW consistency are unaffected. Error-means-failure has the two reported validation exceptions.
- Further recheck: SDK reflection, exception shape, cleanup, existing HTTP wrappers, provider routing, aliases, lazy driver loading, and replay were revisited; no additional verified defect was found.
The public SDK API and rejection type support the overload/exception assumptions; the operator-specific binary was not available for runtime verification. The endpoint check remains point-in-time, with the documented DNS-rebinding/native-client limits. These are implementation findings about the opt-in policy and existing driver validation, not a claim that default external-catalog SSRF is a supported security guarantee. Static review coverage of changed files is complete; no unresolved review candidates remain.
Review converged after 2 round(s).
| ConnectorSecuritySurface securitySurface(Map<String, String> properties) { | ||
| return ConnectorFactory.findProvider(getType(), properties) | ||
| .map(ConnectorProvider::securitySurface) | ||
| .orElse(ConnectorSecuritySurface.EMPTY); |
Contributor
There was a problem hiding this comment.
[P1] Preserve driver validation when the provider is unavailable
A missing or rejected plugin does not remove its persisted catalogs: CatalogFactory deliberately registers them in degraded mode during replay. Here that state now returns EMPTY, so ALTER CATALOG can journal a new driver_url without either JdbcDriverUrlSecurity.check or the operator allowlist (provider-side validation also no-ops). For example, with jdbc_driver_secure_path restricted to a local directory, an existing JDBC catalog can be changed to an arbitrary HTTP jar while its provider is absent. After the matching plugin is restored, lazy reconstruction calls JdbcCatalogProperties.of(), not the statement-only rules or preCreateValidation; JdbcDorisConnector.resolveDriverUrl returns that HTTP URL unchanged and JdbcConnectorClient hands it to URLClassLoader. Thus the skipped check is never recovered. The former type table explicitly prevented this case, and the new test override fabricates a surface even for the test that claims to model an absent plugin. Preserve the security metadata or reject sensitive edits until the provider is available; test the real lookup with an empty manager.
| return null; | ||
| } | ||
| return hostAndPort; | ||
| } catch (IllegalArgumentException e) { |
Contributor
There was a problem hiding this comment.
[P2] Parse the same URI destination that the HMS client uses
With a configured checker, hive.metastore.uris=thrift://127.0.0.1:9083?ignored skips the new gate: only a slash-delimited path is stripped, so HostAndPort receives port text 9083?ignored, throws, and this catch returns null. checkEndpointsAgainstSsrf then continues without probing anything. The value is valid for the actual HiveMetaStoreClient: resolveUris parses java.net.URI and preserves its query/fragment, while open connects using only store.getHost()/getPort(), i.e. 127.0.0.1:9083. A #fragment suffix has the same effect. Consequently ALTER accepts the destination the checker would reject without the suffix; CREATE does too with test_connection=false. Extract host/port using URI semantics for schemed values and add query/fragment/userinfo cases so valid client destinations cannot silently escape the probe.
Contributor
TPC-H: Total hot run time: 28296 ms |
Contributor
TPC-DS: Total hot run time: 152557 ms |
Contributor
ClickBench: Total hot run time: 24.04 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
morningman
added a commit
to morningman/doris
that referenced
this pull request
Oct 26, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#69588 Problem Summary: ALTER CATALOG could persist an unchecked driver_url while its connector provider was unavailable. Restoring the plugin then consumed the URL without statement-time validation. Reject interactive property updates without a provider, while keeping degraded catalog replay unchanged. Also parse schemed endpoints with URI host/port semantics so query, fragment, and userinfo cannot hide an HMS destination from the configured SSRF probe. Exercise production provider lookup and the real degraded replay path in the gate tests. ### Release note ALTER CATALOG property updates require an available connector provider. With an SSRF checker configured, HMS URI query, fragment, and userinfo components no longer bypass destination checks. Replay and lazy rebuild remain unchanged. ### Check List (For Author) - Test: Unit Test - 88 selected tests passed via run-fe-ut.sh; Maven validate with Checkstyle and architecture gates; git diff --check - Behavior changed: Yes - reject property ALTER without a provider and probe the actual destination of schemed HMS URIs - Does this need documentation: No
morrySnow
previously approved these changes
Oct 26, 2026
morningman
added a commit
to morningman/doris
that referenced
this pull request
Oct 26, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#69588 Problem Summary: ALTER CATALOG could persist an unchecked driver_url while its connector provider was unavailable. Restoring the plugin then consumed the URL without statement-time validation. Reject interactive property updates without a provider, while keeping degraded catalog replay unchanged. Also parse schemed endpoints with URI host/port semantics so query, fragment, and userinfo cannot hide an HMS destination from the configured SSRF probe. Exercise production provider lookup and the real degraded replay path in the gate tests. ### Release note ALTER CATALOG property updates require an available connector provider. With an SSRF checker configured, HMS URI query, fragment, and userinfo components no longer bypass destination checks. Replay and lazy rebuild remain unchanged. ### Check List (For Author) - Test: Unit Test - 88 selected tests passed via run-fe-ut.sh; Maven validate with Checkstyle and architecture gates; git diff --check - Behavior changed: Yes - reject property ALTER without a provider and probe the actual destination of schemed HMS URIs - Does this need documentation: No
morningman
force-pushed
the
catalog-ssrf-gate
branch
from
October 26, 2026 07:09
4c6fa20 to
d3b3311
Compare
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Two new P2 validation defects are verified: the configured HMS gate misses the modern metastore URI key and Hadoop-expanded destinations/dependency updates. Both are inline and independently traced from submitted properties through the actual client. The earlier unavailable-provider P1 (4372968032) and URI-suffix P2 (4372968038) are fixed at this head; neither is reposted or carried as an existing blocking finding. No additional user-provided focus was supplied.
Reviewed the complete supplied aggregate diff and all changed paths at d3b3311. The invocation chains checked were:
CREATE -> provider construction -> preCreateValidation -> endpoint probe -> optional connectivity test -> property validation -> register/journal
ALTER -> detached candidate validation/gates -> lock and compare snapshot -> apply/journal -> reset -> lazy connector rebuild
REPLAY -> restore/apply properties -> lazy rebuild (interactive policy gates skipped)
HMS -> typed/canonical properties + raw passthrough -> HiveConf -> MetastoreConf effective URI -> client socket
Critical checkpoint conclusions:
- Goal and proof: provider-owned policy declarations and a configured point-in-time HMS probe are implemented, but the two inline cases defeat the intended effective-destination check. Added tests cover literal endpoints and control-flow branches, not these effective configuration cases.
- Scope and reuse: the refactor is focused and reuses the mandatory JDBC rule/operator resolver. The engine no longer owns a connector-specific driver key table. No unrelated source changes were found.
- Concurrency and locking: SQL ALTER validates a detached snapshot outside CatalogMgr's global write lock, rechecks catalog identity/properties under the lock, and skips duplicate validation on publication. CREATE validates before registration locking. The volatile, synchronized lazy checker is safely published; no new lock-order defect was found.
- Lifecycle/static initialization: missing providers now fail interactive property updates before publication while replay can restore degraded catalogs. Socket ownership is exception-safe, and the local probe stops its hook after successful installation. Exact SDK hook installation/recovery remains the qualification below. No C++ static initialization or ownership-cycle change applies.
- Configuration: no setting is added. Existing driver policy is read during validation; checker selection retains its pre-existing first-access singleton behavior. No new dynamic-update regression was established.
- Compatibility: connector API 21.0 -> 22.0, recorded plugin signatures, filtered resource and inherited manifest version agree. Old-major plugins require rebuilding. ConnectorMetadata methods are unchanged, so its separate baseline has no content delta. No storage-format change.
- Thrift/protobuf: no gensrc definitions changed; field-ID and rolling wire compatibility checks are not applicable.
- Parallel paths: JDBC/Iceberg/Paimon driver spellings and flavor transitions retain the prior policy table. HMS literal aliases, HA lists, IPv6, CREATE with test_connection=false, ALTER and replay were traced. The new endpoint rollout declares the HMS connector only; other connectors' HMS/REST endpoints remain outside it, as their explicit empty-endpoint tests indicate.
- Conditions and errors: disabled/untouched/dead-flavor skips and tolerated connectivity failures are intentional. Hook initialization failure and a socket SecurityException become DdlException. Missing-provider fail-closed behavior fixes the prior P1. Effective-input and dependency skips cause the new findings.
- Test coverage: all added/changed tests were inspected. New production-lookup degraded-catalog tests and URI suffix/userinfo/IPv6 tests cover the earlier findings. Modern-key and interpolation/dependency cases are missing, and fakes do not establish deployed SDK interception. No builds, tests or runtime probes were performed, per the static-only contract.
- Test results: no regression output files changed; no author-reported execution result is claimed as independently reproduced.
- Observability: endpoint policy errors include host/port, provider errors include catalog/type, reflection/cleanup failures are logged, and ignored connectivity failures use debug logging. No additional DDL-frequency metric requirement was established.
- Persistence/failover: no durable field or log format changes. Interactive gates precede existing property publication/journaling; replay deliberately skips statement checks. No new transaction/failover state transition was introduced.
- Writes/atomicity/crashes: detached validation and snapshot fencing preserve catalog-property atomicity. No row-data writes, tablet visibility, delete bitmap or transaction protocol changes occur.
- FE/BE propagation: no new transmitted variable or send path.
- Performance: provider declarations are small DDL-only lookups. Probes are sequential with a five-second TCP-connect timeout per endpoint; DNS time and total HA-list duration are outside that bound. Normal probes do not hold the global catalog lock.
- Core invariants/further sweep: BE memory accounting, committed-data visibility, partition versions and MoW consistency are unaffected. Rechecked provider restoration, aliases, effective configuration, all changed tests, hook callers/nesting, class identity, version gating, cleanup and replay; no additional verified code defect was found.
Review did not converge after three rounds. Source review of the changed paths is complete, but R3/COM-U1 remains unverified: the exact operator-supplied SDK artifact/version and its JDK 17 TCP interception, registration-failure recovery and pooled-thread cleanup are not supplied. Public Alibaba seckit sources corroborate the API/exception assumptions but do not prove equivalence to the configured Aliyun binary. This uncertainty is excluded from inline findings and is not asserted to be a Doris defect. Runtime verification was prohibited by the review contract.
The gate remains point-in-time, with the documented later-DNS/runtime/native-client limits. The two findings concern the opt-in validation feature; no blanket default-catalog SSRF guarantee is claimed. All verified findings are delivered despite the explicit remaining SDK integration limit.
Review did not converge after 3 rounds. This automated review cannot pass the review check.
R3/COM-U1 remains unverified: the exact operator-supplied SDK artifact/version and its JDK 17 TCP-hook interception, registration-failure recovery and pooled-thread cleanup cannot be established from the supplied artifacts; public seckit source is corroboration only. All changed source paths and verified findings were reviewed, but this integration risk prevents convergence after the three-round cap.
| public ConnectorSecuritySurface securitySurface() { | ||
| // hive.metastore.uris (documented alias: uri) is the user-supplied thrift endpoint the FE connects | ||
| // to; the engine's SSRF gate rejects an intranet target there at CREATE / ALTER CATALOG. | ||
| return ConnectorSecuritySurface.ofSsrfEndpoints(HiveCatalogProperties.HIVE_METASTORE_URIS, "uri"); |
Contributor
There was a problem hiding this comment.
[P2] Include the modern HMS URI key in the declared surface
With a checker configured, an existing HMS catalog can execute ALTER CATALOG ... SET PROPERTIES("metastore.thrift.uris"="thrift://127.0.0.1:9083") without any probe: neither declared key is touched. HiveConnector.buildHmsClientConfig preserves this property, HmsConfHelper copies it into HiveConf, and the actual HiveMetaStoreClient.resolveUris reads MetastoreConf.THRIFT_URIS. Hive 3.1.3 gives metastore.thrift.uris precedence over hive.metastore.uris, so the next connector rebuild connects to loopback even though the stored legacy URI is public. CREATE with both values and test_connection=false likewise checks only the public legacy value. Include the modern key in endpoint selection and ALTER triggers, and cover modern-key-only updates. This is a missing effective input key, separate from the resolved query/fragment parsing issue.
Contributor
Author
There was a problem hiding this comment.
Fixed in a36c8b1. HiveConnectorProvider.securitySurface() now declares metastore.thrift.uris alongside hive.metastore.uris and uri (constant HiveCatalogProperties.METASTORE_THRIFT_URIS). An ALTER that touches only the modern key now triggers the probe, and once any endpoint key is touched the gate probes the values of all three keys, which covers whichever one MetastoreConf.getVar returns. A CREATE that carries both keys with test_connection=false therefore probes both values.
The declaration is pinned by HiveConnectorProviderSecuritySurfaceTest, which fails against the previous provider; PluginDrivenExternalCatalogSsrfGateTest adds alterTouchingOnlyModernMetastoreKeyIsProbed and createProbesModernMetastoreKeyBesideLegacyKey. I kept the raw passthrough rather than having the connector override metastore.thrift.uris, because a hive.conf.resources hive-site.xml may set it legitimately. The ofSsrfEndpoints javadoc now requires a connector to declare every key its client may read the destination from.
| } | ||
| // A value may list several comma-separated endpoints (e.g. HA hive.metastore.uris). | ||
| for (String endpoint : rawValue.split(",")) { | ||
| HostAndPort hostAndPort = parseEndpoint(endpoint.trim()); |
Contributor
There was a problem hiding this comment.
[P2] Validate the expanded HMS destination and its dependencies
The client resolves Hadoop configuration variables before parsing its URI, while this loop parses the raw catalog text. For example, hive.metastore.uris="thrift://${hms.target}:9083", hms.target="127.0.0.1", and test_connection=false pass the holder's nonblank check; parseEndpoint rejects the unexpanded braces and this loop silently skips the probe. HiveConnector and HmsConfHelper preserve both properties, so Hadoop 3.4.2 Configuration.get expands the reference before HiveMetaStoreClient.resolveUris connects to loopback. An ALTER changing only hms.target also repoints an existing templated catalog while the touched-key guard skips validation. Resolve the effective endpoint through the connector's configuration layer and account for dependency changes, or reject endpoint interpolation during interactive validation. This configuration-expansion case persists with every URI alias declared and is distinct from the resolved URI-suffix issue.
Contributor
Author
There was a problem hiding this comment.
Fixed in a36c8b1 with the second option. While a checker is configured, checkEndpointsAgainstSsrf now rejects any declared endpoint value that contains ${. The check runs before the touched-key shortcut, so it covers CREATE, an ALTER that writes a placeholder, and an ALTER that changes only the referenced property (hms.target in your example); a catalog stored with a placeholder has to be given a literal endpoint before any other property change. It also covers the bare form ${x}:9083, which HostAndPort would have parsed as a host literally named ${x} whose DNS failure the probe ignores, and ${env.X} / system-property references.
I did not resolve through the connector's configuration layer: that needs a new SPI hook that builds a HiveConf at DDL time (plugin loader, hive.conf.resources, environment, system properties) and diffs the old and new effective endpoints, while no real HMS URI needs a placeholder. Tests: createRejectsPlaceholderEndpointWithConnectionTestDisabled (4 forms), alterRejectsPlaceholderEndpointWithoutProbing, alterOfThePropertyAStoredPlaceholderRefersToIsRejected and alterAcceptsPlaceholderWhenNoCheckerInstalled; against the previous implementation the suite fails 7 of 35 (every placeholder case).
Contributor
FE UT Coverage ReportIncrement line coverage |
…tor security surface
### What problem does this PR solve?
Problem Summary:
External catalogs register user-supplied network endpoints (e.g. an HMS catalog's
`hive.metastore.uris`) that the FE connects to. When the operator installed the SSRF
security SDK (the `security_checker_class_name` config, i.e. Aliyun
`com.aliyun.securitysdk.SecurityUtil`), those endpoints were not checked against the SSRF
policy at CREATE / ALTER CATALOG, so a user with CREATE_PRIV (no SUPER) could point a
catalog at an intranet address.
This PR adds one replay-safe gate that, when an SSRF checker is installed, brackets a
lightweight TCP connect to each user-supplied endpoint with the SDK's net-hook so an
intranet target is rejected before the catalog is accepted. The judgment is delegated
entirely to the SDK. With no checker installed (the default open-source build) the gate is
a no-op and behavior is unchanged.
Connector-agnostic by construction. The engine must not hard-code any catalog type's
property names (the fe-connector invariant: no source-name branching in shared fe-core
nodes). So a connector now declares its security-sensitive property keys through a new SPI
value type, `ConnectorSecuritySurface` (returned by `ConnectorProvider.securitySurface()`),
and the engine looks it up by type -- without initializing a connector -- to drive the gate.
The HMS connector declares `hive.metastore.uris` / `uri`.
The same change unifies the pre-existing jdbc driver-url operator gate
(`jdbc_driver_secure_path` / `jdbc_driver_url_white_list`) onto this mechanism: the
`driver_url` keys and their flavor conditions now come from each jdbc-flavored connector's
`securitySurface()`, and the two fe-core key tables that previously hard-coded catalog types
(`driverUrlKeysOf`, `ssrfEndpointKeysOf`) are deleted. The gates stay exactly where they were
-- CREATE through `checkWhenCreating`, ALTER through `validatePropertiesBeforeUpdate`, never on
replay -- and the driver-url gate's mandatory rule (`JdbcDriverUrlSecurity.check`) still runs
even under `jdbc_driver_secure_path=*`. Sourcing the keys from the provider does not weaken the
gate: when no provider backs a type the catalog cannot initialize and thus cannot load a jar or
open a connection, so there is nothing to police.
This PR also fixes a latent bug in the existing SSRF wiring: `UrlSecurityChecker` bound the
SDK's stop method as `stopSSRFNetHookChecking(String)` but invoked it with no argument, so the
net-hook was never removed and leaked onto the pooled FE thread after any bracketed request. It
now binds and invokes the no-arg `stopSSRFNetHookChecking()`, and adds the no-arg
`startSSRFNetHookChecking()` (TCP form) needed to hook non-http connections such as HMS thrift.
Because this changes the connector plugin SPI surface (a new type and a new provider method),
`connector.plugin.api.version` is bumped 22.0 -> 23.0 and `connector-plugin-surface.txt` is
regenerated in the same commit.
SSRF surface note (per repo policy): the probe is a new FE -> user-named-host outbound TCP
connect, performed only when the operator installed the SSRF SDK and only at CREATE / ALTER
CATALOG (actor: CREATE_PRIV or ADMIN for CREATE, ALTER privilege for ALTER). Its sole purpose is
to let the SDK reject an intranet target; a plain connectivity failure is ignored (not treated
as a rejection), so a catalog pointing at a currently-unreachable public endpoint can still be
created. Known limit: point-in-time check only -- it does not guard a later runtime connection
(DNS rebinding) or clients that bypass the JDK socket layer (e.g. Netty native epoll).
### Release note
None
### Check List (For Author)
- Test:
- Unit Test: SecurityCheckerTest (fe-common); PluginDrivenExternalCatalogSsrfGateTest and the migrated PluginDrivenExternalCatalogDriverUrlGateTest (fe-core); ConnectorPluginSurfaceTest and the four {Hive,Jdbc,Iceberg,Paimon}ConnectorProviderSecuritySurfaceTest (fe-connector). A full `mvn install` builds every module against the new SPI.
- Behavior changed: Yes -- a new CREATE/ALTER CATALOG SSRF gate, the jdbc driver-url gate migrated to a connector-declared security surface, and a fix to the SSRF net-hook stop binding. The SSRF gate is active only when `security_checker_class_name` is configured; the default open-source build is unchanged. Connector plugin API bumped 22.0 -> 23.0.
- Does this need documentation: No
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
### What problem does this PR solve? Issue Number: None Related PR: apache#69588 Problem Summary: ALTER CATALOG could persist an unchecked driver_url while its connector provider was unavailable. Restoring the plugin then consumed the URL without statement-time validation. Reject interactive property updates without a provider, while keeping degraded catalog replay unchanged. Also parse schemed endpoints with URI host/port semantics so query, fragment, and userinfo cannot hide an HMS destination from the configured SSRF probe. Exercise production provider lookup and the real degraded replay path in the gate tests. ### Release note ALTER CATALOG property updates require an available connector provider. With an SSRF checker configured, HMS URI query, fragment, and userinfo components no longer bypass destination checks. Replay and lazy rebuild remain unchanged. ### Check List (For Author) - Test: Unit Test - 88 selected tests passed via run-fe-ut.sh; Maven validate with Checkstyle and architecture gates; git diff --check - Behavior changed: Yes - reject property ALTER without a provider and probe the actual destination of schemed HMS URIs - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#69588 Problem Summary: With an SSRF checker configured, two inputs still let an HMS catalog connect to a destination the CREATE / ALTER CATALOG probe never judged. Hive 3's MetastoreConf.getVar reads metastore.thrift.uris before hive.metastore.uris, and the HMS connector passes every raw catalog property through to HiveConf. The HMS security surface declared only hive.metastore.uris and uri, so ALTER CATALOG ... SET ("metastore.thrift.uris" = "thrift://127.0.0.1:9083") was never probed and the rebuilt client connected to loopback. CREATE with a public legacy URI, a loopback modern URI and test_connection=false passed the same way. The HMS connector now declares metastore.thrift.uris as well. Hadoop's Configuration.get expands ${...} placeholders (other properties, ${env.X}, JVM system properties) before the HMS client parses the URI, while the gate parsed the raw text. thrift://${hms.target}:9083 failed URI parsing and was skipped, ${x}:9083 probed a host literally named ${x} whose DNS failure is ignored, and an ALTER of only the referenced property repointed a templated catalog without touching an endpoint key. While a checker is configured, the gate now refuses a declared endpoint value that contains ${ on every interactive CREATE / ALTER, before its touched-key shortcut, so a catalog stored with a placeholder must be given a literal endpoint before any other property change. Replay, and builds without a checker, are unchanged. ### Release note With an SSRF checker configured, CREATE / ALTER CATALOG also checks an HMS catalog's metastore.thrift.uris, and rejects an HMS endpoint property that contains a ${...} placeholder. ### Check List (For Author) - Test: Unit Test - 771 tests in 97 classes passed via run-fe-ut.sh: the full fe-connector-spi and fe-connector-hive suites, the four *ConnectorProviderSecuritySurfaceTest classes, the SSRF / driver-url gate and concurrency tests of PluginDrivenExternalCatalog, SecurityCheckerTest, PluginApiVersionWiringTest, HMSExternalCatalogParquetTimeZoneTest, CatalogFactoryPluginRoutingTest and CatalogMgrTest. Against the previous implementation the gate suite fails 7 of 35 (every placeholder case) and HiveConnectorProviderSecuritySurfaceTest fails; moving the placeholder check after the touched-key guard, or ahead of the checker guard, fails the test written for that case. Full FE build with Checkstyle (build.sh --fe). - Behavior changed: Yes - with an SSRF checker configured, metastore.thrift.uris is probed and a placeholder in an HMS endpoint property is rejected - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
morningman
force-pushed
the
catalog-ssrf-gate
branch
from
October 26, 2026 10:23
d3b3311 to
a36c8b1
Compare
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review of PR #69588 at a36c8b1f3f21a92e0101e365f04e7d3b5d1fb1b6. No new substantiated findings. Coverage includes all 24 changed files, the CREATE/ALTER/replay and driver-loading call chains, existing threads, and a separate effective-HMS-destination risk review. No additional user focus was supplied.
The four previous findings are fixed on this head: missing-provider ALTER now fails before accepting properties (4372968032); schemed endpoints use the HMS client's URI destination semantics (4372968038); the modern metastore.thrift.uris key is declared and participates in ALTER triggers (4375462196); and ${ endpoints are rejected before the touched-key shortcut, including changes only to a referenced property (4375462206). None remains an independently confirmed existing P0/P1 blocker; none is reposted.
| Required checkpoint | Conclusion |
|---|---|
| Goal and proving tests | Interactive CREATE/ALTER applies the configured checker to declared literal HMS endpoints; the JDBC operator gate retains its aliases and flavor conditions. Added unit cases cover these branches; they were inspected, not executed. |
| Scope and reuse | A shared endpoint-checking primitive and provider declarations replace engine-owned connector key tables without adding connector-specific branches to the new gate. |
| Concurrency and locking | The singleton uses volatile publication. Normal CREATE probes precede registration; ALTER validates a detached snapshot outside the global catalog lock, then checks identity/properties and retries on concurrent changes. |
| Lifecycle | Each real probe owns and closes its socket; successful hook installation is paired with stop in finally. Provider security lookup does not instantiate a connector. |
| Configuration | No new FE setting is introduced. The checker selection remains startup-scoped; existing driver policy settings and the disabled-checker behavior are preserved. |
| Compatibility | Connector API version is bumped to 23.0 and the provider/type surface baseline is updated. The metadata method surface is unchanged. Connector plugins require matching rebuilt artifacts. |
| Thrift/protobuf | No changes. |
| Parallel paths | CREATE, ALTER, HA endpoint lists, aliases, JDBC/Iceberg-JDBC/Paimon-JDBC flavor activation, missing providers, and lazy reconstruction were traced. Replay remains outside interactive checks. |
| Conditional behavior | Checker-enabled, touched-key and JDBC-flavor guards match their purpose. Placeholder rejection deliberately precedes touched-key filtering, while remaining disabled without a checker. |
| Test coverage | New/updated unit cases exercise hook success/rejection/I/O failure, disabled checking, URI suffixes and IPv6, HA lists, modern keys, placeholders/dependencies, and real empty-manager degraded-catalog lookup. Actual third-party SDK integration is outside these fake-driven tests. |
| Test results | No regression result files change. No build, unit test, regression test, live cluster, or network probe was run in this static-only review; author-reported runs were not independently reproduced. |
| Observability | Gate errors identify the endpoint or catalog/property; non-policy connectivity failures are intentionally ignored with debug logging. No new background operation requiring metrics is introduced. |
| Transactions and persistence | Validation precedes property publication and journaling. No persisted schema changes; replay can restore degraded catalogs without the new checks or probes. |
| Data writes and invariants | No row-data, visible-version, delete-bitmap, or BE allocation changes. ALTER's detached candidate avoids publishing rejected properties. Policy rejection and hook-start failure propagate as DDL failures. |
| FE/BE variable passing | No new transmitted variables or FE/BE protocol changes. |
| Performance | New network work stays in interactive DDL, outside normal catalog locks and metadata hot paths. Untouched literal endpoints skip probes; each TCP connect has a five-second timeout. |
| Other risks and scope | No additional reachable defect was established. Local Hadoop XML and URI resolver code require operator-controlled artifacts. DNS rebinding and connections outside the JDK socket layer remain the feature's stated point-in-time limits. |
The public SDK source exposes the no-argument hook start/stop APIs and its rejection exception extends SecurityException. That supports the reviewed binding and catch contract for that implementation, without proving compatibility with every operator-supplied legacy Aliyun binary or SDK JVM configuration.
Convergence: true after one complete review round. All three assigned reviewers completed their coverage and returned NO_NEW_VALUABLE_FINDINGS on this unchanged payload; the final sweep found no unresolved candidate or risk focus within the static-review scope.
Review converged after 1 round(s).
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 28861 ms |
Contributor
TPC-DS: Total hot run time: 151903 ms |
Contributor
ClickBench: Total hot run time: 24.44 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
gavinchou
approved these changes
Oct 26, 2026
CalvinKirs
approved these changes
Oct 26, 2026
5 of 16 tasks
morningman
added a commit
that referenced
this pull request
Nov 2, 2026
…hema (#69488) ### What problem does this PR solve? Issue Number: None Related PR: #66538, #57531, #65851 Problem Summary: **In short.** On an Iceberg table, `DEFAULT(column)` anywhere except `UPDATE ... SET` and `MERGE ... VALUES` reads the default from the read schema, which carries no Iceberg write-default. This PR resolves it against the write schema that the statement pinned for its target, as branch-4.1 and branch-4.2 do. **Background.** An Iceberg `write-default` applies to rows written after it is set. So it lives in the writer schema that a write statement pins (`IcebergWriteSchemaContext`), not in the columns of the read schema. #65851 resolves `DEFAULT(column)` against that pinned schema in `UPDATE ... SET`, `MERGE ... UPDATE SET` and `MERGE ... INSERT VALUES` (`ConnectorWriteSchemaUtils`). Every other `DEFAULT(column)` either fails at binding or reaches `RewriteDefaultExpression` (#57531), which reads the default from the bound column. branch-4.1 #66538, also in branch-4.2, resolves these against the pinned write target too. It rejects `DEFAULT(column)` on an Iceberg column in a statement that does not write the table. **The problem, and what it cost.** Reproduced on a local cluster with a `file://` format-version 3 table: `CREATE TABLE t (id INT, c INT)`, `ALTER TABLE t ADD COLUMN d INT DEFAULT 42`, one row `(0, 0)`, so `d = 42`. | Statement | Master today | After this PR (same as branch-4.2) | |---|---|---| | `INSERT INTO t (id, c, d) VALUES (1, 1, DEFAULT(d))` | `Unknown column 'd' in 'table list'` | writes 42 | | `INSERT INTO t (id, c, d) SELECT 2, 2, DEFAULT(d) FROM t WHERE id = 0` | writes NULL | writes 42 | | `INSERT INTO t (id, c, d) SELECT 3, 3, DEFAULT(d)` | `Unknown column 'd'` | writes 42 | | `UPDATE t SET c = 100 WHERE d = DEFAULT(d)` | updates nothing: `d = NULL` makes the plan an empty set | updates every row, `d = 42` is pushed down to the scan | | `MERGE INTO t USING (SELECT 0 AS id) s ON t.id = s.id AND t.d = DEFAULT(t.d) WHEN MATCHED THEN UPDATE SET c = 300` | matches nothing | updates row 0 | | `SELECT DEFAULT(d) FROM t` | NULL | `DEFAULT(d) must reference a column of the table that this statement writes` | The `UPDATE` and `MERGE` cases fail silently: the statement succeeds and changes nothing. **How this PR fixes it.** - fe-core cannot tell from a column whether its connector keeps defaults in the read schema. Hive does: an HMS default shows up in both schemas. Iceberg does not. The new connector capability `SUPPORTS_WRITE_ONLY_DEFAULTS` says which kind a table is. The Iceberg connector declares it. The Hive connector passes it on to iceberg-on-HMS tables, like the other per-table capabilities, but only through the two-argument `getTableSchema`. Since #69299, a statement that pins the table at its latest snapshot reads the schema from the snapshot overload, which reflects none of them, so on an iceberg-on-HMS table such a statement keeps master's results (for example, `UPDATE t SET c = 100 WHERE d = DEFAULT(d)` still changes no row). Reflecting the sibling's capabilities there changes planning for all of them and is left to a follow-up PR. - For a target that declares it, `InsertUtils.normalizePlan` resolves `DEFAULT(column)` in `INSERT ... VALUES` and in the select lists of `INSERT ... SELECT`, including the replacements of `SELECT * REPLACE(...)`, against the pinned write schema. It does this by name, before the source relations are bound, so the name always refers to a target column, as in branch-4.2. In MySQL, `DEFAULT(column)` in a `SELECT` refers to a source column instead. A derived table in `FROM` is a query of its own and is left alone: `DEFAULT(column)` in it still names a column of its own `FROM` clause. - `RewriteDefaultExpression` takes the default for a `DEFAULT(column)` bound to such a table from the pinned write column with the same field id. That covers the `WHERE` of `UPDATE` and the `ON` of `MERGE`. A MERGE resolves it whatever its WHEN branches, also when they all delete, as branch-4.2 does. If the statement pinned no write schema for that table, it fails. A `DELETE` pins the writer schema too, for its delete sink, but it writes no row values, so `RowLevelDmlCommand` marks its target and `DEFAULT(column)` on it fails with `DEFAULT(d) cannot be used in a DELETE: the defaults of this table apply only to the rows that a statement writes`, as in branch-4.2, whose DELETE installs no write schema context. Master reads NULL there, so `DELETE FROM t WHERE d = DEFAULT(d)` deletes nothing; resolved to the write-default it would delete every row with `d = 42`. - A resolved default is cast to the type of its writer column before the expression around it is bound, as `RewriteDefaultExpression` already does for a bound `DEFAULT(column)`. The parsed literal `-1` is a TINYINT, so without the cast `bit_count(DEFAULT(c))` on an INT column would count 8 bits instead of 32. The external `UPDATE`, `MERGE` and changelog paths use the same helper. `RewriteDefaultExpression` rejects a bound `DEFAULT(column)` whose writer column has another type than the read column the statement was bound with, which happens only when the table changed outside Doris after Doris cached it, and asks to refresh the table: the expressions around it were bound against the read type. - Hive, Paimon and internal tables do not declare the capability, so they behave as before. - The new enum constant changes the connector plugin API surface, so `connector.plugin.api.version` goes from 26.0 to 27.0, as for the other recent changes to that surface (#69299, #69588, #69298, #69683 and #69835 took 22.0 to 26.0). **Where `DEFAULT(column)` resolves on an Iceberg target (direct Iceberg catalogs).** | Statement | Position of `DEFAULT(column)` | Result | branch-4.2 | |---|---|---|---| | `INSERT` / `INSERT OVERWRITE` | `VALUES` rows | the target column's write-default, typed as the column | same | | | the INSERT's own select lists (with or without `FROM`, set operation branches, `SELECT * REPLACE`) | the target column's write-default, by name, before binding | same, except `* REPLACE`, which branch-4.2 binds to the source | | | `GROUP BY` / `ROLLUP` outputs, a derived table in `FROM`, `WITH` definitions, subqueries, `WHERE` / `ON` | bound like any `DEFAULT(column)` (last row) | same, except derived tables, which branch-4.2 resolves against the target | | | a `VALUES` branch of a set operation | fails with an unknown column, as on master | same | | | a qualifier other than the target (`DEFAULT(src.c)`, `DEFAULT(ctl.db.t.c)`) in the positions above | fails: `column in assignment list is invalid` | same | | `UPDATE` | `SET` / `WHERE` | write-default | same | | `MERGE` | `ON` and `WHEN ... AND` conditions, whatever the WHEN branches, also when they all delete | write-default | same | | | `UPDATE SET` / `INSERT VALUES` | write-default | same | | `DELETE` (also `DELETE ... USING`) | anywhere | fails: `cannot be used in a DELETE` | fails: its DELETE has no write schema context | | any | a bound `DEFAULT(x.c)` | `x` is the written table: its write-default by field id; an OLAP or Hive table: that column's default; another Iceberg table: fails with `must reference a column of the table that this statement writes` | same | A bound `DEFAULT(column)` whose writer column has another type than the read column fails and asks to refresh the table. Where `DEFAULT(column)` is bound rather than resolved by name (the third row), it keeps its meaning from before this PR and names a column of its own `FROM` clause, as on master and branch-4.2: for example in `INSERT INTO t SELECT id, DEFAULT(d) FROM src GROUP BY id, d` with an OLAP or Hive `src`, it takes the default of `src.d`. **Results.** - Local cluster, with the table above: the table shows what each statement did before and after. `EXPLAIN UPDATE t SET c = 5 WHERE d = DEFAULT(d)` goes from an empty set to a scan with `d = 42` pushed down to Iceberg. - Unit tests: 6 new tests in `InsertUtilsTest`, 6 in the new `RewriteDefaultExpressionTest` and 1 in `RowLevelDmlCommandTest`. They cover VALUES, and INSERT ... SELECT both from a staging relation whose own column is also `name` and without FROM: each resolves to the pinned default, and an unknown column fails. `DEFAULT(column)` inside a derived table, qualified or not and also under a UNION branch, is left to binding. A `SELECT * REPLACE(DEFAULT(name) AS name)` replacement resolves like the rest of the select list and keeps the star's qualifier and `EXCEPT` list. The pinned write column wins over the read column. A statement that writes nothing fails. A table without the capability is unchanged. With master's `InsertUtils` and `RewriteDefaultExpression`, the 5 tests of the fix fail and the 2 tests of the unchanged path pass. The `SELECT * REPLACE` test fails with this PR's previous `InsertUtils`, where the replacement stays `DEFAULT(name)`. `IcebergConnectorTest`, `HiveConnectorMetadataSiblingDelegationTest` and `ConnectorPluginSurfaceTest` pass. So do the fe-core tests around the write schema: `ConnectorWriteSchemaUtilsTest`, `ExternalRowLevelUpdatePlanBuilderTest`, `ExternalRowLevelMergePlanBuilderTest`, `ChangelogRowLevelDmlTransformTest` and `PluginDrivenExternalTableTest`. Two of the tests pin the typing: `bit_count(DEFAULT(bits))` gets the default as an INT in VALUES and in SELECT, and fails without the cast; a BIGINT writer column over an INT read column is rejected, directly and under an IF bound to INT. `DEFAULT(column)` on the target of a DELETE fails even with a pinned write-default, and only a DELETE marks its target (an UPDATE does not); skipping the check, or marking UPDATE instead of DELETE, makes exactly these two tests fail. `PositionDeleteRowLevelDmlTransformTest`, `PhysicalPlanTranslatorIcebergRowLevelDmlTest`, `DeleteFromCommandTest` and `UpdateCommandTest` pass. - Regression: `test_iceberg_initial_defaults` now writes rows 20 to 22 with `DEFAULT(default_int)` in VALUES and in INSERT ... SELECT with and without FROM. It then updates them with `WHERE default_int = DEFAULT(default_int)` and `MERGE ... ON ... = DEFAULT(...)`, for Parquet and ORC, and checks the rows in the `.out`. A plain `SELECT DEFAULT(default_int)` must fail, and so must a `DELETE ... WHERE default_int = DEFAULT(default_int)` on rows 20 to 22, which hold the write-default. The suite passed locally before the DELETE block was added, and the whole suite, DELETE block included, has since passed in two External regression runs on CI. With master's FE it fails at the first of these statements: `Unknown column 'default_int' in 'table list'`.
yiguolei
pushed a commit
that referenced
this pull request
Nov 4, 2026
… ALTER CATALOG #69588 (#70091) ### What problem does this PR solve? Issue Number: None Related PR: #69588 (Same backport for branch-4.1: #70092) Problem Summary: Backport of #69588 to branch-4.2. The master change cannot be cherry-picked: it is built on the fe-connector plugin SPI (`ConnectorProvider`, `ConnectorSecuritySurface`, `PluginDrivenExternalCatalog`), and none of those exist on 4.2. This PR ports the security fix itself onto the 4.2 catalog classes, with the same rules and the same error messages. **In short** With the SSRF security SDK configured (`security_checker_class_name`), CREATE / ALTER CATALOG on 4.2 never checked an HMS catalog's metastore endpoint, so a user with CREATE_PRIV on a catalog (no ADMIN) could make the FE's HMS client connect to an intranet address. `HMSExternalCatalog` now runs the same CREATE / ALTER gate master got in #69588 (on ALTER, before `CatalogMgr` takes its write lock), and the SDK binding no longer leaks its net-hook onto pooled FE threads. Without a checker configured (the open-source default), the new gate does nothing. **Background** - `security_checker_class_name` names a third-party SDK (Aliyun `com.aliyun.securitysdk.SecurityUtil`), loaded by reflection through `SecurityChecker` / `UrlSecurityChecker` in fe-common. Its net-hook sits at the JDK socket layer: install it on the current thread, connect, and the connect throws `SSRFUnsafeConnectionException` (a `SecurityException`) if the resolved address is intranet. The SDK has no "is this host intranet" string API, so it can only judge a real connect. Today only HTTP callers use it (HTTP TVF, S3, ES, small-file download, stage creation). - An HMS catalog takes its endpoint from `hive.metastore.uris` or its alias `uri`. `HMSBaseProperties` sets that value, plus every other `hive.`-prefixed catalog property, into the `HiveConf` of the FE's HMS client. `HiveMetaStoreClient` reads the endpoint through `MetastoreConf.getVar`, which looks at `metastore.thrift.uris` first, and through Hadoop's `Configuration.get`, which expands `${...}` placeholders (other properties, `${env.X}`, JVM system properties). **1. The problem, and what it cost** - CREATE CATALOG (CREATE_PRIV) and ALTER CATALOG (ALTER privilege on the catalog) accepted any metastore endpoint, so the FE opened a thrift connection to a host inside its own network on behalf of a non-admin user. - `UrlSecurityChecker` bound the SDK's stop method as `stopSSRFNetHookChecking(String)` but invoked it with no argument, so stop never ran and the net-hook stayed on the pooled FE thread after every bracketed HTTP request. - Only the http form of the SDK's start was bound, so nothing could hook a non-http connection such as HMS thrift. **2. How this PR fixes it** - fe-common, `SecurityChecker`: adds the no-arg `startSSRFChecking()` (the SDK's TCP form), `isEnabled()`, and `checkEndpointForSsrf(host, port)`, which brackets a single 5-second TCP connect with the net-hook. A `SecurityException` from the connect is the rejection; a refused / timed-out / unknown-host connect is ignored, so a public endpoint that is down right now can still be used. `UrlSecurityChecker` binds the SDK's no-arg start and no-arg stop; `DummySecurityChecker.isEnabled()` returns false. The singleton becomes lazily initialized with a test setter. - fe-core, `HMSExternalCatalog`: `checkWhenCreating()` (CREATE, before `test_connection`) and the new `validatePropertiesBeforeUpdateUnlocked()` (ALTER) call the new gate on `hive.metastore.uris`, `uri` and `metastore.thrift.uris`. The rules match master: no-op without a checker; a value containing `${` is rejected before anything else (so an ALTER of only the property a stored placeholder refers to is rejected as well); an ALTER probes only when it touches one of these keys; every entry of a comma-separated HA list is probed; schemed values are parsed with `java.net.URI` host/port semantics, so a query, fragment or userinfo part cannot hide the real destination. - fe-core, `CatalogMgr.alterCatalogProps`: on 4.2 ALTER CATALOG is validated under `CatalogMgr`'s write lock, and a probe to an unreachable endpoint can wait up to 5 seconds, so the probe must not run there. ALTER now takes a snapshot of the catalog's properties under the read lock, runs `ExternalCatalog.validatePropertiesBeforeUpdateUnlocked` (new, a no-op by default) on it without holding the lock, and then applies the change under the write lock only if the same catalog still has the snapshot's properties; if another DDL changed or dropped the catalog in between, it starts over from a new snapshot. The rest of the ALTER validation still runs under the lock, unchanged. CREATE already probes before `CatalogMgr` takes its lock. Where the backport differs from master: | Master (#69588) | This PR | Why | | --- | --- | --- | | Endpoint keys declared by the hive connector through `ConnectorSecuritySurface` | Keys are a constant in `HMSExternalCatalog` | 4.2 has no connector SPI; `HMSExternalCatalog` is already the HMS-specific class | | Gate in `PluginDrivenExternalCatalog` | Gate in `HMSExternalCatalog`, through `checkWhenCreating` (CREATE) and the new `validatePropertiesBeforeUpdateUnlocked` (ALTER) | Neither runs on replay: `CatalogFactory` calls `checkWhenCreating` only for a new catalog, and replay goes through `replayAlterCatalogProps`, not `alterCatalogProps` | | `driver_url` gate moved onto the security surface, `driverUrlKeysOf` deleted | Not ported | 4.2 checks `driver_url` in `JdbcExternalCatalog` / `IcebergExternalCatalog` / `PaimonExternalCatalog.checkProperties`; there is no key table to replace | | ALTER rejected while the connector provider is missing | Not ported | 4.2 has no connector providers | | `checkEndpointForSsrf` throws `DdlException` | Throws `Exception` with the same messages; the HMS gate rethrows it as `DdlException` | On 4.2 `DdlException` is in fe-core, which fe-common cannot reference | | Connector plugin API 22.0 -> 23.0 | No version change | No connector plugin API on 4.2 | | All ALTER validation runs before `CatalogMgr` takes its write lock (#68585) | Only the lock-free checks (the probe) run before the write lock, through `validatePropertiesBeforeUpdateUnlocked`, on a snapshot that is checked again under the lock; the rest of ALTER validation stays under the lock | #68585 is not on 4.2 and also changes HDFS HA validation; this keeps the change to the probe | | `metastore.thrift.uris` gated because the hive connector passes every property to `HiveConf` | Gated as well | `HMSBaseProperties` copies only `hive.`-prefixed properties today, so the key does not reach the client yet; gating it keeps the gate correct if that copy widens, and keeps one rule on both branches | **3. Results** With an SSRF checker configured: | Statement | Before | After | | --- | --- | --- | | `CREATE CATALOG ... ("type"="hms", "hive.metastore.uris"="thrift://10.0.0.1:9083")` | Accepted | Rejected: `Endpoint 10.0.0.1:9083 is rejected by the SSRF policy (intranet address not allowed): ...` | | Same, with `test_connection` = false | Accepted | Rejected (the gate does not depend on `test_connection`) | | `ALTER CATALOG c SET PROPERTIES ("hive.metastore.uris"="thrift://10.0.0.1:9083")` | Accepted | Rejected | | `thrift://user@10.0.0.1:9083?x#y`, or an intranet host second in an HA list | Accepted | Rejected | | `"hive.metastore.uris"="thrift://${hive.x}:9083"` with `"hive.x"="127.0.0.1"` | Accepted; the client connects to 127.0.0.1 | Rejected: `... must give its endpoint literally while an SSRF checker is configured ...` | | Public endpoint that is currently unreachable | Accepted | Accepted (a failed connect is not an SSRF signal) | | ALTER of an unrelated property | Accepted | Accepted, no probe | | ALTER that probes an unreachable public endpoint | No probe | Probes for up to 5 seconds without holding `CatalogMgr`'s lock | | Replay of CREATE / ALTER from the edit log | No probe | No probe | Without a checker configured, nothing changes. The only change outside the gate is that the SDK's net-hook is now removed after each bracketed HTTP request, as intended. **4. The classes, and how they call each other** - `SecurityChecker` (fe-common): the SSRF primitive `checkEndpointForSsrf`, plus `isEnabled()` and the TCP start. - `UrlSecurityChecker` / `DummySecurityChecker` (fe-common): SDK binding / no-op. - `HMSExternalCatalog` (fe-core): the endpoint keys, `checkEndpointsAgainstSsrf`, `parseEndpoint`, and the two hooks. - `ExternalCatalog` (fe-core): the new `validatePropertiesBeforeUpdateUnlocked` hook, a no-op by default. - `CatalogMgr` (fe-core): `alterCatalogProps` runs that hook on a snapshot before taking its write lock and checks the snapshot again under it. ``` CREATE CATALOG CatalogFactory.createFromCommand └ HMSExternalCatalog.checkWhenCreating ├ checkEndpointsAgainstSsrf (new) -> SecurityChecker.checkEndpointForSsrf └ super.checkWhenCreating (test_connection) ALTER CATALOG ... SET PROPERTIES CatalogMgr.alterCatalogProps ├ read lock: snapshot the catalog's properties ├ no lock: HMSExternalCatalog.validatePropertiesBeforeUpdateUnlocked (new) │ └ checkEndpointsAgainstSsrf (new) -> SecurityChecker.checkEndpointForSsrf └ write lock: same catalog, same properties? if not, start over └ alterExternalCatalogPropsFenced -> validatePropertiesBeforeUpdate -> checkProperties (unchanged), publish, journal SecurityChecker.checkEndpointForSsrf(host, port) ├ startSSRFChecking() -> SDK startSSRFNetHookChecking() ├ doConnect(host, port, 5s) -> SDK throws SecurityException for an intranet target └ stopSSRFChecking() -> SDK stopSSRFNetHookChecking() Replay: CatalogFactory.createFromLog and replayAlterCatalogProps(isReplay=true) call neither hook. ``` SSRF surface note (per repo policy): the probe is a new FE -> user-named-host outbound TCP connect, made only when the operator installed the SSRF SDK and only at CREATE / ALTER CATALOG (CREATE_PRIV or ADMIN for CREATE, ALTER privilege for ALTER). It exists only so the SDK can reject an intranet target. Known limits, same as master: it is a point-in-time check, so it does not guard a later runtime connection (DNS rebinding) or a client that bypasses the JDK socket layer. ### Release note When an SSRF checker is configured, CREATE / ALTER CATALOG checks HMS endpoint destinations (`hive.metastore.uris`, `uri`, and `metastore.thrift.uris`), including URIs containing query, fragment, or userinfo components, and rejects an HMS endpoint value that contains a `${...}` placeholder. ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [ ] Regression test - [x] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason <!-- Add your reason? --> - Unit Test: the new `SecurityCheckerTest` (6 tests) and `HMSExternalCatalogSsrfGateTest` (48), plus the existing `HMSExternalCatalogParquetTimeZoneTest`, `HttpTableValuedFunctionTest`, `CatalogMgrTest`, `CatalogMgrLanceGuardTest`, `ExternalCatalogTest`, `ExternalCatalogRuntimeStateTest`, `CatalogVarbinaryMigrationTest`, `HMSPropertiesTest`, `HiveTableSinkTest`, `JdbcExternalCatalogTest` and `PaimonExternalCatalogDriverUrlSecurityTest`: 134 tests in 13 classes passed with `TZ=Asia/Shanghai mvn test -pl fe-common,fe-core -am`; Checkstyle reports 0 violations in fe-common and fe-core. - `HMSExternalCatalogSsrfGateTest` also drives ALTER through a real `CatalogMgr`. It checks that the probe runs while the thread holds neither the read nor the write lock, that an intranet target leaves the catalog and the journal untouched, and that an ALTER whose catalog is altered or dropped during the probe checks again, or fails with `No catalog found`, instead of applying a stale check. - Mutation check: each of 21 mutants is killed by at least one new test. The mutants drop either hook call, the `isEnabled()` guard of the gate or of `checkEndpointForSsrf`, the touched-key guard, a declared key, the comma split or the URI parsing; run the placeholder rule only on touched keys or ahead of the `isEnabled()` guard; swallow the SDK's rejection or a failed hook install; treat a failed connect as a rejection; skip `stopSSRFChecking()` in `finally`; probe under `CatalogMgr`'s write lock (from `validatePropertiesBeforeUpdate`, or by calling the lock-free hook under the lock); or drop the snapshot comparison or the catalog identity check under the lock. - Checked on this branch that `HMSBaseProperties` hands `hive.metastore.uris` = `thrift://${hive.x}:9083` with `hive.x` = `127.0.0.1` to the HMS client as `thrift://127.0.0.1:9083`, and does not hand it a `metastore.thrift.uris` catalog property. - Behavior changed: - [ ] No. - [x] Yes. With an SSRF checker configured, CREATE / ALTER CATALOG probes HMS metastore endpoints and rejects intranet targets and `${...}` placeholders; the SDK net-hook is now removed after each bracketed request. Without a checker, nothing changes. - Does this need documentation? - [x] No. - [ ] Yes. <!-- Add document PR link here. eg: apache/doris-website#1214 --> ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into --> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
yiguolei
pushed a commit
that referenced
this pull request
Nov 4, 2026
… ALTER CATALOG #69588 (#70092) ### What problem does this PR solve? Issue Number: None Related PR: #69588 (Same backport for branch-4.2: #70091) Problem Summary: Backport of #69588 to branch-4.1. The master change cannot be cherry-picked: it is built on the fe-connector plugin SPI (`ConnectorProvider`, `ConnectorSecuritySurface`, `PluginDrivenExternalCatalog`), and none of those exist on 4.1. This PR ports the security fix itself onto the 4.1 catalog classes, with the same rules and the same error messages. **In short** With the SSRF security SDK configured (`security_checker_class_name`), CREATE / ALTER CATALOG on 4.1 never checked an HMS catalog's metastore endpoint, so a user with CREATE_PRIV on a catalog (no ADMIN) could make the FE's HMS client connect to an intranet address. `HMSExternalCatalog` now runs the same CREATE / ALTER gate master got in #69588 (on ALTER, before `CatalogMgr` takes its write lock), and the SDK binding no longer leaks its net-hook onto pooled FE threads. Without a checker configured (the open-source default), the new gate does nothing. **Background** - `security_checker_class_name` names a third-party SDK (Aliyun `com.aliyun.securitysdk.SecurityUtil`), loaded by reflection through `SecurityChecker` / `UrlSecurityChecker` in fe-common. Its net-hook sits at the JDK socket layer: install it on the current thread, connect, and the connect throws `SSRFUnsafeConnectionException` (a `SecurityException`) if the resolved address is intranet. The SDK has no "is this host intranet" string API, so it can only judge a real connect. Today only HTTP callers use it (HTTP TVF, S3, ES, small-file download, stage creation). - An HMS catalog takes its endpoint from `hive.metastore.uris` or its alias `uri`. `HMSBaseProperties` sets that value, plus every other `hive.`-prefixed catalog property, into the `HiveConf` of the FE's HMS client. `HiveMetaStoreClient` reads the endpoint through `MetastoreConf.getVar`, which looks at `metastore.thrift.uris` first, and through Hadoop's `Configuration.get`, which expands `${...}` placeholders (other properties, `${env.X}`, JVM system properties). **1. The problem, and what it cost** - CREATE CATALOG (CREATE_PRIV) and ALTER CATALOG (ALTER privilege on the catalog) accepted any metastore endpoint, so the FE opened a thrift connection to a host inside its own network on behalf of a non-admin user. - `UrlSecurityChecker` bound the SDK's stop method as `stopSSRFNetHookChecking(String)` but invoked it with no argument, so stop never ran and the net-hook stayed on the pooled FE thread after every bracketed HTTP request. - Only the http form of the SDK's start was bound, so nothing could hook a non-http connection such as HMS thrift. **2. How this PR fixes it** - fe-common, `SecurityChecker`: adds the no-arg `startSSRFChecking()` (the SDK's TCP form), `isEnabled()`, and `checkEndpointForSsrf(host, port)`, which brackets a single 5-second TCP connect with the net-hook. A `SecurityException` from the connect is the rejection; a refused / timed-out / unknown-host connect is ignored, so a public endpoint that is down right now can still be used. `UrlSecurityChecker` binds the SDK's no-arg start and no-arg stop; `DummySecurityChecker.isEnabled()` returns false. The singleton becomes lazily initialized with a test setter. - fe-core, `HMSExternalCatalog`: `checkWhenCreating()` (CREATE, before `test_connection`) and the new `validatePropertiesBeforeUpdateUnlocked()` (ALTER) call the new gate on `hive.metastore.uris`, `uri` and `metastore.thrift.uris`. The rules match master: no-op without a checker; a value containing `${` is rejected before anything else (so an ALTER of only the property a stored placeholder refers to is rejected as well); an ALTER probes only when it touches one of these keys; every entry of a comma-separated HA list is probed; schemed values are parsed with `java.net.URI` host/port semantics, so a query, fragment or userinfo part cannot hide the real destination. - fe-core, `CatalogMgr.alterCatalogProps`: on 4.1 ALTER CATALOG is validated under `CatalogMgr`'s write lock, and a probe to an unreachable endpoint can wait up to 5 seconds, so the probe must not run there. ALTER now takes a snapshot of the catalog's properties under the read lock, runs `ExternalCatalog.validatePropertiesBeforeUpdateUnlocked` (new, a no-op by default) on it without holding the lock, and then applies the change under the write lock only if the same catalog still has the snapshot's properties; if another DDL changed or dropped the catalog in between, it starts over from a new snapshot. The rest of the ALTER validation still runs under the lock, unchanged. CREATE already probes before `CatalogMgr` takes its lock. Where the backport differs from master: | Master (#69588) | This PR | Why | | --- | --- | --- | | Endpoint keys declared by the hive connector through `ConnectorSecuritySurface` | Keys are a constant in `HMSExternalCatalog` | 4.1 has no connector SPI; `HMSExternalCatalog` is already the HMS-specific class | | Gate in `PluginDrivenExternalCatalog` | Gate in `HMSExternalCatalog`, through `checkWhenCreating` (CREATE) and the new `validatePropertiesBeforeUpdateUnlocked` (ALTER) | Neither runs on replay: `CatalogFactory` calls `checkWhenCreating` only for a new catalog, and replay goes through `replayAlterCatalogProps`, not `alterCatalogProps` | | `driver_url` gate moved onto the security surface, `driverUrlKeysOf` deleted | Not ported | 4.1 checks `driver_url` in `JdbcExternalCatalog` / `IcebergExternalCatalog` / `PaimonExternalCatalog.checkProperties`; there is no key table to replace | | ALTER rejected while the connector provider is missing | Not ported | 4.1 has no connector providers | | `checkEndpointForSsrf` throws `DdlException` | Throws `Exception` with the same messages; the HMS gate rethrows it as `DdlException` | On 4.1 `DdlException` is in fe-core, which fe-common cannot reference | | Connector plugin API 22.0 -> 23.0 | No version change | No connector plugin API on 4.1 | | All ALTER validation runs before `CatalogMgr` takes its write lock (#68585) | Only the lock-free checks (the probe) run before the write lock, through `validatePropertiesBeforeUpdateUnlocked`, on a snapshot that is checked again under the lock; the rest of ALTER validation stays under the lock | #68585 is not on 4.1 and also changes HDFS HA validation; this keeps the change to the probe | | `metastore.thrift.uris` gated because the hive connector passes every property to `HiveConf` | Gated as well | `HMSBaseProperties` copies only `hive.`-prefixed properties today, so the key does not reach the client yet; gating it keeps the gate correct if that copy widens, and keeps one rule on both branches | **3. Results** With an SSRF checker configured: | Statement | Before | After | | --- | --- | --- | | `CREATE CATALOG ... ("type"="hms", "hive.metastore.uris"="thrift://10.0.0.1:9083")` | Accepted | Rejected: `Endpoint 10.0.0.1:9083 is rejected by the SSRF policy (intranet address not allowed): ...` | | Same, with `test_connection` = false | Accepted | Rejected (the gate does not depend on `test_connection`) | | `ALTER CATALOG c SET PROPERTIES ("hive.metastore.uris"="thrift://10.0.0.1:9083")` | Accepted | Rejected | | `thrift://user@10.0.0.1:9083?x#y`, or an intranet host second in an HA list | Accepted | Rejected | | `"hive.metastore.uris"="thrift://${hive.x}:9083"` with `"hive.x"="127.0.0.1"` | Accepted; the client connects to 127.0.0.1 | Rejected: `... must give its endpoint literally while an SSRF checker is configured ...` | | Public endpoint that is currently unreachable | Accepted | Accepted (a failed connect is not an SSRF signal) | | ALTER of an unrelated property | Accepted | Accepted, no probe | | ALTER that probes an unreachable public endpoint | No probe | Probes for up to 5 seconds without holding `CatalogMgr`'s lock | | Replay of CREATE / ALTER from the edit log | No probe | No probe | Without a checker configured, nothing changes. The only change outside the gate is that the SDK's net-hook is now removed after each bracketed HTTP request, as intended. **4. The classes, and how they call each other** - `SecurityChecker` (fe-common): the SSRF primitive `checkEndpointForSsrf`, plus `isEnabled()` and the TCP start. - `UrlSecurityChecker` / `DummySecurityChecker` (fe-common): SDK binding / no-op. - `HMSExternalCatalog` (fe-core): the endpoint keys, `checkEndpointsAgainstSsrf`, `parseEndpoint`, and the two hooks. - `ExternalCatalog` (fe-core): the new `validatePropertiesBeforeUpdateUnlocked` hook, a no-op by default. - `CatalogMgr` (fe-core): `alterCatalogProps` runs that hook on a snapshot before taking its write lock and checks the snapshot again under it. ``` CREATE CATALOG CatalogFactory.createFromCommand └ HMSExternalCatalog.checkWhenCreating ├ checkEndpointsAgainstSsrf (new) -> SecurityChecker.checkEndpointForSsrf └ super.checkWhenCreating (test_connection) ALTER CATALOG ... SET PROPERTIES CatalogMgr.alterCatalogProps ├ read lock: snapshot the catalog's properties ├ no lock: HMSExternalCatalog.validatePropertiesBeforeUpdateUnlocked (new) │ └ checkEndpointsAgainstSsrf (new) -> SecurityChecker.checkEndpointForSsrf └ write lock: same catalog, same properties? if not, start over └ alterExternalCatalogPropsFenced -> validatePropertiesBeforeUpdate -> checkProperties (unchanged), publish, journal SecurityChecker.checkEndpointForSsrf(host, port) ├ startSSRFChecking() -> SDK startSSRFNetHookChecking() ├ doConnect(host, port, 5s) -> SDK throws SecurityException for an intranet target └ stopSSRFChecking() -> SDK stopSSRFNetHookChecking() Replay: CatalogFactory.createFromLog and replayAlterCatalogProps(isReplay=true) call neither hook. ``` SSRF surface note (per repo policy): the probe is a new FE -> user-named-host outbound TCP connect, made only when the operator installed the SSRF SDK and only at CREATE / ALTER CATALOG (CREATE_PRIV or ADMIN for CREATE, ALTER privilege for ALTER). It exists only so the SDK can reject an intranet target. Known limits, same as master: it is a point-in-time check, so it does not guard a later runtime connection (DNS rebinding) or a client that bypasses the JDK socket layer. ### Release note When an SSRF checker is configured, CREATE / ALTER CATALOG checks HMS endpoint destinations (`hive.metastore.uris`, `uri`, and `metastore.thrift.uris`), including URIs containing query, fragment, or userinfo components, and rejects an HMS endpoint value that contains a `${...}` placeholder. ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [ ] Regression test - [x] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason <!-- Add your reason? --> - Unit Test: the new `SecurityCheckerTest` (6 tests) and `HMSExternalCatalogSsrfGateTest` (48), plus the existing `HMSExternalCatalogParquetTimeZoneTest`, `HttpTableValuedFunctionTest`, `CatalogMgrTest`, `CatalogMgrLanceGuardTest`, `ExternalCatalogTest`, `ExternalCatalogRuntimeStateTest`, `CatalogVarbinaryMigrationTest`, `HMSPropertiesTest`, `HiveTableSinkTest`, `JdbcExternalCatalogTest` and `PaimonExternalCatalogDriverUrlSecurityTest`: 134 tests in 13 classes passed with `TZ=Asia/Shanghai mvn test -pl fe-common,fe-core -am`; Checkstyle reports 0 violations in fe-common and fe-core. - `HMSExternalCatalogSsrfGateTest` also drives ALTER through a real `CatalogMgr`. It checks that the probe runs while the thread holds neither the read nor the write lock, that an intranet target leaves the catalog and the journal untouched, and that an ALTER whose catalog is altered or dropped during the probe checks again, or fails with `No catalog found`, instead of applying a stale check. - Mutation check: each of 21 mutants is killed by at least one new test. The mutants drop either hook call, the `isEnabled()` guard of the gate or of `checkEndpointForSsrf`, the touched-key guard, a declared key, the comma split or the URI parsing; run the placeholder rule only on touched keys or ahead of the `isEnabled()` guard; swallow the SDK's rejection or a failed hook install; treat a failed connect as a rejection; skip `stopSSRFChecking()` in `finally`; probe under `CatalogMgr`'s write lock (from `validatePropertiesBeforeUpdate`, or by calling the lock-free hook under the lock); or drop the snapshot comparison or the catalog identity check under the lock. - Checked on this branch that `HMSBaseProperties` hands `hive.metastore.uris` = `thrift://${hive.x}:9083` with `hive.x` = `127.0.0.1` to the HMS client as `thrift://127.0.0.1:9083`, and does not hand it a `metastore.thrift.uris` catalog property. - Behavior changed: - [ ] No. - [x] Yes. With an SSRF checker configured, CREATE / ALTER CATALOG probes HMS metastore endpoints and rejects intranet targets and `${...}` placeholders; the SDK net-hook is now removed after each bracketed request. Without a checker, nothing changes. - Does this need documentation? - [x] No. - [ ] Yes. <!-- Add document PR link here. eg: apache/doris-website#1214 --> ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into --> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
morningman
added a commit
to morningman/doris
that referenced
this pull request
Nov 6, 2026
…udit text ### Background SHOW CREATE CATALOG, SHOW CATALOG, the `catalogs()` table function and the audit text of CREATE CATALOG replace a property value with `*XXX` when `DatasourcePrintableMap.isSensitiveKey` matches its key. Until now the keys came from a list in fe-core and from the keys filesystem plugins report at startup. A connector plugin marks its secret properties `sensitive = true`, but that mark only masks the connector's own log lines. A connector tells the engine about its security-relevant properties through `ConnectorProvider#securitySurface()`. ### What problem does this PR solve? Issue Number: None Related PR: apache#69588 (added the connector security surface this extends), apache#45164 (introduced the unmasked ALTER CATALOG audit text), apache#27115 (masked it for the statement apache#45164 replaced), apache#59502 (added `iceberg.jdbc.password`), apache#54523 (added the Paimon REST DLF keys), apache#65730 (branch-4.1's Lance catalog, whose REST secrets the Lance port will declare on this surface) PR Type: Bugfix Problem Summary: Catalog secrets that only a connector knows are on no masking list, so SHOW CREATE CATALOG and the other renderings print them in clear: `iceberg.jdbc.password`, `paimon.jdbc.password`, the Paimon REST DLF key pair, `mc.access_key`, and credentials passed through under `trino.` or `adbc.`. Separately, `ALTER CATALOG ... SET PROPERTIES` is written to the audit log unmasked, whatever the key. Each connector now declares its secrets on its security surface, taken from its `sensitive` marks, and the surface can also declare a namespace of secret keys for options passed through unread; Fluss's lake options were hard-coded in fe-core for that reason. The engine masks the union, with fe-core's own list kept as the floor. ALTER CATALOG is masked like CREATE CATALOG. ### Details - **Change map:** 42 files, +1035 −76 over two commits: production code +376 −48, tests +645 −24, the connector README +13 −3, the plugin API version in `fe/fe-connector/pom.xml`. The first commit adds secret keys to the surface (reviewed alone before). The second does the rest: 1. Key namespaces on the SPI: `withSensitiveKeyPrefix` and `SensitiveKeyPrefix.covers`, which holds the matching rule; a collection form of `withSensitiveKeys` (`ConnectorSecuritySurface`, +130). 2. Engine: `isSensitiveKey` matches registered namespaces, the Fluss rule becomes one of them; `ConnectorPluginManager` registers them and reads every surface before it publishes a provider (+70). 3. Connectors: every provider declares its secrets; `MetaStoreProviders.sensitivePropertyKeys()` gives the union of a plugin's metastore backends; `IcebergJdbcMetaStoreProvider` reports its keys like its siblings; the keytab paths lose their `sensitive` mark (+140). 4. ALTER CATALOG audit masking (+16). - **Frequency:** Always. Create or alter a catalog that sets one of the keys above, then run SHOW CREATE CATALOG, SHOW CATALOG `<name>` or `select * from catalogs()`, or read the audit log after an ALTER CATALOG ... SET PROPERTIES. - **Root cause:** `isSensitiveKey` consulted only fe-core's list and the filesystem providers' keys. A connector's `sensitive` marks fed only `toMaskedString`; `MetaStoreProvider.sensitivePropertyKeys()` existed for this, but nothing called it, and the Iceberg JDBC backend did not implement it. For ALTER CATALOG, `AlterCatalogPropertiesCommand` does not implement `NeedAuditEncryption` and `LogicalPlanBuilderForEncryption` has no `visitAlterCatalogProperties`, so `AuditLogHelper` records the original text; the statement it replaced in apache#45164 was masked by apache#27115. - **Why existing tests missed it:** Unit tests: `DatasourcePrintableMapTest` checks fe-core's own keys, the provider surface tests checked only the SSRF and driver_url keys, and `EncryptSQLTest` and `RepositoryAuditEncryptionTest` cover CREATE CATALOG, repositories and resources but not ALTER CATALOG. Regression: `audit/test_external_catalog_credential_masking` creates only Iceberg REST catalogs, whose keys are on fe-core's list, and never alters one. - **Fix:** The engine must not name a connector's properties, so the connectors declare their secrets, from the same `sensitive` marks that mask their logs; jdbc and es declare both spellings their legacy prefix allows, and trino and adbc declare the namespace they pass through, a key there being a secret when its rest names `key`, `secret`, `password`, `token` or `credential` (also `authorization` for ADBC). fe-core keeps the connector keys it lists, so a catalog whose plugin is not loaded masks what it masked before. A provider whose `securitySurface()` throws at startup is skipped, as a filesystem provider is, instead of stopping FE. - **Other paths and branches:** Fixed here: every rendering above and FE's statement logging, which share `isSensitiveKey` and `NeedAuditEncryption`. Not covered, since a key name cannot express them safely: raw Iceberg SDK keys copied through (`token`, `credential`), Paimon options re-keyed from `paimon.*`, Hadoop `fs.*` keys, and secrets inside a URL. The ALTER CATALOG gap is on branch-4.0, 4.1 and 4.2 (from apache#45164); branch-3.1 still uses the masked statement. branch-4.1 and 4.2 also print the connector keys above in clear; without the connector SPI their fix differs and is tracked separately. - **Behavior change:** The namespaces also mask non-secrets whose names carry those words, such as `trino.hive.s3.aws-access-key` or a keystore path. Keytab paths, which no release masks, are no longer masked in connector log lines: marked `sensitive`, `hadoop.kerberos.keytab` would be masked in every storage vault and resource. ### Testing All runs were local (macOS), on master `f5c993f2c94` plus this change: - Regression `audit/test_external_catalog_credential_masking`, extended with an Iceberg JDBC catalog: SHOW CREATE CATALOG masks `iceberg.jdbc.password`, and the audit texts of its CREATE CATALOG and of an ALTER CATALOG that sets the password and `s3.secret_key` hold only `*XXX`. Against the local cluster's FE without this change it fails at the first new check, SHOW CREATE CATALOG printing the password; with a FE built from this change it passes. - fe-connector-spi: all 160 tests, with the refreshed surface baseline (+4 lines); the metastore modules: all tests; ten connector modules: 29 classes, 511 tests, including each connector's surface test, which checks real key names. - fe-core: 26 classes, 341 tests, including `ConnectorPluginManagerTest`, `DatasourcePrintableMapTest`, `EncryptSQLTest` and the audit and plugin gate tests; 14 of them again after the last change to registration. - Twenty mutations, each caught by its test: for example, reverting either half of the ALTER CATALOG fix (`EncryptSQLTest` sees the statement in clear), matching a marker anywhere in the key, an empty metastore union, or re-marking a keytab. ### Notes for Reviewers - `SensitiveKeyPrefix.covers` lives on the SPI so that a connector's test can check its declaration against real key names and the engine applies the same rule; the Fluss floor in fe-core uses it too. - The entries both fe-core and a connector declare are deliberate: the floor is frozen, as the comment in `DatasourcePrintableMap` says. - The plugin API stays at 31.0, the first commit's bump. Hudi declares nothing: it owns no catalog properties. ### Release note SHOW CREATE CATALOG, SHOW CATALOG, the `catalogs()` table function and the audit log now mask `iceberg.jdbc.password`, `paimon.jdbc.password`, `paimon.rest.dlf.access-key-id`, `paimon.rest.dlf.access-key-secret`, `mc.access_key`, and the `trino.*` and `adbc.*` properties whose names carry `key`, `secret`, `password`, `token` or `credential` (`authorization` too for `adbc.*`). ALTER CATALOG ... SET PROPERTIES statements are masked in the audit log like CREATE CATALOG. ### Check List (For Author) - Behavior changed: Yes (see Details) - Does this need documentation: No - Pre-submission status: Ready ### AI Disclosure - Tool: Claude Code (Claude Opus 5.5) - Generated: the code, the tests, and this description - Human review: not stated - Prompts: The author first asked for a generic connector SPI to mask a connector's secret catalog properties instead of adding the Lance keys to fe-core's list. Then, in this PR: let the surface express prefix rules such as Fluss's, mask the union of the engine's and the connectors' keys, wire every connector, keep fe-core's existing entries as the floor, mask whatever is marked `sensitive` but stop marking keytab paths, and fix the unmasked ALTER CATALOG audit text on master only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Problem Summary:
External catalogs register user-supplied network endpoints (e.g. an HMS catalog's
hive.metastore.uris) that the FE connects to. When the operator installed the SSRFsecurity SDK (the
security_checker_class_nameconfig, i.e. Aliyuncom.aliyun.securitysdk.SecurityUtil), those endpoints were not checked against the SSRFpolicy at CREATE / ALTER CATALOG, so a user with CREATE_PRIV (no SUPER) could point a
catalog at an intranet address.
This PR adds one replay-safe gate that, when an SSRF checker is installed, brackets a
lightweight TCP connect to each user-supplied endpoint with the SDK's net-hook so an
intranet target is rejected before the catalog is accepted. The judgment is delegated
entirely to the SDK. With no checker installed (the default open-source build), the new
endpoint gate is a no-op. Schemed endpoints use java.net.URI host/port semantics, so query,
fragment, and userinfo components cannot hide the destination used by the HMS client.
Endpoint values must be literal: while a checker is configured, a declared endpoint value that
contains a
${...}placeholder is rejected, because the client's HadoopConfigurationexpands it (another property,
${env.X}, a JVM system property) only after the check. Therejection applies on every interactive CREATE / ALTER, including an ALTER that changes only the
property a stored placeholder refers to.
Connector-agnostic by construction. The engine must not hard-code any catalog type's
property names (the fe-connector invariant: no source-name branching in shared fe-core
nodes). So a connector now declares its security-sensitive property keys through a new SPI
value type,
ConnectorSecuritySurface(returned byConnectorProvider.securitySurface()),and the engine looks it up by type -- without initializing a connector -- to drive the gate.
The HMS connector declares
hive.metastore.uris, its aliasuri, and Hive 3'smetastore.thrift.uris, which the HMS client reads beforehive.metastore.uris.The same change unifies the pre-existing jdbc driver-url operator gate
(
jdbc_driver_secure_path/jdbc_driver_url_white_list) onto this mechanism: thedriver_urlkeys and their flavor conditions now come from each jdbc-flavored connector'ssecuritySurface(), and the fe-core driver URL key table that previously hard-coded catalogtypes (
driverUrlKeysOf) is deleted. The gates stay exactly where they were-- CREATE through
checkWhenCreating, ALTER throughvalidatePropertiesBeforeUpdate, never onreplay -- and the driver-url gate's mandatory rule (
JdbcDriverUrlSecurity.check) still runseven under
jdbc_driver_secure_path=*. If a provider is unavailable, interactive ALTERCATALOG property changes are rejected until the connector plugin is available: the engine
cannot validate properties without its security declaration, and an unchecked driver URL
would otherwise be consumed after the plugin is restored. This also rejects unrelated
property edits while the provider is unavailable. Replay still restores degraded catalogs
without running interactive validation or network probes.
This PR also fixes a latent bug in the existing SSRF wiring:
UrlSecurityCheckerbound theSDK's stop method as
stopSSRFNetHookChecking(String)but invoked it with no argument, so thenet-hook was never removed and leaked onto the pooled FE thread after any bracketed request. It
now binds and invokes the no-arg
stopSSRFNetHookChecking(), and adds the no-argstartSSRFNetHookChecking()(TCP form) needed to hook non-http connections such as HMS thrift.Because this changes the connector plugin SPI surface (a new type and a new provider method),
connector.plugin.api.versionis bumped 22.0 -> 23.0 andconnector-plugin-surface.txtisregenerated in the same commit.
SSRF surface note (per repo policy): the probe is a new FE -> user-named-host outbound TCP
connect, performed only when the operator installed the SSRF SDK and only at CREATE / ALTER
CATALOG (actor: CREATE_PRIV or ADMIN for CREATE, ALTER privilege for ALTER). Its sole purpose is
to let the SDK reject an intranet target; a plain connectivity failure is ignored (not treated
as a rejection), so a catalog pointing at a currently-unreachable public endpoint can still be
created. Known limit: point-in-time check only -- it does not guard a later runtime connection
(DNS rebinding) or clients that bypass the JDK socket layer (e.g. Netty native epoll).
Release note
When an SSRF checker is configured, CREATE / ALTER CATALOG checks HMS endpoint destinations
(
hive.metastore.uris,uri, andmetastore.thrift.uris), including URIs containing query,fragment, or userinfo components, and rejects an HMS endpoint value that contains a
${...}placeholder. ALTER CATALOG property
changes now require an available connector provider; degraded catalogs remain replayable.
The JDBC driver policy remains enforced independently of the SSRF checker configuration.
Connector plugins must be rebuilt for API version 23.0.
Check List (For Author)
TZ=Asia/Shanghai bash run-fe-ut.sh --runon the rebased head, including the complete fe-connector-spi suite (35 classes) and fe-connector-hive suite (51 classes, with HiveConnectorProviderSecuritySurfaceTest), PluginApiVersionWiringTest, SecurityCheckerTest, the {Jdbc,Iceberg,Paimon}ConnectorProviderSecuritySurfaceTest classes, PluginDrivenExternalCatalogDriverUrlGateTest, PluginDrivenExternalCatalogSsrfGateTest, PluginDrivenExternalCatalogConcurrencyTest, HMSExternalCatalogParquetTimeZoneTest, CatalogFactoryPluginRoutingTest, and CatalogMgrTest.BUILD_TYPE=ASAN DISABLE_BUILD_UI=ON MVN_OPT="-o -Dmaven.build.cache.enabled=false" FE_MAVEN_THREADS=4 bash build.sh --fe;git diff --check.security_checker_class_nameis configured. With a checker configured,metastore.thrift.urisis probed too and a${...}placeholder in an HMS endpoint property is rejected. Rejecting ALTER property changes without a provider also applies when no SSRF checker is configured; replay remains unchanged. Connector plugin API bumped 22.0 -> 23.0.