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

zwang28 · 2023-12-25T11:41:53Z

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

What's changed and what's your intention?

Review tips:

src/utils/redaction/redaction/src/lib.rs defines a trait MarkRedaction. It simply returns property names that need redaction.
src/utils/redaction/redaction_derive/src/lib.rs defines a derive macro to implement MarkRedaction.
src/frontend/src/utils/redaction.rs provides function to redact SQL.
Other LOC is mostly contributed by tests.
This PR doesn't actually replace any original SQL with redacted SQL when displayed, e.g. show create table, which should be another PR.

Basic idea:

To make it easier to redact sensitive connector properties from table definition when displaying,

Firstly implement a MarkRedaction trait for all connector properties types, using the new derive macro. Developers are responsible for marking properties that should be redacted. In this PR I marked AK/SK of S3Properties for example.

#[derive(MarkRedaction)]
pub struct S3Properties {
    ...
    #[serde(rename = "s3.credentials.access", default)]
    #[mark_redaction(rename = "s3.credentials.access")]
    pub access: Option<String>,
    #[serde(rename = "s3.credentials.secret", default)]
    #[mark_redaction(rename = "s3.credentials.secret")]
    pub secret: Option<String>,
   ...

Then to redact a SQL, try_redact_sql invokes MarkRedaction::marks to get all property names that should be redacted. See src/frontend/src/utils/redaction.rs.
More detail on the macro's usage can be found in the doc in src/utils/redaction/redaction_derive/src/lib.rs.

An example from `test_redact_s3_source`.

Original SQL:

CREATE SOURCE ad_click (
    ...
) WITH (
    connector = 's3',
    s3.credentials.access = 'test',
    s3.credentials.secret = 'test',
    s3.endpoint_url = 'http://localstack:4566'
    ...
) FORMAT PLAIN ENCODE JSON;

After redaction:

CREATE SOURCE ad_click (
    ...
) WITH (
    connector = 's3',
    s3.credentials.access = '[REDACTED]',
    s3.credentials.secret = '[REDACTED]',
    s3.endpoint_url = 'http://localstack:4566'
    ...
) FORMAT PLAIN ENCODE JSON;

#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.

xxchan

Haven't took a look yet. QQ: Why did you use a trait, instead of the safer (IMO) approach like #13034?

tabVersion · 2023-12-26T16:03:40Z

src/connector/src/source/filesystem/s3/mod.rs

@@ -33,8 +34,10 @@ pub struct S3Properties {
    #[serde(rename = "match_pattern", default)]
    pub match_pattern: Option<String>,
    #[serde(rename = "s3.credentials.access", default)]
+    #[mark_redaction(rename = "s3.credentials.access")]


access key seems not that sensitive. cc @hzxa21

src/connector/src/source/datagen/mod.rs

zwang28 · 2023-12-27T00:26:10Z

Why did you use a trait, instead of the safer (IMO) approach like #13034?

Because to redact connector properties from a Statement, we need to rewrite some SqlOption::Value of the Statement's Vec<SqlOption>. So we need a way to know which SqlOption::name indicate properties that need redaction. These info spread across many *Properties structs.

This PR implements MarkRedaction for all those *Properties structs, so that altogether they can tell whether a property name need redaction, given a connector type and a property name of String type. This method doesn't care about types but names in *Properties, so I don't know how safe type like redact::secret in #13034 can help here.

See try_redact_statement for detail.

@xxchan @BugenZhao

BugenZhao · 2024-01-03T03:47:24Z

If we have...

impl serde::Serializer for Serializer {
    type Ok = Vec<SqlOption>;
    ..
}

then wrap secrets with Secret newtype and serialize back will work. We normalize the aliases by the way.

The most tricky part I can think of is to escape back the unescaped CstyleEscapedString when serializing it. When introducing it in #11323, we store the raw representation for the sake of simpliness.

#11195 (comment)

BugenZhao · 2024-01-03T04:17:50Z

src/connector/src/source/filesystem/s3/mod.rs

@@ -33,8 +34,10 @@ pub struct S3Properties {
    #[serde(rename = "match_pattern", default)]
    pub match_pattern: Option<String>,
    #[serde(rename = "s3.credentials.access", default)]
+    #[mark_redaction(rename = "s3.credentials.access")]


Can we reuse the serde attributes for mark_redaction to avoid duplication?

BugenZhao · 2024-01-03T04:20:11Z

src/frontend/src/utils/redaction.rs

+    let mut stmt = stmt.clone();
+    let sql_options = match &mut stmt {
+        Statement::CreateSource { stmt } => &mut stmt.with_properties.0,
+        Statement::CreateTable { with_options, .. } => with_options,
+        Statement::CreateSink { stmt } => &mut stmt.with_properties.0,
+        _ => {
+            return Some(stmt);
+        }
+    };
+    let Ok(mut with_properties) =
+        WithOptions::try_from(sql_options.as_slice()).map(WithOptions::into_inner)
+    else {
+        return None;
+    };
+    let Some(connector) = with_properties.remove(UPSTREAM_SOURCE_KEY) else {
+        tracing::error!("Must specify 'connector' in WITH clause");
+        return None;
+    };


What about finding a way to unify this snippet with ...

https://github.com/risingwavelabs/risingwave/blob/5417d18ee47628df4c668e23966604c7fb9c5a86/src/frontend/src/utils/with_options.rs#L208-L246

xxchan · 2024-01-08T04:00:43Z

(Sorry for the late review.)

Oh I get it. The type-safe approach is good for redacting the Property structs themselves (e.g., Debug/Display them in log). However, we can't redact the type-unsafe raw string/hashmap input when the structs are created (the output of sqlparser AST). The problem is mainly because sqlparser is unaware of the properties. 😟

xxchan · 2024-01-08T04:14:49Z

src/frontend/src/utils/redaction.rs

+    fn redact_marked_field(options: &mut Vec<SqlOption>, marks: std::collections::HashSet<String>) {
+        for option in options {
+            if marks.contains(&option.name.real_value()) {
+                option.value = Value::SingleQuotedString("[REDACTED]".into());
+            }
+        }
+    }


I'm thinking do we really need to get the keys to redact precisely? What about simply pass ["secret", "credential", "key"] etc. ? 🤣

Bugen's idea on relying on serde is also acceptable.

xxchan · 2024-01-08T04:16:20Z

The most tricky part I can think of is to escape back the unescaped CstyleEscapedString when serializing it.

Can you give an example of how this can go wrong? 👀

xxchan · 2024-01-08T04:19:11Z

I see the alternative approach in #14310, this is quite a simple solution. 🤣

impl Masking for Vec<SqlOption> {
    fn mask<'a>(&mut self, _context: &mut MaskingContext) {
        for sql_option in self {
            sql_option.value = Value::SingleQuotedString("[REDACTED]".to_string());
        }
    }
}

BugenZhao · 2024-01-08T04:44:28Z

I see the alternative approach in #14310, this is quite a simple solution. 🤣

impl Masking for Vec<SqlOption> {
    fn mask<'a>(&mut self, _context: &mut MaskingContext) {
        for sql_option in self {
            sql_option.value = Value::SingleQuotedString("[REDACTED]".to_string());
        }
    }
}

Will this be too aggressive? For example, connector = [REDACTED]. 😄

BugenZhao · 2024-01-08T04:57:00Z

However, we can't redact the type-unsafe raw string/hashmap input when the structs are created (the output of sqlparser AST). The problem is mainly because sqlparser is unaware of the properties. 😟

It's us that convert it to type-unsafe maps. 😕

risingwave/src/frontend/src/utils/with_options.rs

Lines 177 to 206 in 975bfac

    
           impl TryFrom<&[SqlOption]> for WithOptions { 
        
               type Error = RwError; 
        
               fn try_from(options: &[SqlOption]) -> Result<Self, Self::Error> { 
        
                   let mut inner: BTreeMap<String, String> = BTreeMap::new(); 
        
                   for option in options { 
        
                       let key = option.name.real_value(); 
        
                       let value: String = match option.value.clone() { 
        
                           Value::CstyleEscapedString(s) => s.value, 
        
                           Value::SingleQuotedString(s) => s, 
        
                           Value::Number(n) => n, 
        
                           Value::Boolean(b) => b.to_string(), 
        
                           _ => { 
        
                               return Err(RwError::from(ErrorCode::InvalidParameterValue( 
        
                                   "`with options` or `with properties` only support single quoted string value and C style escaped string" 
        
                                       .to_owned(), 
        
                               ))) 
        
                           } 
        
                       }; 
        
                       if inner.insert(key.clone(), value).is_some() { 
        
                           return Err(RwError::from(ErrorCode::InvalidParameterValue(format!( 
        
                               "Duplicated option: {}", 
        
                               key 
        
                           )))); 
        
                       } 
        
                   } 
        
                   Ok(Self { inner }) 
        
               } 
        
           }

tabVersion

the overall idea LGTM, can we also cover some struct from sdk, such as

https://github.com/risingwavelabs/risingwave/blob/be30f20e2c14b5d12efd5b33a4ac9e537602fc37/src/connector/src/source/kafka/mod.rs#L156https://github.com/risingwavelabs/risingwave/blob/be30f20e2c14b5d12efd5b33a4ac9e537602fc37/src/connector/src/source/kafka/mod.rs#L156

zwang28 · 2024-01-12T02:18:38Z

can we also cover some struct from sdk, such as

https://github.com/risingwavelabs/risingwave/blob/be30f20e2c14b5d12efd5b33a4ac9e537602fc37/src/connector/src/source/kafka/mod.rs#L156https://github.com/risingwavelabs/risingwave/blob/be30f20e2c14b5d12efd5b33a4ac9e537602fc37/src/connector/src/source/kafka/mod.rs#L156

This approach is more universal, which can solve the redaction of structs in connector crate as well. WIP
#14193 (comment)

zwang28 · 2024-01-12T06:07:22Z

#14193 (comment)

github-actions bot added the type/feature label Dec 25, 2023

zwang28 force-pushed the wangzheng/redact_options branch 4 times, most recently from 7d01bee to 609503c Compare December 26, 2023 11:16

zwang28 added 2 commits December 26, 2023 19:23

feat(catalog): add support for redacting connector properties

21a854c

revert redacted_definition field

8c46491

zwang28 force-pushed the wangzheng/redact_options branch from 609503c to 8c46491 Compare December 26, 2023 12:04

zwang28 changed the title ~~feat(catalog): add support for redacted_definition~~ feat(catalog): add support for redacting connector properties from SQL Dec 26, 2023

zwang28 marked this pull request as ready for review December 26, 2023 12:22

zwang28 requested a review from a team as a code owner December 26, 2023 12:22

zwang28 added the component/connector label Dec 26, 2023

zwang28 requested review from tabVersion and xxchan December 26, 2023 12:43

zwang28 changed the title ~~feat(catalog): add support for redacting connector properties from SQL~~ feat(catalog): add support for redacting sensitive connector properties Dec 26, 2023

fixup: fix doc

417796d

xxchan reviewed Dec 26, 2023

View reviewed changes

xxchan requested a review from BugenZhao December 26, 2023 14:30

tabVersion reviewed Dec 26, 2023

View reviewed changes

src/connector/src/source/datagen/mod.rs Show resolved Hide resolved

xxchan self-requested a review January 1, 2024 14:05

BugenZhao reviewed Jan 3, 2024

View reviewed changes

xxchan reviewed Jan 8, 2024

View reviewed changes

tabVersion reviewed Jan 10, 2024

View reviewed changes

zwang28 closed this Jan 12, 2024

zwang28 mentioned this pull request Jan 24, 2024

feat(parser): redact SqlOption #14760

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

zwang28 commented Dec 25, 2023 •

edited

Loading

xxchan left a comment

tabVersion Dec 26, 2023 •

edited

Loading

zwang28 commented Dec 27, 2023 •

edited

Loading

BugenZhao commented Jan 3, 2024 •

edited

Loading

BugenZhao Jan 3, 2024

BugenZhao Jan 3, 2024

xxchan commented Jan 8, 2024 •

edited

Loading

xxchan Jan 8, 2024

xxchan commented Jan 8, 2024

xxchan commented Jan 8, 2024 •

edited

Loading

BugenZhao commented Jan 8, 2024

BugenZhao commented Jan 8, 2024 •

edited

Loading

tabVersion left a comment

zwang28 commented Jan 12, 2024

zwang28 commented Jan 12, 2024

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

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

Conversation

zwang28 commented Dec 25, 2023 • edited Loading

What's changed and what's your intention?

Review tips:

Basic idea:

An example from test_redact_s3_source.

Checklist

Documentation

Release note

xxchan left a comment

Choose a reason for hiding this comment

tabVersion Dec 26, 2023 • edited Loading

Choose a reason for hiding this comment

zwang28 commented Dec 27, 2023 • edited Loading

BugenZhao commented Jan 3, 2024 • edited Loading

BugenZhao Jan 3, 2024

Choose a reason for hiding this comment

BugenZhao Jan 3, 2024

Choose a reason for hiding this comment

xxchan commented Jan 8, 2024 • edited Loading

xxchan Jan 8, 2024

Choose a reason for hiding this comment

xxchan commented Jan 8, 2024

xxchan commented Jan 8, 2024 • edited Loading

BugenZhao commented Jan 8, 2024

BugenZhao commented Jan 8, 2024 • edited Loading

tabVersion left a comment

Choose a reason for hiding this comment

zwang28 commented Jan 12, 2024

zwang28 commented Jan 12, 2024

zwang28 commented Dec 25, 2023 •

edited

Loading

An example from `test_redact_s3_source`.

tabVersion Dec 26, 2023 •

edited

Loading

zwang28 commented Dec 27, 2023 •

edited

Loading

BugenZhao commented Jan 3, 2024 •

edited

Loading

xxchan commented Jan 8, 2024 •

edited

Loading

xxchan commented Jan 8, 2024 •

edited

Loading

BugenZhao commented Jan 8, 2024 •

edited

Loading