Conversation
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
🌿 Preview your docs: https://nvidia-preview-pr-3557.docs.buildwithfern.com/openshell |
elezar
left a comment
There was a problem hiding this comment.
The active/decrypt-only keyring model, envelope rewrap, CAS updates, backup guidance, and Helm Secret wiring are directionally sound. I found three issues that should be addressed before this is used to close FSR-DATA-KEK-ROTATION:
-
The rotation audit events do not reach structured OCSF JSONL output.
emit_credential_rewrap_eventbuilds aConfigStateChangebut callstracing::info!directly (crates/openshell-server/src/credentials.rs:831-836).OcsfJsonlLayeronly writes when an event has been placed in the OCSF bridge; the gateway path for that isemit_ocsf_event_routed. As written, operators may see shorthand logs whileopenshell-ocsf.logomits start/completion/failure and counts. Please use the structured routed emitter and add a JSONL capture regression test. -
The operational sequence needs an explicit mixed-version prerequisite and rollback boundary. The previous gateway rejects the new keyring fields because its credential-storage config uses
deny_unknown_fields. After the active key changes, new envelopes use it, and the new test correctly demonstrates that an old-key-only runtime cannot read migrated records (crates/openshell-driver-db-credstore/src/lib.rs:1620-1629). Please document that every replica must first run a keyring-aware gateway before adding these fields or switching the active key; no legacy replica may remain when the switch occurs; and binary downgrade is unsafe after the switch or partial rewrap until the old key has been made active and a reverse rewrap has completed and been verified. -
The migration tests do not cover the paths needed to justify retiring the previous key. The production sweep pages at 100 records, but the new tests exercise one record, one injected conflict, and one startup migration. Please add coverage crossing a page boundary, interruption followed by idempotent resume, and concurrent sweepers or equivalent HA contention.
Local verification at db0f3e97: the credential-driver tests, mise run helm:test (159 tests), and mise run pre-commit passed. The focused server startup test could not link on my host because libz3 is unavailable. GitHub's required Branch Checks and Helm Lint contexts have not run and remain pending with “Waiting for /ok to test mirror.”
The smallest safe path is to fix and test structured OCSF emission, document the version/rollback constraints, add the migration boundary/failure tests, then rebase and run the required CI gates. The PR is currently still a draft; Seth's Red Hat feedback should also be incorporated before marking it ready.
Summary
Improve key lifecycle management for the default encrypted database credential store.
Changes
Testing
mise run pre-commitmise run testmise run helm:testmise run ci(host-level Go checks are blocked by a pre-existing system gateway fixture and stale linter cache; changed-area checks pass)Checklist