feat(secrets): keyless-secrets RFC + Phase-1 store version guard#346
Merged
Conversation
Adds a `version` field to secrets.yaml and a CurrentSecretsFileVersion this build understands (0 = the original format; absent version is treated as 0). unmarshalSecretsFile now REFUSES to read any store whose version exceeds what this build supports, instead of partially parsing it and then silently dropping the unknown fields on the next write (which would corrupt the store). This is the prerequisite for any future secrets-format change: this fail-closed reader must ship and roll out fleet-wide BEFORE a higher-versioned store is ever written, so older binaries refuse rather than corrupt. `version,omitempty` keeps existing stores byte-identical on rewrite (no churn, full back-compat). Tests: a version-99 store is refused with a clear upgrade message; a version-less (current) store still reads. Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
Cre-eD
requested review from
Laboratory,
smecsia and
universe-ops
as code owners
Semgrep Scan ResultsRepository:
Scanned at 2026-06-28 14:36 UTC |
Security Scan ResultsRepository:
Scanned at 2026-06-28 14:36 UTC |
📊 Statement coverageMeasured on the documented included set (see
Baseline: |
This comment has been minimized.
This comment has been minimized.
Proposes pluggable recipient types whose private key never leaves a remote authority (cloud KMS / Vault) plus a keyless CI mode via OIDC federation, so the store private key (SIMPLE_CONTAINER_CONFIG) no longer has to be materialized on CI runners. Cloud-agnostic and backward-compatible: the local recipient stays the default; migration is additive, per-repo, and reversible. Covers the v2 envelope prerequisite (single AEAD DEK + per-recipient wrapping + AAD binding), the OIDC trust/authorization model, format versioning with a fail-closed guard, revocation reality (value rotation), operational concerns, and a phased migration plan. Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
…OPS, file-per-scope Folds the multi-model design review outcome into the RFC: adopt SOPS for the inline-value crypto/format, one readable file per scope (not a multiplexed single file — avoids the partial-MAC paradox), per-scope isolated keys, the non-negotiables (hard-fail on missing required secret, MAC+AAD, CODEOWNERS-gated recipients, plaintext lint, rotate-not-just-remove, OIDC->KMS provider keys), the strict-backcompat constraint (separate files + fail-closed version reader first), and a minimal v1. Resolves the format/native-vs-SOPS open questions. Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
Cre-eD
changed the title
The guard returned an error from unmarshalSecretsFile, but the root PersistentPreRunE inits with IgnoreAllErrors, so the CLI swallowed it and read a too-new store as empty — a subsequent write would then clobber the newer file (the exact data loss the guard prevents). It only fired on the GitHub-Actions path, which checks the error. Make the too-new-version error a sentinel (ErrSecretsStoreVersionUnsupported) and special-case it in root_cmd.Init so it stays fatal regardless of IgnoreConfigDirError. Found via the branch preview build (sc secrets list on a version:99 store returned empty, exit 0). Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
Review (Codex) found the guard was only half-wired: besides the CLI, the GitHub Actions deploy/reveal paths swallowed ReadSecretFiles errors as 'no client secrets -> use parent' (operation_executor revealSecrets x2, parent_repo revealParentRepositorySecrets, and the revealCurrentRepositorySecrets wrapper). A too-new store was silently treated as empty there too. Add secrets.IsUnsupportedStoreVersion(err) helper and special-case it at every tolerant read-path swallow site so the too-new-version error is ALWAYS fatal. Refactor root_cmd.Init to use the helper. Cover the helper contract with a unit test; document the version field + fail-closed behaviour in the secrets guide. Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
smecsia
reviewed
Jun 28, 2026
smecsia
previously approved these changes
Jun 28, 2026
Rename the secrets.yaml schema field 'version' to 'schemaVersion' (clearer than a bare 'version', which reads like a data/app version) per dev review. Renames the on-disk key + Go field (SchemaVersion) + const (CurrentSecretsSchemaVersion) and updates the guard error wording, tests (keyed on schemaVersion), and the secrets guide. Helper/sentinel names unchanged. No behaviour change; back-compat preserved (absent = 0). Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
smecsia
approved these changes
Jun 28, 2026
universe-ops
approved these changes
Jun 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Consolidates the keyless-secrets design and its Phase-1 implementation into one PR (supersedes #344, which was doc-only).
1. RFC — keyless secret backend (
docs/design/keyless-secrets/README.md)Design for getting the master key out of GitHub: KMS-held recipients + OIDC, no behaviour change. Records the decision: inline values + per-scope keys, via SOPS, file-per-scope (
.sc/secrets.<scope>.yaml) — avoids the multiplexed "partial-MAC paradox", gives per-scope key isolation.2. Phase-1 — fail-closed store version guard (code + tests)
Prerequisite for any future on-disk format change (incl. the SOPS migration above):
versionto thesecrets.yamlschema (EncryptedSecretFiles.Version); absent/0= the original format (full back-compat).CurrentSecretsFileVersion = 0— the highest schema version this build understands.versionexceeds what the build supports, instead of silently dropping the fields it can't model (which would corrupt the store on the next write).Why it ships first: this reader must roll out fleet-wide before any higher-versioned store is ever written, or older binaries clobber it. It is a no-op for every existing (version-0) store today.
Branch is synced with
main, so history includes the already-merged #345 (X25519) — net new here = the guard + RFC + tests.Validated via a preview build (see comments).