[FLINK-39685][table] Redact sensitive options in SHOW CREATE and DESCRIBE CATALOG statements#28167
Conversation
featzhang
left a comment
There was a problem hiding this comment.
LGTM overall. Good use of existing GlobalConfiguration.isSensitive() infrastructure.
Questions:
-
Line 485-505 in ShowCreateUtil.java - The
extractFormattedOptions(conf, printIndent, boolean lowerCaseKeys)overload was removed. Can you confirm this was truly unused? If it's@InternalAPI, fine; just want to make sure no external code relied on it. -
DefaultCatalogTable.toString() L167 - The redaction here uses
Collections.emptyList()for additionalSensitiveKeys because there's no TableConfig context. This means user-configuredADDITIONAL_SENSITIVE_KEYSwon't apply to logs. Acceptable trade-off, but might be worth a doc note.
Minor:
-
Missing end-to-end test for
ShowCreateModelOperation- only unit tests present. Not critical but would be nice to have. -
Consider adding a debug log when redaction happens (L459-466 in ShowCreateUtil) for security auditing.
Tests look solid - good coverage of the redaction logic.
e79d0ca to
ff9c43c
Compare
ff9c43c to
8514ea8
Compare
snuyanzin
left a comment
There was a problem hiding this comment.
Thanks for addressing feedback
…RIBE CATALOG statements
8514ea8 to
c7c6d69
Compare
|
Thanks for the review. Rebased to eliminate unrelated test failures. |
What is the purpose of the change
Sensitive table/catalog/model options (e.g. password, api-key, token) were exposed verbatim in the output of SHOW
CREATE TABLE,SHOW CREATE CATALOG,SHOW CREATE MATERIALIZED TABLE,SHOW CREATE MODEL, andDESCRIBE CATALOG EXTENDED. Flink already had redaction infrastructure (GlobalConfiguration.isSensitive/HIDDEN_CONTENT) used for Flink config display and factory error messages, but it was not wired to SQL display operations.Brief change log
ShowCreateUtil-extractFormattedOptionsnow acceptsList<String> additionalSensitiveKeysand redacts matching values with******. The unused 2-arg and lowerCaseKeys overloads were removed (dead code).ShowCreate*Operation/DescribeCatalogOperation- eachexecute()readsSecurityOptions.ADDITIONAL_SENSITIVE_KEYSfromTableConfigand threads it through to the rendering layer.DefaultCatalogTable/DefaultCatalogModel-toString()now usesConfigurationUtils.hideSensitiveValuesto avoid leaking secrets in logs. Built-in sensitive key patterns apply; user-configured additional keys cannot be applied here (no config context intoString()).Verifying this change
ShowCreateUtilTest- extended with redaction cases for table, catalog, materialized table, and custom additionalSensitiveKeys.DescribeCatalogOperationTest- new; verifies extended output redacts password/token, non-sensitive values are unchanged, and non-extended output exposes no options at all.DefaultCatalogTableTest- new; verifiestoString()redacts sensitive keys and preserves safe ones.Does this pull request potentially affect one of the following parts:
@Public(Evolving): noDocumentation
Was generative AI tooling used to co-author this PR?
Generated-by: Claude code