morningman opened a new pull request, #70092:
URL: https://github.com/apache/doris/pull/70092
### What problem does this PR solve?
Issue Number: None
Related PR: #69588
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://[email protected]: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:
https://github.com/apache/doris-website/pull/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 -->
--
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]
