cp-rotation-skip-feat: Optional rotation and bring-your-own-password for credential-provision

[tbjones-ks]

Summary

Extends credential-provision (alias cp) to support onboarding existing PAM accounts whose passwords must not be rotated at provisioning time. Three new YAML capabilities plus an absorbed bug fix.

Customer driver: Tandem.

Tracking

No JIRA ticket — internal tracking via cp-rotation-skip-feat. Full design docs in .claude-context/Commander/development/cp-rotation-skip-feat/.

Changes

YAML schema (all additions are opt-in; existing YAMLs unchanged)

Field	Section	Type	Purpose
(existing) rotation:	top-level	object	Now optional (was required). Omit to indicate Commander will not manage rotation for the new record.
rotation.on_demand	inside rotation:	bool	true configures rotation for manual triggering only (no cron). Mutually exclusive with rotation.schedule.
rotation.rotate_on_provision	inside rotation:	bool (default true)	false defers the provisioning-time immediate rotation. Default preserves today's behavior.
account.existing_password	inside account:	non-empty string	Onboard an existing account using its current real password. Never pushed to the target.

Behavior matrix (8 supported cells)

See ARCHITECTURE_AND_DESIGN.md § Behavior Matrix. The validator rejects the single combination that would push a customer-supplied password to a target (INVARIANT-001).

Code touch points

• credential_provision.py:
  • New module-level DEFAULT_COMPLEXITY = "32,5,5,5,5" constant
  • New _determine_password, _should_rotate_on_provision, _should_create_ad_user, _log_existing_password_use helpers
  • _validate_config — rotation: removed from required_sections; INVARIANT-001 cross-section check added
  • _validate_rotation_section — schedule/on_demand mutex + on_demand + rotate_on_provision type checks
  • _validate_account_section — accepts existing_password; keeps initial_password rejection
  • _configure_rotation — branches on on_demand vs schedule
  • _dry_run_report — full rewrite; absorbs commit 97dc8893 (orphan credential-provision-fixes branch) which fixed the JSON-mode NameError at line 1027
  • execute() — gate _create_ad_user_via_gateway on not existing_password; gate _rotate_immediately on rotate_on_provision; emit one logging.info line for existing_password use
  • Argparse description text updated
• Three new test files: test_credential_provision_validation.py, test_credential_provision_execute.py, test_credential_provision_dryrun.py — 71 new tests

Breaking Changes

None. Schema relaxation is strict (rotation: becomes optional; new fields are opt-in). Every previously-valid YAML continues to validate and execute identically.

Migration Guide

Not required. The deprecated pam.rotation: migration error (introduced in a prior commit) is unchanged.

Rollback Instructions

Pure code revert (git revert <merge-commit>). No data migrations, no DB changes, no infrastructure changes. Users with YAMLs that opt into the new fields would see them become invalid validator inputs again — pre-feature behavior.

Testing Performed

• ✅ 71 new unit tests (Phase 4 TDD + signature guard)
• ✅ 36 pre-existing test_credential_provision_kc1035.py tests pass (regression)
• ✅ cp --output json --dry-run smoke-tested for all 8 behavior-matrix cells (closes the absorbed NameError fix)
• ✅ Structural regression guards (source-code grep for log-leak patterns; signature constraint on _log_existing_password_use)
• ✅ All 4 demo YAMLs validate (2 new + 2 existing — modulo offline limitations on directory_uid resolution)
• ⏸️ 3 pre-existing e2e tests skipped (require live Keeper session + Gateway infrastructure)

Security Notes

This PR introduces a narrow carve-out from BLOCKER_KC-1007-2_Password-Generation-Flow_2025-11-17. The previous decision (reject account.initial_password because it would let YAML dictate the password Commander pushes to a target) is preserved unchanged. The new account.existing_password field has deliberately distinct semantics: it represents the password that already exists on the account, and is never pushed to any target.

Three enforcement layers:

1. Validator INVARIANT-001 rejects YAML combinations that would fire an immediate rotation with existing_password set.
2. Helper _should_create_ad_user skips _create_ad_user_via_gateway (the other push-to-target route) when existing_password is set.
3. Source-code structural guard test rejects any future log statement that interpolates the existing_password value.

See TECHNICAL_DECISIONS.md ADR-001 and SECURITY_ARCHITECTURE.md for the full rationale + threat model.

Reviewer Guidance

Focus on:

1. Security review — confirm INVARIANT-001 is reachable from every combination in the behavior matrix that ought to be rejected. Test file: tests/test_credential_provision_validation.py § TestInvariant001.
2. Backwards compatibility — confirm the schema relaxation (removing 'rotation' from required_sections) doesn't accidentally accept malformed YAMLs. Test file: TestEmptyRotationDict, TestRequiredSectionsRegression.
3. Dry-run output format — the text-mode dry-run format changed (uses an actions array instead of hardcoded numbered prints). Output now includes (config: <name>) for the email step. Mention this to anyone tooling against the text format.
4. The absorbed bug fix — commit 97dc8893 from the orphan credential-provision-fixes branch is folded into Story 4 here. The customer's reported cp --output json --dry-run NameError is closed.

Checklist

• Code follows project conventions
• Tests added/updated
• Documentation updated (inline + design docs)
• No security vulnerabilities (KC-1007-2 invariant preserved; new field gated by INVARIANT-001)
• Performance acceptable (no perf-sensitive code touched)
• Backwards compatibility verified

[tbjones-ks]

E2E verification complete

Beyond the 164 unit tests and CI green, this feature was verified against the live kc1035.test AD container.

Real-provisioning suite (12 cells, all ✅)

Cells	Path	Result
1, 2	No rotation block	rotation_status:"not_configured", generated or BYO password stored, no AD touch
3, 5	Schedule/on-demand + default rotate_on_provision	AD account created and rotated (AD pwdLastSet updated +14-16s after whenCreated)
4, 6	Schedule/on-demand + rotate_on_provision:false	AD account created, no rotation fired (pwdLastSet ≈ whenCreated)
7, 8	BYO password + AD context	AD-create skipped (gate verified — no AD writes), correct rotation_status
R1-R4	Validator rejections (INVARIANT-001, ROT-001, KC-1007-2 carry-over)	All reject with verbatim remediation prose

Full Tandem journey (cells 7 & 8 against pre-existing AD accounts)

1. Pre-created svc-cprot-cell{7,8}-... in AD with the BYO password. Recorded baseline pwdLastSet.
2. Ran cp against them → both returned success.
3. AD query confirmed pwdLastSet and whenChanged were byte-identical to baseline — Commander did not touch AD during onboarding.
4. Triggered pam action rotate <cell8_uid> (the CLI equivalent of "Rotate Now"). Commander generated a fresh password, gateway pushed it to AD via admin creds, vault record updated.
5. End-to-end Tandem story confirmed: onboard → AD untouched → operator rotates → AD now holds Commander's password.

Dry-run / output=json regression

10 dry-run JSON cells + 2 dry-run text cells exercised. The originally-reported cp --output json --dry-run NameError: name 'pam' is not defined is closed across all behavior-matrix cells. The new rotation_mode field and 4-value rotation_status enum (not_configured/scheduled/on_demand/synced) emit correctly.

Security gate proven at three layers

1. Validator INVARIANT-001 rejects push-to-target combinations.
2. _should_create_ad_user predicate gates _create_ad_user_via_gateway when existing_password is set.
3. Live AD state directly observed: pre-existing accounts' pwdLastSet unchanged after onboarding (no writes occurred).

Mid-run we found and fixed one bug (success-path JSON's rotation_status was still using the pre-feature 2-value enum); regression tests added; CI re-green. That fix is in commit 44a0db94.

[tbjones-ks]

@claude please re-review the changes in c0ca9387. This commit addresses all three findings from your previous review:

1. Finding Make loading of a record from data into a utility function #1 (transfer_ownership guard missed on_demand): has_rotation predicate now covers both modes. Regression tests added in TestRotationDeliveryInvariant.
2. Finding KC-13 Clean up #2 (state.ad_gateway_uid was None for BYO + ad_groups): group-add call now uses the local gateway_uid. A structural source-grep regression-guard test (test_add_ad_user_to_groups_uses_local_gateway_uid) forbids future regressions of the same shape.
3. Finding Streamlining the record object #3 (dry-run actions list diverged from execute()): action list builder rewritten to mirror execute()'s call sequence — emits group-add and direct-share actions, gates share-URL on has_email. New test class TestDryRunActionsMirrorExecute covers 6 path combinations.

Net: 173 tests passing (+10 new), 3 skipped, CI green. Please verify the fixes are sound and check for any other gaps.

[tbjones-ks]

@claude please re-review the changes in c663a127. This commit addresses both new findings from your round-2 review:

1. package setup and deployment #7 (offline-gateway + existing_password + ad_groups silent failure): Added a None-precondition inside _add_ad_user_to_groups_via_gateway that raises CommandError if gateway_uid is falsy — restores the pre-PR fail-fast for every caller, not just this site. Followed your Option B.
2. Revert "package setup and deployment" #8 (rollback orphans AD group memberships): Reordered — _add_ad_user_to_groups_via_gateway now runs AFTER _create_pam_user + _create_dag_link + _configure_rotation. Followed your Option B. Only graceful (non-rollback) failures remain after group-add, so any failure during the critical steps cleanly leaves AD untouched.

Note on round-2 comments #1 and #3 — those were duplicates of round-1 findings already fixed in c0ca9387. The current code at the cited lines (813 for has_rotation predicate, 1148-area for the dry-run actions list) has the correct logic; please confirm.

Net: 175 tests passing (+2 new structural regression guards). Please verify the fixes and check for any further cascading gaps.

[tbjones-ks]

@claude please re-review the changes in a571a9c3. This commit addresses all three new findings from your round-3 review:

1. A — _rotate_immediately failure hint wrong in on_demand mode: the failure branch now emits a manual-trigger hint (pam action rotate/Rotate Now) when config['rotation'].get('on_demand'), and the scheduled-rotation hint only fires in schedule mode. Regression test test_rotate_immediately_failure_warning_branches_on_mode covers both branches.
2. B — dry-run actions list group-add ordering: moved the Add to AD groups append in _dry_run_report to AFTER the rotation block, mirroring the c663a12 reorder of execute(). Regression test test_group_add_action_appears_after_create_pam_user_action enforces the new invariant.
3. C — rotation: null crash: normalized at the top of _validate_config — a bare rotation: (None) now returns a clean validator error with remediation guidance before any downstream .get() access. New test_null_rotation_value_rejected_with_helpful_error covers it (alongside the existing test for the {} case).

Note: two of your round-3 comments duplicated round-1 findings #1 and #3. I verified both are still fixed in the current code — has_rotation predicate at line 819-820 covers on_demand, and _dry_run_report includes group-add / direct-share / email-gated share-URL. The bot may be running against stale diff state for those.

Net: 178 tests passing (+3 new), 3 skipped, CI re-running. Please look for any further cascading gaps.

[claude]

⚠️ Code review skipped — your organization's overage spend limit has been reached.

Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.

Once credits are available, reopen this pull request to trigger a review.