github-actions[bot] commented on code in PR #69588:
URL: https://github.com/apache/doris/pull/69588#discussion_r4375462196
##########
fe/fe-connector/fe-connector-hive/src/main/java/org/apache/doris/connector/hive/HiveConnectorProvider.java:
##########
@@ -80,4 +81,11 @@ public void validatePropertiesForUpdate(
candidate.putAll(updatedProperties);
HiveCatalogProperties.of(candidate).checkCreateTimeOnlyRules(updatedProperties);
}
+
+ @Override
+ 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");
Review 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`](https://github.com/apache/hive/blob/rel/release-3.1.3/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java#L1275),
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/frag
ment parsing issue.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/plugin/PluginDrivenExternalCatalog.java:
##########
@@ -345,41 +352,102 @@ private void
checkDriverUrlsAgainstOperatorGate(Map<String, String> candidate,
}
}
- /** One row of the driver-jar key table: which properties name the jar,
live under which flavor key. */
- private static final class DriverUrlKeys {
- /** The properties whose value the connector hands to a class loader,
documented aliases included. */
- final List<String> urlKeys;
- /** The key whose candidate value must be "jdbc" for the urlKeys to be
live; null = always live. */
- final String flavorKey;
+ /**
+ * The connector's declared security-sensitive property surface, looked up
by catalog type WITHOUT
+ * initializing a connector (so neither gate forces a catalog to come to
life). A missing provider
+ * cannot validate newly submitted properties: accepting them would bypass
the gates when its plugin
+ * is restored, since replay and lazy reconstruction deliberately skip
statement-time validation.
+ * Only an available provider may declare {@link
ConnectorSecuritySurface#EMPTY}.
+ */
+ private ConnectorSecuritySurface securitySurface(Map<String, String>
properties) throws DdlException {
+ return ConnectorFactory.findProvider(getType(), properties)
+ .map(ConnectorProvider::securitySurface)
+ .orElseThrow(() -> new DdlException("No ConnectorProvider
found for catalog '" + name
+ + "', type '" + getType() + "'. Install the connector
plugin "
+ + "before altering catalog properties."));
+ }
- DriverUrlKeys(String flavorKey, String... urlKeys) {
- this.flavorKey = flavorKey;
- this.urlKeys = Arrays.asList(urlKeys);
+ /**
+ * Applies the operator's SSRF policy (the third-party {@code
SecurityChecker} SDK) to every
+ * user-supplied network endpoint these properties would make the FE
connect to, rejecting an intranet
+ * target. It mirrors {@link #checkDriverUrlsAgainstOperatorGate}: the
connector declares the endpoint
+ * keys through {@link ConnectorSecuritySurface#ssrfEndpointKeys()}, it
runs on CREATE
+ * (through {@code checkWhenCreating}) and on the {@code !isReplay} ALTER
path (here and in
+ * {@code validatePropertiesBeforeUpdate}) but never on replay, and it is
a no-op unless the operator
+ * installed an SSRF checker.
+ *
+ * <p>The judgment itself is delegated to the SDK — see {@link
SecurityChecker#checkEndpointForSsrf}.
+ * This is a point-in-time check of the endpoint supplied at CREATE/ALTER;
it does not guard a later
+ * runtime connection (DNS rebinding, lazy metadata access) — that is a
deliberate scope limit.
+ *
+ * <p>A connector whose property names a host to connect to declares it in
its
+ * {@code securitySurface()} and nowhere else.
+ */
+ private void checkEndpointsAgainstSsrf(Map<String, String> candidate,
Map<String, String> updatedProperties)
+ throws DdlException {
+ SecurityChecker checker = SecurityChecker.getInstance();
+ if (!checker.isEnabled()) {
+ return;
+ }
+ List<String> endpointKeys =
securitySurface(candidate).ssrfEndpointKeys();
+ if (endpointKeys.isEmpty()) {
+ return;
+ }
+ // Only an ALTER that touches an endpoint key can repoint the catalog
at a new host; a stored value
+ // was gated at its own CREATE/ALTER time. On CREATE the full property
map is passed as both
+ // arguments, so every supplied endpoint counts as touched and is
checked.
+ boolean touched =
endpointKeys.stream().anyMatch(updatedProperties::containsKey);
+ if (!touched) {
+ return;
+ }
+ for (String key : endpointKeys) {
+ String rawValue = candidate.get(key);
+ if (rawValue == null || rawValue.trim().isEmpty()) {
+ continue;
+ }
+ // A value may list several comma-separated endpoints (e.g. HA
hive.metastore.uris).
+ for (String endpoint : rawValue.split(",")) {
+ HostAndPort hostAndPort = parseEndpoint(endpoint.trim());
Review 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](https://github.com/apache/hadoop/blob/rel/release-3.4.2/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java#L1263)
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 valida
tion. This configuration-expansion case persists with every URI alias declared
and is distinct from the resolved URI-suffix issue.
--
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]
