risingwave icon indicating copy to clipboard operation
risingwave copied to clipboard

WIP: feat(secret): introduce secret management

Open yuhao-su opened this issue 1 year ago • 7 comments

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

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

yuhao-su avatar Jun 26 '24 00:06 yuhao-su

️✅ 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.

gitguardian[bot] avatar Jun 26 '24 00:06 gitguardian[bot]

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 avatar Jun 26 '24 03:06 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

yuhao-su avatar Jun 26 '24 04:06 yuhao-su

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

xxchan avatar Jun 26 '24 05:06 xxchan

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.

tabVersion avatar Jun 26 '24 06:06 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

Opened an issue: https://github.com/risingwavelabs/risingwave/issues/17465

BugenZhao avatar Jun 26 '24 07:06 BugenZhao

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 avatar Jun 26 '24 20:06 yuhao-su

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

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

yuhao-su avatar Jul 02 '24 21:07 yuhao-su

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

yuhao-su avatar Jul 03 '24 19:07 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.

yuhao-su avatar Jul 15 '24 02:07 yuhao-su