morningman commented on code in PR #69588:
URL: https://github.com/apache/doris/pull/69588#discussion_r4376193126
##########
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:
Fixed in a36c8b1f3f2. `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.
##########
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:
Fixed in a36c8b1f3f2 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).
--
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]
