Fix session reuse route-binding validation to prevent cross-target session confusion#312
Fix session reuse route-binding validation to prevent cross-target session confusion#312KinshukSS2 wants to merge 3 commits intovolcano-sh:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @KinshukSS2! It looks like this is your first PR to volcano-sh/agentcube 🎉 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces session target validation to prevent cross-target session confusion by implementing a sessionTargetMatches check and a corresponding NewSessionTargetMismatchError. Feedback highlights that the current validation logic is "fail-open" when metadata fields are empty, which may bypass checks for new sessions. Additionally, comparing the ephemeral sandbox.Name against the requested resource name could lead to incorrect rejections. It is also recommended to include the structured logging and metrics mentioned in the PR description to improve observability.
| func sessionTargetMatches(sandbox *types.SandboxInfo, namespace, name, kind string) bool { | ||
| if sandbox == nil { | ||
| return false | ||
| } | ||
| if sandbox.Kind != "" && sandbox.Kind != kind { | ||
| return false | ||
| } | ||
| if sandbox.Name != "" && sandbox.Name != name { | ||
| return false | ||
| } | ||
| if sandbox.SandboxNamespace != "" && sandbox.SandboxNamespace != namespace { | ||
| return false | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
The validation logic in sessionTargetMatches has two significant issues that may undermine the intent of this pull request:
- Ineffective Validation: The function uses a "fail-open" approach by returning
trueif metadata fields likeKindorSandboxNamespaceare empty in the storedSandboxInfo. However, the current implementation ofcreateSandbox(lines 207-212) does not populate these fields when creating new sessions. Consequently, this validation will be bypassed for all newly created sessions, failing to prevent cross-target session confusion for new workloads. - Potential Session Reuse Breakage: Comparing
sandbox.Nameagainst the requestednamemay cause incorrect rejections.sandbox.Nameis populated fromres.SandboxName(the actual sandbox name), which often differs from the resource name used in the route target (e.g., if the workload manager generates unique names or uses suffixes).
To address these, ensure createSandbox stores the requested target metadata and that the validation logic correctly identifies the route target identity rather than the ephemeral sandbox name.
There was a problem hiding this comment.
Nice Catch, will make the necessary changes
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #312 +/- ##
==========================================
+ Coverage 47.57% 48.12% +0.55%
==========================================
Files 30 30
Lines 2819 2874 +55
==========================================
+ Hits 1341 1383 +42
- Misses 1338 1343 +5
- Partials 140 148 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Fix fail-open vulnerability: store requested metadata (namespace, name, kind) in SandboxInfo instead of ephemeral sandbox name for proper session reuse validation - Fix ephemeral name mismatch: sessionTargetMatches now compares against stored requested metadata instead of generated sandbox names - Add structured logging: log session target mismatches at V(2) level for better observability - Add comprehensive table-driven tests: cover perfect matches, mismatches, nil sandboxes, empty fields (wildcard matching), and edge cases
KinshukSS2
force-pushed
the
fix/session-route-binding
branch
from
f97dec0 to
b13d75d
Compare
volcano-sh-bot
added
size/L
and removed
do-not-merge/invalid-commit-message
size/M
labels
Fix session reuse route-binding validation
Summary
Add strict validation between route target identity and reused session identity during request forwarding.
Currently, requests using
x-agentcube-session-idonly validate the session ID and do not verify that the recovered sandbox belongs to the requested(namespace, name, kind)route target. This can lead to cross-target session reuse and incorrect runtime dispatch.Changes
409 ConflictSESSION_TARGET_MISMATCHTesting