feat: handle key rotation in plain pull, push and diff behind features.key_rotation by piotrek-janus · Pull Request #54 · SecureAuthCorp/ciam-config-as-code

piotrek-janus · 2026-10-13T09:36:33Z

Stacked on #53 (base add-key-rotation-v2).

Summary

Adds a features.key_rotation config flag. When it is on, the plain pull, push and diff commands also handle workspaces/<wid>/key_rotation.yaml, so one cac diff or cac push covers it, for example in a CI diff bot. The pull|push|diff key-rotation subcommands run the same code path as the plain command with --filter key_rotation, for the selected workspace, and work with the flag on or off; diff key-rotation compares local against remote.

Config

client:
  scopes:
    - manage_configuration
    - manage_servers   # required when features.key_rotation is true
features:
  key_rotation: true   # default false; can be overridden per profile, like client/storage

A profile without features gets an independent copy of the top-level section. InitApp rejects features.key_rotation: true when the effective client.scopes lacks manage_servers, with the error features.key_rotation requires the manage_servers scope in client.scopes.

Gating rules

Key rotation is handled when the feature is on AND (--filter is not given OR it includes key_rotation).
--filter key_rotation on its own skips the regular configuration pipeline entirely. Running it with an empty filtered patch would make push --method import send an empty configuration. A mixed filter such as clients,key_rotation runs both.
Workspace mode works on --workspace. Tenant mode works per workspace, one at a time:
- pull: the workspaces in the pulled tenant config (servers). When the regular pull was skipped or filtered them out, cac runs a full tenant export and keeps only servers client-side; that export is paid once
- push: the local workspace dirs that contain key_rotation.yaml
- diff: the source side's workspaces. A remote or merged source lists them with the same full tenant export filtered client-side, paid once per such source

Output shapes

push: local key rotation is read and validated before anything is pushed, then pushed after the regular push succeeds. The remote settings are read first and a use whose enabled and cron already match is skipped (Key rotation unchanged), unless it sets the write-only starting_from, which is always written. push key-rotation behaves the same.
push --dry-run:
- --out -: the regular output, then each workspace as a separate YAML document, so the stream stays valid YAML:
```
---
# key_rotation (<wid>)
sig:
  enabled: true
  cron: 0 0 1 * *
```
- --out <existing dir>: written to <out>/workspaces/<wid>/key_rotation.yaml.
- --out <file>: the file is emptied once at the start of the run, then the regular output and the same sections are appended in that order. A key-rotation-only dry run with nothing to push leaves an empty file.
- push key-rotation --dry-run is the same with the regular output skipped.
diff: after the regular diff, a section per workspace, added only when that workspace has differences. It follows --out and --colors:
```
--- key_rotation (<wid>)
  models.Rfc7396PatchOperation{ ... }
```
How --source/--target map: local reads the file, remote reads the API, merged reads the local file if present, else the remote. [profile@] is resolved the same way as for the regular diff. Target uses that are absent from the source are ignored. A workspace the target lacks (404 from the keys API) diffs as all-source, like the regular tenant diff. diff key-rotation behaves the same and prints the --- key_rotation (<wid>) header too.

Warnings / errors

Feature off + --filter key_rotation (pull, push or diff): error --filter key_rotation requires features.key_rotation: true.
Feature off: plain pull, push and diff ignore key rotation without a warning (push key-rotation still pushes the file).
Plain diff with a [profile@] side that needs the server of another profile with the feature off: with --filter key_rotation it fails with profile <name> cannot diff key rotation: features.key_rotation is not enabled; without a filter the output carries --- key_rotation skipped: profile <name> has features.key_rotation disabled instead of the sections and exits 0.
Such a side whose profile enables the feature without manage_servers fails with profile <name>: features.key_rotation requires the manage_servers scope in client.scopes.

Live verification (a development tenant)

Run before the subcommands were folded into the plain code path; not rerun since.

--workspace <wid> pull: wrote the normal files and key_rotation.yaml (sig @monthly enabled, enc disabled).
diff --source local --target remote --colors=false: empty.
Changed the sig cron locally. diff showed an empty regular part plus --- key_rotation (<wid>) with the cron change. push --method import --dry-run printed the regular dry-run, then the --- / # key_rotation (<wid>) document. push --method import pushed both. diff was then empty, and a fresh pull wrote the new cron.
push --method import --filter key_rotation, with a local server change also pending: only key rotation was pushed (no pushed configuration log). A follow-up diff still showed the unpushed server change and no key rotation difference.
Feature off: plain pull did not write key_rotation.yaml. Plain push --method import with the file present pushed only the configuration (it also logged a skip warning, since removed). push/diff --filter key_rotation failed with the error above.
Feature on without manage_servers: pull and diff key-rotation both failed at startup with the scope error.
Tenant mode: --tenant pull pulled key rotation for all 21 workspaces (1 configured, 20 reported as not configured) with no rate limiting during the run. --tenant diff --source local --target remote --colors=false was empty. After a local cron edit it showed only that workspace's key rotation section. No tenant push was run.
The remote settings were restored to their original values afterwards.

Tests

config: features parsing, per-profile override and inheritance (independent copy), env override, scope validation (including a profile that inherits the feature but declares its own client without scopes)
cmd: scopeOf table, diff --filter key_rotation skipping the regular diff, cross-profile side error and skip marker, the subcommand scope run through runPull/runPush/runDiff with the feature off (pull, diff with header, push, dry run to a directory, to a file, and with nothing to push emptying the file), subcommand flag scoping
cac: ResolveSource
keyrotation: Pull/ReadAll/Push (fake remote), push skipping unchanged uses with starting_from always written, diff sections and skip marker, 404-tolerant diff side read, starting_from stripped on both diff sides, dry-run output (stdout / directory / appended file), local and tenant workspace discovery
go build, go vet, go test (network tests skipped), golangci-lint (only the 3 pre-existing issues), gofmt, go mod tidy -diff all clean

Introduces the cac-owned schema for workspaces/<wid>/key_rotation.yaml ({sig,enc} -> {enabled, cron, starting_from}) and the helpers the rest of the pipeline uses to carry it in a patch under the key_rotation key: Pop/Get (strict decode, unknown fields rejected), Validate (cron required for every present use and parsed with gorhill/cronexpr, the same library and version the server uses), and conversions to and from the admin AutomaticKeyRotation model. The read-only scheduled_at and the never-echoed starting_from are dropped when converting from the server. Adds utils.AsPatch for the recurring any -> patch map conversion.

Key rotation gets its own file store, mirroring workspace secrets: workspaces/<wid>/key_rotation.yaml is taken whole from the first storage directory that has it, rendered through templates and strictly decoded (unknown fields, including the read-only scheduled_at, are rejected). Writes go to the first directory. The patch-map helpers (Pop/Get) and utils.AsPatch are removed; nothing carries key rotation in a patch anymore.

Reads use=sig and use=enc, omitting a use the server reports as never configured, and replaces each present use on write. Built from the existing client and exposed as Application.KeyRotation, like the secrets store.

A workspace directory holding only key_rotation.yaml (as produced by a key-rotation pull into a tenant layout) yielded an empty map and a nil interface conversion panic on the id field.

Exclusive root flag, mutually exclusive with --workspace and --tenant and required as one of the three. Each command returns early into its own function: pull writes the file from the server, push validates and replaces each present use (--dry-run prints the YAML), diff compares the local file against the remote workspace and shows only what a push would change. --filter, --method, --source and --target are rejected in this mode. The cobra required-flag markers for --method and --source/--target are replaced by checks after the mode branch, as on the secrets branch.

The bumped client adds read-only created_at and updated_at to the system ServerDump model. Writing their zero values made the pulled server.yaml fail strict decoding into the hub TreeServer on push, and broke the read round trip. Also expect the new id_jag_ttl field.

The admin keys API rejects the system workspace client-credentials token cac uses. Switch KeyRotationAPIStore and the keyrotation model conversions to the system keys service, which sits behind the manage_servers scope, and document that scope in the README.

…merged

…ey_rotation.yaml

…commands Replace the exclusive root flag --workspace-key-rotation <wid> with a key-rotation subcommand under pull, push and diff that operates on the workspace selected with --workspace and rejects --tenant. The parent commands' own flags are now local, so the subcommands neither inherit nor accept them, and cobra's required-flag checks for --method and --source/--target no longer need hand-written equivalents. --dry-run/--out on push and --out/--colors on diff stay persistent for the subcommands. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…heck The new features section is overridable per profile like client and storage. InitApp rejects features.key_rotation without the manage_servers scope in the effective client.scopes.

…atures.key_rotation With the feature enabled, plain pull, push and diff also handle workspaces/<wid>/key_rotation.yaml, for the selected workspace or every workspace in tenant mode, unless --filter leaves key_rotation out. --filter key_rotation alone skips the regular configuration. Push validates key rotation before pushing anything; dry-run emits it as a separate YAML document or into the out directory; diff appends a per-workspace section. With the feature disabled, --filter key_rotation is rejected and push warns about skipped files.

…on both diff sides A tenant diff compares every workspace the source lists, so a target lacking one made the keys API return 404 and failed the whole diff. The plain-diff side read now maps that 404 to "no settings", so the workspace diffs as all-source like the regular tenant diff. diff key-rotation stays strict. keyRotationDiffConfigs cleared starting_from on the source only; it is now cleared on both sides, since either side can be a local file.

…ow it A [profile@] diff side used that profile's client for the keys API even when the profile had features.key_rotation off or lacked the manage_servers scope, failing the whole diff with a 403. Such a side now warns once and the key rotation section is omitted.

In workspace mode a plain push now considers [workspace] directly instead of scanning every workspace dir. tenantWorkspaces drops its unreachable default branch: the tenant export is decoded into a generic map, so servers is always a map or null.

…h and profile caveats The README stage profile enables features.key_rotation but declares its own client, which replaces the top-level one as a whole, so it now carries the manage_configuration and manage_servers scopes. A config test covers a profile that inherits the feature but declares a client without scopes. The README also notes that a tenant push PUTs for stale workspace dirs without server.yaml, and that a profile inheriting features.key_rotation: true must turn it off or carry the scope. The keyrotation package comment now mentions the plain commands.

keyRotationScope returned four values mixing validation, filter rewriting and control flow; the regular pipeline already ignores the key_rotation filter, so only pull and push keep a guard for key_rotation alone, whose empty result would rewrite server.yaml or reset settings on import.

The key-rotation subcommands and the features.key_rotation path of plain pull, push and diff now share one implementation in the keyrotation package; cmd keeps only flag handling, profile resolution and cobra wiring.

Copilot

🟡 Changes recommended

Key-rotation-only diffing and several source-side edge cases can skip data or produce incorrect results.

3 open findings

What changed in this PR

Adds feature-gated key-rotation handling to plain pull, push, and diff workflows.

Changes:

Adds features.key_rotation configuration and scope validation.
Integrates key rotation into workspace and tenant command flows.
Adds diff, dry-run, discovery, documentation, and tests.

File	Description
README.md	Documents configuration and command behavior.
internal/cac/keyrotation/service.go	Adds key-rotation orchestration.
internal/cac/keyrotation/service_test.go	Tests orchestration behavior.
internal/cac/keyrotation/keyrotation.go	Updates package documentation.
internal/cac/keyrotation/dry_run.go	Adds dry-run output support.
internal/cac/keyrotation/dry_run_test.go	Tests dry-run destinations.
internal/cac/keyrotation/dir_store.go	Adds workspace discovery.
internal/cac/keyrotation/dir_store_test.go	Tests workspace discovery.
internal/cac/keyrotation/diff.go	Adds plain-diff integration primitives.
internal/cac/keyrotation/diff_test.go	Tests diff sides and composition.
internal/cac/keyrotation/diff_internal_test.go	Tests internal diff normalization.
internal/cac/config/testdata/features.yaml	Adds feature inheritance fixture.
internal/cac/config/testdata/features_profile.yaml	Adds profile override fixture.
internal/cac/config/config.go	Defines and validates feature configuration.
internal/cac/config/config_test.go	Tests parsing, inheritance, and validation.
internal/cac/app.go	Validates configuration during startup.
cmd/push.go	Integrates key rotation into plain push.
cmd/pull.go	Integrates key rotation into plain pull.
cmd/key_rotation.go	Reuses the shared service in subcommands.
cmd/key_rotation_test.go	Retains command flag tests.
cmd/key_rotation_feature.go	Implements feature gating and command helpers.
cmd/key_rotation_feature_test.go	Tests feature-gating decisions.
cmd/diff.go	Integrates key rotation into plain diff.

🧠 Review effort: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

piotrek-janus · 2026-10-15T09:30:28Z

Deferred review items tracked in #55.

diff --filter key_rotation still read and compared the regular configuration, so it failed on unrelated template or server errors. Pull and push already skip the regular pipeline in that case; diff now does too and only builds the key rotation sides.

…isabled A cross-profile remote or merged side whose profile does not enable features.key_rotation was dropped with only a log warning, so the key rotation section silently disappeared from the diff. With --filter key_rotation the diff now fails naming the profile; without a filter it shows a '--- key_rotation skipped' line instead of the sections and still exits 0.

…side A remote or merged key rotation side of another profile that enables features.key_rotation without the manage_servers scope was dropped with a warning. It now fails with the same error InitApp reports for the selected profile, prefixed with the profile name. keyRotationRemoteAllowed is gone and the scope constant is no longer exported.

…ation push key-rotation --dry-run had its own writer that printed bare YAML and failed when --out was an existing directory. It now uses the same writer as plain push --dry-run: a '# key_rotation (<wid>)' document to stdout or a replaced --out file, or workspaces/<wid>/key_rotation.yaml under an --out directory.

Push now reads the remote settings of each workspace and only writes the uses whose enabled or cron differ, logging 'Key rotation unchanged' for the others. A use that sets starting_from is always written: the server never returns it, so cac cannot tell whether it is already applied. This covers both plain push and push key-rotation; dry-run is unaffected.

scopeOf turns features.key_rotation and --filter into a scope value (regular, key rotation, explicitly asked for) that pull, push and diff read, replacing keyRotationRequested, keyRotationOnly and the second look at --filter when a diff side has the feature disabled.

keyrotation.Diff and keyrotation.Skipped return only the key rotation lines and runDiff appends them to the configuration diff, which already ends with a newline when it is not empty. This drops AppendDiff, AppendSkipped and their newline bookkeeping.

Application.ResolveSource parses a [profile@]source-type value for both PickSource and the key rotation sides. Each side is gated first: a side reading another profile's server fails or leaves the skipped marker when that profile has the feature disabled, and is validated otherwise. Only then are the sides built, so plain errors replace the keyRotationDisabledError type used for control flow.

…ush and diff pull, push and diff key-rotation now call runPull, runPush and runDiff with a fixed scope, key rotation alone and asked for explicitly, the way --filter key_rotation does, for the selected workspace whether or not features.key_rotation is enabled. diff key-rotation compares local against remote of the selected profile. This removes pullKeyRotation, pushKeyRotation, diffKeyRotation and Service.Diff. diff key-rotation now prints the "--- key_rotation (<wid>)" header, and a workspace missing remotely diffs as all-source instead of failing. Application.KeyRotation is typed as keyrotation.Remote.

Pull, ReadAll and Push take the server and the key rotation files they need instead of hanging off a Service built for every run. ReadAll validates in place of the exported ReadLocal, and Side.Remote is typed as Remote, dropping the Reader interface.

With the feature disabled, plain push warned about every key_rotation.yaml it skipped. The file is not ignored, though: push key-rotation pushes it whether or not the feature is enabled, so the warning was misleading. This also removes DirStore.Has, which only the warning used.

A dry run now empties a flat --out file at the start of the run, and the key rotation documents are appended after the regular output. A single WriteDryRun builds the key rotation stream and writes it once, replacing DryRun with its AppendFile flag that the first write flipped. A key rotation only dry run with nothing to push used to leave a stale --out file from an earlier run; it now leaves an empty one.

It had a single caller left, and the comments no longer describe key rotation as a plain-command-only feature.

State the manage_servers requirement, the missing-use rule and the dry-run format once, describe each key-rotation subcommand as the plain command with --filter key_rotation, and note that a dry run empties the --out file first.

Side.Read mapped a keys-API 404 to "no settings" on both sides, so a source server without the workspace (for example a mistyped --workspace with --filter key_rotation or diff key-rotation) gave an empty diff. Only the target read in Diff tolerates a missing workspace now; the source read fails.

piotrek-janus · 2026-10-16T11:40:49Z

Live e2e matrix for this branch (commit 941e072): 37 pull/push/diff runs across subcommands, plain commands with the feature on and off, and tenant mode, all passing, with the captured output of every case and four behaviour notes.

https://claude.ai/artifact/VsuVRQwV1QrJEoTJSq4Qhe

piotrek-janus and others added 21 commits September 24, 2026 13:38

feat(client): add KeyRotationAPIStore for the admin keys endpoint

cf44760

Reads use=sig and use=enc, omitting a use the server reports as never configured, and replaces each present use on write. Built from the existing client and exposed as Application.KeyRotation, like the secrets store.

fix(storage): skip workspace dirs without server.yaml in tenant read

53779ce

A workspace directory holding only key_rotation.yaml (as produced by a key-rotation pull into a tenant layout) yielded an empty map and a nil interface conversion panic on the id field.

docs: document managing key rotation

38fb7df

chore: bump acp-client-go to 128244c for the system keys service

6e7817f

chore: re-pin acp-client-go to master now that ciam-client-go#76 has …

af62202

…merged

fix: honor --out on key rotation dry-run and reject id/tenant_id in k…

a5061a8

…ey_rotation.yaml

feat(config): add features.key_rotation with a manage_servers scope c…

a610461

…heck The new features section is overridable per profile like client and storage. InitApp rejects features.key_rotation without the manage_servers scope in the effective client.scopes.

docs: document features.key_rotation

8b89ae6

piotrek-janus requested a balanced review from Copilot October 14, 2026 15:36

Copilot started reviewing on behalf of piotrek-janus October 14, 2026 15:37

View session

Copilot AI reviewed Oct 14, 2026

View reviewed changes

Comment thread internal/cac/keyrotation/diff.go Resolved

Comment thread cmd/diff.go Resolved

Comment thread cmd/key_rotation_feature.go Resolved

jdabrowski approved these changes Oct 14, 2026

View reviewed changes

Base automatically changed from add-key-rotation-v2 to master October 14, 2026 15:43

chore: merge master after the key-rotation subcommands landed (#53)

9e3cc18

piotrek-janus mentioned this pull request Oct 15, 2026

Key rotation follow-ups from the PR #54 review #55

Open

4 of 11 tasks

piotrek-janus added 16 commits October 15, 2026 11:34

docs: explain what the manage_servers scope grants

795fb4e

refactor(cmd): fold keyRotationWorkspaces into readKeyRotations

fd1d3fd

It had a single caller left, and the comments no longer describe key rotation as a plain-command-only feature.

style: gofmt PickSource

da25a01

piotrek-janus marked this pull request as ready for review October 16, 2026 08:46

ikawalec approved these changes Oct 16, 2026

View reviewed changes

piotrek-janus merged commit 949872f into master Oct 16, 2026
10 checks passed

piotrek-janus deleted the key-rotation-feature-flag branch October 16, 2026 11:41

Navigation Menu

Repository navigation

feat: handle key rotation in plain pull, push and diff behind features.key_rotation - #54

feat: handle key rotation in plain pull, push and diff behind features.key_rotation#54
piotrek-janus merged 39 commits into
masterfrom
key-rotation-feature-flag

piotrek-janus commented Oct 13, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

piotrek-janus commented Oct 15, 2026

Uh oh!

piotrek-janus commented Oct 16, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

piotrek-janus commented Oct 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Config

Gating rules

Output shapes

Warnings / errors

Live verification (a development tenant)

Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

piotrek-janus commented Oct 15, 2026

Uh oh!

piotrek-janus commented Oct 16, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

piotrek-janus commented Oct 13, 2026 •

edited

Loading