Skip to content

[enhancement](audit) hide password and other sensitive information in log and audit log - #27115

Merged
dataroaring merged 2 commits into
apache:masterfrom
nextdreamblue:hidden_audit
Nov 24, 2023
Merged

dataroaring merged 2 commits into
apache:masterfrom
nextdreamblue:hidden_audit

Conversation

@nextdreamblue

Copy link
Copy Markdown
Contributor

Proposed changes

Issue Number: close #xxx

Further comments

If this is a relatively large or complex change, kick off the discussion at [email protected] by explaining why you chose the solution you did and what alternatives you considered, etc...

@nextdreamblue

Copy link
Copy Markdown
Contributor Author

run buildall

Signed-off-by: nextdreamblue <[email protected]>
@nextdreamblue nextdreamblue changed the title [enhancement](audit) hide password and other sensitive information inlog and audit log [enhancement](audit) hide password and other sensitive information in log and audit log Nov 16, 2023
@doris-robot

Copy link
Copy Markdown

(From new machine)TeamCity pipeline, clickbench performance test result:
the sum of best hot time: 44.62 seconds
stream load tsv: 566 seconds loaded 74807831229 Bytes, about 126 MB/s
stream load json: 18 seconds loaded 2358488459 Bytes, about 124 MB/s
stream load orc: 65 seconds loaded 1101869774 Bytes, about 16 MB/s
stream load parquet: 32 seconds loaded 861443392 Bytes, about 25 MB/s
insert into select: 28.6 seconds inserted 10000000 Rows, about 349K ops/s
storage size: 17100815431 Bytes

@nextdreamblue

Copy link
Copy Markdown
Contributor Author

run buildall

@xy720 xy720 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@github-actions

Copy link
Copy Markdown
Contributor

PR approved by at least one committer and no changes requested.

@github-actions github-actions Bot added approved Indicates a PR has been approved by one committer. reviewed labels Nov 22, 2023
@github-actions

Copy link
Copy Markdown
Contributor

PR approved by anyone and no changes requested.

@Yulei-Yang Yulei-Yang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nextdreamblue

Copy link
Copy Markdown
Contributor Author

run clickbench

@doris-robot

Copy link
Copy Markdown
TPC-H test result on machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
Tpch sf100 test result on commit f718937eacb01ea1e3ac4ba2e69559c5f7dce193, data reload: false

run tpch-sf100 query with default conf and session variables
q1	4914	4663	4653	4653
q2	354	160	159	159
q3	2022	1911	1886	1886
q4	1385	1223	1251	1223
q5	3957	3926	4058	3926
q6	247	131	131	131
q7	1414	883	908	883
q8	2733	2761	2762	2761
q9	9702	9715	9526	9526
q10	3481	3559	3556	3556
q11	373	247	249	247
q12	439	292	295	292
q13	4547	3812	3832	3812
q14	308	289	303	289
q15	583	538	522	522
q16	663	578	585	578
q17	1114	926	906	906
q18	7821	7409	7416	7409
q19	1685	1669	1653	1653
q20	520	330	316	316
q21	4368	3950	3906	3906
q22	466	367	364	364
Total cold run time: 53096 ms
Total hot run time: 48998 ms

run tpch-sf100 query with default conf and set session variable runtime_filter_mode=off
q1	4593	4586	4578	4578
q2	351	233	258	233
q3	4010	4019	3988	3988
q4	2700	2696	2690	2690
q5	9634	9612	9645	9612
q6	240	126	125	125
q7	3011	2501	2531	2501
q8	4491	4468	4411	4411
q9	13131	13002	13083	13002
q10	4109	4210	4179	4179
q11	767	646	676	646
q12	980	815	818	815
q13	4302	3607	3581	3581
q14	388	337	348	337
q15	584	529	518	518
q16	732	689	678	678
q17	3815	3925	3864	3864
q18	9457	9099	8826	8826
q19	1813	1777	1757	1757
q20	2399	2077	2032	2032
q21	8803	8741	8818	8741
q22	904	807	810	807
Total cold run time: 81214 ms
Total hot run time: 77921 ms

@doris-robot

Copy link
Copy Markdown

(From new machine)TeamCity pipeline, clickbench performance test result:
the sum of best hot time: 44.95 seconds
stream load tsv: 566 seconds loaded 74807831229 Bytes, about 126 MB/s
stream load json: 18 seconds loaded 2358488459 Bytes, about 124 MB/s
stream load orc: 65 seconds loaded 1101869774 Bytes, about 16 MB/s
stream load parquet: 32 seconds loaded 861443392 Bytes, about 25 MB/s
insert into select: 28.8 seconds inserted 10000000 Rows, about 347K ops/s
storage size: 17101383180 Bytes

@dataroaring
dataroaring merged commit c24a33c into apache:master Nov 24, 2023
seawinde pushed a commit to seawinde/doris that referenced this pull request Nov 28, 2023
XuJianxu pushed a commit to XuJianxu/doris that referenced this pull request Dec 14, 2023
morningman added a commit to morningman/doris that referenced this pull request Nov 6, 2026
…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 <[email protected]>
morningman added a commit to morningman/doris that referenced this pull request Nov 13, 2026
…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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by one committer. dev/2.0.4 reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants