RPF-WP-0069-T07: approve sets approved only once every required role approves.
credential-change.py approve set status approved after the first reviewer; for role-gated request types validation then refused the second approve before it was recorded. append_decision now records each approval and keeps the prior status while the lane validator still reports a missing role approval. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Assistant: claude-code Assistant-Model: opus Assistant-Process: 509845@bnt-lap001 Assistant-Session: ed7a7d95-d8eb-455c-b594-963fb1830f76
This commit is contained in:
parent
371f787493
commit
aeb21434bd
5 changed files with 108 additions and 5 deletions
|
|
@ -240,7 +240,7 @@
|
|||
| task | RPF-WP-0069-T04 | done | — | workplans/RPF-WP-0069-bounded-executor-lanes.md |
|
||||
| task | RPF-WP-0069-T05 | wait | — | workplans/RPF-WP-0069-bounded-executor-lanes.md |
|
||||
| task | RPF-WP-0069-T06 | cancel | — | workplans/RPF-WP-0069-bounded-executor-lanes.md |
|
||||
| task | RPF-WP-0069-T07 | todo | — | workplans/RPF-WP-0069-bounded-executor-lanes.md |
|
||||
| task | RPF-WP-0069-T07 | done | — | workplans/RPF-WP-0069-bounded-executor-lanes.md |
|
||||
| task | RPF-WP-0069-T08 | done | — | workplans/RPF-WP-0069-bounded-executor-lanes.md |
|
||||
| task | RPF-WP-0069-T09 | wait | — | workplans/RPF-WP-0069-bounded-executor-lanes.md |
|
||||
| task | RPF-WP-0070-T01 | done | — | workplans/RPF-WP-0070-mas-evidence-recovery.md |
|
||||
|
|
|
|||
|
|
@ -191,6 +191,14 @@ renders the reviewed non-secret `bao policy write` and `bao write auth/.../role`
|
|||
commands for a platform operator; the actual secret value is still provisioned
|
||||
through approved OpenBao/operator custody only.
|
||||
|
||||
For request types that require an explicit approval from every role in
|
||||
`review.required_approvers` (for example `workload-kv-write-next-version`,
|
||||
`attended-service-credential-delivery`, `attended-host-key-delivery`), each
|
||||
`approve` records that reviewer's approval. The CCR moves to `approved` only when
|
||||
the last required role approves; until then it keeps its prior status and the
|
||||
tool prints the roles still awaited. Run `approve` once per role with
|
||||
`--reviewer <role>`.
|
||||
|
||||
The same operations can be exposed through chat by having the agent create the
|
||||
proposal, show the rendered summary, then call the CLI only after the human
|
||||
gives an explicit approval phrase.
|
||||
|
|
|
|||
|
|
@ -2,6 +2,7 @@
|
|||
from __future__ import annotations
|
||||
|
||||
import argparse
|
||||
import copy
|
||||
import difflib
|
||||
import json
|
||||
import os
|
||||
|
|
@ -480,9 +481,15 @@ def validate_kv_metadata(value: Any, errors: list[str]) -> None:
|
|||
|
||||
def validate_ccr(path: Path) -> tuple[dict[str, Any], list[str], list[str]]:
|
||||
errors: list[str] = []
|
||||
warnings: list[str] = []
|
||||
scan_for_secrets(path, errors)
|
||||
ccr = load_yaml(path)
|
||||
data_errors, warnings = validate_ccr_data(ccr)
|
||||
return ccr, errors + data_errors, warnings
|
||||
|
||||
|
||||
def validate_ccr_data(ccr: dict[str, Any]) -> tuple[list[str], list[str]]:
|
||||
errors: list[str] = []
|
||||
warnings: list[str] = []
|
||||
for field in (
|
||||
"id",
|
||||
"kind",
|
||||
|
|
@ -526,7 +533,7 @@ def validate_ccr(path: Path) -> tuple[dict[str, Any], list[str], list[str]]:
|
|||
errors.extend(lane_module(ccr).validate(ccr, REPO_DIR))
|
||||
else:
|
||||
errors.append("unsupported request_type")
|
||||
return ccr, errors, warnings
|
||||
return errors, warnings
|
||||
|
||||
|
||||
def render_summary(ccr: dict[str, Any], warnings: list[str]) -> str:
|
||||
|
|
@ -1857,6 +1864,21 @@ def render_status(payload: dict[str, Any]) -> str:
|
|||
return "\n".join(lines)
|
||||
|
||||
|
||||
ROLE_APPROVAL_MISSING = "explicit role approval missing: "
|
||||
|
||||
|
||||
def pending_role_approvals(ccr: dict[str, Any]) -> list[str]:
|
||||
"""Roles the request type still requires before the CCR may be `approved`.
|
||||
|
||||
Only lane validators that enforce role approvals report these; for other
|
||||
request types the list is always empty.
|
||||
"""
|
||||
candidate = copy.deepcopy(ccr)
|
||||
candidate["status"] = "approved"
|
||||
errors, _warnings = validate_ccr_data(candidate)
|
||||
return [e[len(ROLE_APPROVAL_MISSING):] for e in errors if e.startswith(ROLE_APPROVAL_MISSING)]
|
||||
|
||||
|
||||
def append_decision(path: Path, status: str, reviewer: str, comment: str) -> dict[str, Any]:
|
||||
reject_secret_text(comment, "review comment")
|
||||
ccr, _warnings = validate_or_exit(path)
|
||||
|
|
@ -1872,6 +1894,10 @@ def append_decision(path: Path, status: str, reviewer: str, comment: str) -> dic
|
|||
"comment": comment,
|
||||
}
|
||||
)
|
||||
# An approval is recorded per reviewer, but the CCR only becomes `approved`
|
||||
# once every role its request type requires has approved (RPF-WP-0069-T07).
|
||||
if status == "approved" and pending_role_approvals(ccr):
|
||||
status = ccr["status"]
|
||||
ccr["status"] = status
|
||||
ccr["updated"] = datetime.now(timezone.utc).date().isoformat()
|
||||
dump_yaml(path, ccr)
|
||||
|
|
@ -2261,7 +2287,12 @@ def command_record_evidence(args: argparse.Namespace) -> int:
|
|||
def command_decision(args: argparse.Namespace, status: str) -> int:
|
||||
path = resolve_ccr(args.ref)
|
||||
ccr = append_decision(path, status, args.reviewer, args.comment)
|
||||
print(f"[OK] {path.name} -> {status}")
|
||||
print(f"[OK] {path.name} -> {ccr['status']}")
|
||||
if status == "approved" and ccr["status"] != "approved":
|
||||
print(
|
||||
f"[INFO] {args.reviewer} approval recorded; still awaiting: "
|
||||
+ ", ".join(pending_role_approvals(ccr))
|
||||
)
|
||||
if args.record_state_hub:
|
||||
openbao = ccr["openbao"]
|
||||
auth = openbao["auth"]
|
||||
|
|
|
|||
|
|
@ -644,6 +644,61 @@ class CredentialChangeTests(unittest.TestCase):
|
|||
type("Args", (), {"ref": str(self.unapproved_ccr())})()
|
||||
)
|
||||
|
||||
def test_role_gated_approval_stays_open_until_every_required_role_approves(self) -> None:
|
||||
# RPF-WP-0069-T07: the first approval used to set `approved`, after which
|
||||
# validation refused the second reviewer's approve before it was recorded.
|
||||
source = REPO_DIR / (
|
||||
"credential-change-requests/"
|
||||
"CCR-2026-0058-net-kingdom-bind-executor-svc-authelia-write.yaml"
|
||||
)
|
||||
tmp = Path(tempfile.mkdtemp())
|
||||
self.addCleanup(shutil.rmtree, tmp, True)
|
||||
copied = tmp / source.name
|
||||
shutil.copy2(source, copied)
|
||||
data = credential_change.load_yaml(copied)
|
||||
self.assertEqual(
|
||||
data["review"]["required_approvers"], ["platform-owner", "net-kingdom-owner"]
|
||||
)
|
||||
data["status"] = "proposed"
|
||||
data["review"]["comments"] = []
|
||||
credential_change.dump_yaml(copied, data)
|
||||
|
||||
ccr = credential_change.append_decision(copied, "approved", "platform-owner", "ok")
|
||||
self.assertEqual(ccr["status"], "proposed")
|
||||
self.assertEqual(credential_change.pending_role_approvals(ccr), ["net-kingdom-owner"])
|
||||
_ccr, errors, _warnings = credential_change.validate_ccr(copied)
|
||||
self.assertEqual(errors, [])
|
||||
with self.assertRaises(SystemExit):
|
||||
credential_change.require_apply_ready(ccr, "apply-plan")
|
||||
|
||||
ccr = credential_change.append_decision(copied, "approved", "net-kingdom-owner", "ok")
|
||||
self.assertEqual(ccr["status"], "approved")
|
||||
self.assertEqual(credential_change.pending_role_approvals(ccr), [])
|
||||
self.assertEqual(
|
||||
[c["reviewer"] for c in ccr["review"]["comments"]],
|
||||
["platform-owner", "net-kingdom-owner"],
|
||||
)
|
||||
_ccr, errors, _warnings = credential_change.validate_ccr(copied)
|
||||
self.assertEqual(errors, [])
|
||||
|
||||
def test_approval_by_unlisted_reviewer_does_not_complete_role_gate(self) -> None:
|
||||
source = REPO_DIR / (
|
||||
"credential-change-requests/"
|
||||
"CCR-2026-0058-net-kingdom-bind-executor-svc-authelia-write.yaml"
|
||||
)
|
||||
tmp = Path(tempfile.mkdtemp())
|
||||
self.addCleanup(shutil.rmtree, tmp, True)
|
||||
copied = tmp / source.name
|
||||
shutil.copy2(source, copied)
|
||||
data = credential_change.load_yaml(copied)
|
||||
data["status"] = "proposed"
|
||||
data["review"]["comments"] = []
|
||||
credential_change.dump_yaml(copied, data)
|
||||
credential_change.append_decision(copied, "approved", "unit-test", "ok")
|
||||
ccr = credential_change.append_decision(copied, "approved", "platform-owner", "ok")
|
||||
self.assertEqual(ccr["status"], "proposed")
|
||||
self.assertEqual(credential_change.pending_role_approvals(ccr), ["net-kingdom-owner"])
|
||||
|
||||
def test_approve_records_comment_but_unconfirmed_claim_still_blocks_apply(self) -> None:
|
||||
copied = self.unapproved_ccr()
|
||||
ccr_dir = copied.parent
|
||||
|
|
|
|||
|
|
@ -147,7 +147,7 @@ rotates its own password (hub decision 3d0c8f86), so the executor needs no LLDAP
|
|||
|
||||
```task
|
||||
id: RPF-WP-0069-T07
|
||||
status: todo
|
||||
status: done
|
||||
priority: medium
|
||||
state_hub_task_id: "b458e369-73b4-515e-ae01-e8afc0dd35eb"
|
||||
```
|
||||
|
|
@ -158,6 +158,15 @@ net-kingdom-owner approval was recorded by hand in the tool's format). Make vali
|
|||
that is still collecting role approvals, or have `approve` set `approved` only once all required roles are present,
|
||||
with a test.
|
||||
|
||||
Done 2026-11-03 (second option): `append_decision` records every approval, but sets `approved` only when the
|
||||
request type's validator reports no `explicit role approval missing` error. Until then the CCR keeps its prior
|
||||
status and `approve` prints the roles still awaited. The State Hub decision sync uses the same path. Lanes without
|
||||
role gates are unchanged. `validate_ccr_data` validates in memory. Tests:
|
||||
`test_role_gated_approval_stays_open_until_every_required_role_approves` and
|
||||
`test_approval_by_unlisted_reviewer_does_not_complete_role_gate` (CCR-2026-0058 copy). The guide in
|
||||
`docs/credential-change-approval.md` is updated. Three unrelated `test_credential_change.py` failures predate this
|
||||
change (CCR drift).
|
||||
|
||||
|
||||
## Broker admits oidc-service-client@0.2 for mint-secret
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue