feat(frontend): redact sql option in log #16871

zwang28 · 2024-05-21T08:54:27Z

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

What's changed and what's your intention?

This PR might be reverted in the future, once the more universal secure store solution is prepared.

This PR redacts SQL options when recording CREATE queries in the log.

For queries that can be successfully parsed, it redacts the strongly typed SQL options within the parsed Statement.
For queries that can not be successfully parsed, such as those with syntax errors, it's not redacted.
Find the unit tests for examples.

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.

fuyufjh · 2024-05-21T10:11:23Z

src/utils/pgwire/src/pg_protocol.rs

+            v4 = '',
+        ) FORMAT plain ENCODE json (a='1',b='2')
+        ";
+        assert_eq!(redact_sql(sql), "CREATE SOURCE temp (k BIGINT, v CHARACTER VARYING) WITH (connector = [REDACTED], v1 = [REDACTED], v2 = [REDACTED], v3 = [REDACTED], v4 = [REDACTED]) FORMAT PLAIN ENCODE JSON (a = [REDACTED], b = [REDACTED])");


Hmm, it might be too aggressive to me... Can we just redact some sensitive fields? For example, any fields that contain "password"

Can we just redact some sensitive fields? For example, any fields that contain "password"

By maintaining a blacklist of sql options in the kernel?

Yeah, it's acceptable. As a quick fix, we can do it in some quick & dirty way, such as checking whether field_name.contains("password") or something.

src/sqlparser/src/ast/mod.rs

BugenZhao · 2024-05-23T03:09:09Z

src/utils/pgwire/src/pg_protocol.rs

+            .into_iter()
+            .map(|sql| sql.to_redacted_string())
+            .join(";"),
+        Err(_) => {


Actually I also find this not that necessary. 😕 We can just leave the statements that failed to parse as they are.

In other words, what if users misspell "with" as "wth"? It appears that we are unable to handle all cases.

# Conflicts: # src/utils/pgwire/src/pg_server.rs

zwang28 · 2024-05-23T08:12:21Z

example:

The log below was redacted. abctokenabc was redacted because it contained keyword token.

2024-05-23T16:10:00.243564+08:00 INFO handle_query{mode="simple query" session_id=0 sql=CREATE TABLE tomb (v1 INT) WITH (connector = 'datagen', fields.v1.kind = 'sequence', datagen.rows.per.second = '1', abctokenabc = [REDACTED]) FORMAT PLAIN ENCODE JSON}: pgwire_query_log: status="ok" time=70ms

The log below was not redacted because the statement failed to parse.

2024-05-23T16:10:07.48916+08:00 ERROR handle_query{mode="simple query" session_id=0 sql=CREATE TABLEXXXXX tomb (v1 int)
WITH (
  connector = 'datagen',
  fields.v1.kind = 'sequence',
  datagen.rows.per.second = '1',
  abctokenabc = '222'
)
FORMAT PLAIN
ENCODE JSON;}: pgwire::pg_protocol: error when process message error=Failed to run the query: sql parser error: Expected an object type after CREATE, found: TABLEXXXXX at line:1, column:18

BugenZhao

Generally LGTM as it will be eventually superseded by the SECRET approach.

src/common/src/config.rs

BugenZhao · 2024-05-27T02:17:38Z

src/sqlparser/Cargo.toml

@@ -27,6 +27,7 @@ normal = ["workspace-hack"]
 [dependencies]
 itertools = { workspace = true }
 serde = { version = "1.0", features = ["derive"], optional = true }
+tokio = { version = "0.2", package = "madsim-tokio" }


nit: Actually tokio::task_local does not rely on any components of the tokio runtime. We may consider extracting it into a separate crate in the future.

BugenZhao · 2024-05-27T02:24:09Z

src/frontend/src/lib.rs

+        pg_serve(
+            &listen_addr,
+            session_mgr,
+            TlsConfig::new_default(),
+            Some(redact_sql_option_keywords),
+        )


This looks a little bit dirty though. 🤣

Yes...
We can wrap it with another context object.
Anyway the PR will be reverted in the near future..

hzxa21

LGTM as a quick fix.

# Conflicts: # src/sqlparser/Cargo.toml

feat(frontend): redact sql option in log

00cd04f

zwang28 requested review from hzxa21, fuyufjh, BugenZhao and yuhao-su May 21, 2024 08:54

zwang28 requested a review from a team as a code owner May 21, 2024 08:54

github-actions bot added the type/feature label May 21, 2024

fmt

dba6391

This comment was marked as outdated.

Sign in to view

fmt

4cddf3d

fuyufjh reviewed May 21, 2024

View reviewed changes

BugenZhao reviewed May 23, 2024

View reviewed changes

zwang28 added 3 commits May 23, 2024 14:07

fixup: leave unparsable statement as it is

05de385

fixup: redact based on keywords

032bd41

Merge branch 'main' into wangzheng/redact_log

b4119dd

# Conflicts: # src/utils/pgwire/src/pg_server.rs

zwang28 requested review from fuyufjh and BugenZhao May 23, 2024 08:16

BugenZhao approved these changes May 27, 2024

View reviewed changes

BugenZhao requested a review from xxchan May 27, 2024 02:24

hzxa21 approved these changes May 27, 2024

View reviewed changes

zwang28 added 2 commits May 27, 2024 10:59

add more keywords

f01a129

Merge branch 'main' into wangzheng/redact_log

be0f7a6

# Conflicts: # src/sqlparser/Cargo.toml

zwang28 enabled auto-merge May 28, 2024 02:20

zwang28 disabled auto-merge May 28, 2024 02:27

zwang28 enabled auto-merge May 28, 2024 02:35

zwang28 added this pull request to the merge queue May 28, 2024

Merged via the queue into main with commit 6c81beb May 28, 2024
27 of 28 checks passed

zwang28 deleted the wangzheng/redact_log branch May 28, 2024 02:50

zwang28 added a commit that referenced this pull request May 29, 2024

feat(frontend): redact sql option in log (#16871)

9e1491e

zwang28 mentioned this pull request May 29, 2024

feat(frontend): redact sql option in log #16993

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(frontend): redact sql option in log #16871

feat(frontend): redact sql option in log #16871

zwang28 commented May 21, 2024 •

edited

This comment was marked as outdated.

fuyufjh May 21, 2024

zwang28 May 23, 2024

fuyufjh May 23, 2024 •

edited

zwang28 May 23, 2024

BugenZhao May 23, 2024

zwang28 May 23, 2024

zwang28 commented May 23, 2024

BugenZhao left a comment

BugenZhao May 27, 2024

BugenZhao May 27, 2024

zwang28 May 27, 2024 •

edited

hzxa21 left a comment

feat(frontend): redact sql option in log #16871

feat(frontend): redact sql option in log #16871

Conversation

zwang28 commented May 21, 2024 • edited

What's changed and what's your intention?

Checklist

Documentation

Release note

This comment was marked as outdated.

fuyufjh May 21, 2024

Choose a reason for hiding this comment

zwang28 May 23, 2024

Choose a reason for hiding this comment

fuyufjh May 23, 2024 • edited

Choose a reason for hiding this comment

zwang28 May 23, 2024

Choose a reason for hiding this comment

BugenZhao May 23, 2024

Choose a reason for hiding this comment

zwang28 May 23, 2024

Choose a reason for hiding this comment

zwang28 commented May 23, 2024

BugenZhao left a comment

Choose a reason for hiding this comment

BugenZhao May 27, 2024

Choose a reason for hiding this comment

BugenZhao May 27, 2024

Choose a reason for hiding this comment

zwang28 May 27, 2024 • edited

Choose a reason for hiding this comment

hzxa21 left a comment

Choose a reason for hiding this comment

zwang28 commented May 21, 2024 •

edited

fuyufjh May 23, 2024 •

edited

zwang28 May 27, 2024 •

edited