docs(secrets): add the Secrets Storage operator guide#3651
Conversation
Postgres-backed secrets storage shipped with envelope encryption, a configurable read chain, and a staged Vault migration -- and nothing in docs/ told an operator how to turn any of it on. The new Configuration page covers the whole `[secrets]` surface: the field reference with a complete TOML example, the KMS providers (`integrated` and `transit`, including where key material is allowed to live and why inline values leak into debug surfaces), routing with its required "/" catch-all, the four-step Vault-to-Postgres walk with the one-time import semantics and the two fleet-level caveats (services with their own read chains, rolling-upgrade writer skew), and KEK rotation/retirement via `secrets re-wrap` and `stale_remaining`. Every claim is verified against the current config structs and doc-comments (`SecretsConfig`, `ProviderConfig`, `KeySource`, the `run.rs` boot validation) rather than the design doc, which is still in flight as NVIDIA#3225 -- a link to it can land once it merges. This supports NVIDIA#3642 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
Summary by CodeRabbit
WalkthroughAdds a Secrets Storage documentation page covering configuration, routing, encrypted Postgres storage, Vault migration, and key rotation. Adds the page to the configuration navigation. ChangesSecrets storage documentation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3651.docs.buildwithfern.com/infra-controller |
🔍 Container Scan SummaryNo Grype artifacts were found to aggregate. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/configuration/secrets-storage.md`:
- Line 74: Update the migration rollback description near “The migration is a
config walk” to state that restoring the previous configuration reverses
read/write routing only; it does not remove rows already imported or written to
Postgres. Preserve the explanation that the journal is append-only and Vault
remains untouched, while avoiding claims that each migration step is fully
reversible.
- Line 70: Update the routing-edit guidance near “Reads ignore routing entirely”
to qualify that changing routing is not always safe: invalid routes may prevent
startup, while valid changes affect subsequent writes. Preserve the existing
instruction to re-wrap when migrating existing entries, and align the wording
with the SecretRouting validation and write-routing behavior.
- Line 99: Update the command documentation to clarify that each invocation
starts scanning from the beginning with a new in-memory cursor; remove any
implication that an interrupted run resumes from a persisted cursor. State that
committed batches remain effective and already-current rows are skipped, making
the operation restartable but not cursor-resumable.
- Line 55: Update the integrated key-material documentation near the keys
mapping to distinguish env/file references from inline value entries: state that
env and file keep key material out of the configuration, while value embeds the
base64 key directly and therefore exposes it wherever the configuration is
logged or served. Remove the false blanket claim that key material never appears
in config.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7d3c845a-acb1-4c40-b6d4-cd0328717ee7
📒 Files selected for processing (2)
docs/configuration/secrets-storage.mddocs/index.yml
|
|
||
| Providers are named. The `active` provider wraps DEKs for new writes; every configured provider answers unwraps for the `kek_id`s it holds, which is what keeps old entries readable while keys move. Two provider types exist: | ||
|
|
||
| - `integrated`: local key material. `keys` maps each `kek_id` to where its base64-encoded 256-bit key loads from: `{ env = "NAME" }`, `{ file = "/path" }`, or `{ value = "..." }`. Key material never appears in the config, only where to find it. Prefer `env` or `file` for real keys: the config file is debug-logged at startup and served on the web debug page, so an inline `value` lands in both. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Correct the statement about inline key material.
{ value = "..." } embeds the base64 key directly in configuration, so “Key material never appears in the config” is false. Explicitly distinguish env/file from inline value; otherwise operators may choose the least-safe option while believing the secret is not exposed.
Proposed wording
-- `integrated`: local key material. `keys` maps each `kek_id` to where its base64-encoded 256-bit key loads from: `{ env = "NAME" }`, `{ file = "/path" }`, or `{ value = "..." }`. Key material never appears in the config, only where to find it. Prefer `env` or `file` for real keys: the config file is debug-logged at startup and served on the web debug page, so an inline `value` lands in both.
+- `integrated`: local key material. `keys` maps each `kek_id` to a base64-encoded 256-bit key loaded from `{ env = "NAME" }`, `{ file = "/path" }`, or inline `{ value = "..." }`. `env` and `file` keep key material out of the config; `value` embeds it directly. Prefer `env` or `file` for real keys because the config is debug-logged at startup and served on the web debug page.As per path instructions, this documentation must be technically correct and safe for operators handling secrets.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - `integrated`: local key material. `keys` maps each `kek_id` to where its base64-encoded 256-bit key loads from: `{ env = "NAME" }`, `{ file = "/path" }`, or `{ value = "..." }`. Key material never appears in the config, only where to find it. Prefer `env` or `file` for real keys: the config file is debug-logged at startup and served on the web debug page, so an inline `value` lands in both. | |
| `integrated`: local key material. `keys` maps each `kek_id` to a base64-encoded 256-bit key loaded from `{ env = "NAME" }`, `{ file = "/path" }`, or inline `{ value = "..." }`. `env` and `file` keep key material out of the config; `value` embeds it directly. Prefer `env` or `file` for real keys because the config is debug-logged at startup and served on the web debug page. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/configuration/secrets-storage.md` at line 55, Update the integrated
key-material documentation near the keys mapping to distinguish env/file
references from inline value entries: state that env and file keep key material
out of the configuration, while value embeds the base64 key directly and
therefore exposes it wherever the configuration is logged or served. Remove the
false blanket claim that key material never appears in config.
Source: Path instructions
| "machines/bmc" = "bmc-key" | ||
| ``` | ||
|
|
||
| Reads ignore routing entirely, so editing it is always safe for existing data. Pair a routing change with a re-wrap (below) when the goal is to move existing entries onto the new key. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Qualify “always safe” for routing edits.
Routing is ignored when decrypting existing rows, but an invalid route can fail startup, and a valid route changes every subsequent write. The supplied SecretRouting contract in crates/api-core/src/secrets/routing.rs:22-145 confirms these validation and routing effects.
Proposed wording
-Reads ignore routing entirely, so editing it is always safe for existing data. Pair a routing change with a re-wrap (below) when the goal is to move existing entries onto the new key.
+Reads ignore routing when decrypting existing rows, so a valid routing edit does not change their decryption. Routing edits still affect startup validation and all new writes. Pair a routing change with a re-wrap (below) when the goal is to move existing entries onto the new key.As per path instructions, this guide must accurately describe operational behavior and failure modes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Reads ignore routing entirely, so editing it is always safe for existing data. Pair a routing change with a re-wrap (below) when the goal is to move existing entries onto the new key. | |
| Reads ignore routing when decrypting existing rows, so a valid routing edit does not change their decryption. Routing edits still affect startup validation and all new writes. Pair a routing change with a re-wrap (below) when the goal is to move existing entries onto the new key. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/configuration/secrets-storage.md` at line 70, Update the routing-edit
guidance near “Reads ignore routing entirely” to qualify that changing routing
is not always safe: invalid routes may prevent startup, while valid changes
affect subsequent writes. Preserve the existing instruction to re-wrap when
migrating existing entries, and align the wording with the SecretRouting
validation and write-routing behavior.
Source: Path instructions
|
|
||
| ## Migrating from Vault to Postgres | ||
|
|
||
| The migration is a config walk. Each step is a config change plus a restart, and each step reverses by putting the previous config back: the journal is append-only and the import leaves Vault untouched. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Clarify what migration rollback reverses.
Restoring the previous configuration rolls back read/write routing, but it does not remove rows already imported or written to Postgres. Because the journal is append-only, describing every step as fully reversible can mislead operators about persisted state.
Proposed wording
-The migration is a config walk. Each step is a config change plus a restart, and each step reverses by putting the previous config back: the journal is append-only and the import leaves Vault untouched.
+The migration is a config walk. Each step is a config change plus a restart, and traffic can be rolled back by restoring the previous config. Rollback does not remove rows already imported or written to Postgres: the journal is append-only and the import leaves Vault untouched.As per path instructions, this documentation must be technically accurate and operator-usable.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| The migration is a config walk. Each step is a config change plus a restart, and each step reverses by putting the previous config back: the journal is append-only and the import leaves Vault untouched. | |
| The migration is a config walk. Each step is a config change plus a restart, and traffic can be rolled back by restoring the previous config. Rollback does not remove rows already imported or written to Postgres: the journal is append-only and the import leaves Vault untouched. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/configuration/secrets-storage.md` at line 74, Update the migration
rollback description near “The migration is a config walk” to state that
restoring the previous configuration reverses read/write routing only; it does
not remove rows already imported or written to Postgres. Preserve the
explanation that the journal is append-only and Vault remains untouched, while
avoiding claims that each migration step is fully reversible.
Source: Path instructions
| nico-admin-cli secrets re-wrap | ||
| ``` | ||
|
|
||
| The report counts `re_wrapped`, `already_current`, and `stale_remaining`. Only the DEK wrapping is redone (credential ciphertext is untouched), batches commit independently, and an advisory lock keeps it to one walk at a time, so the command is safe to re-run and resumes where it left off. `--batch-size` sets the rows scanned per batch; a smaller batch lightens the load on an external KMS. |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Do not imply a persisted re-wrap cursor.
re_wrap_stale starts with a new in-memory cursor on each invocation, so an interrupted run scans from the beginning again. Committed batches remain effective and already-current rows are skipped, making the operation restartable but not cursor-resumable.
Proposed wording
-Only the DEK wrapping is redone (credential ciphertext is untouched), batches commit independently, and an advisory lock keeps it to one walk at a time, so the command is safe to re-run and resumes where it left off.
+Only the DEK wrapping is redone (credential ciphertext is untouched), batches commit independently, and an advisory lock keeps it to one walk at a time, so the command is safe to re-run and preserves committed progress; an interrupted run scans again and skips rows already current.As per path instructions, operator documentation must accurately describe command behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| The report counts `re_wrapped`, `already_current`, and `stale_remaining`. Only the DEK wrapping is redone (credential ciphertext is untouched), batches commit independently, and an advisory lock keeps it to one walk at a time, so the command is safe to re-run and resumes where it left off. `--batch-size` sets the rows scanned per batch; a smaller batch lightens the load on an external KMS. | |
| The report counts `re_wrapped`, `already_current`, and `stale_remaining`. Only the DEK wrapping is redone (credential ciphertext is untouched), batches commit independently, and an advisory lock keeps it to one walk at a time, so the command is safe to re-run and preserves committed progress; an interrupted run scans again and skips rows already current. `--batch-size` sets the rows scanned per batch; a smaller batch lightens the load on an external KMS. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/configuration/secrets-storage.md` at line 99, Update the command
documentation to clarify that each invocation starts scanning from the beginning
with a new in-memory cursor; remove any implication that an interrupted run
resumes from a persisted cursor. State that committed batches remain effective
and already-current rows are skipped, making the operation restartable but not
cursor-resumable.
Source: Path instructions
1 participant
Adds
docs/configuration/secrets-storage.md-- the operator guide for Postgres-backed secrets: the[secrets]field reference with a complete TOML example, KMS providers (integrated/transit), routing, the four-step Vault-to-Postgres migration walk (one-time import semantics, fleet caveats), and KEK rotation/retirement viasecrets re-wrap. Verified against the current config structs and boot validation; the design doc (#3225) gets linked once it merges.This supports #3642