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.

@rocklambros

Copy link
Copy Markdown
Collaborator

Hi Pramod, OWASP requires a sign-off on every commit, and four commits on this PR don't have one: 7b03a7c, 4e52621, 5c8a2da and 2fb4976. The dco check we're adding in PR #407 will fail them.

Please run these commands:

git checkout fix/session-identity-binding
git fetch https://github.com/GenAI-Security-Project/agent-control-standard.git integration
git rebase --signoff FETCH_HEAD
git push --force-with-lease

They add a Signed-off-by: line to each commit and move your branch onto the latest integration. On a copy of your branch they ran with no conflicts, and the two merge commits went away, which is fine. Thanks!

@rocklambros rocklambros mentioned this pull request Nov 1, 2026
11 of 15 tasks
@rocklambros

Copy link
Copy Markdown
Collaborator

Hi Pramod, this comment replaces my earlier one. The dco check is now required on integration, so this pull request can't merge until the check passes.

Nothing is wrong with your work. The check looks for a sign-off, and OWASP requires one on every commit.

A sign-off is a Signed-off-by: line in the commit message. It states that you have the right to submit the work. It is not a GPG or SSH signature, so you don't need any keys.

Four commits on this pull request don't have the line: 7b03a7c, 4e52621, 5c8a2da, and 2fb4976. Please run these steps on your computer.

  1. Set your name and email. You only need to do this once. Git puts them in each commit, and they are public.
git config --global user.name "Your Name"
git config --global user.email "you@example.com"
git config user.name

The last command prints your name, so you can check it.

  1. Go to your branch and connect it to the project. If Git says upstream already exists, that's fine. Continue with the next command.
git checkout fix/session-identity-binding
git remote add upstream https://github.com/GenAI-Security-Project/agent-control-standard.git
git fetch upstream
  1. Add the sign-off to every commit and push.
git rebase --signoff upstream/integration
git push --force-with-lease

If Git says your branch has no upstream branch, run git push --force-with-lease origin HEAD.

These steps ran on a copy of your branch with no conflicts. The two merge commits go away, which is fine.

If the rebase does stop on a conflict, open each file Git lists and fix those lines. Then run git add <file> and git rebase --continue. To stop and go back to where you started, run git rebase --abort.

  1. Check that it worked.
git log -1 --format=%B

The output should end with Signed-off-by: and your name and email. The dco check runs again by itself after you push.

The full guide is here: How to sign your commits.

Thanks!

@drk-ram drk-ram 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.

I reproduced a cross-identity write at 2fb4976 when eviction removes a binding during an outstanding policy evaluation.

With maxSessions: 1, I held agent A's bridge.evaluate() response after its request entered session S. A handshake for another session evicts S; agent B can then handshake with S and bind it.

Releasing A's verdict with result_labels: ["from-agent-a"] changes B's labels from ["public"] to ["from-agent-a"]. The old request still returns allow. bindIdentity checks before evaluation, but persistIfcLabels writes after the await without checking which session lifecycle now owns S.

Rechecking the binding after the await makes my regression pass. Please guard replacement state against retired evaluations and add this interleaving to the tests.

All 18 committed identity tests and bun run typecheck pass. Removing either digest field breaks the corresponding request/result tests.

The description also needs updating: user fields remain request-scoped, and eviction now removes bindings.

@President

Copy link
Copy Markdown

@upramod thanks - all three points are fixed: bindings are removed with the session (7b03a7c), a refused handshake returns -32000 (4e52621), and the README says policies don't get the user context yet (5c8a2da). The race drk-ram found is still open, though.

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.

4 participants