github-actions[bot] commented on code in PR #69588:
URL: https://github.com/apache/doris/pull/69588#discussion_r4372968032
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/plugin/PluginDrivenExternalCatalog.java:
##########
@@ -345,41 +350,97 @@ 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). {@link
+ * ConnectorSecuritySurface#EMPTY} when no provider backs this type —
exactly the state in which the
+ * connector can neither load a driver jar nor open a connection, so there
is nothing to police.
+ */
+ // Package-private, not private: a unit test overrides it to inject a
surface without registering a
+ // connector plugin, exactly as it exercises the gate logic in isolation.
+ ConnectorSecuritySurface securitySurface(Map<String, String> properties) {
+ return ConnectorFactory.findProvider(getType(), properties)
+ .map(ConnectorProvider::securitySurface)
+ .orElse(ConnectorSecuritySurface.EMPTY);
Review 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 me
tadata or reject sensitive edits until the provider is available; test the
real lookup with an empty manager.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/plugin/PluginDrivenExternalCatalog.java:
##########
@@ -345,41 +350,97 @@ 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). {@link
+ * ConnectorSecuritySurface#EMPTY} when no provider backs this type —
exactly the state in which the
+ * connector can neither load a driver jar nor open a connection, so there
is nothing to police.
+ */
+ // Package-private, not private: a unit test overrides it to inject a
surface without registering a
+ // connector plugin, exactly as it exercises the gate logic in isolation.
+ ConnectorSecuritySurface securitySurface(Map<String, String> properties) {
+ return ConnectorFactory.findProvider(getType(), properties)
+ .map(ConnectorProvider::securitySurface)
+ .orElse(ConnectorSecuritySurface.EMPTY);
+ }
- 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());
+ if (hostAndPort == null) {
+ continue;
+ }
+ checker.checkEndpointForSsrf(hostAndPort.getHost(),
hostAndPort.getPort());
+ }
}
}
/**
- * The driver-jar properties of the three jdbc-flavored catalog types,
spelled out here because the
- * fe.conf policy is the engine's to apply while the keys belong to the
connectors, and widening the
- * plugin SPI for three constants is not worth a plugin API major bump.
The keys are the user-facing
- * property names (with their documented aliases), which are wire-stable.
This single table drives
- * BOTH halves of the gate — the trigger set (urlKeys plus flavorKey: the
changes that can repoint
- * the loaded jar) and the values that get checked — so the two can never
drift apart. Owners:
- * JdbcCatalogProperties, IcebergJdbcMetaStoreProperties,
PaimonJdbcMetaStoreProperties. A new
- * consumer of a jdbc-flavored driver_url adds its row here and nowhere
else (see the "Remote
- * Artifacts and Dynamic Code Loading" section of AGENTS.md).
+ * Parse one endpoint — {@code scheme://host:port} (e.g. {@code
thrift://host:9083}) or a bare
+ * {@code host:port} — into host and port for the SSRF probe. Returns null
when no concrete host:port
+ * can be extracted (nothing to probe); {@link HostAndPort} handles IPv6
literals in brackets natively.
*/
- private static DriverUrlKeys driverUrlKeysOf(String catalogType) {
- if ("jdbc".equalsIgnoreCase(catalogType)) {
- return new DriverUrlKeys(null, "driver_url", "jdbc.driver_url");
+ static HostAndPort parseEndpoint(String value) {
+ if (value == null || value.isEmpty()) {
+ return null;
}
- if ("iceberg".equalsIgnoreCase(catalogType)) {
- return new DriverUrlKeys("iceberg.catalog.type",
"iceberg.jdbc.driver_url");
+ // Drop a leading scheme (thrift://, http://, ...) and any trailing
path, leaving the authority.
+ String authority = value;
+ int schemeIdx = authority.indexOf("://");
+ if (schemeIdx >= 0) {
+ authority = authority.substring(schemeIdx + 3);
}
- if ("paimon".equalsIgnoreCase(catalogType)) {
- return new DriverUrlKeys("paimon.catalog.type",
"paimon.jdbc.driver_url", "jdbc.driver_url");
+ int slashIdx = authority.indexOf('/');
+ if (slashIdx >= 0) {
+ authority = authority.substring(0, slashIdx);
+ }
+ try {
+ HostAndPort hostAndPort = HostAndPort.fromString(authority);
+ if (!hostAndPort.hasPort() || hostAndPort.getHost().isEmpty()) {
+ return null;
+ }
+ return hostAndPort;
+ } catch (IllegalArgumentException e) {
Review 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.
--
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]
