Repository navigation
[fix](catalog) Mask connector catalog secrets and the ALTER CATALOG audit text - #70338
Open
morningman wants to merge 3 commits into
Open
morningman wants to merge 3 commits into
morningman wants to merge 3 commits into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Review recovery stopped: shared time budget or helper timeout exhausted Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
TPC-H: Total hot run time: 28644 ms |
Contributor
TPC-DS: Total hot run time: 154111 ms |
Contributor
ClickBench: Total hot run time: 24.13 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
morningman
requested review from
englefly,
morrySnow and
starocean999
as code owners
November 6, 2026 12:51
morningman
changed the title
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.
Reviewed all 42 changed paths at b0bfd3c. One new P2 finding: eager security-surface discovery does not isolate nested metastore ServiceLoader failures, so one broken connector can prevent FE startup. No P0/P1 findings or existing blocking comments were confirmed.
Critical checkpoint conclusions:
| Checkpoint | Conclusion |
|---|---|
| Goal and proof | Connector declarations reach SHOW CREATE CATALOG, SHOW CATALOG, catalogs(), and CREATE/ALTER audit/log masking. Unit and audit regression coverage was inspected; execution was outside this static review. |
| Scope and reuse | Focused rendering change using existing annotations, the security surface, and NeedAuditEncryption; no connector property parsing added to generic engine paths. |
| Concurrency and locks | Normal registration runs before catalog replay/request access. New prefix reads use copy-on-write snapshots and immutable rules; no new threads, blocking IO under metadata locks, or lock-order changes. |
| Lifecycle and static initialization | Provider-owned loaders and normal package dependencies are consistent. The inline P2 covers uncaught ServiceConfigurationError during newly eager metastore initialization. |
| Configuration | No new configuration or dynamic-update requirement. |
| Compatibility | Connector API major 31.0 matches the resource, manifests and test; plugin surface baseline includes additions. Storage and FE/BE protocols are unchanged. |
| Thrift and protobuf | No changes. |
| Parallel paths | Reviewed exact aliases, metastore flavors, pass-through prefixes, all three catalog renderings, CREATE/ALTER audit, statement logging, forwarding and multi-statement dispatch. Legacy masking floor is preserved. |
| Conditional logic | Required ALTER property spans follow from grammar. Case-insensitive prefix/suffix rules match the declared behavior; keytab properties contain paths. |
| Test coverage | Positive/negative aliases, namespace matching, immutability, registration/isolation and persisted audit checks are present. The new exception fixture misses ServiceConfigurationError, as noted inline. |
| Changed expected results | No result files changed; added audit assertions use known placeholders and check masking plus retained nonsecret values. No generated test result is claimed. |
| Observability | Skip logs identify providers/plugins; the reported exception currently bypasses them. No additional metrics are needed for this startup/rendering change. |
| Transactions and persistence | Catalog journaling, replay, failover and persisted credential values are unchanged; startup registration also applies masking to historical catalogs. |
| Data writes and atomicity | Table writes, committed-data visibility, delete bitmaps and crash recovery are unaffected. |
| FE/BE variables | None added. |
| Performance | Exact-key lookup and a small startup-defined prefix scan remain outside row-processing hot paths; no material regression established. |
| Other concerns | No further substantiated issue after the final recheck. |
No additional user focus was supplied. Existing inline threads were empty and the finding was deduplicated across reviewers. The PR's disclosed limits for arbitrary SDK/re-keyed/fs.* options and secrets embedded in URLs remain outside its declared masking coverage. Review coverage is complete with no unresolved candidates. Review converged after two rounds.
Validation was static source, test, dependency and control-flow inspection only. No builds, tests, benchmarks, runtime plugin loading or source edits were performed.
Review converged after 2 round(s).
| ConnectorSecuritySurface securitySurface; | ||
| try { | ||
| securitySurface = provider.securitySurface(); | ||
| } catch (RuntimeException | LinkageError e) { |
Contributor
There was a problem hiding this comment.
[P2] Catch ServiceConfigurationError during eager surface discovery
An otherwise valid connector directory with a stale metastore service descriptor, or a backend whose constructor fails, now prevents FE startup. Iceberg/Paimon/Hive securitySurface() calls MetaStoreProviders.sensitivePropertyKeys(), whose static initializer runs ServiceLoader; those lookup/instantiation failures throw ServiceConfigurationError, an Error outside both RuntimeException and LinkageError. It escapes this catch through Env.initConnectorPluginManager() before image loading, defeating the documented per-plugin skip behavior. Previously these nested backends were discovered during catalog validation/access rather than directory registration. Catch ServiceConfigurationError here and at the new builtin surface boundary, and cover a failing nested loader; the added IllegalStateException fixture does not exercise this case.
Contributor
Author
There was a problem hiding this comment.
Fixed in ae2cebe. Confirmed against the code: the Hive, Iceberg and Paimon surfaces call MetaStoreProviders.sensitivePropertyKeys(), whose static initializer iterates a ServiceLoader over the plugin's jars. A descriptor naming a backend that cannot be loaded throws ServiceConfigurationError, an Error a static initializer rethrows unwrapped, so it escaped this RuntimeException | LinkageError catch and the builtin path's RuntimeException catch, and Env.initConnectorPluginManager() catches nothing: a stale jar in a connector's lib/ stopped FE at startup, where before this PR only that connector's catalogs failed. Both catches now include ServiceConfigurationError (the builtin one matches the construction catch above it), so the provider is skipped and logged like any other surface failure.
ConnectorPluginManagerTest#pluginWhoseSecuritySurfaceFailsInItsOwnServiceLoaderIsSkippedNotFatal (a plugin directory whose lib/ jar names a missing backend; the plugin next to it still loads) and #classpathProviderWhoseSecuritySurfaceFailsInItsOwnServiceLoaderIsSkippedNotFatal fail on b0bfd3c with that error and pass now; dropping it from either catch fails only that path's case.
Contributor
TPC-H: Total hot run time: 28677 ms |
Contributor
TPC-DS: Total hot run time: 153854 ms |
Contributor
ClickBench: Total hot run time: 23.8 s |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Reviewed all 43 changed paths at ae2cebe. One new P1 finding: the newly enabled ALTER CATALOG masking uses code-point token positions as UTF-16 substring offsets and leaves plaintext credential suffixes in audit and FE statement logs. This prevents the intended complete masking fix for accepted supplementary-Unicode SQL.
The inspected flow is:
provider -> securitySurface -> startup masking union -> SHOW CREATE / SHOW CATALOG / catalogs()
ALTER CATALOG -> NeedAuditEncryption -> property spans -> rewriteSql -> audit / FE logs
Critical checkpoint conclusions:
| Checkpoint | Conclusion |
|---|---|
| Goal and proof | Connector-declared secrets reach the three catalog renderings and CREATE/ALTER logging. The inline P1 identifies the Unicode gap in the new ALTER path. Added unit and audit regression cases cover ASCII behavior; no execution is claimed. |
| Scope, clarity and reuse | Focused masking change reusing property annotations, the existing security surface and NeedAuditEncryption. Generic engine paths gain no connector-specific property parsing. |
| Concurrency and locks | Production registration occurs before image/replay/catalog access. Prefix rules use copy-on-write snapshots and immutable descriptors. No new thread, metadata-lock IO, nested lock order or supported production read/write race was identified. |
| Lifecycle and static initialization | Metastore discovery explicitly uses the defining plugin loader. Directory failures discard/close handles before routing publication; providers and masking descriptors otherwise live for FE lifetime. Both startup ServiceConfigurationError boundaries now catch the failure reported in the existing thread. |
| Configuration and dynamic updates | No new configuration or dynamic-update requirement. |
| Compatibility and rolling upgrades | SPI major and test pin both move from 30.0 to 31.0; the plugin API baseline records the additions. Legacy masking floor is retained. No durable format or FE/BE protocol changes. |
| Thrift and protobuf | No changes. |
| Parallel paths | Checked aliases, metastore flavors, pass-through prefixes, all three catalog renderings, CREATE/ALTER audit, FE statement logging, forwarding and multi-statement dispatch. Shared rewrite index units cause the inline finding. |
| Conditional logic | ALTER property lists are mandatory in the grammar. Namespace matching is case-insensitive and checks only the suffix after the declared prefix. Keytab annotations removed here name paths; keytab-content masking remains. |
| Test coverage | Inspected positive/negative aliases, prefix boundaries, immutability, admitted/refused providers, ADBC discovery isolation, actual nested ServiceLoader failure fixtures and persisted audit assertions. Supplementary Unicode values/comments are missing. |
| Changed expected results | No golden result files changed. Added assertions match ASCII masking and retained nonsecret values; they do not establish Unicode correctness. Result generation and author-reported test runs were not verified by execution. |
| Observability | Skip logs identify provider/plugin failures. The Unicode leak is a successful string rewrite, so existing exception fallback cannot detect it. No additional metric is needed for this startup/rendering change. |
| Transactions and persistence | Catalog journaling/replay/failover and stored credentials are unchanged. Startup registration applies masks to historical catalogs. |
| Data writes and atomicity | Table writes, committed-data visibility, version consistency, delete bitmaps and crash recovery are unaffected. No BE allocation/ownership change. |
| FE/BE variables | None added. |
| Performance | Small startup-defined prefix scans and exact-key lookups run in rendering paths; no new IO or row-processing hot-path work. No material performance defect substantiated. |
| Other concerns | No further substantiated issue after full-scope and missed-case rechecks. |
Existing thread 4492689479 is independently confirmed addressed at this head: both relevant catches include ServiceConfigurationError, and the new classpath/directory fixtures exercise a missing nested backend. It is not duplicated. No existing P0/P1 comments remain to carry forward. No additional user review focus was supplied.
The PR's disclosed limits for arbitrary raw/re-keyed SDK options, fs.* options and secrets embedded in URLs remain outside its declared masking coverage. Static review coverage is complete with no unresolved candidates or risk questions. Review converged after two rounds.
Validation was static source, test, dependency and control-flow inspection, with a standalone character-index calculation for the inline example. No Doris builds/tests, benchmarks, runtime plugin loading, submodule initialization or source edits were performed.
Review converged after 2 round(s).
| public LogicalPlan visitAlterCatalogProperties(DorisParser.AlterCatalogPropertiesContext ctx) { | ||
| DorisParser.PropertyItemListContext properties = ctx.propertyItemList(); | ||
| encryptProperty(visitPropertyItemList(properties), properties.start.getStartIndex(), | ||
| properties.stop.getStopIndex()); |
Contributor
There was a problem hiding this comment.
[P1] Convert code-point spans before slicing the original SQL
The new ALTER masking path passes ANTLR offsets directly to BaseViewInfo.rewriteSql, which slices a Java String using UTF-16 offsets. CaseInsensitiveStream.fromString uses a code-point stream whenever the SQL contains a supplementary character, so the replacement ends early and copies part of the original credential back into the result. For example, ALTER CATALOG ctl SET PROPERTIES("password"="😀😀😀😀😀😀😀secret") produces ALTER CATALOG ctl SET PROPERTIES("password" = "*XXX"secret"): the entire ASCII suffix remains in the audit and FE statement logs, without an exception to trigger their fallback. Supplementary characters in an earlier SQL comment can expose an otherwise ASCII password too. Convert both token boundaries to UTF-16 positions (including the exclusive end) before rewriting, and cover Unicode values/comments in the masking tests.
Contributor
Author
There was a problem hiding this comment.
Thanks, confirmed. This is a defect the engine already had, not one this PR introduces: when a statement contains a supplementary character, the parser's token positions count code points, while BaseViewInfo.rewriteSql cuts the Java string by UTF-16 offsets. Every masking visitor in LogicalPlanBuilderForEncryption shares it (CREATE CATALOG, repositories, SET PASSWORD and the rest), and so does the rewriting of view and MTMV definitions; the ALTER CATALOG masking added here goes through the same rewriteSql.
It is fixed separately in #70366, which converts the positions at all five places that cut the original SQL and adds cases with emoji in values, in comments before the masked span, and across several replacements in one statement. Once that is merged, the ALTER CATALOG path in this PR gets the fix without a change here.
Contributor
TPC-H: Total hot run time: 28298 ms |
Contributor
TPC-DS: Total hot run time: 153663 ms |
Contributor
ClickBench: Total hot run time: 24.08 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Reviewed all 43 changed files at ae2cebe using static inspection. No new distinct inline findings. The review converged after one round: both full-scope reviewers and the separate discovery-risk reviewer returned NO_NEW_VALUABLE_FINDINGS, and every risk/candidate has an evidence-backed disposition. This does not clear the existing P1 blocker.
The existing P1 Unicode masking finding still applies to this exact head. ALTER CATALOG forwards ANTLR code-point spans through NeedAuditEncryption into BaseViewInfo.rewriteSql, which uses Java UTF-16 string offsets. Supplementary characters in a value or preceding comment can leave credential text in audit and FE statement logs without an exception. The separately referenced fix is absent from this snapshot. Retained comment 4496273297 as an existing blocker; no duplicate inline.
For the existing P2 eager-discovery finding, directory loading and the first builtin ServiceConfigurationError are now isolated. A residual of that same concern exists for supported embedded/classpath use with multiple Hive/Iceberg/Paimon providers sharing MetaStoreProviders: a stale backend descriptor fails its static initializer for the first provider; the next provider gets NoClassDefFoundError, a LinkageError absent from the builtin surface catch at ConnectorPluginManager.java:202. The directory catch includes LinkageError. The new fixture performs ServiceLoader iteration directly and exercises one provider, so it does not cover the poisoned shared static registry. Treat this as part of the existing thread, without another inline; the builtin catch should also cover LinkageError and its test should exercise two providers sharing the failing initializer.
The traced flows were:
connector annotations / namespaces -> securitySurface -> provider admission
-> process-wide masking union -> SHOW CREATE / SHOW CATALOG / catalogs()
ALTER CATALOG -> NeedAuditEncryption -> encryption visitor -> SQL rewrite
-> audit text and FE statement logging
| Critical checkpoint | Conclusion |
|---|---|
| Goal and evidence | Connector-declared keys now feed shared renderings and ordinary ALTER audit masking. Tests cover these paths, but the existing Unicode P1 prevents a complete masking claim. |
| Scope and reuse | Changes stay within declarations, shared registration/matching, the ALTER audit hook, documentation and related tests; reuse of the annotation walker and common predicate is appropriate. |
| Concurrency and locking | Env registers providers before image loading/catalog access. No supported runtime exact-key mutation path found; prefix publication uses CopyOnWriteArrayList and final copied matcher state. No new lock ordering or expensive work under metadata locks. |
| Lifecycle and initialization | Surfaces are read before directory provider/inventory publication; failed directory handles are discarded. Discovery uses the SPI defining classloader. Shared-classpath initialization retains the known P2 case above. |
| Configuration | No configuration items added. |
| Compatibility | Connector plugin API and pinned test both advance 30.0 to 31.0; recorded outer SPI signatures are updated. No persistent-format change. |
| Thrift/protobuf | No gensrc definitions changed. |
| Parallel paths | SHOW CREATE, SHOW CATALOG and catalogs() share the union matcher; audit and both FE logging paths recognize the new interface. ES/JDBC legacy aliases and inherited metastore aliases are covered. |
| Conditional behavior | Namespace boundary/suffix matching and no-marker matching are consistent with the declared contract; the legacy Fluss floor is preserved. Admission rejects duplicate providers before contributing secrets. |
| Test coverage | Inspected connector/metastore declarations, SPI matching/copying, registration/rejection, loader failures, ordinary ALTER encryption and audit regression checks. Missing Unicode coverage corresponds to the known P1; missing shared-static-registry coverage corresponds to the known P2. |
| Changed expected results | No golden output files changed; new assertions align with ordinary ASCII behavior and the intended registration/matching rules. |
| Observability | Existing skip warnings identify failed providers and directories; no additional metric requirement identified for startup/property rendering. |
| Transactions and persistence | No transaction, EditLog write/replay or failover protocol changes. |
| Data writes and atomicity | No data-write or storage-visibility path changed; the table/version/delete-bitmap core invariants are unaffected. |
| FE/BE variables | No new cross-process variable or send path. |
| Performance and memory | Reflection/union construction occurs at discovery; per-key matching scans small declaration lists. No material new allocation, native-memory or hot-path issue established. |
| Other issues | No further distinct substantiated regression found after full changed-file and upstream/downstream sweeps. |
There was no additional user-provided focus. Coverage is static only: no builds, tests, benchmarks or runtime validation were executed, and no repository source was modified. Arbitrary raw SDK option names, credentials embedded in URLs, and new keys from unloaded plugins remain the documented pre-existing scope limits. No unresolved static review questions remain.
Review converged after 1 round(s).
Existing P0/P1 findings confirmed for this head: #70338 (comment)
…roperties to mask ### Background SHOW CREATE CATALOG, and the other places that print catalog properties, replace a value with `*XXX` when `DatasourcePrintableMap.isSensitiveKey` matches its key. The keys come from a list hard-coded in fe-core and from the keys that filesystem plugins report at startup through `FileSystemProvider.sensitivePropertyKeys()`. A connector plugin tells the engine about its security-relevant properties through `ConnectorProvider#securitySurface()`, which today lists only the SSRF endpoint keys and the jdbc `driver_url` keys that the CREATE and ALTER CATALOG gates check. ### What problem does this PR solve? Issue Number: None Related PR: apache#69588 (added the connector security surface this extends), apache#69575 (master loads the Lance catalogs that 4.1.4 persisted), apache#65730 (branch-4.1's Lance catalog, whose two REST secrets 4.x masks in fe-core) PR Type: Enhancement Problem Summary: A connector plugin parses its own catalog properties, but their secret values are masked only if fe-core hard-codes the key names, which the connector framework rules out. The Lance connector being ported from branch-4.x needs this for `lance.rest.bearer-token` and `lance.rest.api-key`: 4.1.4 masks them, and without a way to declare them master would print them in clear. `ConnectorSecuritySurface` can now declare secret keys, and the engine adds an admitted provider's keys to its masking list at FE startup. Nothing changes after an upgrade until a connector declares keys, and none does in this PR. ### Details - **Before and after:** Before, a property that only a connector knows, such as `lance.rest.bearer-token`, showed its value in SHOW CREATE CATALOG. After, a key the connector declares with `ConnectorSecuritySurface.ofSensitiveKeys(...)`, or with `.withSensitiveKeys(...)` on top of its other declarations, shows `*XXX`, matched case-insensitively like the engine's own list; undeclared keys show as before. - **Configs and defaults:** None. The default surface declares no secret. - **Impact on existing users:** None for SQL, metadata, or configs. As for every change to the connector SPI surface, the plugin API major moves from 30.0 to 31.0, so a third-party connector plugin built against 30.x has to be rebuilt; the in-tree plugins are built with the FE. ### Testing All runs were local (macOS), on master `59c49656240` plus this change: - `mvn -f fe/pom.xml -pl :fe-connector-spi -am test`: the module's 156 tests pass. They include the new `ConnectorSecuritySurfaceTest`, which covers combining secrets with the SSRF and driver_url declarations, appending, and that no existing surface, `EMPTY` included, is changed. They also include `ConnectorPluginSurfaceTest` with the refreshed baseline and 31.0. - fe-core, 12 classes and 157 tests, all passing. `ConnectorPluginManagerTest` has three new cases: - an admitted provider's key is masked, case-insensitively, in the rendering SHOW CREATE CATALOG uses, and undeclared keys stay visible; - a refused provider adds nothing; - the `registerProvider` test seam registers keys too. - The other eleven classes register providers or check the security surface, such as `PluginDrivenExternalCatalog{SsrfGate,DriverUrlGate}Test`, `CatalogFactoryPluginRoutingTest` and `DatasourcePrintableMapTest`. - Six mutations, each caught by the test written for it: - not registering; - registering before the admission checks; - skipping the test seam; - three in `withSensitiveKeys`: dropping the SSRF keys, replacing instead of appending, and returning a mutable list. No connector declares keys yet, so nothing changes end to end. The Lance connector, ported in follow-up PRs, will be the first to declare them. ### Notes for Reviewers - The keys are read when a provider is registered and added only after the admission checks in `ConnectorPluginManager#registerDiscovered` pass, so a refused provider adds nothing. Connector plugins load only at FE startup, before any catalog property is shown, as filesystem plugins do. `registerProvider`, the seam tests use to stand in for a plugin, registers the keys too, like `FileSystemPluginManager#registerProvider`. - A catalog whose connector plugin is not loaded (a degraded catalog) keeps only the engine's own list. - Why the surface rather than a provider method like `FileSystemProvider.sensitivePropertyKeys()`: the connector SPI already gathers its security-relevant property keys on the surface, and a connector with both an endpoint and a secret declares them on one object. ### 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 code, the tests, and this description - Human review: not stated - Prompts: The author asked for a generic connector SPI to mask a connector's secret catalog properties instead of adding the Lance keys to fe-core's list, approved the `ConnectorSecuritySurface` design, and asked for it as a separate PR ahead of the Lance connector port. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…udit text ### Background SHOW CREATE CATALOG, SHOW CATALOG, the `catalogs()` table function and the audit text of CREATE CATALOG replace a property value with `*XXX` when `DatasourcePrintableMap.isSensitiveKey` matches its key. Until now the keys came from a list in fe-core and from the keys filesystem plugins report at startup. A connector plugin marks its secret properties `sensitive = true`, but that mark only masks the connector's own log lines. A connector tells the engine about its security-relevant properties through `ConnectorProvider#securitySurface()`. ### What problem does this PR solve? Issue Number: None Related PR: apache#69588 (added the connector security surface this extends), apache#45164 (introduced the unmasked ALTER CATALOG audit text), apache#27115 (masked it for the statement apache#45164 replaced), apache#59502 (added `iceberg.jdbc.password`), apache#54523 (added the Paimon REST DLF keys), apache#65730 (branch-4.1's Lance catalog, whose REST secrets the Lance port will declare on this surface) PR Type: Bugfix Problem Summary: Catalog secrets that only a connector knows are on no masking list, so SHOW CREATE CATALOG and the other renderings print them in clear: `iceberg.jdbc.password`, `paimon.jdbc.password`, the Paimon REST DLF key pair, `mc.access_key`, and credentials passed through under `trino.` or `adbc.`. Separately, `ALTER CATALOG ... SET PROPERTIES` is written to the audit log unmasked, whatever the key. Each connector now declares its secrets on its security surface, taken from its `sensitive` marks, and the surface can also declare a namespace of secret keys for options passed through unread; Fluss's lake options were hard-coded in fe-core for that reason. The engine masks the union, with fe-core's own list kept as the floor. ALTER CATALOG is masked like CREATE CATALOG. ### Details - **Change map:** 42 files, +1035 −76 over two commits: production code +376 −48, tests +645 −24, the connector README +13 −3, the plugin API version in `fe/fe-connector/pom.xml`. The first commit adds secret keys to the surface (reviewed alone before). The second does the rest: 1. Key namespaces on the SPI: `withSensitiveKeyPrefix` and `SensitiveKeyPrefix.covers`, which holds the matching rule; a collection form of `withSensitiveKeys` (`ConnectorSecuritySurface`, +130). 2. Engine: `isSensitiveKey` matches registered namespaces, the Fluss rule becomes one of them; `ConnectorPluginManager` registers them and reads every surface before it publishes a provider (+70). 3. Connectors: every provider declares its secrets; `MetaStoreProviders.sensitivePropertyKeys()` gives the union of a plugin's metastore backends; `IcebergJdbcMetaStoreProvider` reports its keys like its siblings; the keytab paths lose their `sensitive` mark (+140). 4. ALTER CATALOG audit masking (+16). - **Frequency:** Always. Create or alter a catalog that sets one of the keys above, then run SHOW CREATE CATALOG, SHOW CATALOG `<name>` or `select * from catalogs()`, or read the audit log after an ALTER CATALOG ... SET PROPERTIES. - **Root cause:** `isSensitiveKey` consulted only fe-core's list and the filesystem providers' keys. A connector's `sensitive` marks fed only `toMaskedString`; `MetaStoreProvider.sensitivePropertyKeys()` existed for this, but nothing called it, and the Iceberg JDBC backend did not implement it. For ALTER CATALOG, `AlterCatalogPropertiesCommand` does not implement `NeedAuditEncryption` and `LogicalPlanBuilderForEncryption` has no `visitAlterCatalogProperties`, so `AuditLogHelper` records the original text; the statement it replaced in apache#45164 was masked by apache#27115. - **Why existing tests missed it:** Unit tests: `DatasourcePrintableMapTest` checks fe-core's own keys, the provider surface tests checked only the SSRF and driver_url keys, and `EncryptSQLTest` and `RepositoryAuditEncryptionTest` cover CREATE CATALOG, repositories and resources but not ALTER CATALOG. Regression: `audit/test_external_catalog_credential_masking` creates only Iceberg REST catalogs, whose keys are on fe-core's list, and never alters one. - **Fix:** The engine must not name a connector's properties, so the connectors declare their secrets, from the same `sensitive` marks that mask their logs; jdbc and es declare both spellings their legacy prefix allows, and trino and adbc declare the namespace they pass through, a key there being a secret when its rest names `key`, `secret`, `password`, `token` or `credential` (also `authorization` for ADBC). fe-core keeps the connector keys it lists, so a catalog whose plugin is not loaded masks what it masked before. A provider whose `securitySurface()` throws at startup is skipped, as a filesystem provider is, instead of stopping FE. - **Other paths and branches:** Fixed here: every rendering above and FE's statement logging, which share `isSensitiveKey` and `NeedAuditEncryption`. Not covered, since a key name cannot express them safely: raw Iceberg SDK keys copied through (`token`, `credential`), Paimon options re-keyed from `paimon.*`, Hadoop `fs.*` keys, and secrets inside a URL. The ALTER CATALOG gap is on branch-4.0, 4.1 and 4.2 (from apache#45164); branch-3.1 still uses the masked statement. branch-4.1 and 4.2 also print the connector keys above in clear; without the connector SPI their fix differs and is tracked separately. - **Behavior change:** The namespaces also mask non-secrets whose names carry those words, such as `trino.hive.s3.aws-access-key` or a keystore path. Keytab paths, which no release masks, are no longer masked in connector log lines: marked `sensitive`, `hadoop.kerberos.keytab` would be masked in every storage vault and resource. ### Testing All runs were local (macOS), on master `f5c993f2c94` plus this change: - Regression `audit/test_external_catalog_credential_masking`, extended with an Iceberg JDBC catalog: SHOW CREATE CATALOG masks `iceberg.jdbc.password`, and the audit texts of its CREATE CATALOG and of an ALTER CATALOG that sets the password and `s3.secret_key` hold only `*XXX`. Against the local cluster's FE without this change it fails at the first new check, SHOW CREATE CATALOG printing the password; with a FE built from this change it passes. - fe-connector-spi: all 160 tests, with the refreshed surface baseline (+4 lines); the metastore modules: all tests; ten connector modules: 29 classes, 511 tests, including each connector's surface test, which checks real key names. - fe-core: 26 classes, 341 tests, including `ConnectorPluginManagerTest`, `DatasourcePrintableMapTest`, `EncryptSQLTest` and the audit and plugin gate tests; 14 of them again after the last change to registration. - Twenty mutations, each caught by its test: for example, reverting either half of the ALTER CATALOG fix (`EncryptSQLTest` sees the statement in clear), matching a marker anywhere in the key, an empty metastore union, or re-marking a keytab. ### Notes for Reviewers - `SensitiveKeyPrefix.covers` lives on the SPI so that a connector's test can check its declaration against real key names and the engine applies the same rule; the Fluss floor in fe-core uses it too. - The entries both fe-core and a connector declare are deliberate: the floor is frozen, as the comment in `DatasourcePrintableMap` says. - The plugin API stays at 31.0, the first commit's bump. Hudi declares nothing: it owns no catalog properties. ### Release note SHOW CREATE CATALOG, SHOW CATALOG, the `catalogs()` table function and the audit log now mask `iceberg.jdbc.password`, `paimon.jdbc.password`, `paimon.rest.dlf.access-key-id`, `paimon.rest.dlf.access-key-secret`, `mc.access_key`, and the `trino.*` and `adbc.*` properties whose names carry `key`, `secret`, `password`, `token` or `credential` (`authorization` too for `adbc.*`). ALTER CATALOG ... SET PROPERTIES statements are masked in the audit log like CREATE CATALOG. ### Check List (For Author) - Behavior changed: Yes (see Details) - Does this need documentation: No - Pre-submission status: Ready ### AI Disclosure - Tool: Claude Code (Claude Opus 5.5) - Generated: the code, the tests, and this description - Human review: not stated - Prompts: The author first asked for a generic connector SPI to mask a connector's secret catalog properties instead of adding the Lance keys to fe-core's list. Then, in this PR: let the surface express prefix rules such as Fluss's, mask the union of the engine's and the connectors' keys, wire every connector, keep fe-core's existing entries as the floor, mask whatever is marked `sensitive` but stop marking keytab paths, and fix the unmasked ALTER CATALOG audit text on master only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n its own ServiceLoader ### Background FE reads every connector provider's security surface at startup, before it publishes the provider, and skips a provider whose surface throws, so that one broken plugin directory does not stop FE. The Hive, Iceberg and Paimon providers build theirs from their metastore backends' secret keys, which `MetaStoreProviders` collects in a static initializer that runs a `ServiceLoader` over the backends the plugin's jars register. ### What problem does this PR solve? Issue Number: None Related PR: None PR Type: Bugfix Problem Summary: A `ServiceLoader` that cannot load a backend its descriptors name throws `ServiceConfigurationError`, an `Error` that is neither a `RuntimeException` nor a `LinkageError`, and a static initializer rethrows an `Error` as it is. The plugin directory path caught only those two and the classpath path only `RuntimeException`, so a descriptor naming a backend that is not there, such as one in a jar an earlier release left in a connector's `lib/`, stopped FE at startup; before the previous commit, which reads the surfaces at startup, it failed only that connector's catalogs. Both paths now skip the provider on a `ServiceConfigurationError` as well, and the plugins next to it still load. ### Release note None Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
morningman
added a commit
to morningman/doris
that referenced
this pull request
Nov 13, 2026
…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>
morningman
added a commit
to morningman/doris
that referenced
this pull request
Nov 13, 2026
…characters ### Background Several FE features rewrite the text of a statement by cutting the original SQL string at token positions from the parser. Audit masking replaces credential properties and passwords with `*XXX` before the statement reaches `fe.audit.log`, `__internal_schema.audit_log` and the FE log; CREATE / ALTER VIEW and CREATE MATERIALIZED VIEW expand `*` and qualify names in the definition they store; the parser reads `/*+ ... */` hints from comment tokens. The lexer counts its input by Unicode code point, while a Java `String` holds a supplementary character, such as an emoji, as two `char`s. ### What problem does this PR solve? Issue Number: None Related PR: apache#47595 (introduced the bug in audit masking), apache#32743 (introduced the bug in view rewriting), apache#70338 (its review found the masking leak) PR Type: Bugfix Problem Summary: After each supplementary character in a statement, these token positions point one `char` too early, so the text is cut at the wrong place. A credential after or inside such characters stays partly or fully in clear text in the audit log, CREATE VIEW fails with a syntax error, CREATE MATERIALIZED VIEW stores a broken query, and a hint after such a character is silently ignored. This PR converts the positions to `String` offsets, or reads the text through the token, at the five places that cut the original SQL. Tests are 89% of the diff. ### Details - **Change map:** production code +33 -33 in 3 files; tests +240 in 5 files (FE unit tests and one regression suite); generated `.out` +16. (1) `BaseViewInfo.rewriteSql`, shared by audit masking, views and MTMV, converts its positions; the sync-MV builder drops its private copy of it, and `rewriteProjectsToUserDefineAlias` replaces the select list through it. (2) `NereidsParser.getHintMap` reads hints from the token text. (3) `NereidsParser.tryParseExplainPlan` converts the start of the EXPLAIN PLAN body. - **Frequency:** Always, when a character above U+FFFF (an emoji, CJK Extension B) comes before or inside the rewritten part, for example a catalog COMMENT of emoji in front of `'s3.secret_key' = 'secret'`. - **Root cause:** `CaseInsensitiveStream.fromString` keeps ANTLR's code-point stream for any input with a surrogate (before apache#67452 every input used `CharStreams.fromString`), so `Token.getStartIndex()` and `getStopIndex()` count code points. The five places in the Change map used them as `String` indexes. - **Why existing tests missed it:** Unit tests: the masking (`EncryptSQLTest`, `RepositoryAuditEncryptionTest`), view and MTMV (`CreateViewTest`, `CreateMTMVCommandTest`) and hint tests (`LeanTokenModeTest`) use only BMP text, where both kinds of position coincide; `CaseInsensitiveStreamTest` uses an emoji but checks only the stream's own positions. Regression: `audit/test_external_catalog_credential_masking` and the `ddl_p0/create_view_nereids` suites have no supplementary characters, and the masking suite only checks that the whole credential is absent, which a leaked tail passes. - **Fix:** Convert where the text is cut. `rewriteSql` converts each position with `String.offsetByCodePoints`, walking the sorted positions once so the cost stays linear; `rewriteProjectsToUserDefineAlias` reads expressions through the token's input stream; `getHintMap` reads the hint from the token text, as `removeCommentAndTrimBlank` does. Changing the stream to report `String` positions was rejected: apache#67452 made code-point positions part of `CaseInsensitiveStream`'s contract, and `CaseInsensitiveStreamTest` pins them. - **Other paths and branches:** Fixed here: the 19 masking visitors of `LogicalPlanBuilderForEncryption` with every caller of `geneEncryptionSQL` (audit log, FE statement log, `_query_plan`) and the streaming insert job; the view, MTMV and sync-MV rewrites. Not affected: the other uses of token positions read text through the token's own stream (such as `getOriginSql`) or only compare positions with each other. branch-3.1, branch-4.0, branch-4.1 and branch-4.2 have the same code. Introduced by apache#32743 (views), apache#47595 (masking), apache#33699 (sync MV) and apache#33262 (EXPLAIN PLAN); hints since at least apache#39590. ### Testing - New unit tests fail on master `f1bc8b3690c` and pass with the fix, run locally: `EncryptSQLTest#testEncryptionAfterSupplementaryCharacters`, through `MysqlConnectProcessor` to the audit event (emoji inside a credential, in a catalog COMMENT, in the first of two property clauses, in a user name before `PASSWORD(...)`; master left `secret`, `abc`, `123` and `123456` in clear text); `CreateViewTest#testCreateViewRewritesAfterSupplementaryCharacters` (master: `mismatched input '`e`'`); `CreateMTMVCommandTest#testCreateMTMVRewritesQuerySqlAfterSupplementaryCharacters` (master kept ``SELECT '😀😀' A`e` e`internal`...``); `SupplementaryCharacterOffsetTest` (master dropped the hint, cut the EXPLAIN PLAN body to `AN SELECT 100`, and rewrote the sync MV to `avg_stateg(k)))`). - New regression suite `ddl_p0/create_view_nereids/test_create_view_supplementary_chars`: fails without the fix with the same syntax error and passes with it; the `.out` is generated by the script. Run locally on one FE and one BE, an older master FE build with the three changed classes swapped in. - 27 existing FE UT classes around the change (467 tests) pass. Reverting each part of the fix alone, moving the end of a replaced span by one, or not advancing the position cursor each fails a new test; the off-by-one also fails existing ASCII cases. - By hand on that cluster, without the fix: the secret stayed in `audit_log`, `/* 😀 */ select /*+ SET_VAR(query_timeout=1) */ sleep(3)` ran for 3 s, and an MTMV over `select '😀😀' as e, ...` was created but failed to refresh (`Nereids parse failed`). With the fix: `"s3.secret_key" = "*XXX")`, and the query times out after 1 s. ### Notes for Reviewers - `getHintMap` loses its `sql` parameter, which nothing reads any more; its three callers are in `NereidsParser`. - `rewriteProjectsToUserDefineAlias` replaces the select list through `rewriteSql` instead of `StringUtils.overlay`, so the conversion lives in one method. - Stored definitions are not repaired: an MTMV created by an affected version from such a query keeps its broken definition and has to be recreated. ### Release note Fix passwords and credentials left partly or fully unmasked in the audit log and the FE log when a supplementary character, such as an emoji, comes before or inside them. Also fix CREATE VIEW failing, CREATE MATERIALIZED VIEW storing a broken query, and hints such as SET_VAR being ignored after such a character; materialized views already created with a broken query must be recreated. ### 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: After the review bot on apache#70338 reported the masking leak, the author confirmed it is an existing engine defect shared by all masking paths and asked for a separate engine PR that fixes it, followed by a reply on apache#70338 with the link. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
morningman
pushed a commit
to morningman/doris
that referenced
this pull request
Nov 13, 2026
### Background
Connector plugins (Hudi, and the coming Lance connector) describe their columns with the SPI's `ConnectorType`, and fe-core's `ConnectorColumnConverter` turns each into a Doris type. Connectors put a temporal column's fractional-second scale in the type's precision field: `("DATETIMEV2", 3, 0)` is `datetime(3)`, and Hudi's `HudiTypeMapping` sends `("TIMEV2", 3, 0)` for an Avro `time-millis` column and `("TIMEV2", 6, 0)` for `time-micros`.
### What problem does this PR solve?
Issue Number: None
Related PR: apache#62183 (introduced the converter without a TIMEV2 case), apache#64304 (moved Hudi onto it)
PR Type: Bugfix
Problem Summary:
Since Hudi became a connector plugin, its `time-millis` and `time-micros` columns show as `time` instead of `time(3)` and `time(6)`, which is what 4.x shows and what `HudiTypeMapping` asks for: the converter drops the scale and builds a scale-0 TIMEV2. The Lance connector, ported in a follow-up, maps Arrow `time32(ms)` and `time64` the same way and would lose its scale too. The converter now reads the TIMEV2 scale from the precision field, as it already does for DATETIMEV2.
### Details
- **Frequency:** Always, for every connector column whose `ConnectorType` is TIMEV2 with a scale; today that is every Hudi `time-millis` / `time-micros` column on master. Found while porting the Lance connector, by reading the converter, and confirmed by the unit test below.
- **Root cause:** `ConnectorColumnConverter.convertScalarType` reads the precision field for CHAR, VARCHAR, VARBINARY, the decimals, DATETIMEV2 and TIMESTAMPTZ, but has no TIMEV2 case, so TIMEV2 falls through to `ScalarType.createType("TIMEV2")`, which returns the scale-0 `ScalarType.TIMEV2`.
- **Why existing tests missed it:** Unit tests: `ConnectorColumnConverterTest` checks decimals, CHAR/VARCHAR lengths and the TIMESTAMPTZ extra info but has no TIMEV2 case, and `HudiTypeMappingTest` asserts the `("TIMEV2", 3, 0)` the plugin emits but stops before fe-core converts it. Regression: no suite under `regression-test/suites` reads a Hudi time column.
- **Fix:** The TIMEV2 case uses the precision field as the scale when one is given; without one the type stays the scale-0 TIMEV2 it has always been. The fix sits in the converter because every connector goes through it. The reverse direction, `toConnectorType`, is unchanged: only the read-only Hudi and Lance connectors produce TIMEV2, and the JDBC connector maps TIME to STRING.
- **Other paths and branches:** `toConnectorType` is not affected for reads (see Fix). branch-4.1 and branch-4.2 are not affected: they have no `ConnectorColumnConverter`, and their fe-core Hudi mapping (`HudiUtils`) builds `createTimeV2Type(3)` / `createTimeV2Type(6)` directly. The bug is on master since apache#64304.
### Testing
- `ConnectorColumnConverterTest#timeScaleRidesInThePrecisionSlot` (new): fails before the fix with `expected: <timev2(3)> but was: <timev2(0)>` and passes after; it also pins that a TIMEV2 without a precision stays scale 0. Run locally on master `f5c993f2c94` with apache#70338 applied, which touches neither file.
- `ConnectorColumnConverterTest` (35 tests) and `HudiTypeMappingTest` (17 tests): pass locally on the same tree.
### 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: code, tests, this description
- Human review: not stated
- Prompts: The author asked to port the branch-4.2 Lance FE read path to master as a connector plugin; this fix came out of that port, and the author's rule for such finds is to fix them in their own PR.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
morningman
force-pushed
the
b2m-t38c-connector-sensitive-keys
branch
from
November 13, 2026 08:39
ae2cebe to
b1fcc69
Compare
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Reviewed all 43 changed paths at b1fcc6942705e80c80a7ae650b0006462d34df69 by static inspection, including related callers, consumers, tests and existing threads. No new inline findings.
Existing P1 4496273297 still applies to this head: the new ALTER visitor supplies code-point token offsets to BaseViewInfo.rewriteSql, which slices UTF-16. Supplementary characters can leave a credential suffix in audit/statement text. The separate fix referenced in the reply is absent from this source. This finding is carried by ID without another inline comment.
For existing P2 4492689479, both loading paths now catch the original ServiceConfigurationError. The directory path also catches LinkageError and discards its loader. A malformed shared test/embedded classpath can still produce a subsequent NoClassDefFoundError after metastore static initialization is poisoned; the builtin catch omits LinkageError. This remains within that existing thread's startup-failure mechanism. Production connector implementations are loaded from separate directories; their fe-core classpath dependencies are test-scoped. No duplicate inline finding is added.
The inspected control flow is:
Env startup -> provider securitySurface -> register exact keys / namespaces
-> DatasourcePrintableMap
-> SHOW CREATE / SHOW / catalogs()
ALTER command -> NeedAuditEncryption -> encryption visitor -> same masking policy
-> BaseViewInfo.rewriteSql -> audit / FE statement logs
Critical checkpoint conclusions:
- Goal and proof: declarations reach catalog renderers and ordinary ALTER audit masking. Added unit and regression cases exercise these paths; the known Unicode case remains uncovered by this PR's additions.
- Scope and reuse: changes stay focused on masking metadata, startup registration and tests/docs, reusing annotations and the existing security surface.
- Concurrency and locks: production registration finishes before image loading/catalog access; the manager is published through a volatile reference. Immutable declarations and copy-on-write prefix iteration add no query-time writer or lock-order change. No new remote work under metadata locks was found.
- Lifecycle/static initialization: defining-classloader metastore discovery, publication order and rejected-directory disposal were traced. The original nested-loader failure and the classpath residual have the dispositions above.
- Configuration: no new configuration item or dynamic-update contract.
- Compatibility: connector API and its pinned test advance from 30.0 to 31.0, with the changed public signature baseline updated. Existing endpoint/driver-url declarations survive secret builders. No storage/wire format changes.
- Thrift/protobuf: no changed definitions or field IDs.
- Parallel paths: CREATE/ALTER audit, statement logging, SHOW renderings and catalog property consumers share the policy. JDBC/ES aliases, metastore flavor unions and ADBC/Trino/Fluss namespaces were checked; the legacy masking floor is preserved.
- Special conditions: prefix matching is case-insensitive, searches only the suffix for markers and supports an entire namespace. Keytab-path declassification matches the stated behavior. Missing-plugin and raw SDK/URL-embedded credential limits are explicit existing limits.
- Test coverage: inspected positive/negative matching, immutable builders, provider declarations, version gates, isolation, nested-loader faults, SHOW and ASCII ALTER audit cases. No builds, tests, benchmarks or runtime validation were executed under this static-only contract.
- Test results: no generated expected-output files change; no executed-result claim is made.
- Observability: skip warnings retain provider/directory and cause; no additional masking metric is needed.
- Persistence/failover: no EditLog, transaction or persistence-format changes; startup registration also covers restored catalog properties.
- Data writes/atomicity: no new data-write or commit behavior; data visibility, partition versions and delete bitmaps are unaffected.
- FE/BE propagation: no new cross-process variables or transmission sites.
- Performance/memory: reflection and unions occur at startup; rendering scans a small registered prefix list. No demonstrated performance or memory-ownership defect was found.
- Other issues and user focus: no additional focus was supplied. No further distinct verified issue or unreviewed changed scope remains after the reviewer rechecks. The known P1 remains blocking.
Coverage limits: static reasoning only; runtime behavior and the author's reported test runs were not independently executed. These limits are separate from review convergence.
Review converged after 2 round(s).
Existing P0/P1 findings confirmed for this head: #70338 (comment)
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
SHOW CREATE CATALOG, SHOW CATALOG, the
catalogs()table function and the audit text of CREATE CATALOG replace a property value with*XXXwhenDatasourcePrintableMap.isSensitiveKeymatches its key. Until now the keys came from a list in fe-core and from the keys filesystem plugins report at startup. A connector plugin marks its secret propertiessensitive = true, but that mark only masks the connector's own log lines. A connector tells the engine about its security-relevant properties throughConnectorProvider#securitySurface().What problem does this PR solve?
Issue Number: None
Related PR: #69588 (added the connector security surface this extends), #45164 (introduced the unmasked ALTER CATALOG audit text), #27115 (masked it for the statement #45164 replaced), #59502 (added
iceberg.jdbc.password), #54523 (added the Paimon REST DLF keys), #65730 (branch-4.1's Lance catalog, whose REST secrets the Lance port will declare on this surface)PR Type: Bugfix
Problem Summary:
Catalog secrets that only a connector knows are on no masking list, so SHOW CREATE CATALOG and the other renderings print them in clear:
iceberg.jdbc.password,paimon.jdbc.password, the Paimon REST DLF key pair,mc.access_key, and credentials passed through undertrino.oradbc.. Separately,ALTER CATALOG ... SET PROPERTIESis written to the audit log unmasked, whatever the key. Each connector now declares its secrets on its security surface, taken from itssensitivemarks, and the surface can also declare a namespace of secret keys for options passed through unread; Fluss's lake options were hard-coded in fe-core for that reason. The engine masks the union, with fe-core's own list kept as the floor. ALTER CATALOG is masked like CREATE CATALOG.Details
fe/fe-connector/pom.xml. The first commit adds secret keys to the surface (reviewed alone before). The second does the rest, and the third keeps a surface that fails in the connector's ownServiceLoaderfrom stopping FE:withSensitiveKeyPrefixandSensitiveKeyPrefix.covers, which holds the matching rule; a collection form ofwithSensitiveKeys(ConnectorSecuritySurface, +130).isSensitiveKeymatches registered namespaces, the Fluss rule becomes one of them;ConnectorPluginManagerregisters them and reads every surface before it publishes a provider, skipping a provider whose surface throws (+84).MetaStoreProviders.sensitivePropertyKeys()gives the union of a plugin's metastore backends;IcebergJdbcMetaStoreProviderreports its keys like its siblings; the keytab paths lose theirsensitivemark (+140).<name>orselect * from catalogs(), or read the audit log after an ALTER CATALOG ... SET PROPERTIES.isSensitiveKeyconsulted only fe-core's list and the filesystem providers' keys. A connector'ssensitivemarks fed onlytoMaskedString;MetaStoreProvider.sensitivePropertyKeys()existed for this, but nothing called it, and the Iceberg JDBC backend did not implement it. For ALTER CATALOG,AlterCatalogPropertiesCommanddoes not implementNeedAuditEncryptionandLogicalPlanBuilderForEncryptionhas novisitAlterCatalogProperties, soAuditLogHelperrecords the original text; the statement it replaced in [Enhancement] (nereids)implement alterCatalogPropertiesCommand in ner… #45164 was masked by [enhancement](audit) hide password and other sensitive information in log and audit log #27115.DatasourcePrintableMapTestchecks fe-core's own keys, the provider surface tests checked only the SSRF and driver_url keys, andEncryptSQLTestandRepositoryAuditEncryptionTestcover CREATE CATALOG, repositories and resources but not ALTER CATALOG. Regression:audit/test_external_catalog_credential_maskingcreates only Iceberg REST catalogs, whose keys are on fe-core's list, and never alters one.sensitivemarks that mask their logs; jdbc and es declare both spellings their legacy prefix allows, and trino and adbc declare the namespace they pass through, a key there being a secret when its rest nameskey,secret,password,tokenorcredential(alsoauthorizationfor ADBC). fe-core keeps the connector keys it lists, so a catalog whose plugin is not loaded masks what it masked before. A provider whosesecuritySurface()throws at startup is skipped, as a filesystem provider is, instead of stopping FE; that includes theServiceConfigurationErroraServiceLoaderthrows when a descriptor names a backend it cannot load, which is how the Hive, Iceberg and Paimon surfaces fail, since they read their metastore backends' keys through one.isSensitiveKeyandNeedAuditEncryption. Not covered, since a key name cannot express them safely: raw Iceberg SDK keys copied through (token,credential), Paimon options re-keyed frompaimon.*, Hadoopfs.*keys, and secrets inside a URL. The ALTER CATALOG gap is on branch-4.0, 4.1 and 4.2 (from [Enhancement] (nereids)implement alterCatalogPropertiesCommand in ner… #45164); branch-3.1 still uses the masked statement. branch-4.1 and 4.2 also print the connector keys above in clear; without the connector SPI their fix differs and is tracked separately.trino.hive.s3.aws-access-keyor a keystore path. Keytab paths, which no release masks, are no longer masked in connector log lines: markedsensitive,hadoop.kerberos.keytabwould be masked in every storage vault and resource.Testing
All runs were local (macOS), on master
f5c993f2c94plus this change:audit/test_external_catalog_credential_masking, extended with an Iceberg JDBC catalog: SHOW CREATE CATALOG masksiceberg.jdbc.password, and the audit texts of its CREATE CATALOG and of an ALTER CATALOG that sets the password ands3.secret_keyhold only*XXX. Against the local cluster's FE without this change it fails at the first new check, SHOW CREATE CATALOG printing the password; with a FE built from this change it passes.ConnectorPluginManagerTest,DatasourcePrintableMapTest,EncryptSQLTestand the audit and plugin gate tests; 14 of them again after the last change to registration, and 7 classes, 76 tests, after the third commit. Its two newConnectorPluginManagerTestcases, a plugin directory whoselib/holds a jar with a service descriptor naming a missing metastore-style backend (the plugin next to it must still load) and the same descriptor on the classpath, fail without the third commit with thatServiceConfigurationErrorescapingloadPluginsandloadBuiltins, and pass with it.EncryptSQLTestsees the statement in clear), matching a marker anywhere in the key, an empty metastore union, re-marking a keytab, or droppingServiceConfigurationErrorfrom either startup catch (each fails only its own new case).Notes for Reviewers
SensitiveKeyPrefix.coverslives on the SPI so that a connector's test can check its declaration against real key names and the engine applies the same rule; the Fluss floor in fe-core uses it too.DatasourcePrintableMapsays.Release note
SHOW CREATE CATALOG, SHOW CATALOG, the
catalogs()table function and the audit log now maskiceberg.jdbc.password,paimon.jdbc.password,paimon.rest.dlf.access-key-id,paimon.rest.dlf.access-key-secret,mc.access_key, and thetrino.*andadbc.*properties whose names carrykey,secret,password,tokenorcredential(authorizationtoo foradbc.*). ALTER CATALOG ... SET PROPERTIES statements are masked in the audit log like CREATE CATALOG.Check List (For Author)
AI Disclosure
sensitivebut stop marking keytab paths, and fix the unmasked ALTER CATALOG audit text on master only. Then, from a review finding: keep a connector whose security surface fails in its ownServiceLoaderfrom stopping FE.