feat(parser): redact SqlOption #14760

zwang28 · 2024-01-24T03:16:25Z

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

What's changed and what's your intention?

Implement this idea.

Only redact::Secret<String> is supported (used in *Properties), via a wrapper type SecretString.
Other redact::Secret<T> are not supported, for the sake of implementation simplicity. Because otherwise redacted string need be able to convert back to the original type.
~~This PR doesn't perform "escape back the unescaped CstyleEscapedString when serializing it", because I think it's non-trivial and always converting to SingleQuotedString is just fine.~~

#14115

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.

src/connector/src/source/pulsar/source/reader.rs

BugenZhao · 2024-01-24T03:37:27Z

src/connector/src/source/common.rs

+#[derive(Clone, Debug, Deserialize, PartialEq, with_options::WithOptions)]
+pub struct SecretString {
+    inner: redact::Secret<String>,
+}


What about #[serde(transparent)]?

redact::Secret doesn't implement Serialize.

Meanwhile impl Serialize for SecretString should be chosen according to use case, i.e. whether the original value or the redacted one is expected.

Though currently there is no other use case that serializes SecretString.

src/connector/src/source/common.rs

src/sqlparser/src/ast/mod.rs

src/frontend/src/handler/util.rs

BugenZhao · 2024-02-29T06:44:36Z

src/sqlparser/src/ast/utils.rs

+    }
+
+    fn serialize_str(self, v: &str) -> Result<Self::Ok, Self::Error> {
+        Ok(Value::SingleQuotedString(v.to_string()))


If there's a special character (like \t), I guess we should serialize into a CstyleEscapedString instead. #14193 (comment)

BugenZhao · 2024-02-29T06:48:57Z

src/sqlparser/src/ast/utils.rs

+                SqlOption {
+                    name: to_object_name("d"),
+                    value: Value::Null
+                },


Should we omit the field that's unset? I guess we need an end-to-end test like

SQL --parse--> AST --deser--> Connector Options --redact--> Redacted Connector Options --ser--> AST --unparse--> SQL

to check whether it works well.

Omitted the field that's unset.
Modified pubsub.slt to cover the code path.

src/sqlparser/src/ast/mod.rs

# Conflicts: # src/connector/src/source/common.rs

gitguardian · 2024-03-05T08:54:01Z

⚠️ 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	`f2644fd`	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!}

xxchan · 2024-03-06T07:18:48Z

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

+                    definition: if source.owner == user_id {
+                        source.create_sql()
+                    } else {
+                        source.redacted_create_sql()
+                    },


Why do we distinguish owner? I'm not sure whether this is reasonable 🤣

User should be able to view secrets in definition somehow.

I think one reason user want redaction is that they want to SHOW CREATE and share it to us for debugging. For this use case, it seems not very convenient if we distinguish by owner.

@BugenZhao comes up with an idea that maybe we can add sth like SHOW REDACTD CREATE. 🤣 (Maybe alternatively we can add a session variable or sth..?)

I'm not sure what users want now. @fuyufjh Do you have any ideas about users' requests? Since I saw you opened #15402

BTW on the other hand, force redaction if source.owner != user_id sounds good 🤔

The major requirement is that users are concerning about the secret leaks. Before having a real secret store, I think we should redact for all users.

xxchan · 2024-03-06T07:37:31Z

src/frontend/src/utils/redact.rs

+    try_redact_statement(&stmt).map(|stmt| stmt.to_string())
+}
+
+fn try_redact_statement(stmt: &Statement) -> Result<Statement, Error> {


Let me try to summarize my understanding of this PR:

try_redact_statement (Statement -> Statement) is the end goal we want to achieve.

The implementation is Statement -> ConnectorProperties -> Statement.

The ConnectorProperties -> Statement is achieved by the SqlOptionVecSerializer, and the Serialize impl (which actually does the redaction)

This approach generally sounds good to me. But I have some thoughts:

In order to redact Statement -> Statement, an easier approach IMO is just modifying the hashmap directly. We can check the key's name (like having a blacklist for redaction).

The redaction needs to be performed manually, so we may still access original SQL in e.g., SQL log. I don't know if this is good enough.

Oh, #14310 looks very like the solution in my mind, but it contains more stuff, including redacting table/column names. Why don't we go with that approach? 🤔

In order to redact Statement -> Statement, an easier approach IMO is just modifying the hashmap directly. We can check the key's name (like having a blacklist for redaction).

Yes, #14310 basically redact all with_options values, i.e. an empty whitelist.

The upside of this PR is it's strong typed, and secrets in ConnectorProperties won't be printed unexpectedly.

secrets in ConnectorProperties won't be printed unexpectedly

I agree this is quite a good change.

However, previously ConnectorProperties doesn't implements Serialize (and Debug) (Actually I removed some unnecessary #[derive(Serialize)]), so probably it won't be printed unexpectedly.

Now we adds #[derive(Serialize)] (although redacted), I feel this isn't very cool..

However, previously ConnectorProperties doesn't implements Serialize (and Debug)

It seems I'm wrong. It already implemented Debug. 🤡

zwang28 · 2024-03-14T07:00:58Z

prefer #14310

feat(parser): redact SqlOption

0c3ac42

zwang28 requested a review from BugenZhao January 24, 2024 03:16

zwang28 requested a review from a team as a code owner January 24, 2024 03:16

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

BugenZhao reviewed Jan 24, 2024

View reviewed changes

src/connector/src/source/pulsar/source/reader.rs Outdated Show resolved Hide resolved

BugenZhao reviewed Jan 24, 2024

View reviewed changes

zwang28 added 2 commits January 24, 2024 11:51

fix tests

3cd5f06

refactor

25d0d30

zwang28 requested a review from BugenZhao January 29, 2024 04:30

BugenZhao requested a review from xxchan February 29, 2024 06:00

BugenZhao reviewed Feb 29, 2024

View reviewed changes

BugenZhao mentioned this pull request Mar 4, 2024

feat: hide credentials from show create source/table #15402

Closed

zwang28 added 2 commits March 5, 2024 10:39

refactor

8e53c27

Merge branch 'main' into wangzheng/redact

f2644fd

# Conflicts: # src/connector/src/source/common.rs

zwang28 mentioned this pull request Mar 5, 2024

support redact SHOW CREATE on demand #14115

Closed

zwang28 added 2 commits March 5, 2024 16:40

refactor

dcea69d

add e2e test

b00dfec

zwang28 added 2 commits March 5, 2024 17:26

bugfix

3912da6

minor

754d1ba

xxchan reviewed Mar 6, 2024

View reviewed changes

zwang28 closed this Mar 14, 2024

zwang28 mentioned this pull request Jun 4, 2024

fix(connector): don't log config/props that may contain sensitive information #17079

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(parser): redact SqlOption #14760

feat(parser): redact SqlOption #14760

zwang28 commented Jan 24, 2024 •

edited

BugenZhao Jan 24, 2024

zwang28 Jan 24, 2024

zwang28 Jan 24, 2024 •

edited

BugenZhao Feb 29, 2024

BugenZhao Feb 29, 2024

zwang28 Mar 5, 2024

gitguardian bot commented Mar 5, 2024 •

edited

xxchan Mar 6, 2024

zwang28 Mar 7, 2024 •

edited

xxchan Mar 7, 2024

xxchan Mar 7, 2024

fuyufjh Mar 11, 2024

xxchan Mar 6, 2024

xxchan Mar 6, 2024

zwang28 Mar 7, 2024

zwang28 Mar 7, 2024

xxchan Mar 7, 2024

xxchan Mar 7, 2024

zwang28 commented Mar 14, 2024

feat(parser): redact SqlOption #14760

feat(parser): redact SqlOption #14760

Conversation

zwang28 commented Jan 24, 2024 • edited

What's changed and what's your intention?

Checklist

Documentation

Release note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zwang28 Jan 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitguardian bot commented Mar 5, 2024 • edited

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

Choose a reason for hiding this comment

zwang28 Mar 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zwang28 commented Mar 14, 2024

zwang28 commented Jan 24, 2024 •

edited

zwang28 Jan 24, 2024 •

edited

gitguardian bot commented Mar 5, 2024 •

edited

zwang28 Mar 7, 2024 •

edited