risingwave
risingwave copied to clipboard
WIP: feat(secret): introduce secret management
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 secretin frontend binder
- with properties was parsed to
WithOptions(src/connector/src/with_options.rs), which contains two parts, non-ref-secret properties and ref-secret properties. - secret properties in
WithOptionswas resolved (bound) toWithOptionsSecResolved, in which processObjectNamewas resolved toPbSecretRef. WithOptionsSecResolvedwill be used in binder to check properties in binder just asWithOptionsused to do. It will also be used to build connections in frontend to get schema registry and iceberg schema etc..WithOptionsSecResolvedwillinto_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
SecretManagerand merge with other properties infill_secrets.
-
In source connector:
ConnectorProperties::extractis used to convertbtreemaptoConnectorProperties, now we fill the secret in that function with one exceptionExternalTableConfig. -
In sink connector:
SinkParamis used for sink connectors. We fill the secret before create theSinkParam
TODO:
- Remove the Deref BTreeMap for
WithOptionsandWithOptionsSecResolve. One of the reason is if weref secretfor properties likeconnection, RW will yieldconnectionnot found since it's insecret_refs, but currentlyWithOptions::getwill only return those ininner - 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 #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.
️✅ 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.
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.
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
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:
- change
.protoand all related things. types inconnectorare 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
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.
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: https://github.com/risingwavelabs/risingwave/issues/17465
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.
We will remind the users to change it before going into prod.
If they forget, we don't support alter the key
I have a parameter temp_secret_file_dir that is a directory to put some secret files. I tried to set this in common.sh but in simulation test multiple risingwave_simulation runs concurrently causing file operation conflict. (This can also cause potential problem to file cache)
I can do this in following ways:
- Use a cluster id as suffix
- Maybe hack in risingwave_simulation::start to add a suffix?
- modify all the simulation test script Any suggestions? @xxchan @BugenZhao
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.