feat(secrets): add Bitwarden/GSM secret management - #161
Conversation
|
/lgtm |
|
Thanks for the work here. I reviewed the full PR branch (39 files, ~6.8k net additions); the build passes and the test suite passes (270/270). I have two security concerns:
There is also a design concern for future maintainability: runtime secret consumption, child-process transport, and two-provider create/update/delete reconciliation are all bundled into the same package. That is workable for E2E tooling, but a typical Node application would use a provider interface for reads, keep administration/rotation in a separate CLI or CI job, and use one authoritative secret store. The custom FD3 stream should remain an explicit internal adapter rather than the primary application API. Finally, the PR is much larger than its final Linux-process-limit title suggests because it includes the entire Bitwarden/GSM lifecycle feature, the FD3 streaming protocol, rollback/process-lifecycle hardening, tests, and documentation across 19 commits. Splitting the provider lifecycle, stream transport, and final Linux-limit fix into separate PRs would make review easier. |
Assisted-by: OpenCode
Replace rotation-only operations with create, update, delete, describe, and list commands. Add forced reconciliation and unified dry-run behavior while removing rotation journals and resume support. BREAKING CHANGE: replace rotate and --resume with create, update, delete, describe, and list. Assisted-by: OpenCode
Use the wrapper default describe output and distinguish missing Google ADC credentials from missing secrets. Assisted-by: OpenCode
Add subcommand help and option aliases, make piped GSM creates terminal-safe, and include provider-specific collection names in mutation output. Assisted-by: OpenCode
Avoid stale writes, misleading retry guidance, and cross-platform process leaks while reducing redundant Bitwarden CLI calls. Assisted-by: OpenCode
Add opt-in child metadata for selected environment names without exposing secret values or provider credentials. Assisted-by: OpenCode
Assisted-by: OpenCode
Prevent orphaned Bitwarden attachments, stale child metadata, and ambiguous GSM mutations. Make GSM cleanup offline-safe while preserving the 2.1.15 public contracts. Assisted-by: OpenCode
Assisted-by: OpenCode
Assisted-by: OpenCode
Assisted-by: OpenCode
Assisted-by: OpenCode
Assisted-by: OpenCode
Assisted-by: OpenCode
Assisted-by: OpenCode
Assisted-by: OpenCode
Assisted-by: OpenCode
Assisted-by: OpenCode
Treat ECONNRESET as an early FD 3 close and point E2BIG failures to the stream transport. Assisted-by: OpenCode
Validate the moving GSM wrapper URL and restrict its image repository. Use private temporary-secret directories with stale-process cleanup and preserve cleanup failures. Assisted-by: OpenCode
Assisted-by: OpenCode
dcb6471 to
5e90d46
Compare
|
Hi @subhashkhileri,
I agree it's not ideal, but I believe we should continue to use the newest version of the
As a security measure, I added hardening that unifies the folder being used. I also added cleanup guidance for cases where it's required. Having a CI job for synchronization is an interesting idea. I'm still leaning towards a local CLI, because any errors would surface early, with an immediate response, so they can be resolved. But I agree that separating it into two CLIs can make sense here. |
|
I thought about having two CLIs - one for local execution and the other for secrets administration, but I believe it will just complicate things and add extra overhead, and make it harder to understand for users. |
|
/lgtm |
Summary
rhdh-e2e-secretsCLI and the@red-hat-developer-hub/e2e-test-utils/secretsAPI.exec.create,update, anddeletecommands.describe,list,gsm-login, andgsm-cleancommands.--stream-secretsexecution for passing sorted secret entries over file descriptor 3 instead of the child environment.Human comment
bwCLI inside the container, because there will be a lengthy login process each time.rhdhrepo.How to test
Run the automated checks:
To interact with the CLI directly, set
BW_SESSIONand use an existing profile:Authenticate the CLI's Google Secret Manager (GSM) wrapper before reading or changing GSM secrets:
List the paired collections, list paths in one collection, or inspect a secret's GSM metadata:
Mutation commands write Bitwarden first and GSM second. Add
--dry-runto validate both providers and print a value-free plan without changing either provider.Create an attachment-backed secret from a file:
Use
--from-stdininstead of--from-fileto create a note-backed Bitwarden item. GSM prompts for non-secret metadata duringcreate.Rotate a secret with
update. The command requires the secret in both providers and preserves its current Bitwarden storage form:Delete the secret from both providers:
Use
--forcewithcreateordeleteto reconcile a secret that exists in only one provider.updatedoes not support--force. If an empty value is intentional, use--allow-emptywithcreateorupdate. Runnode dist/secrets/cli.js gsm-cleanto remove the CLI's cached GSM authentication.Use RHDH PR #5404 to test FD 3 integration. Build this PR in an adjacent
rhdh-e2e-test-utilscheckout, then run the host-side test wrapper from the RHDH checkout:local-test.shinvokesrhdh-e2e-secrets exec --stream-secrets, reads the selected secrets from FD 3, and passes the remaining arguments to Playwright. SetK8S_CLUSTER_URLandK8S_CLUSTER_TOKENfor cluster-aware tests.Architecture
--dry-runvalidates the preflight plan without writing either provider.secret-manager.shwith a validated local wrapper cache and a CLI-owned Google ADC directory.0700and files with mode0600. Normal cleanup runs after each mutation. Before another temporary secret is created, the CLI removes directories owned by dead processes; operating-system pruning is the final fallback.SIGKILL, power loss, or a runtime crash can leave files until recovery. The CLI never places GSM values in process arguments through--from-literal.Limitations
rhdh-aws-credentials.bwonPATHand an already unlockedBW_SESSION. The CLI does not manage the Bitwarden session.secret-manager.shcopy.createrequires a controlling terminal for metadata prompts, including when the secret value comes from piped stdin.--forceto reconcilecreateordelete.updatedoes not support--force.execmode still places selected values in the child environment. Stream mode requires explicit opt-in and targets a trusted local parent-child boundary.mainbranch. Validation checks its expected structure rather than a signature or pinned checksum.