Repository navigation
Conversation
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>
|
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. |
|
@upramod - FYI - one case this would break: one session with several users. For example:
With
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>
|
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 |
|
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
left a comment
There was a problem hiding this comment.
@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:
- 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
maxSessionssetting). 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
- 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
- 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.
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
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
Checklist
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.