harden(agent-team): fold C1 cross-review MEDIUMs into p3_rollback.sh
GPT-4.1 cross-family review (APPROVE, no critical/high) raised two MEDIUMs on the rollback tooling; addressed both: - require_keys: each restore_* asserts its required baseline keys up front and refuses a PARTIAL (silently-weaker) restore. environment accepts ids OR logins (equivalent); a missing protection.full now REFUSES the enforce_admins-only degrade unless P3_ROLLBACK_ALLOW_PARTIAL=1 is set (loud DEGRADED warning). - out-of-band ACK: an --apply that needs a MANUAL App neutralise (no APP JWT, or action=out-of-band) refuses unless P3_ROLLBACK_OOB_ACK=1 — so the App is never left un-neutralised without a conscious operator sign-off; with the ack the other surfaces still restore. Documented both env vars in usage. +3 tests (required-key refuse, partial-protection ack, oob ack). Suite: 1362 passed, ruff clean.
This commit is contained in:
parent
cb84629c2b
commit
c4bea7270b
2 changed files with 131 additions and 9 deletions
|
|
@ -192,6 +192,16 @@ Options:
|
|||
--merged flip already merged (post-merge revert path)
|
||||
--flip-commit SHA merged flip commit to revert (post-merge path)
|
||||
-h, --help show this help
|
||||
|
||||
Operator ACK env vars (fail-closed by default; a partial/manual step must be
|
||||
acknowledged so a restore is never silently weaker than the baseline):
|
||||
AGENT_APPLY_APP_JWT=<jwt> App JWT for the automated App uninstall.
|
||||
P3_ROLLBACK_OOB_ACK=1 acknowledge you will neutralise the App by hand
|
||||
when no APP JWT is available (lets the other
|
||||
surfaces restore instead of aborting).
|
||||
P3_ROLLBACK_ALLOW_PARTIAL=1 accept a DEGRADED branch-protection restore
|
||||
(enforce_admins only) when the baseline has no
|
||||
protection.full — otherwise this is refused.
|
||||
USAGE
|
||||
}
|
||||
|
||||
|
|
@ -302,6 +312,27 @@ require_baseline() {
|
|||
fi
|
||||
}
|
||||
|
||||
# require_keys KEY... — fail HARD if any baseline key is absent (MEDIUM-1). A
|
||||
# partial baseline must never yield a partial/weaker restore (e.g. restoring only
|
||||
# enforce_admins while silently dropping status checks): each restore_* asserts
|
||||
# the keys IT needs up front, BEFORE any mutation, so a missing key aborts the
|
||||
# whole surface instead of half-restoring it.
|
||||
require_keys() {
|
||||
local missing="" k
|
||||
for k in "$@"; do
|
||||
if [ -z "$(jget "${k}")" ]; then
|
||||
missing="${missing} ${k}"
|
||||
fi
|
||||
done
|
||||
if [ -n "${missing}" ]; then
|
||||
err "baseline ${BASELINE} is missing required key(s):${missing}"
|
||||
err " refusing a PARTIAL restore — record a complete baseline first (--record-baseline)."
|
||||
return 1
|
||||
fi
|
||||
}
|
||||
|
||||
warn() { printf ' \033[1;35m[WARN]\033[0m %s\n' "$*" >&2; }
|
||||
|
||||
# run_or_plan "<human description>" gh ... — print the plan; only execute on --apply.
|
||||
run_or_plan() {
|
||||
local desc="$1"; shift
|
||||
|
|
@ -326,6 +357,7 @@ assert_equal() {
|
|||
# ── surface 1: workflow flip ─────────────────────────────────────────────────
|
||||
restore_workflow() {
|
||||
say "1. Restore the apply/verify workflow flip"
|
||||
require_keys workflow_path workflow_baseline_sha || return 1
|
||||
local wf_path baseline_sha
|
||||
wf_path="$(jget workflow_path)"
|
||||
[ -n "${wf_path}" ] || wf_path=".github/workflows/agent-team-apply-verify.yml"
|
||||
|
|
@ -404,6 +436,16 @@ reviewers_put_args() {
|
|||
# ── surface 2: agent-apply environment ───────────────────────────────────────
|
||||
restore_environment() {
|
||||
say "2. Restore the agent-apply environment (required reviewer ids + policy)"
|
||||
require_keys environment.name || return 1
|
||||
# The reviewer can be recorded as numeric ids OR logins (the login->id resolve
|
||||
# is an EQUIVALENT, not weaker, restore) — but at least one source is required,
|
||||
# else there is nothing to restore the required reviewer FROM (MEDIUM-1).
|
||||
if [ -z "$(jget environment.required_reviewer_ids)" ] \
|
||||
&& [ -z "$(jget environment.required_reviewers)" ]; then
|
||||
err "baseline has neither environment.required_reviewer_ids nor environment.required_reviewers"
|
||||
err " — nothing to restore the required reviewer from; record a complete baseline."
|
||||
return 1
|
||||
fi
|
||||
local env_name policy raw line id ids
|
||||
env_name="$(jget environment.name)"
|
||||
[ -n "${env_name}" ] || env_name="agent-apply"
|
||||
|
|
@ -490,8 +532,29 @@ EOF
|
|||
# NOT an installation token, NOT a fine-grained PAT). Provide the JWT via
|
||||
# $AGENT_APPLY_APP_JWT; this surface only --apply's the uninstall when it is
|
||||
# set. Without it, the step degrades to an out-of-band instruction.
|
||||
# Out-of-band ACK gate (MEDIUM-2): when neutralising the App needs a MANUAL
|
||||
# operator action (no APP JWT for the uninstall, or action=out-of-band), --apply
|
||||
# must not silently skip it. Require an explicit acknowledgement
|
||||
# (P3_ROLLBACK_OOB_ACK=1) that the operator WILL perform the manual step, so the
|
||||
# App is never left un-neutralised without a conscious sign-off. WITH the ack the
|
||||
# surface returns 0 so the other surfaces still restore (the App step is operator-
|
||||
# owed, loudly warned); WITHOUT it, --apply aborts this surface.
|
||||
require_oob_ack() {
|
||||
local what="$1"
|
||||
if [ "${P3_ROLLBACK_OOB_ACK:-}" = "1" ]; then
|
||||
warn "OUT-OF-BAND ACK accepted (P3_ROLLBACK_OOB_ACK=1): ${what}"
|
||||
warn " the App is NOT neutralised by this script — you MUST do it by hand NOW."
|
||||
return 0
|
||||
fi
|
||||
err "${what}"
|
||||
err " set P3_ROLLBACK_OOB_ACK=1 to acknowledge you will perform the manual step"
|
||||
err " (lets the other surfaces restore), or set \$AGENT_APPLY_APP_JWT for the automated uninstall."
|
||||
return 1
|
||||
}
|
||||
|
||||
restore_app() {
|
||||
say "3. Neutralise the GitHub App installation"
|
||||
require_keys app.action || return 1
|
||||
local action installation_id slug
|
||||
action="$(jget app.action)"
|
||||
[ -n "${action}" ] || action="uninstall"
|
||||
|
|
@ -520,8 +583,8 @@ restore_app() {
|
|||
plan " remove the installation in the org UI, or run:"
|
||||
plan " gh api -X DELETE app/installations/${installation_id} -H 'Authorization: Bearer <APP_JWT>'"
|
||||
if [ "${APPLY}" = "1" ]; then
|
||||
err "uninstall needs an APP JWT (\$AGENT_APPLY_APP_JWT) — operator gh cannot do this; do it out-of-band"
|
||||
return 1
|
||||
require_oob_ack \
|
||||
"uninstall needs an APP JWT (\$AGENT_APPLY_APP_JWT); operator gh cannot do this" || return 1
|
||||
fi
|
||||
fi
|
||||
;;
|
||||
|
|
@ -532,8 +595,8 @@ restore_app() {
|
|||
plan " - rotate the App private key out-of-band (App settings > Generate a new private key,"
|
||||
plan " update the AGENT_APPLY_APP_PRIVATE_KEY Actions secret), which invalidates new-token minting."
|
||||
if [ "${APPLY}" = "1" ]; then
|
||||
err "app.action 'out-of-band' has no API path — perform the steps above by hand, then re-run --apply with a recorded uninstall"
|
||||
return 1
|
||||
require_oob_ack \
|
||||
"app.action 'out-of-band' has no API path — neutralise the App by hand" || return 1
|
||||
fi
|
||||
;;
|
||||
*)
|
||||
|
|
@ -544,6 +607,21 @@ restore_app() {
|
|||
# ── surface 4: branch protection ─────────────────────────────────────────────
|
||||
restore_protection() {
|
||||
say "4. Restore the FULL branch protection baseline (include_administrators ON)"
|
||||
require_keys protection.branch protection.include_administrators || return 1
|
||||
# protection.full is the COMPLETE payload (status checks, PR reviews, ...).
|
||||
# Restoring only enforce_admins when it is absent leaves a WEAKER posture than
|
||||
# baseline (MEDIUM-1). Refuse that silent degrade by default; the operator must
|
||||
# explicitly accept the enforce_admins-only restore via P3_ROLLBACK_ALLOW_PARTIAL=1.
|
||||
if [ -z "$(jget protection.full)" ]; then
|
||||
if [ "${P3_ROLLBACK_ALLOW_PARTIAL:-}" != "1" ]; then
|
||||
err "baseline has no protection.full — an enforce_admins-only restore would leave a WEAKER"
|
||||
err " posture (status checks / PR reviews NOT restored). Set P3_ROLLBACK_ALLOW_PARTIAL=1 to"
|
||||
err " accept the degraded restore, or record a complete baseline (--record-baseline)."
|
||||
return 1
|
||||
fi
|
||||
warn "DEGRADED protection restore (P3_ROLLBACK_ALLOW_PARTIAL=1): enforce_admins ONLY;"
|
||||
warn " status checks / PR reviews are NOT restored — restore them by hand or re-record."
|
||||
fi
|
||||
local branch include_admins full
|
||||
branch="$(jget protection.branch)"
|
||||
[ -n "${branch}" ] || branch="$(jget default_branch)"
|
||||
|
|
|
|||
|
|
@ -165,11 +165,14 @@ def _run(
|
|||
*args: str,
|
||||
baseline: Path,
|
||||
shim: tuple[Path, Path] | None = None,
|
||||
extra_env: dict[str, str] | None = None,
|
||||
) -> subprocess.CompletedProcess[str]:
|
||||
env = dict(os.environ)
|
||||
if shim is not None:
|
||||
bin_dir, _ = shim
|
||||
env["PATH"] = f"{bin_dir}{os.pathsep}{env['PATH']}"
|
||||
if extra_env:
|
||||
env.update(extra_env)
|
||||
return subprocess.run(
|
||||
["bash", str(_SCRIPT), *args, "--baseline", str(baseline)],
|
||||
capture_output=True,
|
||||
|
|
@ -321,17 +324,27 @@ def test_dry_run_protection_restores_full_baseline(baseline: Path) -> None:
|
|||
assert "assert LIVE protection == baseline protection.full" in out
|
||||
|
||||
|
||||
def test_dry_run_protection_without_full_falls_back_to_enforce_admins(
|
||||
def test_protection_without_full_refused_unless_partial_acked(
|
||||
tmp_path: Path,
|
||||
) -> None:
|
||||
# MEDIUM-1: a baseline missing protection.full must NOT silently restore only
|
||||
# enforce_admins (a weaker posture). Without the explicit ack it fails hard.
|
||||
data = json.loads(json.dumps(_BASELINE))
|
||||
del data["protection"]["full"]
|
||||
p = tmp_path / "nofull.json"
|
||||
p.write_text(json.dumps(data), encoding="utf-8")
|
||||
res = _run("protection", baseline=p)
|
||||
out = res.stdout
|
||||
assert "no protection.full recorded" in out
|
||||
assert "branches/main/protection/enforce_admins" in out
|
||||
|
||||
refused = _run("protection", baseline=p)
|
||||
assert refused.returncode != 0
|
||||
assert "no protection.full" in (refused.stdout + refused.stderr)
|
||||
assert "P3_ROLLBACK_ALLOW_PARTIAL" in (refused.stdout + refused.stderr)
|
||||
|
||||
# With the explicit ack the degraded enforce_admins-only restore proceeds, but
|
||||
# LOUDLY warns it is partial.
|
||||
acked = _run("protection", baseline=p, extra_env={"P3_ROLLBACK_ALLOW_PARTIAL": "1"})
|
||||
out = acked.stdout + acked.stderr
|
||||
assert "DEGRADED protection restore" in out
|
||||
assert "branches/main/protection/enforce_admins" in acked.stdout
|
||||
|
||||
|
||||
def test_dry_run_incident_path_full_sequence(baseline: Path) -> None:
|
||||
|
|
@ -800,3 +813,34 @@ def test_apply_protection_DETECTS_divergent_live_restore(
|
|||
+ res.stderr
|
||||
)
|
||||
assert "protection.full" in (res.stdout + res.stderr)
|
||||
|
||||
|
||||
def test_missing_required_key_refuses_partial_restore(tmp_path: Path) -> None:
|
||||
# MEDIUM-1: a baseline missing a required key for a surface aborts that
|
||||
# surface up front rather than half-restoring it.
|
||||
data = json.loads(json.dumps(_BASELINE))
|
||||
del data["workflow_baseline_sha"]
|
||||
p = tmp_path / "no_sha.json"
|
||||
p.write_text(json.dumps(data), encoding="utf-8")
|
||||
res = _run("workflow", baseline=p)
|
||||
assert res.returncode != 0
|
||||
out = res.stdout + res.stderr
|
||||
assert "missing required key" in out
|
||||
assert "workflow_baseline_sha" in out
|
||||
|
||||
|
||||
def test_app_uninstall_without_jwt_requires_oob_ack(tmp_path: Path) -> None:
|
||||
# MEDIUM-2: an --apply that needs a MANUAL App neutralise (no APP JWT) must
|
||||
# not silently skip it — it refuses unless the operator acknowledges.
|
||||
p = tmp_path / "bl.json"
|
||||
p.write_text(json.dumps(_BASELINE), encoding="utf-8")
|
||||
|
||||
# No JWT, no ack -> refuse.
|
||||
refused = _run("app", "--apply", baseline=p)
|
||||
assert refused.returncode != 0
|
||||
assert "P3_ROLLBACK_OOB_ACK" in (refused.stdout + refused.stderr)
|
||||
|
||||
# No JWT, but acked -> proceeds (App neutralise is operator-owed, loudly warned).
|
||||
acked = _run("app", "--apply", baseline=p, extra_env={"P3_ROLLBACK_OOB_ACK": "1"})
|
||||
assert acked.returncode == 0, acked.stdout + acked.stderr
|
||||
assert "OUT-OF-BAND ACK accepted" in (acked.stdout + acked.stderr)
|
||||
|
|
|
|||
Reference in a new issue