Skip to content

spec(ask): clarify approver authentication in §9 - #342

Open
drk-ram wants to merge 1 commit into
GenAI-Security-Project:integrationfrom
drk-ram:spec/approver-auth-timeout-51
Open

drk-ram wants to merge 1 commit into
GenAI-Security-Project:integrationfrom
drk-ram:spec/approver-auth-timeout-51

Conversation

@drk-ram

@drk-ram drk-ram commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

What changed

Clarifies in docs/spec/instrument/specification.md (§9 and §9.1) that approver authentication is required across human, agent, and service approvers (with the optional approver.auth object describing machine-approver authentication and human approver identity verified out of band against policy), requires a verified approver grant in §9.1 before extending Intent.parsed, and adds tests/test_approver_model.py.

Which issue does this implement

Part of #51

  • This is an editorial correction (typo, grammar, link, formatting) with no change in
    meaning, so it needs no issue

Base branch

  • integration, because this touches the specification, schemas, a reference
    implementation, an adapter, tests, or CI
  • main, because every changed path is on the documentation lane allowlist in
    CONTRIBUTING.md

Type of change

  • Specification change (schema, hooks, events, AgBOM)
  • Reference implementation or adapter
  • Documentation
  • Tooling or CI
  • Governance (licensing, security policy, contributor docs)

Specification changes

  • I opened a Discussion before this PR
  • Schema changes validate against the JSON Schema spec
  • I described the impact on downstream implementers below

Breaking for implementers? No. Keeps ask-details.json wire validation rules unchanged in v0.1.0 while clarifying the separation between approver.auth descriptors and out-of-band human approver verification.

I tested this

  • I synced my branch with the base branch before opening this
  • uv run pytest -v passes on my machine
  • uv run mkdocs build --strict passes on my machine

Checklist

  • Commits are signed off with git commit -s (required by the DCO)
  • Prose follows STYLE.md
  • No secrets, tokens, or internal URLs in the diff

Security

  • This change has no security impact

Clarifies that a claimed approver.id alone is insufficient and that Intent.parsed extensions on scope: session apply when and only when a verified approver grants the request.

@github-actions

Copy link
Copy Markdown

This pull request's description says it closes #51, which is on the project roadmap. When it merges, GitHub closes that issue and the roadmap counts it as delivered. Please confirm the change delivers the whole issue before merging.

@rocklambros rocklambros left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raghuvar, your §9 text answers the second gap in issue #51. The issue says the spec never tells us how a human approver meets the rule, and your text does: out of band, through the deployment's approval interface. Your schema edit changes only a description, so all 12 payloads I tested get the same result as before. The merged tree on integration 19b1197 passes pytest (924 passed, 1 skipped), mkdocs build --strict, and both Bun versions with the same counts as before.

This PR doesn't close issue #51, though. The first gap is "auth is optional even for the approvers it is scoped to". The issue's example is { "type": "service", "id": "...", "endpoint": "https://..." } with no auth. That payload validates before and after this PR, and §9 now calls auth "optional".

This PR shouldn't try to fix that, because a required auth rejects payloads that are valid today, and the issue says that needs a spec version change. So the right line today is Part of #51. Fixes #51 closes the issue when this merges.

@bar-capsule @afogel, this PR adds normative text that no lead has accepted. It follows Carlos Hernandez's 2026-09-13 proposal on issue #51, which has no lead answer. The new text is in §9 (the human approver sentence and a MUST NOT), §9.1 ("when and only when a verified approver grants", and a MUST NOT), and the timeout_disposition description in ask-details.json. I need four answers:

  1. Which issue #51 option do you want? I said option 3 looked right and left the call to you. This PR is option 1. Under option 3 it is Part of, and the issue stays open for a schema change in a later spec version. Under option 1 alone, Fixes can stay. Either way, how does a service approver with no auth meet "REQUIRED"? §9 answers that for humans only, and Carlos asked whether a preconfigured channel counts.

  2. Who must follow the two timeout MUST NOTs, and where does the timeout outcome go? The text says a timeout "records" an outcome, and it names no actor and no place. In the reference host, the Observed Agent resolves the timeout and the Guardian never hears about it. I ran the real governStep on an expired ASK. The step ran as allow with reason code ask_expired, 0 audit events, and no second Guardian request. ContextEntry is kept by the Guardian, so a Guardian rule can't see this timeout.

  3. Can a timeout allow run a request outside Intent.parsed? This PR stops the timeout from adding capabilities, but the action still runs (same probe). §8.4 says the capability set "can grow only through explicit, audited approver action". Carlos's proposal said a timeout "cannot supply missing authority", and this PR left that out.

    The words "no verified approver decision" have the same problem. As I read them, a refusal with a bad token counts as no answer, and allow then runs the action. I couldn't run that case, because the spec has no path for the answer.

  4. Can this text land before issue #175? §9 says the deployment "conveys the verified identity to the Guardian". Issue #175 says the spec has no path for the approver's answer, and it is still in triage. On that issue, Konstantin Trunin and Imran Siddique suggest recording an unsigned answer as declared or agent-asserted. Does "a claimed approver.id alone is insufficient" rule that out?

    Carlos wrote that a deployment "can" do this, and this PR says it "authenticates", which reads as a requirement.

Once you answer, Raghuvar can push and one of you can approve and merge.

Raghuvar, please wait for that answer, so you only push once. The words in §9 and §9.1 may change. I applied these four asks to a scratch copy. pytest and mkdocs build --strict pass, and PR #339 merges clean on either side.

  1. In the PR body, under "Which issue does this implement", change Fixes #51 to Part of #51, unless the leads pick option 1 alone. The closing-choice code from main passes that line.

  2. docs/spec/instrument/specification.md:317 needs two small changes. STYLE.md asks for 2 to 4 sentences in a paragraph, and this one has six, so start a new paragraph at "Approver authentication is REQUIRED". Also, auth is an object, and bearer and mtls are values of auth.method, so "auth": "bearer" fails the schema. Change "The optional approver.auth member (bearer or mtls)" to "The optional approver.auth object (method: bearer or mtls)".

  3. specification/v0.1.0/ask-details.json:43 repeats the rule from §9 and §9.1, so the spec has it three times. SPEC_REVIEW_PRINCIPLES.md section 1 asks us to state a rule in one place. Change the description to:

    Disposition applied when timeout_seconds elapses without a verified approver decision. See Specification §9 and §9.1.
    
  4. Your test at tests/test_spec_structure.py:57 has two problems. Its docstring says it checks the human approver rule, but nothing does. I tried 15 breaks and 12 passed, including deleting the human sentence and changing either MUST NOT to MAY. It also conflicts with PR #339 in both orders. The two tests share lines, so if someone only deletes the conflict markers, PR #339's test has 0 asserts and the suite stays green at 925 passed.

    Moving the test fixes both problems. Delete it, and add this file as tests/test_approver_model.py. All 15 breaks then fail, and this PR no longer has to wait for PR #339. If the leads change the words, change these strings to match.

    """Approver rules in Specification §9 and ask-details.json (#51)."""
    
    import json
    from pathlib import Path
    
    ROOT = Path(__file__).resolve().parent.parent
    SPEC = ROOT / "docs" / "spec" / "instrument" / "specification.md"
    ASK = ROOT / "specification" / "v0.1.0" / "ask-details.json"
    
    
    def approver_section():
        text = SPEC.read_text(encoding="utf-8")
        return text.split("## 9. Approver Model", 1)[1].split("## 10.", 1)[0]
    
    
    def test_every_approver_type_must_authenticate():
        section = approver_section()
        assert "REQUIRED across all three approver types" in section
        assert "the Guardian MUST verify" in section
        assert "out of band" in section
        assert "a claimed `approver.id` alone is insufficient" in section
        assert "does not by itself establish that verification occurred" in section
    
    
    def test_a_timeout_is_never_recorded_as_an_approval():
        section = approver_section()
        assert 'default `"deny"`' in section
        assert "it MUST NOT record the named" in section
        assert "it MUST NOT append `intent_extension` capabilities" in section
        assert "when and only when a verified approver grants" in section
    
    
    def test_the_v0_1_0_wire_contract_is_unchanged():
        """A required `auth` breaks valid payloads, so issue #51 keeps it for a later spec version."""
        schema = json.loads(ASK.read_text(encoding="utf-8"))
        assert schema["properties"]["approver"]["required"] == ["type", "id"]
        assert schema["properties"]["timeout_disposition"]["default"] == "deny"
        assert "§9" in schema["properties"]["timeout_disposition"]["description"]

@bar-capsule

Copy link
Copy Markdown
Collaborator

@rocklambros @drk-ram, answers to the four:

1: Option 3. This PR is the text now, and requiring auth waits for the version bump, so Part of #51 stays. Whether a preconfigured authenticated channel counts for a service approver with no auth is an identity question, so I'd hand it to the Identity workstream (@RbBuiltWrong).

2+3: I see ASK as a soft deny: "I won't do this unless someone with authority says yes." So the Guardian holds the timeout, and no answer in time means deny. If the Guardian can't be reached at that point, it's an ordinary decision failure (#32). The Observed Agent never turns an ASK into a go on its own. That leaves timeout_disposition: allow with no meaning, so it goes away with the #32 PR's version bump, and a timeout can't add authority because it can't allow anything. An answer that fails verification blocks, as any integrity failure does (#32).

4: Yes, the authentication text can land first. "A claimed approver.id alone is insufficient" stays: an answer that only the agent vouches for can be recorded as declared, as proposed on #175, but it doesn't approve the step. Approval needs a verified approver: through the deployment's trusted channel today, or the approver's signature once #175 defines it. Rock, that also names the timeout owner you asked for on #175: the Guardian.

@drk-ram, since the timeout rules now come with the #32 PR, please drop the timeout paragraph in §9, the timeout sentence in §9.1 and the timeout_disposition description, and keep the authentication text and "when and only when a verified approver grants". Rock's asks 2 and 4 still apply, without the timeout checks in the test; ask 3 goes away with the description.

cc: @afogel

…ervice approvers in §9

Clarifies in §9 and §9.1 of the Instrument specification that approver
authentication is required across all three approver types, distinguishes the
optional `approver.auth` object (`method`: `bearer` or `mtls`) from out-of-band
human approver verification, and requires a verified approver grant before
extending `Intent.parsed`.

Part of GenAI-Security-Project#51

Signed-off-by: Raghuvar Karthik Durga <177662415+drk-ram@users.noreply.github.com>
@drk-ram drk-ram changed the title spec(ask): clarify approver authentication and timeout_disposition attribution in §9 spec(ask): clarify approver authentication in §9 Nov 5, 2026
@drk-ram
drk-ram force-pushed the spec/approver-auth-timeout-51 branch from 3df0745 to 86df4e7 Compare November 5, 2026 17:30
@drk-ram

drk-ram commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator Author

@bar-capsule @rocklambros Rebased onto integration and pushed 86df4e7:

  1. Dropped the timeout paragraph in §9, the timeout sentence in §9.1, and the timeout_disposition description edit in ask-details.json.
  2. Kept the approver authentication text in §9 (split into a new paragraph at Approver authentication is REQUIRED, with The optional approver.auth object (method: bearer or mtls)) and when and only when a verified approver grants the request in §9.1.
  3. Replaced the test in tests/test_spec_structure.py with tests/test_approver_model.py.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In progress

Development

Successfully merging this pull request may close these issues.

3 participants