Skip to content

fix(agt): bind session state to declared identity before evaluation - #246

Open
upramod wants to merge 8 commits into
GenAI-Security-Project:integrationfrom
upramod:fix/session-identity-binding
Open

upramod wants to merge 8 commits into
GenAI-Security-Project:integrationfrom
upramod:fix/session-identity-binding

Conversation

@upramod

@upramod upramod commented Oct 10, 2026 •

Copy link
Copy Markdown

What changed

The reference Guardian accepts different agent or user identities into one session chain. This patch binds the session to its first validated identity declaration and rejects later changes before policy evaluation, label access, or chain mutation.

The binding covers agent ID, tenant ID, user ID, authentication method, and the role set. Role order and duplicates are insignificant. Both supported tool hooks check the binding, and a repeated handshake cannot replace it. Bindings survive audit-chain eviction; a separate bounded identity store refuses new sessions at capacity instead of silently permitting rebinding.

Which issue does this implement

Part of #166. This addresses declared-identity consistency, not the full authenticated-principal requirement, so the issue remains open.

Review scope

Ready for review of the declared-identity consistency change. Current integration has no authenticated-principal resolver and does not dispatch sessionStart. The first validated handshake or supported tool step establishes the declaration; this does not authenticate the caller. When combined with #229, binding must happen after signature verification.

Bindings survive chain eviction but not process restart. The default maximum is 65,536 bindings, configurable through maxIdentityBindings. There is no automatic expiry. Durable identity state and safe retirement need lifecycle design. Custom SessionContextStore implementations must provide synchronous, atomic bindIdentity. README documents these limits and the requirement for a new session ID after identity changes.

Base branch

Type of change

  • Reference implementation
  • Documentation and tests

Upstream CI

GitHub Actions passed on commit 53bde88: AGT suite 1,130 pass, 1 skip, 0 fail, including all 18 new identity tests. Typecheck, Python guards, docs build, triage, and closing-choice also passed. The IPv6 test passes on the upstream runner.

AGT run: https://github.com/GenAI-Security-Project/agent-control-standard/actions/runs/38017592925

I tested this

  • uv run pytest -q: 902 passed
  • uv run mkdocs build --strict passes
  • bun run typecheck passes
  • 18 new tests pass across both tool hooks, repeated handshakes, concurrent competing identities, separate sessions, chain eviction, capacity, role normalization, and existing unbound state.
  • Five changed-identity HTTP cases fail with the original server dispatch and pass with the patch; normal and separate-session controls pass.
  • Full Bun suite after installing the documented trash-cli prerequisite: 1,129 pass, 1 skip, 1 fail. The IPv6 wildcard-connectivity assertion also fails on the original server in this environment. No claim of a fully passing local suite.
  • The local tested tree matches the published Git tree bcaaf3a16762d69f2a58392ca2940542b0c094e1.

Checklist

  • Signed-off-by trailer included
  • No secrets or internal URLs
  • Prose checked against STYLE.md

Security

Mitigates the already-public identity-consistency defect in #166. A caller able to impersonate the initial declaration is not stopped by this patch. Rejected identity changes do not enter the original session chain.

Part of GenAI-Security-Project#166. Reject identity changes without appending to the session chain; preserve bindings across chain eviction and fail closed at capacity.

Signed-off-by: Pramod Ubbala <16706816+upramod@users.noreply.github.com>
@upramod

upramod commented Oct 17, 2026

Copy link
Copy Markdown
Author

Could a maintainer review this identity-consistency change when available?

This PR binds a session to its first validated declared identity and rejects later identity changes before policy evaluation, label access, or chain mutation. It intentionally does not claim to authenticate the caller or implement the full authenticated-principal requirement.

The AGT suite and the documented validation checks pass. Please let me know whether the current scope is appropriate for merging or whether you would prefer this to be split or coordinated with #229.

@President

Copy link
Copy Markdown

@upramod - FYI - one case this would break: one session with several users.

For example:

  • a bot in a Slack channel
  • OpenClaw in a group chat: by default each group or channel is one session for all its members (OpenClaw docs)

With user_id bound to the session:

  • the first person to write takes the session
  • everyone else gets session_identity_mismatch

Suggestion:

Bind session consistency to agent and tenant only. Keep user context
request-scoped and update HTTP regression expectations and documentation.

Signed-off-by: Pramod Ubbala <16706816+upramod@users.noreply.github.com>
@upramod

upramod commented Oct 25, 2026

Copy link
Copy Markdown
Author

Addressed in c468098. Sessions now bind only agent and tenant; users, roles, and authentication methods remain request-scoped. Updated regression tests and documentation. Upstream typecheck, tests, and documentation workflow passed. Ready for re-review.@President

@upramod

upramod commented Oct 27, 2026

Copy link
Copy Markdown
Author

The multi-user session concern has been addressed in c468098.

Session identity now binds only the agent and tenant. User identity, roles, and authentication methods remain request-scoped, allowing multiple users within a shared session.

The regression tests and documentation have been updated, and the upstream validation checks passed.

Could a maintainer confirm whether this addresses the concern and whether the PR is ready for approval?

@President President left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@upramod, thanks! Group chats work now: several people can write in one session, and the Guardian no longer blocks the second person.

I ran a small test on the latest version. I found 2 problems in the new code and 1 wrong sentence in the README:

  1. After 65,536 sessions in total, the Guardian refuses every new session, until it restarts:
  • before this PR, the Guardian remembers at most 1,024 sessions (the maxSessions setting). When session 1,025 starts, it forgets the oldest one. So it never refuses a new session
  • this PR adds a second list: "session X belongs to agent A". This list has its own limit, 65,536 (maxIdentityBindings), and the Guardian never deletes anything from it. When the Guardian forgets an old session, the line "session X belongs to agent A" stays in this list
  • so session 65,537 is refused, and every session after it too, for every agent. Only a restart helps
  • anyone who can send messages to the Guardian can do this: 65,536 hello messages (the message that starts a session), each with a new session ID, took 17 seconds in my test. Normal use also reaches the limit, but more slowly
  • suggestion: keep "belongs to agent A" inside the session itself, next to its other data. Then, when the Guardian forgets an old session, it forgets its owner too, and the second list is not needed
  1. Wrong error code when an agent sends another agent's session ID:
  • agent B sends hello with the session ID of agent A. The Guardian refuses, which is correct
  • BUT it answers -32010 "invalid message". For a refused session the spec has -32000 "session refused" (section 17.1)
  • why it matters: on "session refused" the agent does not start the session. On other errors, the spec (section 4.1) lets the agent start the session without the Guardian, and that is the default
  1. The README says "policies must check the user of each request", but the policy rules can't check the user yet:
  • the agent sends the user info with every request, and the Guardian receives it
  • BUT the Guardian does not pass the user info to the policy rules. The rules see only the tool and its arguments, so a rule like "only admins may delete files" can't be written yet
  • the rules could not see the user before your PR either. On 20 Oct I said the opposite - my mistake🤗
  • suggestion: say in the README that the rules can't see the user yet

P.S. The PR description still describes the first version: it says the user and roles are tied to the session. The user and roles are no longer tied to the session - maybe update it, so reviewers read what the code does now.

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.

2 participants