github-actions[bot] commented on code in PR #63817:
URL: https://github.com/apache/doris/pull/63817#discussion_r3362003838
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java:
##########
@@ -313,12 +316,34 @@ public void checkWhenCreating() throws DdlException {
boolean testConnection = Boolean.parseBoolean(
catalogProperty.getOrDefault(TEST_CONNECTION,
String.valueOf(DEFAULT_TEST_CONNECTION)));
+ // Best-effort property parsing for the SSRF check. Some catalog types
(e.g. the
+ // in-tree `test` catalog) intentionally use non-standard metastore
values that
+ // MetastoreProperties.create() rejects; before this method the
failure was hidden
+ // by the lazy `test_connection=false` path, so we preserve that
compatibility here
+ // — invalid catalogs simply have no URIs to validate and fall through.
+ MetastoreProperties msProps;
+ Map<StorageProperties.Type, StorageProperties> spMap;
+ try {
+ msProps = catalogProperty.getMetastoreProperties();
+ spMap = catalogProperty.getStoragePropertiesMap();
+ } catch (RuntimeException e) {
+ if (testConnection) {
+ throw e;
+ }
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Skipping SSRF check for catalog '{}': {}", name,
e.getMessage());
+ }
+ return;
+ }
+
+ // SSRF: reject user-supplied URIs (HMS / HDFS / S3 endpoint / Iceberg
REST / Glue)
+ // that point at internal or loopback hosts. Always runs so attackers
cannot bypass
+ // by setting test_connection=false.
Review Comment:
This check only runs from catalog creation
(`CatalogFactory.createFromCommand()` calls `checkWhenCreating()`). `ALTER
CATALOG ... SET PROPERTIES` goes through
`CatalogMgr.replayAlterCatalogProps()`, mutates `CatalogProperty`, and
validates with `checkProperties()`, which never calls `CatalogSsrfChecker`. A
user can therefore create a catalog with a safe endpoint and then alter
`iceberg.rest.uri`, `hive.metastore.uris`, `s3.endpoint`, etc. to
`127.0.0.1`/private hosts; the updated properties are persisted and later used
after cache reset. Please apply the same SSRF validation on property updates as
well, preserving the existing rollback-on-DdlException behavior.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/property/storage/AzureProperties.java:
##########
@@ -68,7 +68,8 @@ public class AzureProperties extends StorageProperties {
@Getter
@ConnectorProperty(names = {"azure.endpoint", "s3.endpoint",
"AWS_ENDPOINT", "endpoint", "ENDPOINT"},
required = false,
- description = "The endpoint of S3.")
+ description = "The endpoint of S3.",
+ checkSsrf = true)
protected String endpoint = "";
Review Comment:
Azure OAuth2 still has an unvalidated outbound endpoint. When
`azure.auth_type=OAuth2`, `buildRules()` requires `azure.oauth2_server_uri`,
and `setHDFSAzureOauth2Config()` writes it to
`fs.azure.account.oauth2.client.endpoint.*`, which the ABFS/Hadoop client later
contacts for tokens. Because `CatalogSsrfChecker` only discovers fields
annotated with `checkSsrf=true`, a catalog can pass this new check while
pointing `azure.oauth2_server_uri` at an internal host. Please annotate that
URI field too and add a regression/unit test for it.
--
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]
