Repository navigation
[fix](catalog) Probe an http or https catalog endpoint that names no port at its default port - #70364
Open
morningman wants to merge 1 commit into
Open
morningman wants to merge 1 commit into
morningman wants to merge 1 commit into
Conversation
…port at its default port ### Background When an operator configures `security_checker_class_name`, CREATE CATALOG and ALTER CATALOG ask that checker whether the FE may connect to each endpoint the catalog's connector declares through `ConnectorSecuritySurface.ofSsrfEndpoints`, and refuse an intranet target (apache#69588). `PluginDrivenExternalCatalog.parseEndpoint` turns each declared value into the host and port to probe: `scheme://host:port`, or a bare `host:port`. A value it cannot turn into a concrete host and port is not probed. ### What problem does this PR solve? Issue Number: None Related PR: apache#69588 (added the gate) PR Type: Bugfix Problem Summary: An http or https endpoint written without a port, such as `http://10.0.0.1/ns` or `https://10.0.0.1`, passes the gate unprobed, although an HTTP client connects to port 80 or 443 for such a URI. Today only the Hive connector declares endpoints, and its thrift URIs carry their port, so no connector on master reaches this yet; the first http endpoint will be the Lance REST catalog's `lance.rest.uri`, and an https service URL usually names no port. The gate now probes such an endpoint at its scheme's default port. ### Details - **Frequency:** Not observed; found by code review while declaring the Lance REST catalog's endpoint. Triggers when an SSRF checker is configured and a declared endpoint is an http or https URL without a port. - **Root cause:** `parseEndpoint` returned null for a `scheme://` value whose `URI.getPort()` is -1, and `checkEndpointsAgainstSsrf` skips a null as nothing to probe. - **Why existing tests missed it:** Unit tests: `PluginDrivenExternalCatalogSsrfGateTest` covers thrift URIs, bare `host:port`, IPv6 and URI suffixes, and asserts that a thrift URI without a port is not probed; no case used an http or https URL. Regression: no suite reaches the gate, since it needs the operator's security checker SDK, which CI does not have; the unit tests use a fake. - **Fix:** For an http or https URI that names no port, `parseEndpoint` takes 80 or 443, the ports RFC 9110 assigns to the scheme; an explicit port still wins. A URI of another scheme without a port is still not probed: its default port, if it has one, belongs to the client, and the engine knows no connector's client. The fix is in the engine because the port follows from the scheme alone. - **Other paths and branches:** CREATE (`checkWhenCreating`) and both ALTER paths go through `parseEndpoint`, so all are fixed. branch-4.1 and branch-4.2 have the same rule in `HMSExternalCatalog.parseEndpoint`, but gate only Hive metastore thrift URIs, so they are not affected. Introduced by apache#69588. ### Testing Local (macOS), on master `f1bc8b3690c` plus this change: - `PluginDrivenExternalCatalogSsrfGateTest`: the new `parseEndpointUsesTheDefaultPortOfAWebScheme` (seven URIs: with and without a path, an upper-case scheme, user-info, query and fragment, IPv6, an explicit port) and `alterRejectsAnIntranetWebEndpointWithoutAPort` (an http and an https endpoint through the real ALTER validation, with `10.0.0.1` flagged as intranet). On the code before this change 8 of them fail: the six URIs without a port parse to null, and both ALTER cases accept the endpoint ("Expected DdlException to be thrown, but nothing was thrown"). With it, all 44 cases of the class pass, as do the 18 of `PluginDrivenExternalCatalogDriverUrlGateTest`; fe-core checkstyle passes. ### Notes for Reviewers - The Javadoc of `ConnectorSecuritySurface.ofSsrfEndpoints` still describes the value as a `host:port`, optionally scheme-prefixed. It is left alone here because apache#70338, still open, edits that file. ### Release note None ### Check List (For Author) - Behavior changed: No - Does this need documentation: No - Pre-submission status: Ready ### AI Disclosure - Tool: Claude Code (Claude Opus 5.5) - Generated: the fix, the tests, and this description - Human review: The author reviewed part of the changes. - Prompts: While porting the Lance REST catalog, which declares `lance.rest.uri` as an endpoint, the author learned that the gate skips a URL without a port, and asked for a separate engine PR that probes http and https at their default ports. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Static review of PR #70364 at head 809f183 against base f1bc8b3 found no new blocking or non-blocking findings. The change correctly maps an omitted port on case-insensitive http/https URIs to 80/443, preserves explicit ports and existing unknown-scheme behavior, and feeds the resulting destination through the existing SecurityChecker gate. CREATE and detached non-replay ALTER validation use the merged candidate, while touched-key, placeholder, no-checker, multi-endpoint, and replay semantics remain intact. Concurrency, lifecycle, persistence, FE/BE compatibility, and error propagation introduce no new issue in this patch; no thrift/protobuf or configuration changes are involved. The new tests cover paths, userinfo, query/fragment suffixes, uppercase schemes, IPv6, explicit ports, and ALTER rejection at the web defaults. Validation was static only: the authoritative diff and related call paths were inspected and git diff --check was clean; builds and tests were not executed under the review contract. There is no additional user-provided focus. Both independent review agents returned NO_NEW_VALUABLE_FINDINGS, so this review converged after round 1. Future connector-specific URI semantics are outside this head and do not leave a current coverage gap.
Review converged after 1 round(s).
Contributor
TPC-H: Total hot run time: 28543 ms |
Contributor
TPC-DS: Total hot run time: 154062 ms |
Contributor
ClickBench: Total hot run time: 23.96 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review of both changed files at 809f1833429194d534da2680347db3280afebe8f: no new actionable findings. There are no existing inline findings to carry forward and no additional user focus points.
The changed flow is CREATE / non-replay ALTER -> declared endpoint keys -> parseEndpoint -> SecurityChecker TCP probe. An absent HTTP/HTTPS port now becomes 80/443; an explicit port still wins. All identified source-level risks were checked and dismissed with evidence.
Critical checkpoint conclusions:
- Goal and proof: the shared parser implements the intended default-port mapping. Seven added parser cases check exact host/port results; two added ALTER cases require both policy rejection and the exact recorded default-port probe. These tests were inspected, not executed.
- Scope and reuse: the change is focused on one private helper, its call site, documentation and tests; it reuses the existing gate and socket-check primitive.
- Concurrency and locking: parsing adds only local state. Ordinary ALTER validates a detached snapshot outside the catalog write lock and rechecks it before applying; the alternate non-replay path retains its existing locking. No new lock order or race was found.
- Lifecycle and initialization: no new resource owner or mutable static state. Existing socket/hook cleanup and test singleton teardown remain intact; catalog initialization/reset and database-name mapping behavior are unchanged.
- Configuration: no configuration item or dynamic-update behavior is added.
- Compatibility: no public symbol, SPI, storage or wire format changes. Replay and lazy reconstruction retain their existing behavior; the stricter interactive probe is the intended change.
- Thrift/protobuf IDs: no generated protocol definitions are changed.
- Parallel paths: CREATE and both non-replay ALTER paths use the same parser. Disabled-checker and replay paths remain unchanged. Only Hive currently declares SSRF endpoint keys; this patch does not add an HTTP connector.
- Conditions: explicit-port precedence and case-insensitive HTTP/HTTPS defaults are clear and documented; other schemes and bare endpoints preserve previous behavior.
- Test coverage: new cases cover path/no path, uppercase scheme, user-info/query/fragment, IPv6, explicit port and HTTP/HTTPS rejection. Existing cases cover thrift/bare endpoints, aliases, HA lists, placeholders, disabled/untouched gates and CREATE. No new web-specific CREATE integration case is present; its shared path was traced statically.
- Expected results: no result artifacts changed; the new assertions match the intended destinations and cannot pass merely because an unrelated validation step throws.
- Observability and errors: existing rejection errors identify the endpoint, setup failures propagate, and connectivity-only failures retain their documented handling. No extra metrics are needed for this parser branch.
- Persistence and transactions: no persisted state or transaction logic changes. Interactive rejection precedes catalog acceptance; replay behavior remains unchanged. Committed-data visibility, partition versions and delete bitmaps are unaffected.
- Data writes and atomicity: detached validation does not publish a tentative candidate; existing mutation and deferred cleanup are unchanged. No storage write or new crash-recovery obligation is introduced.
- FE/BE propagation: no new variable or cross-process value is added.
- Performance: the fallback adds constant-time scheme checks in DDL validation; newly enabled probes use the existing bounded connect timeout. No query hot-path change was found.
- Further review: explicit/default ports, invalid authorities, non-web schemes, provider lookup and test isolation were rechecked. No additional introduced defect or unresolved source-level candidate remains.
Coverage limits: static inspection only, with no builds, tests, benchmarks, live network checks or third-party SDK execution. The fake-provider tests do not establish end-to-end behavior of a future HTTP connector. Existing point-in-time validation limits, including later DNS changes and undeclared endpoints, are unchanged. These limits are separate from source-review convergence.
Review converged after 2 round(s).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
When an operator configures
security_checker_class_name, CREATE CATALOG and ALTER CATALOG ask that checker whether the FE may connect to each endpoint the catalog's connector declares throughConnectorSecuritySurface.ofSsrfEndpoints, and refuse an intranet target (#69588).PluginDrivenExternalCatalog.parseEndpointturns each declared value into the host and port to probe:scheme://host:port, or a barehost:port. A value it cannot turn into a concrete host and port is not probed.What problem does this PR solve?
Issue Number: None
Related PR: #69588 (added the gate)
PR Type: Bugfix
Problem Summary:
An http or https endpoint written without a port, such as
http://10.0.0.1/nsorhttps://10.0.0.1, passes the gate unprobed, although an HTTP client connects to port 80 or 443 for such a URI. Today only the Hive connector declares endpoints, and its thrift URIs carry their port, so no connector on master reaches this yet; the first http endpoint will be the Lance REST catalog'slance.rest.uri, and an https service URL usually names no port. The gate now probes such an endpoint at its scheme's default port.Details
parseEndpointreturned null for ascheme://value whoseURI.getPort()is -1, andcheckEndpointsAgainstSsrfskips a null as nothing to probe.PluginDrivenExternalCatalogSsrfGateTestcovers thrift URIs, barehost:port, IPv6 and URI suffixes, and asserts that a thrift URI without a port is not probed; no case used an http or https URL. Regression: no suite reaches the gate, since it needs the operator's security checker SDK, which CI does not have; the unit tests use a fake.parseEndpointtakes 80 or 443, the ports RFC 9110 assigns to the scheme; an explicit port still wins. A URI of another scheme without a port is still not probed: its default port, if it has one, belongs to the client, and the engine knows no connector's client. The fix is in the engine because the port follows from the scheme alone.checkWhenCreating) and both ALTER paths go throughparseEndpoint, so all are fixed. branch-4.1 and branch-4.2 have the same rule inHMSExternalCatalog.parseEndpoint, but gate only Hive metastore thrift URIs, so they are not affected. Introduced by [feature](fe) Gate catalog SSRF endpoints and driver_url via a connector security surface #69588.Testing
Local (macOS), on master
f1bc8b3690cplus this change:PluginDrivenExternalCatalogSsrfGateTest: the newparseEndpointUsesTheDefaultPortOfAWebScheme(seven URIs: with and without a path, an upper-case scheme, user-info, query and fragment, IPv6, an explicit port) andalterRejectsAnIntranetWebEndpointWithoutAPort(an http and an https endpoint through the real ALTER validation, with10.0.0.1flagged as intranet). On the code before this change 8 of them fail: the six URIs without a port parse to null, and both ALTER cases accept the endpoint ("Expected DdlException to be thrown, but nothing was thrown"). With it, all 44 cases of the class pass, as do the 18 ofPluginDrivenExternalCatalogDriverUrlGateTest; fe-core checkstyle passes.Notes for Reviewers
ConnectorSecuritySurface.ofSsrfEndpointsstill describes the value as ahost:port, optionally scheme-prefixed. It is left alone here because [fix](catalog) Mask connector catalog secrets and the ALTER CATALOG audit text #70338, still open, edits that file.Release note
None
Check List (For Author)
AI Disclosure
lance.rest.urias an endpoint, the author learned that the gate skips a URL without a port, and asked for a separate engine PR that probes http and https at their default ports.