feat: create secret catalog #16288

tabVersion · 2024-04-12T13:47:32Z

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

What's changed and what's your intention?

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/frontend/src/utils/with_options.rs

src/meta/service/src/notification_service.rs

src/meta/model_v2/src/secret.rs

github-actions

license-eye has totally checked 5070 files.

Valid	Invalid	Ignored	Fixed
2171	1	2898	0

Click to see the invalid file list

src/frontend/src/handler/create_secret.rs

src/frontend/src/handler/create_secret.rs

gitguardian · 2024-05-02T06:40:41Z

⚠️ GitGuardian has uncovered 2 secrets 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 secrets in your pull request

GitGuardian id	GitGuardian status	Secret	Commit	Filename
9425213	Triggered	Generic Password	`8e4637f`	e2e_test/source/cdc/cdc.validate.postgres.slt	View secret
9425213	Triggered	Generic Password	`1a75e35`	e2e_test/source/cdc/cdc.validate.postgres.slt	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 secrets safely. Learn here the best practices.
Revoke and rotate these secrets.
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.}

yezizp2012

This PR is too large, can we separate it into two parts, like meta and frontend. Btw, we need to add some e2e to check if it works.
Btw, the backup and migration from etcd to sql backend are still missing, we need to add them as well. It's acceptable for me to add them in some other PRs.

src/common/src/config.rs

proto/catalog.proto

src/frontend/src/catalog/schema_catalog.rs

src/frontend/src/handler/create_secret.rs

src/sqlparser/src/ast/statement.rs

yezizp2012 · 2024-05-07T08:58:46Z

src/frontend/src/catalog/schema_catalog.rs

+    secret_by_name: HashMap<String, Arc<SecretCatalog>>,
+    secret_by_id: HashMap<SecretId, Arc<SecretCatalog>>,
+
+    secret_source_ref: HashMap<SecretId, Vec<SourceId>>,


Do we have any usage of these two fields? We need to update them when issuing create/drop source/sink.

I believe they're necessary for command like show secrets.

Do we have any usage of these two fields? We need to update them when issuing create/drop source/sink.

I am planning to impl this in the next pr. It is more intuitive to do it with ref secret syntax.

I believe they're necessary for command like show secrets.

Yes, show secrets will be in this pr.

yezizp2012 · 2024-05-07T09:09:33Z

src/meta/model_v2/src/sink.rs

@@ -127,6 +129,13 @@ impl From<PbSink> for ActiveModel {
            sink_from_name: Set(pb_sink.sink_from_name),
            sink_format_desc: Set(pb_sink.format_desc.as_ref().map(|x| x.into())),
            target_table: Set(pb_sink.target_table.map(|x| x as _)),
+            secret_ref: Set({


Better to define this column as json_binary in migration, then you can define a struct wrapping the map and use it here. Check examples that are using derive_from_json_struct for more details.

src/meta/src/manager/catalog/mod.rs

src/meta/src/controller/catalog.rs

yezizp2012 · 2024-05-07T09:17:17Z

src/meta/src/manager/catalog/mod.rs

+        let user_core = &mut core.user;
+        let mut secrets = BTreeMapTransaction::new(&mut database_core.secrets);
+
+        match database_core.relation_ref_count.get(&secret_id) {


Ditto, secret referring count is never updated when create/drop sink and source.

Not resolved, you can remove the check and leave a TODO here.

fuyufjh

Overall LGTM

fuyufjh · 2024-05-08T07:05:03Z

src/frontend/src/handler/create_secret.rs

+    let session = handler_args.session.clone();
+    let db_name = session.database();
+    let (schema_name, connection_name) =
+        Binder::resolve_schema_qualified_name(db_name, stmt.secret_name.clone())?;


Do we follow the same rule of identifiers for secrets? That is,

foo will be converted to lower-case, so that Foo and foo will be the same

"foo" will be interpreted as case-sensitive, i.e. "foo" and "Foo" are different

For the secret name, yes. The impl leverages the existing funds and tries best to keep the same as other catalogs.

fuyufjh · 2024-05-08T07:06:20Z

src/frontend/src/observer/observer_manager.rs

+                    secret.schema_id,
+                    SecretId::new(secret.id),
+                ),
+                Operation::Update => unimplemented!(),


Considering key rotation, I think it makes sense to provide an "update secret" command

fuyufjh · 2024-05-08T07:11:43Z

src/rpc_client/src/meta_client.rs

@@ -184,6 +203,14 @@ impl MetaClient {
        Ok(resp.version)
    }

+    pub async fn drop_secret(&self, secret_id: SecretId) -> Result<CatalogVersion> {


Not a big deal but why not placing it with create_secret() together

Signed-off-by: tabVersion <[email protected]>

tabVersion · 2024-05-21T21:32:38Z

src/meta/src/rpc/ddl_controller.rs

+        let encrypted_payload = simplestcrypt::encrypt_and_serialize(
+            self.env.opts.secret_store_private_key.as_slice(),
+            secret.get_value().as_slice(),
+        )


we do the encryption and the secret key here.
@yuhao-su

Why not using the implementation in our built-in function encrypt(bytea, bytea, varchar) -> bytea?

risingwave/src/expr/impl/src/scalar/encrypt.rs

Lines 141 to 157 in 1cc36e1

fn eval_inner(

&self,

input: &[u8],

operation: CipherMode,

) -> std::result::Result<Box<[u8]>, ErrorStack> {

let mut decrypter = Crypter::new(self.cipher, operation, self.crypt_key.as_ref(), None)?;

let enable_padding = match self.padding {

Padding::Pkcs => true,

Padding::None => false,

};

decrypter.pad(enable_padding);

let mut decrypt = vec![0; input.len() + self.cipher.block_size()];

let count = decrypter.update(input, &mut decrypt)?;

let rest = decrypter.finalize(&mut decrypt[count..])?;

decrypt.truncate(count + rest);

Ok(decrypt.into())

}

Benefits include

Avoid duplicates in dependencies

Easy to encrypt & decrypt with SQL interface when debugging

Agree with #16288 (comment)

Better to consider a more popular encryption lib

Such as openssl directly.

It's a bit risky to use a 3rd-party unpopular lib for decryption & encryption here. Considering the possibility that the lib author may discontinue its maintenance, we should either use ourselves's implementation, or ensure we know 100% how the lib works so that we can write an exactly same one. The former one is easier, I think.

Why not using the implementation in our built-in function encrypt(bytea, bytea, varchar) -> bytea?

Good point! Let me fix it.

risingwave/src/meta/src/rpc/ddl_controller.rs

Lines 633 to 660 in af892fa

let encrypted_payload = {

let data = secret.get_value().as_slice();

let key = self.env.opts.secret_store_private_key.as_slice();

let encrypt_key = {

let mut k = key[..(std::cmp::min(key.len(), 32))].to_vec();

k.resize_with(32, || 0);

k

};

let mut rng = rand::thread_rng();

let mut nonce: [u8; 16] = [0; 16];

rng.fill_bytes(&mut nonce);

let nonce_array = GenericArray::from_slice(&nonce);

let cipher = Aes128SivAead::new(encrypt_key.as_slice().into());

let ciphertext = cipher.encrypt(nonce_array, data).map_err(|e| {

MetaError::from(MetaErrorInner::InvalidParameter(format!(

"failed to encrypt secret {}: {:?}",

secret.name, e

)))

})?;

bincode::serialize(&SecretEncryption { nonce, ciphertext }).map_err(|e| {

MetaError::from(MetaErrorInner::InvalidParameter(format!(

"failed to serialize secret {}: {:?}",

secret.name, e

)))

})?

};

Impl the encryption myself :-)

yuhao-su · 2024-05-21T21:54:14Z

src/meta/Cargo.toml

@@ -64,6 +64,7 @@ scopeguard = "1.2.0"
 sea-orm = { workspace = true }
 serde = { version = "1.0.196", features = ["derive"] }
 serde_json = "1.0.113"
+simplestcrypt = "0.1"


Better to consider a more popular encryption lib

https://github.com/TotalKrill/simplestcrypt/blob/master/src/lib.rs
Took a rough look at the crate, the owner just wrapped a basic usage of an audited encryption crate, instead of writing the real "encryption" part. Seems not that vital. What do you think?

Signed-off-by: tabVersion <[email protected]>

yezizp2012

Rest generally LGTM. This PR is so large, rubber stamp for it. We need to add more e2e and secret usage support for it. After this PR, we also need to support it in migration script from etcd to sql backend.

src/sqlparser/src/ast/statement.rs

yezizp2012 · 2024-05-24T07:21:49Z

src/sqlparser/src/ast/mod.rs

@@ -1861,6 +1868,7 @@ impl fmt::Display for Statement {
            Statement::DeclareCursor { stmt } => write!(f, "DECLARE {}", stmt,),
            Statement::FetchCursor { stmt } => write!(f, "FETCH {}", stmt),
            Statement::CloseCursor { stmt } => write!(f, "CLOSE {}", stmt),
+            Statement::CreateSecret { stmt } => write!(f, "CREATE SECRET {}", stmt, ),


Suggested change

Statement::CreateSecret { stmt } => write!(f, "CREATE SECRET {}", stmt, ),

Statement::CreateSecret { stmt } => write!(f, "CREATE SECRET {}", stmt),

src/meta/src/rpc/ddl_controller.rs

src/meta/src/manager/catalog/database.rs

yezizp2012 · 2024-05-24T07:35:13Z

src/meta/src/manager/catalog/mod.rs

+        let user_core = &mut core.user;
+        let mut secrets = BTreeMapTransaction::new(&mut database_core.secrets);
+
+        match database_core.relation_ref_count.get(&secret_id) {


Not resolved, you can remove the check and leave a TODO here.

src/meta/model_v2/migration/src/m20240422_090457_secret.rs

yezizp2012 · 2024-05-24T07:43:26Z

src/meta/model_v2/migration/src/lib.rs

@@ -24,6 +25,7 @@ impl MigratorTrait for Migrator {
            Box::new(m20240410_154406_session_params::Migration),
            Box::new(m20240417_062305_subscription_internal_table_name::Migration),
            Box::new(m20240418_142249_function_runtime::Migration),
+            Box::new(m20240422_090457_secret::Migration),


I'm not sure whether a smaller date naming for this migration can be applied when upgrade from the version before this PR. Didn't check, if not work you can rename to a larger date for it.

e2e_test/ddl/secret.slt

src/meta/src/controller/mod.rs

Signed-off-by: tabVersion <[email protected]>

yezizp2012

Generally LGTM, thx for the PR.

src/meta/model_v2/migration/src/lib.rs

…#16939)

Co-authored-by: August <[email protected]>

fuyufjh · 2024-05-28T05:21:55Z

Bump up #16288 (comment). Seems to be overlooked.

The rest LGTM

Signed-off-by: tabVersion <[email protected]>

tabVersion · 2024-05-28T12:57:44Z

Bump up #16288 (comment). Seems to be overlooked.

The rest LGTM

Not sure if it is ok to dep risingwave_expr from risingwave_meta. I impl the encrypting process using aes-siv.

Signed-off-by: tabVersion <[email protected]>

fuyufjh

The rest LGTM

src/meta/Cargo.toml

Co-authored-by: Eric Fu <[email protected]>

Signed-off-by: tabVersion <[email protected]>

init

6906021

github-actions bot added type/feature ci/run-e2e-single-node-tests labels Apr 12, 2024

tabVersion added 3 commits April 15, 2024 12:25

Merge remote-tracking branch 'origin' into tab/create-secret

bc25a9e

stage

d2a171a

Merge remote-tracking branch 'origin' into tab/create-secret

612eb4a

yezizp2012 reviewed Apr 22, 2024

View reviewed changes

src/frontend/src/utils/with_options.rs Outdated Show resolved Hide resolved

src/meta/service/src/notification_service.rs Show resolved Hide resolved

src/meta/model_v2/src/secret.rs Show resolved Hide resolved

tabVersion added 9 commits April 24, 2024 15:06

stash

1e3961e

stash

49eefc0

stash

e3e7795

stash

1dfe129

stash

94f0993

support create secret.

ec8f375

support drop secret

53d7d6c

Merge remote-tracking branch 'origin' into tab/create-secret

961fde4

fix

b2de4da

github-actions bot reviewed Apr 28, 2024

View reviewed changes

src/frontend/src/handler/create_secret.rs Show resolved Hide resolved

tabVersion added 4 commits April 28, 2024 17:36

fix meta snapshot

b64288b

fix license

15bfd02

encrypt

bf11ee2

Merge branch 'main' into tab/create-secret

8e4637f

tabVersion added 2 commits May 2, 2024 20:35

create secret with backend

157f156

Merge branch 'main' into tab/create-secret

1a75e35

yezizp2012 reviewed May 7, 2024

View reviewed changes

fuyufjh reviewed May 8, 2024

View reviewed changes

tabVersion added 4 commits May 14, 2024 15:26

Merge remote-tracking branch 'origin' into tab/create-secret

fa15399

fix

8240a0e

Signed-off-by: tabVersion <[email protected]>

minor

dd5e7e6

Signed-off-by: tabVersion <[email protected]>

minor

384fb85

Signed-off-by: tabVersion <[email protected]>

tabVersion commented May 21, 2024

View reviewed changes

yuhao-su reviewed May 21, 2024

View reviewed changes

fix config test

eca94ab

Signed-off-by: tabVersion <[email protected]>

yezizp2012 reviewed May 24, 2024

View reviewed changes

tabVersion added 5 commits May 25, 2024 19:48

Merge remote-tracking branch 'origin' into tab/create-secret

1a4dc62

resolve

12d4182

Signed-off-by: tabVersion <[email protected]>

rename migration

1180cac

Signed-off-by: tabVersion <[email protected]>

remove comment

7720f33

Signed-off-by: tabVersion <[email protected]>

fix

2015899

Signed-off-by: tabVersion <[email protected]>

tabVersion mentioned this pull request May 25, 2024

fix: use dedicated secret_ref_count instead of relation_ref_count #16939

Merged

9 tasks

tabVersion added 2 commits May 25, 2024 23:21

add sql logic test

78027b1

Signed-off-by: tabVersion <[email protected]>

fix meta-backend

a1280fb

Signed-off-by: tabVersion <[email protected]>

yezizp2012 approved these changes May 27, 2024

View reviewed changes

src/meta/model_v2/migration/src/lib.rs Outdated Show resolved Hide resolved

tabVersion and others added 2 commits May 27, 2024 15:30

fix: use dedicated secret_ref_count instead of relation_ref_count (…

0c00675

…#16939)

Update lib.rs

62eba9c

Co-authored-by: August <[email protected]>

tabVersion added 3 commits May 28, 2024 11:47

Merge branch 'main' into tab/create-secret

6e04612

fix

f475dbc

Signed-off-by: tabVersion <[email protected]>

impl aes-siv

af892fa

Signed-off-by: tabVersion <[email protected]>

fix dylint

cef7115

Signed-off-by: tabVersion <[email protected]>

fuyufjh approved these changes May 28, 2024

View reviewed changes

src/meta/Cargo.toml Outdated Show resolved Hide resolved

tabVersion and others added 2 commits May 28, 2024 21:52

Update src/meta/Cargo.toml

bb9a4c9

Co-authored-by: Eric Fu <[email protected]>

fix cargo lock

9ac8c60

Signed-off-by: tabVersion <[email protected]>

tabVersion enabled auto-merge May 28, 2024 21:08

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

Merged via the queue into main with commit 11aac57 May 28, 2024
29 of 31 checks passed

tabVersion deleted the tab/create-secret branch May 28, 2024 22:26

yezizp2012 mentioned this pull request Jun 3, 2024

feat: support secret in migration script from etcd to sql #17064

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: create secret catalog #16288

feat: create secret catalog #16288

tabVersion commented Apr 12, 2024 •

edited

Loading

github-actions bot left a comment

gitguardian bot commented May 2, 2024 •

edited

Loading

yezizp2012 left a comment

yezizp2012 May 7, 2024

yezizp2012 May 7, 2024

tabVersion May 7, 2024

tabVersion May 7, 2024

yezizp2012 May 7, 2024

yezizp2012 May 7, 2024

yezizp2012 May 24, 2024

fuyufjh left a comment

fuyufjh May 8, 2024

tabVersion May 14, 2024

fuyufjh May 8, 2024

fuyufjh May 8, 2024

tabVersion May 21, 2024

fuyufjh May 27, 2024

fuyufjh May 28, 2024 •

edited

Loading

tabVersion May 28, 2024

tabVersion May 28, 2024

yuhao-su May 21, 2024

tabVersion May 23, 2024

yezizp2012 left a comment

yezizp2012 May 24, 2024

yezizp2012 May 24, 2024

yezizp2012 May 24, 2024

tabVersion May 25, 2024

yezizp2012 left a comment

fuyufjh commented May 28, 2024 •

edited

Loading

tabVersion commented May 28, 2024

fuyufjh left a comment

	fn eval_inner(
	&self,
	input: &[u8],
	operation: CipherMode,
	) -> std::result::Result<Box<[u8]>, ErrorStack> {
	let mut decrypter = Crypter::new(self.cipher, operation, self.crypt_key.as_ref(), None)?;
	let enable_padding = match self.padding {
	Padding::Pkcs => true,
	Padding::None => false,
	};
	decrypter.pad(enable_padding);
	let mut decrypt = vec![0; input.len() + self.cipher.block_size()];
	let count = decrypter.update(input, &mut decrypt)?;
	let rest = decrypter.finalize(&mut decrypt[count..])?;
	decrypt.truncate(count + rest);
	Ok(decrypt.into())
	}

	let encrypted_payload = {
	let data = secret.get_value().as_slice();
	let key = self.env.opts.secret_store_private_key.as_slice();
	let encrypt_key = {
	let mut k = key[..(std::cmp::min(key.len(), 32))].to_vec();
	k.resize_with(32, \|\| 0);
	k
	};

	let mut rng = rand::thread_rng();
	let mut nonce: [u8; 16] = [0; 16];
	rng.fill_bytes(&mut nonce);
	let nonce_array = GenericArray::from_slice(&nonce);
	let cipher = Aes128SivAead::new(encrypt_key.as_slice().into());

	let ciphertext = cipher.encrypt(nonce_array, data).map_err(\|e\| {
	MetaError::from(MetaErrorInner::InvalidParameter(format!(
	"failed to encrypt secret {}: {:?}",
	secret.name, e
	)))
	})?;
	bincode::serialize(&SecretEncryption { nonce, ciphertext }).map_err(\|e\| {
	MetaError::from(MetaErrorInner::InvalidParameter(format!(
	"failed to serialize secret {}: {:?}",
	secret.name, e
	)))
	})?
	};

	Statement::CreateSecret { stmt } => write!(f, "CREATE SECRET {}", stmt, ),
	Statement::CreateSecret { stmt } => write!(f, "CREATE SECRET {}", stmt),

feat: create secret catalog #16288

feat: create secret catalog #16288

Conversation

tabVersion commented Apr 12, 2024 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

github-actions bot left a comment

Choose a reason for hiding this comment

gitguardian bot commented May 2, 2024 • edited Loading

⚠️ GitGuardian has uncovered 2 secrets following the scan of your pull request.

yezizp2012 left a 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

fuyufjh left a 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

fuyufjh May 28, 2024 • edited Loading

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

yezizp2012 left a 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

yezizp2012 left a comment

Choose a reason for hiding this comment

fuyufjh commented May 28, 2024 • edited Loading

tabVersion commented May 28, 2024

fuyufjh left a comment

Choose a reason for hiding this comment

tabVersion commented Apr 12, 2024 •

edited

Loading

gitguardian bot commented May 2, 2024 •

edited

Loading

fuyufjh May 28, 2024 •

edited

Loading

fuyufjh commented May 28, 2024 •

edited

Loading