diff --git a/CHANGELOG.md b/CHANGELOG.md index 790d96d..af8816e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,38 @@ ### Fixed +- **[SDK]** `verify_manifest()` returned `MISMATCH`/`EXPIRED`/`INVALID`/ + `UNVERIFIABLE` for `hitl_record` when *any* approval in `hitl_record.approvals` + failed, even if a later approval in the same array was present, valid, + unexpired, and sufficient for the declared risk tier. Spec 5.3 only + requires *at least one* approval to satisfy every condition; it does not + require every approval in the array to. The verifier's loop broke out on + the first expired, malformed, unverifiable, or insufficient approval it + encountered and never evaluated the approvals after it, so an operator + who appended a fresh re-approval ahead of (or alongside) an old, expired, + or otherwise stale one had the whole record incorrectly rejected. The + loop now evaluates every approval and only short-circuits once one is + found that clears every check. When none do, the prior code never had a + real precedence to preserve: it broke the whole loop on the first + approval that failed, so the reported reason was whichever check that + one approval happened to fail first, not a ranking of failure types (an + `[EXPIRED, INVALID]` array reported `EXPIRED`; `[INVALID, EXPIRED]` + reported `INVALID` - purely positional, order-dependent, and never a + documented contract). Reviewing this against multiple approvals for the + first time requires an actual decision here, since more than one failure + reason can now be true at once. This fix establishes and tests an + explicit, order-independent precedence: `INVALID` > `APPROVAL_INSUFFICIENT` + > `EXPIRED` > `UNVERIFIABLE`. `INVALID`, `APPROVAL_INSUFFICIENT`, and + `EXPIRED` are each positive, concrete evidence of a problem and always add + a mismatch, so their relative order never changes the overall result; + `INVALID` is ranked highest among them because a broken or tampered + signature is the strongest evidence of active tampering. `UNVERIFIABLE` + (this verifier has no trusted key to check that approval against) adds no + mismatch, so it is ranked last and only reported when it is the *only* + problem in the array - otherwise it would silently drop a concrete + finding from `mismatch_details` and downgrade the overall result from + `MISMATCH` to `UNVERIFIABLE` (HITL-004). + - **[SECURITY][SDK]** `attach_receipt()`, `attach_attestation()`, and `attach_approvals()` could crash with `cbor2.CBOREncodeError` instead of the documented `CoseError` on a malformed COSE envelope. `_decode_tagged()` diff --git a/python/src/agent_manifest/_verify.py b/python/src/agent_manifest/_verify.py index 3083c6d..4c031c2 100644 --- a/python/src/agent_manifest/_verify.py +++ b/python/src/agent_manifest/_verify.py @@ -1216,27 +1216,36 @@ def _check(field_name: str, manifest_val: Optional[str], runtime_val: Optional[s # Presence, lifetime, and method checks alone do not prove that a # human approved this manifest: the signature pre-image binds the # manifest ID, approver, timestamp, and exact scope. + from datetime import timedelta + from ._delegation import verify_hitl_approval from ._signing import _b64url_decode now = datetime.now(timezone.utc) - all_ok = True + # Spec 5.3: a VALID result requires *at least one* approval that is + # present, valid, unexpired, and sufficient for the declared risk + # tier - not that *every* approval in the array meets that bar. + # Each approval is therefore evaluated independently and the loop + # only stops early once an approval that clears every check is + # found. Approvals that fail must not short-circuit the + # evaluation of the approvals that follow them (HITL-004). + any_approved = False approval_insufficient = False approval_invalid = False approval_unverifiable = False + any_expired = False for approval in approvals: approved_at = approval.get("approved_at", "") duration = approval.get("approved_scope", {}).get("approval_duration_seconds", 0) try: ap_time = datetime.fromisoformat(approved_at.replace("Z", "+00:00")) - from datetime import timedelta if now > ap_time + timedelta(seconds=duration): - all_ok = False - break + any_expired = True + continue except (ValueError, AttributeError): # Unparseable timestamp - treat as expired to fail safe (HITL-001) - all_ok = False - break + any_expired = True + continue if context.conformance_level >= 2: scope = approval.get("approved_scope") or {} risk_tier = scope.get("risk_tier") @@ -1245,13 +1254,13 @@ def _check(field_name: str, manifest_val: Optional[str], runtime_val: Optional[s risk_tier in {"high", "critical"} and method != "hardware-key" ): approval_insufficient = True - break + continue approver_id = approval.get("approver_id") public_key_b64 = context.approver_public_keys.get(approver_id) if public_key_b64 is None: approval_unverifiable = True - break + continue try: verify_hitl_approval( approval, @@ -1260,15 +1269,28 @@ def _check(field_name: str, manifest_val: Optional[str], runtime_val: Optional[s ) except (InvalidSignature, KeyError, TypeError, ValueError): approval_invalid = True - break + continue + + # This approval independently satisfies every requirement + # (present, unexpired, sufficient for the risk tier, and + # authenticated). Spec 5.3 only requires one such approval. + any_approved = True + break + + if any_approved: + fields.hitl_record = HitlResult.APPROVED - if approval_unverifiable: - fields.hitl_record = HitlResult.UNVERIFIABLE - result.warnings.append( - "HITL approval could not be authenticated: no trusted " - "approver key is available for its approver_id" - ) elif approval_invalid: + # A cryptographically broken/tampered approval is positive, + # concrete evidence of a problem. It always outranks + # UNVERIFIABLE (a mere lack of key configuration on this + # verifier's part, which produces no mismatch entry below) + # so that proof of tampering is never masked by an unrelated + # approval this verifier simply lacks the key to check + # (HITL-004). This mirrors the final overall-result + # computation elsewhere in this function, where any + # concrete `mismatches` entry already takes priority over + # an UNVERIFIABLE state. mismatches.append(MismatchDetail( field="hitl_record.approval_signature", expected_hash="", @@ -1282,7 +1304,7 @@ def _check(field_name: str, manifest_val: Optional[str], runtime_val: Optional[s actual_hash="", )) fields.hitl_record = HitlResult.APPROVAL_INSUFFICIENT - elif not all_ok: + elif any_expired: # Expired approvals always add to mismatches regardless of enforce_hitl (HITL-002) mismatches.append(MismatchDetail( field="hitl_record", @@ -1290,8 +1312,28 @@ def _check(field_name: str, manifest_val: Optional[str], runtime_val: Optional[s actual_hash="", )) fields.hitl_record = HitlResult.EXPIRED + elif approval_unverifiable: + # Reached only when no approval produced concrete evidence + # of a problem (no invalid signature, no insufficient + # method, no expiry) - i.e. every failing approval failed + # solely because this verifier has no trusted key for its + # approver_id. There is nothing to add to `mismatches` here; + # this is an indeterminate state, not a proven negative. + fields.hitl_record = HitlResult.UNVERIFIABLE + result.warnings.append( + "HITL approval could not be authenticated: no trusted " + "approver key is available for its approver_id" + ) else: - fields.hitl_record = HitlResult.APPROVED + # Defensive fallback: approvals was non-empty, so every entry + # must have set one of the flags above unless it was + # approved. Fail closed rather than silently pass. + mismatches.append(MismatchDetail( + field="hitl_record", + expected_hash="", + actual_hash="", + )) + fields.hitl_record = HitlResult.EXPIRED elif context.enforce_hitl: # enforce_hitl with no hitl_record at all - fail closed. Omitting the # record entirely MUST NOT be weaker than declaring it with no approvals. diff --git a/python/tests/test_cose.py b/python/tests/test_cose.py index 72f232f..747ef81 100644 --- a/python/tests/test_cose.py +++ b/python/tests/test_cose.py @@ -1024,6 +1024,22 @@ def test_signed_hitl_requirement_cannot_be_satisfied_by_editing_the_header(): assert result.result == OverallResult.MISMATCH +def test_engine_approves_when_a_later_approval_in_the_unprotected_header_is_valid(): + """A bad approval earlier in the array must not block a good one later + in it - same requirement as the v0.1 path (spec 5.3, HITL-004), now + exercised through the COSE unprotected-header attachment path.""" + manifest = base_manifest(hitl_record={"required": True}) + dummy_signature_approval = approval(approval_signature="c2ln") + good_approval = approval() + signed = attach_approvals( + sign_cose_sign1(manifest, KP), + [dummy_signature_approval, good_approval], + ) + result = verify_manifest(signed, base_context(enforce_hitl=True), store()) + assert result.fields_verified.hitl_record == HitlResult.APPROVED + assert result.result == OverallResult.VALID + + # --------------------------------------------------------------------------- # Malformed input # diff --git a/python/tests/test_verify.py b/python/tests/test_verify.py index a29969a..3a101f3 100644 --- a/python/tests/test_verify.py +++ b/python/tests/test_verify.py @@ -885,6 +885,218 @@ def test_level_2_accepts_hardware_key_for_high_risk_approval(): assert result.result == OverallResult.VALID +# --------------------------------------------------------------------------- +# HITL: at least one approval must satisfy every requirement, not every +# approval (spec 5.3: "at least one HITL approval is present, valid, not +# expired, and meets the approval_method requirement"). A verifier MUST NOT +# reject the whole record because *some other* approval in the array is +# expired, malformed, unverifiable, or insufficient. See HITL-004. +# --------------------------------------------------------------------------- + + +def test_hitl_valid_approval_after_expired_approval_is_approved(): + expired_time = (NOW - timedelta(hours=5)).isoformat().replace("+00:00", "Z") + valid_time = (NOW - timedelta(minutes=30)).isoformat().replace("+00:00", "Z") + m = base_manifest(hitl_record={ + "required": True, + "approvals": [ + hitl_approval(expired_time, {"approval_duration_seconds": 3600}), + hitl_approval(valid_time, {"approval_duration_seconds": 7200}), + ], + }) + result = verify_manifest(m, base_context(enforce_hitl=True), store()) + assert result.fields_verified.hitl_record == HitlResult.APPROVED + assert result.result == OverallResult.VALID + assert result.mismatch_details == [] + + +def test_hitl_valid_approval_after_invalid_signature_approval_is_approved(): + valid_time = (NOW - timedelta(minutes=30)).isoformat().replace("+00:00", "Z") + valid_approval = hitl_approval(valid_time, {"approval_duration_seconds": 7200}) + tampered_approval = dict(valid_approval) + tampered_approval["approval_signature"] = "not-a-real-signature" + m = base_manifest(hitl_record={ + "required": True, + "approvals": [tampered_approval, valid_approval], + }) + result = verify_manifest(m, base_context(enforce_hitl=True), store()) + assert result.fields_verified.hitl_record == HitlResult.APPROVED + assert result.result == OverallResult.VALID + + +def test_hitl_valid_approval_after_unverifiable_approval_is_approved(): + valid_time = (NOW - timedelta(minutes=30)).isoformat().replace("+00:00", "Z") + valid_approval = hitl_approval(valid_time, {"approval_duration_seconds": 7200}) + unverifiable_approval = dict(valid_approval) + unverifiable_approval["approver_id"] = "mailto:unknown@example.com" + m = base_manifest(hitl_record={ + "required": True, + "approvals": [unverifiable_approval, valid_approval], + }) + result = verify_manifest(m, base_context(enforce_hitl=True), store()) + assert result.fields_verified.hitl_record == HitlResult.APPROVED + assert result.result == OverallResult.VALID + + +def test_hitl_valid_approval_after_insufficient_approval_is_approved(): + valid_time = (NOW - timedelta(minutes=30)).isoformat().replace("+00:00", "Z") + software_key_approval = { + "approver_id": APPROVER_ID, + "approved_at": valid_time, + "approved_scope": { + "approval_duration_seconds": 7200, + "risk_tier": "high", + }, + "approval_method": "software-key", + } + hardware_key_approval = hitl_approval( + valid_time, + {"approval_duration_seconds": 7200, "risk_tier": "high"}, + approval_method="hardware-key", + ) + m = base_manifest(hitl_record={ + "required": True, + "approvals": [software_key_approval, hardware_key_approval], + }) + attach_transparency_entry(m) + result = verify_manifest( + m, + base_context( + enforce_hitl=True, + conformance_level=2, + verified_transparency_entry_ids={TRANSPARENCY_ENTRY_ID}, + transparency_evidence_manifest_id=m["manifest_id"], + ), + store(), + ) + assert result.fields_verified.hitl_record == HitlResult.APPROVED + assert result.result == OverallResult.VALID + + +def test_hitl_all_approvals_expired_is_still_expired(): + expired_time = (NOW - timedelta(hours=5)).isoformat().replace("+00:00", "Z") + m = base_manifest(hitl_record={ + "required": True, + "approvals": [ + hitl_approval(expired_time, {"approval_duration_seconds": 3600}), + hitl_approval(expired_time, {"approval_duration_seconds": 60}), + ], + }) + result = verify_manifest(m, base_context(enforce_hitl=True), store()) + assert result.fields_verified.hitl_record == HitlResult.EXPIRED + assert result.result == OverallResult.MISMATCH + + +def test_hitl_all_approvals_invalid_is_still_invalid(): + valid_time = (NOW - timedelta(minutes=30)).isoformat().replace("+00:00", "Z") + valid_approval = hitl_approval(valid_time, {"approval_duration_seconds": 7200}) + tampered_1 = dict(valid_approval) + tampered_1["approval_signature"] = "not-a-real-signature-1" + tampered_2 = dict(valid_approval) + tampered_2["approval_signature"] = "not-a-real-signature-2" + m = base_manifest(hitl_record={ + "required": True, + "approvals": [tampered_1, tampered_2], + }) + result = verify_manifest(m, base_context(enforce_hitl=True), store()) + assert result.fields_verified.hitl_record == HitlResult.INVALID + assert result.result == OverallResult.MISMATCH + + +# --------------------------------------------------------------------------- +# HITL: precedence when approvals fail for *different* reasons and none is +# valid. No approval in the array is fully valid in any of the pairs below, +# so `any_approved` is never true; the result is a deliberate, order- +# independent precedence, not the position of the first bad approval in the +# array (that was the pre-fix behavior and was never a documented contract - +# see HITL-004 changelog entry). The precedence is: +# +# INVALID > APPROVAL_INSUFFICIENT > EXPIRED > UNVERIFIABLE +# +# INVALID, APPROVAL_INSUFFICIENT, and EXPIRED are all positive, concrete +# evidence of a problem and always add a MismatchDetail, so their relative +# order does not change the overall result (always MISMATCH either way) - +# INVALID is ranked highest among them because a broken/tampered signature +# is the strongest evidence of active tampering. UNVERIFIABLE is ranked +# last and deliberately excluded from that ordering question: it means +# "this verifier lacks the key to even check," not proof of a problem, and +# it adds no MismatchDetail. Letting it outrank a concrete finding would +# silently drop that finding from `mismatch_details` and downgrade the +# overall result from MISMATCH to UNVERIFIABLE - so it only wins when it is +# the *only* thing wrong with the array. +# --------------------------------------------------------------------------- + +_HITL_FAILURE_BUILDERS = {} + + +def _hitl_precedence_case(label): + def register(fn): + _HITL_FAILURE_BUILDERS[label] = fn + return fn + return register + + +@_hitl_precedence_case("INVALID") +def _mk_invalid(): + t = (NOW - timedelta(minutes=30)).isoformat().replace("+00:00", "Z") + a = dict(hitl_approval(t, {"approval_duration_seconds": 7200})) + a["approval_signature"] = "not-a-real-signature" + return a + + +@_hitl_precedence_case("UNVERIFIABLE") +def _mk_unverifiable(): + t = (NOW - timedelta(minutes=30)).isoformat().replace("+00:00", "Z") + a = dict(hitl_approval(t, {"approval_duration_seconds": 7200})) + a["approver_id"] = "mailto:unknown@example.com" + return a + + +@_hitl_precedence_case("EXPIRED") +def _mk_expired(): + t = (NOW - timedelta(hours=5)).isoformat().replace("+00:00", "Z") + return hitl_approval(t, {"approval_duration_seconds": 3600}) + + +@_hitl_precedence_case("APPROVAL_INSUFFICIENT") +def _mk_insufficient(): + t = (NOW - timedelta(minutes=30)).isoformat().replace("+00:00", "Z") + return { + "approver_id": APPROVER_ID, + "approved_at": t, + "approved_scope": {"approval_duration_seconds": 7200, "risk_tier": "high"}, + "approval_method": "software-key", + } + + +_HITL_PRECEDENCE_RANK = ["INVALID", "APPROVAL_INSUFFICIENT", "EXPIRED", "UNVERIFIABLE"] + + +@pytest.mark.parametrize("first", _HITL_PRECEDENCE_RANK) +@pytest.mark.parametrize("second", _HITL_PRECEDENCE_RANK) +def test_hitl_mixed_failure_precedence_is_order_independent(first, second): + if first == second: + pytest.skip("covered by the homogeneous-failure tests above") + approvals = [_HITL_FAILURE_BUILDERS[first](), _HITL_FAILURE_BUILDERS[second]()] + m = base_manifest(hitl_record={"required": True, "approvals": approvals}) + result = verify_manifest( + m, base_context(enforce_hitl=True, conformance_level=2), store() + ) + expected = min((first, second), key=_HITL_PRECEDENCE_RANK.index) + assert result.fields_verified.hitl_record.name == expected, ( + f"[{first}, {second}] should resolve to {expected} regardless of " + f"array order, got {result.fields_verified.hitl_record.name}" + ) + if expected == "UNVERIFIABLE": + assert result.result == OverallResult.UNVERIFIABLE + assert result.mismatch_details == [] + else: + assert result.result == OverallResult.MISMATCH + # A concrete failure elsewhere must never be dropped just because + # an unrelated approval in the same array was unverifiable. + assert result.mismatch_details != [] + + # --------------------------------------------------------------------------- # Fail-closed delegation chain verification (spec 3.4.1 / 5.2) # ---------------------------------------------------------------------------