feat: redact SqlOption in SHOW CREATE #14310

zwang28 · 2024-01-02T14:39:49Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Redact all SqlOption when show create and query system catalog. Implement this idea.

This PR adds a helper function to redact identifiers, e.g. table names and column names, as well as with_option values, from CREATE TABLE/VIEW/INDEX/SOURCE/SINK/MATERIALIZED VIEW statement. ~~~
Implementation details:

For all types used in Statement type that requires redaction, implement Masking trait for them, which redacts the type in place. Then Masking::mask is called recursively from CREATE statement.

Masking::mask takes a MaskingContext as input, which maintains a String to int mapping. It's used to keep the relation between identifiers globally. e.g. Using the same MaskingContext, CREATE TABLE table_foo (bar int); CREATE MATERIALIZED VIEW mv_foo as SELECT bar from table_foo will be redacted to CREATE TABLE _1_ (_2_ int); CREATE MATERIALIZED VIEW _3_ as SELECT _2_ from _1_.

See test_masking for usage example.

#12673

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
I have added test labels as necessary. See details.
I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
All checks passed in ./risedev check (or alias, ./risedev c)
My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

zwang28 · 2024-01-05T02:27:13Z

As an alternative solution, instead of implementing Masking for types case by case, I considered to make parse function able to redact all encountered Ident, ObjectName and SqlOption. Because they are the only primitive types being redacted. But it seems won't work, because not all these primitive type should be redacted, e.g. function name.

src/sqlparser/src/redaction.rs

xxchan

I think this approach can't handle all cases (e.g., we can still use Display/Debug without redaction)i.e., we need to call it explicitly, but it's acceptable to me. @BugenZhao How do you think?

Regarding this implementation, what about adding a visitor, and impl redaction as a visitor?

xxchan · 2024-01-08T04:28:55Z

Oh, upstream sqlparser-rs has visitor now... 🤣

sqlparser-rs/sqlparser-rs#765

(But it might have limitations according to related issues. Haven't checked the details

zwang28 · 2024-03-07T06:58:02Z

Oh, upstream sqlparser-rs has visitor now... 🤣

sqlparser-rs/sqlparser-rs#765

(But it might have limitations according to related issues. Haven't checked the details

Yes, upstream's visit macro can significantly simplify the implementation. I'll try it.

BugenZhao · 2024-03-07T10:10:59Z

Note that Display impl of AST is actually a visitor. Can we directly hack into that? I suppose there could be only few changes (like on Value) if we read a task-local flag to decide whether we should format with a redacted value.

zwang28 · 2024-03-07T10:40:06Z

Note that Display impl of AST is actually a visitor. Can we directly hack into that? I suppose there could be only few changes (like on Value) if we read a task-local flag to decide whether we should format with a redacted value.

I'll look into it.

BTW, when taking redacting table names and column names into consideration, it becomes more complicated. That is, whether a type should be redacted also depends on its context. e.g. type ObjectName in a built-in Function shouldn't be redacted, while ObjectName in a CreateTable should be redacted because it's the table name.

This reverts commit 96036d8.

This reverts commit 363a177.

gitguardian · 2024-03-07T14:26:02Z

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secret in your pull request

GitGuardian id	GitGuardian status	Secret	Commit	Filename
9425213	Triggered	Generic Password	`f7fc8a5`	ci/scripts/regress-test.sh	View secret

🛠 Guidelines to remediate hardcoded secrets

Understand the implications of revoking this secret by investigating where it is used in your code.
Replace and store your secret safely. Learn here the best practices.
Revoke and rotate this secret.
If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

following these best practices for managing and storing secrets including API keys and other credentials
install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

^{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!}

zwang28 · 2024-03-07T14:30:20Z

This commit implements this idea, which customizes SqlOption's Display. It either redacts all options or none of them.

As SqlOption's Display will redact by default, we need to explicitly call Statement::to_unredacted_string instead of Statement::to_string here and there. Forget to call it will be a critical bug.

Also added an e2e test.

PTAL @xxchan @BugenZhao

Table names and column names are not redacted in this PR, as explained below.

BTW, when taking redacting table names and column names into consideration, it becomes more complicated. That is, whether a type should be redacted also depends on its context. e.g. type ObjectName in a built-in Function shouldn't be redacted, while ObjectName in a CreateTable should be redacted because it's the table name.

BugenZhao · 2024-03-08T04:07:31Z

src/frontend/src/catalog/system_catalog/rw_catalog/rw_sinks.rs

@@ -61,7 +63,12 @@ fn read_rw_sinks_info(reader: &SysCatalogReaderImpl) -> Result<Vec<RwSink>> {
                    .to_uppercase(),
                sink_type: sink.sink_type.to_proto().as_str_name().into(),
                connection_id: sink.connection_id.map(|id| id.connection_id() as i32),
-                definition: sink.create_sql(),
+                definition: if sink.owner.user_id == user_id {


I'm wondering if this (only showing raw SQL if the user is the owner) is a common behavior in other DBMS? Even so, I guess it would be based on the permission system but not simply determined by the ownership. cc @yezizp2012

In my personal opinion I would suggest adding a new column of redacted_definition. Users are allowed to decide which form to reveal or share with us.

src/sqlparser/src/ast/mod.rs

BugenZhao · 2024-03-08T04:16:26Z

src/frontend/src/handler/alter_source_column.rs

-    Ok(stmt.to_string())
+    Ok(stmt.to_unredacted_string())


May I ask how you find all references of to_string call on Statement? That is to say, how could we avoid showing an unredacted string where we should redact it in the future or vice versa?

how you find all references of to_string call on Statement?

Most of them are found by test failure 🥵

how could we avoid showing an unredacted string where we should redact it

to_string or display are redacted by default.

or vice versa

Caller is responsible for explicitly using to_unredacted_string. Similar to crdb's behavior: "everything gets redacted except for those bits which we know are safe"

zwang28 · 2024-04-08T04:25:37Z

go for risingwavelabs/rfcs#86

feat: add support for statement masking

363a177

github-actions bot added the type/feature label Jan 2, 2024

zwang28 changed the title ~~feat: add support for redacting CREATE statement~~ feat: redact CREATE statement Jan 2, 2024

zwang28 commented Jan 5, 2024

View reviewed changes

src/sqlparser/src/redaction.rs Outdated Show resolved Hide resolved

fixup

96036d8

xxchan mentioned this pull request Jan 8, 2024

feat(catalog): add support for redacting sensitive connector properties #14193

Closed

9 tasks

xxchan reviewed Jan 8, 2024

View reviewed changes

xxchan mentioned this pull request Mar 6, 2024

feat(parser): redact SqlOption #14760

Closed

9 tasks

zwang28 marked this pull request as draft March 7, 2024 09:55

zwang28 added 7 commits March 7, 2024 21:39

Revert "fixup"

9a44cbd

This reverts commit 96036d8.

Revert "feat: add support for statement masking"

1ee23d6

This reverts commit 363a177.

Merge branch 'main' into wangzheng/redact_table_name

f7fc8a5

customize SqlOption Display

ecb1ebe

customize show create and system_catalog

0a9e681

Add e2e test

009b4f5

fmt

e53346e

zwang28 marked this pull request as ready for review March 7, 2024 14:32

zwang28 requested a review from a team as a code owner March 7, 2024 14:32

zwang28 changed the title ~~feat: redact CREATE statement~~ feat: redact SqlOption in SHOW CREATE Mar 7, 2024

zwang28 requested review from BugenZhao and xxchan March 7, 2024 14:34

fix bug and test

8091858

zwang28 force-pushed the wangzheng/redact_table_name branch from 2e4d10e to 8091858 Compare March 7, 2024 15:41

zwang28 added 3 commits March 7, 2024 23:48

fmt

63cb4ee

bugfix

006df4b

fix test

459c9d6

BugenZhao reviewed Mar 8, 2024

View reviewed changes

xxchan mentioned this pull request Mar 19, 2024

rfc: Introducing SECRETs for Secure Storage of Sensitive Information risingwavelabs/rfcs#86

Open

zwang28 closed this Apr 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: redact SqlOption in SHOW CREATE #14310

feat: redact SqlOption in SHOW CREATE #14310

zwang28 commented Jan 2, 2024 •

edited

zwang28 commented Jan 5, 2024 •

edited

xxchan left a comment •

edited

xxchan commented Jan 8, 2024 •

edited

zwang28 commented Mar 7, 2024 •

edited

BugenZhao commented Mar 7, 2024

zwang28 commented Mar 7, 2024

gitguardian bot commented Mar 7, 2024 •

edited

zwang28 commented Mar 7, 2024 •

edited

BugenZhao Mar 8, 2024

BugenZhao Mar 8, 2024

zwang28 Mar 12, 2024

zwang28 commented Apr 8, 2024

feat: redact SqlOption in SHOW CREATE #14310

feat: redact SqlOption in SHOW CREATE #14310

Conversation

zwang28 commented Jan 2, 2024 • edited

What's changed and what's your intention?

Checklist

Documentation

Release note

zwang28 commented Jan 5, 2024 • edited

xxchan left a comment • edited

Choose a reason for hiding this comment

xxchan commented Jan 8, 2024 • edited

zwang28 commented Mar 7, 2024 • edited

BugenZhao commented Mar 7, 2024

zwang28 commented Mar 7, 2024

gitguardian bot commented Mar 7, 2024 • edited

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

zwang28 commented Mar 7, 2024 • edited

BugenZhao Mar 8, 2024

Choose a reason for hiding this comment

BugenZhao Mar 8, 2024

Choose a reason for hiding this comment

zwang28 Mar 12, 2024

Choose a reason for hiding this comment

zwang28 commented Apr 8, 2024

zwang28 commented Jan 2, 2024 •

edited

zwang28 commented Jan 5, 2024 •

edited

xxchan left a comment •

edited

xxchan commented Jan 8, 2024 •

edited

zwang28 commented Mar 7, 2024 •

edited

gitguardian bot commented Mar 7, 2024 •

edited

zwang28 commented Mar 7, 2024 •

edited