feat(secret): introduce secret management

[yuhao-su]

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

What's changed and what's your intention?

• Create/Drop secret
  On create/drop secret, frontend will run command on meta and meta will distribute the plaintext secret to meta, frontend and compute node via notification service.
  

• Handle ref secret in frontend binder

1. with properties was parsed to WithOptions (src/connector/src/with_options.rs), which contains two parts, non-ref-secret properties and ref-secret properties.
2. secret properties in WithOptions was resolved (bound) to WithOptionsSecResolved, in which process ObjectName was resolved to PbSecretRef.
3. WithOptionsSecResolved will be used in binder to check properties in binder just as WithOptions used to do. It will also be used to build connections in frontend to get schema registry and iceberg schema etc..
4. WithOptionsSecResolved will into_part() to properties map and secret properties map and used by source/sink catalogs and further used by plan nodes (Source/FsSource/Sink etc..) in planner.

• Fill secret
  Secret will be filled from SecretManager and merge with other properties in fill_secrets.

1. In source connector: ConnectorProperties::extract is used to convert btreemap to ConnectorProperties, now we fill the secret in that function with one exception ExternalTableConfig.

2. In sink connector: SinkParam is used for sink connectors. We fill the secret before create the SinkParam

TODO:

• Remove the Deref BTreeMap for WithOptions and WithOptionsSecResolve. One of the reason is if we ref secret for properties like connection, RW will yield connection not found since it's in secret_refs, but currently WithOptions::get will only return those in inner
• use a concrete type for properties with filled secrets. This is to avoid forgetting to use secret ref.
• ACL
• more test

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)

Secret Management

create secret

CREATE SECRET secret-name WITH ( backend = 'meta') AS 'your-secret';
Currently only meta backend is supported

DROP SECRET secret-name;

example:

CREATE SECRET mysql_pwd WITH (
  backend = 'meta'
) AS '123';

use secret

Use SECRET your-secret-name as the option value in the with clause.

example:

create source mysql_source with (
 connector = 'mysql-cdc',
 hostname = 'localhost',
 port = '8306',
 username = 'rwcdc',
 password = secret mysql_pwd,
 database.name = 'test',
 server.id = '5601'
);

Use as file

Convert a secret stored to a file path. The file content is the secret stored.

CREATE TABLE district (
    d_id INTEGER,
    PRIMARY KEY (d_id, d_w_id)
) with (
    connector = 'kafka',
    topic = 'your-topic',
    properties.bootstrap.server = 'your-broker-address:29092',
    ssl.ca.location =  SECRET kafka_ca AS FILE,
    ssl.certificate.location = SECRET kafka_cert AS FILE,
    ssl.key.location = SECRET kafka_key AS FILE,
   ssl.key.password = SECRET kafka_password,
) FORMAT DEBEZIUM ENCODE JSON;

To use secret management

you need to set the environment variable RW_SECRET_STORE_PRIVATE_KEY_HEX as a hex representation of a 128-bit key. e.g. 0123456789abcdef0123456789abcdef

Set the temporary secret file directory RW_TEMP_SECRET_FILE_DIR. It will only be used when using as file

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.

[gitguardian]

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
Once a secret has been leaked into a git repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.

---

^{_{🦉 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.}}

[xxchan]

What about separating into 2 PRs (or 2 commits), one for backend (this part might not be testable, but at least it will be more reviewable) and another one for frontend?

[yuhao-su]

What about separating into 2 PRs (or 2 commits), one for backend (this part might not be testable, but at least it will be more reviewable) and another one for frontend?

I understand this is too big to review. But it can be hard to separate it because some data types are used as concrete type for both frontend and backend. Also I might not be that helpful for review considering filling secret are used in both frontend and backend (if you consider compute node as backend) and also meta node.

I'll add more implementation detail/structure in the PR description. I'll also add comments about those who might raise concerns and guide for review.

[yuhao-su]

It will be great if we RW can know about if it's a test environment. We need a test encryption key for secret manger or we need to pass the key env var everywhere we start the test. cc @xxchan @BugenZhao

[xxchan]

But it can be hard to separate it because some data types are used as concrete type for both frontend and backend. Also I might not be that helpful for review considering filling secret are used in both frontend and backend (if you consider compute node as backend) and also meta node.

Oh, I got it. Do you mean things like ConnectorProperties is used in both fe, be, and meta..? That's indeed tricky and cannot be separated cleanly.

I'm thinking if there are still relatively self-contained things can be splitted out, e.g., the meta's RPC and storage of secrets.

e.g., is this possible:

1. change .proto and all related things. types in connector are not changed. I guess this mainly affects meta
2. change connector, and every where, fe, meta and compute
3. change user-facing part, i.e., parser and fe handler

[tabVersion]

It will be great if we RW can know about if it's a test environment. We need a test encryption key for secret manger or we need to pass the key env var everywhere we start the test. cc @xxchan @BugenZhao

I think it can be done by specifying a test private key? We will remind the users to change it before going into prod.

[BugenZhao]

It will be great if we RW can know about if it's a test environment. We need a test encryption key for secret manger or we need to pass the key env var everywhere we start the test. cc @xxchan @BugenZhao

Opened an issue: #17465

[yuhao-su]

Do you mean things like ConnectorProperties is used in both fe, be, and meta..?

Not necessarily ConnectorProperties, but secrets need to be filled in both fe, be, and meta (e.g. for validation, schema registry, build reader...)

change .proto and all related things. types in connector are not changed. I guess this mainly affects meta
change connector, and every where, fe, meta and compute
change user-facing part, i.e., parser and fe handler

I managed to do 1 in #17474. But 2 and 3 are a little tricky. I need to use WithOptionsSecResolved and new WithOptions in fe handler, which propagates pretty wildly. Also filling secret also relies on WithOptionsSecResolved. So I guess it's probably not worth doing that. I think we can try start review after I add the review guide. I'll see what I can do if you still find it hard to review.

[yuhao-su]

We will remind the users to change it before going into prod.

If they forget, we don't support alter the key

[fuyufjh]

LGTM. Will approve once the following comment gets resolved.

[fuyufjh]

(Continue from #17456 (comment))

Introducing the cluster_id just for simulation tests is unacceptable to me. :(

Maybe hack in risingwave_simulation::start to add a suffix?

It's not a hack. IIUC, simulation test runs multiple RW instance at the same time, so it should prepare separated working folders for them.

[yuhao-su]

It's a problem not just limited to test. Users can 1. start multiple RW clusters on 1 machine. 2. start multiple nodes on 1 machine.

Making users config a distinct path for each cluster and each node is not ideal. Misconfiging this can cause unexpected race on 1 directory. So not just cluster id, I actually think we should also add node id.

Such race condition can also happens if we have file cache later.

[fuyufjh]

Making users config a distinct path for each cluster and each node is not ideal.

Hmmm, I think this is quite natural. Image that you are running multiple MySQL/PG service in one Linux machine, you can't use the default data dir /var/mysql/. Instead, you must manually create and provide multiple data dir to them.

On the other hand, our simulation test framework was developed early when RisingWave doesn't use local disk at all, so it overlooked this part.

[yuhao-su]

Then we need a way like lock file to make sure a dir won't be shared by two RW instance

Not only different RW instances, different working nodes and their replication can be potentially deployed on a single machine.

Using RW cluster id to distinguish different RW instances can be used in other circumstances like connection name when two RW instance connection to the same up/downstream. So I guess users shouldn't be surprised to find a dir with uuid name

[tabVersion]

Having the same question. Shall we accept the hypothesis that tempfile's name seldom collide?

---

In my design, a secret file on CN is never reused so we'd pass the content in each request.
Using a local manager to keep the files live long enough is okay, but why we need to take the local one as the ground truth.

[yuhao-su]

in each request

If you mean the request from meta to cn, we have a lot of those request including altering/create/recovery... also frontend need secret files.

seldom collide

It can still happen. Do we have any reason not to handle this case?

[tabVersion]

just confirm: we are distributing encrypted files to nodes, right?

[yuhao-su]

just confirm: we are distributing encrypted files to nodes, right?

More precisely, we are distributing file content to node. And form files on each node.

[tabVersion]

@yuhao-su is there a reason why introducing an Either here? I see the right branch is only called in refresh_sr_and_get_columns_diff, why don't we need to resolve props coming from the func?

[yuhao-su]

Because that function is using a WithOptionsSecResolved type option from SourceCatalog. That means it's already been resolved before.