feat(sidecar): support multi-client identity isolation in server-demo#934
Open
hGrany wants to merge 6 commits into
Open
feat(sidecar): support multi-client identity isolation in server-demo#934hGrany wants to merge 6 commits into
hGrany wants to merge 6 commits into
Conversation
When multiple CLI sandbox environments share a single sidecar instance, user tokens (UAT) were not isolated -- the last user to log in would overwrite previous users' tokens, causing identity cross-contamination. This change introduces per-client HMAC key isolation: - Each client gets a unique client-*.key file for data-plane HMAC signing, allowing the sidecar to identify request origin. - A new auth_bridge.go handles management endpoints (login/poll/status) with explicit client-to-feishuOpenId binding. - User token resolution is strictly bound to the matched client -- no fallback to other users' tokens when a client has no mapping. - The shared proxy.key is reused across restarts instead of regenerated, fixing a race condition when multiple sidecar instances start together. Wire protocol (sidecar package) is unchanged; existing single-client deployments are fully backward compatible. Signed-off-by: Gao Yang <grany@yeah.net> (topwin.tech)
|
|
github-actions
Bot
added
the
size/L
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds a multi-tenant auth sidecar demo: per-client HMAC key isolation, a management HMAC bridge with OAuth device-flow login/poll/status and persisted client→open_id mapping, strict allowlist validation, secure HTTPS forwarding with token injection, CLI/server wiring, and comprehensive tests and docs. ChangesMulti-Tenant Auth Sidecar & Device-Flow OAuth
Sequence Diagram(s)sequenceDiagram
participant Client
participant Bridge as Auth Bridge
Client->>Bridge: POST /_sidecar/auth/* with headers
Bridge->>Bridge: verifyManagementHMAC (validate timestamp, body-sha, signature)
alt Valid
Bridge->>Bridge: Route by path: /login, /poll, /status
else Invalid
Bridge-->>Client: 401 Unauthorized
end
sequenceDiagram
participant Client
participant AuthBridge
participant Feishu
Client->>AuthBridge: POST /login (scope, client_id)
AuthBridge->>Feishu: RequestDeviceAuthorization
Feishu-->>AuthBridge: device_code, user_code, verification_url
AuthBridge-->>Client: JSON response
Client->>AuthBridge: POST /poll (device_code, client_id?)
AuthBridge->>Feishu: PollDeviceToken
Feishu-->>AuthBridge: access_token, refresh_token
AuthBridge->>Feishu: GET /open-apis/authen/v1/user_info (Bearer token)
Feishu-->>AuthBridge: open_id, name
AuthBridge->>AuthBridge: Save token, update config, persist client mapping
AuthBridge-->>Client: success JSON with open_id
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sidecar/server-demo/auth_bridge.go`:
- Around line 76-96: Replace direct os.* calls in authBridge.loadUserMap and
authBridge.saveUserMap with the vfs package equivalents (use vfs.ReadFile and
vfs.WriteFile) so filesystem access is mockable; in loadUserMap keep the same
JSON unmarshal logic but read via vfs.ReadFile and propagate or log the
read/unmarshal error instead of silently returning, and in saveUserMap marshal
ab.userMap and use vfs.WriteFile while checking and handling the returned error
(do not ignore it) and preserve file mode 0600 semantics; update imports to
include "github.com/larksuite/cli/internal/vfs" and ensure references to
ab.mapFile and ab.userMap remain unchanged.
- Around line 141-146: Replace the unbounded io.ReadAll(r.Body) call with a
size-limited reader (e.g. wrap r.Body with http.MaxBytesReader or
io.LimitReader) before reading so attackers can't exhaust memory; check for the
limit-exceeded error and return an appropriate jsonError (use
http.StatusRequestEntityTooLarge or http.StatusBadRequest) and still ensure
r.Body.Close() is called. Specifically modify the code around io.ReadAll, r.Body
and jsonError to enforce a defined max (e.g. a small constant like maxBodyBytes)
and handle/read errors from the limited reader accordingly.
- Around line 493-519: The loadCachedScopes function should use the vfs
filesystem helpers instead of os.* so tests can mock FS; replace os.ReadDir(dir)
with vfs.ReadDir(dir) and os.ReadFile(filepath.Join(dir, e.Name())) with
vfs.ReadFile(filepath.Join(dir, e.Name())), import the vfs package, and keep the
same error handling/return behavior and the existing struct/json unmarshal logic
in loadCachedScopes to preserve functionality.
In `@sidecar/server-demo/handler.go`:
- Around line 64-84: The scan currently overwrites duplicate keys and doesn't
guard against a client key matching the shared proxy key; update the loop that
builds newKeys (map[string]clientKeyEntry) to: reject and log duplicate keyHex
values instead of overwriting (if keyHex already exists in newKeys, skip and
log), and also skip+log any keyHex that equals the shared proxy key (e.g.,
h.sharedKey or the shared-key variable used later) so it cannot collide with the
shared-key path that sets matchedClient; ensure clientKeyEntry population and
use of h.keysDir remain unchanged.
- Around line 54-75: The loadClientKeys function is reading user-supplied paths
with os.ReadDir/os.ReadFile and lacks SafeInputPath validation; update
loadClientKeys to first validate h.keysDir via validate.SafeInputPath (or
similar helper) and then replace direct os.* calls with the vfs equivalents
(e.g., vfs.ReadDir / vfs.ReadFile) so the keysDir is validated and filesystem
access is virtualized for tests; keep existing behavior for filtering
client-*.key filenames and error logging via h.logger.
In `@sidecar/server-demo/main.go`:
- Around line 71-89: The code reads the user-supplied keyFile with os.ReadFile
before validating and using the virtual FS; change the logic so you call
validate.SafeInputPath(keyFile) up front and use vfs.ReadFile/vfs.WriteFile (not
os.ReadFile) everywhere you touch keyFile; update the existing-key branch to use
vfs.ReadFile, trim/validate the hex length (64) as before, and keep
vfs.MkdirAll/vfs.WriteFile for creation so all file ops go through the vfs and
the path is validated before access.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 85f86690-c316-4d05-b4fc-d36da15b3881
📒 Files selected for processing (3)
sidecar/server-demo/auth_bridge.gosidecar/server-demo/handler.gosidecar/server-demo/main.go
Comment on lines
+71
to
+89
| // Reuse existing key if present; generate a new one only on first run. | ||
| keyDir := filepath.Dir(keyFile) | ||
| if err := vfs.MkdirAll(keyDir, 0700); err != nil { | ||
| return fmt.Errorf("failed to create key directory: %v", err) | ||
| } | ||
| if err := vfs.WriteFile(keyFile, []byte(keyHex), 0600); err != nil { | ||
| return fmt.Errorf("failed to write key file: %v", err) | ||
|
|
||
| var keyHex string | ||
| if existing, err := os.ReadFile(keyFile); err == nil && len(strings.TrimSpace(string(existing))) == 64 { | ||
| keyHex = strings.TrimSpace(string(existing)) | ||
| } else { | ||
| keyBytes := make([]byte, 32) | ||
| if _, err := rand.Read(keyBytes); err != nil { | ||
| return fmt.Errorf("failed to generate HMAC key: %v", err) | ||
| } | ||
| keyHex = hex.EncodeToString(keyBytes) | ||
| if err := vfs.WriteFile(keyFile, []byte(keyHex), 0600); err != nil { | ||
| return fmt.Errorf("failed to write key file: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate and virtualize --key-file reads at the boundary.
keyFile is user-supplied, but Line 78 reads it via os.ReadFile before any validate.SafeInputPath check. That bypasses the repo’s path validation contract and the vfs abstraction right at the CLI boundary.
Based on learnings, validate.SafeInputPath is required for user-supplied local file paths; as per coding guidelines, **/*.go: use vfs.* instead of os.* for all filesystem access to enable test mocking.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sidecar/server-demo/main.go` around lines 71 - 89, The code reads the
user-supplied keyFile with os.ReadFile before validating and using the virtual
FS; change the logic so you call validate.SafeInputPath(keyFile) up front and
use vfs.ReadFile/vfs.WriteFile (not os.ReadFile) everywhere you touch keyFile;
update the existing-key branch to use vfs.ReadFile, trim/validate the hex length
(64) as before, and keep vfs.MkdirAll/vfs.WriteFile for creation so all file ops
go through the vfs and the path is validated before access.
- Replace os.ReadFile/WriteFile/ReadDir with vfs.* equivalents for test mockability, consistent with project coding guidelines. - Limit auth bridge request body to 64KB to prevent memory exhaustion. - Log errors in saveUserMap instead of silently discarding them. - Reject client keys that collide with the shared proxy key. - Reject duplicate client keys instead of silently overwriting. Signed-off-by: Gao Yang <grany@yeah.net> (topwin.tech)
- parseClientID: only accept "client_id" field, remove legacy fallback - loadClientKeys: scan all *.key (excluding proxy.key), no prefix required - Remove legacy file migration logic in newAuthBridge - Update flag description to reflect generic key scanning Signed-off-by: Gao Yang <grany@yeah.net> (topwin.tech)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #934 +/- ##
==========================================
+ Coverage 65.90% 66.78% +0.88%
==========================================
Files 518 564 +46
Lines 48834 52441 +3607
==========================================
+ Hits 32185 35024 +2839
- Misses 13882 14516 +634
- Partials 2767 2901 +134 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@1abdeb4a0fdfd5b1a34599c8cb4db20fb7a9ced8🧩 Skill updatenpx skills add hGrany/cli#feat/multi-client-identity-isolation -y -g |
Collaborator
|
hi @hGrany ： 1. Tests for the multi-key path The new HMAC multi-key logic is correct (
Without these the "isolation by HMAC" contract lives only in your head and the PR description. 2. Placement: extract to a dedicated example Right now Suggest extracting the new code into |
Address review feedback from sang-neo03: 1. Extract multi-client code into sidecar/server-multi-tenant-demo/, keeping server-demo as the minimal single-tenant reference. 2. Add unit tests for the isolation guarantee: - loadClientKeys: shared-key collision and duplicate keyHex are skipped - verifyWithClientKeys: correct client matched, unknown key rejected - loadUserMap/saveUserMap: round-trip persistence across restart 3. Cross-link READMEs between server-demo and server-multi-tenant-demo. Signed-off-by: Gao Yang <grany@yeah.net> (topwin.tech)
github-actions
Bot
added
size/XL
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
sidecar/server-multi-tenant-demo/main.go (1)
72-89: 💤 Low valueConsider validating user-supplied paths with
validate.SafeInputPath.The
--key-fileand--log-fileflags are user-supplied CLI arguments. Per coding guidelines, CLI arguments should be validated withvalidate.SafeInputPathbefore filesystem operations. However, since this is a demo server run by administrators on their own machines, the risk is limited.♻️ Optional: Add path validation
func run(ctx context.Context, listen, keyFile, keysDir, logFile, profile string) error { + // Validate user-supplied paths + if err := validate.SafeInputPath(keyFile); err != nil { + return fmt.Errorf("invalid --key-file: %w", err) + } + if logFile != "" { + if err := validate.SafeInputPath(logFile); err != nil { + return fmt.Errorf("invalid --log-file: %w", err) + } + } + if keysDir != "" { + if err := validate.SafeInputPath(keysDir); err != nil { + return fmt.Errorf("invalid --keys-dir: %w", err) + } + } + if v := os.Getenv(envvars.CliAuthProxy); v != "" {Add the import:
import "github.com/larksuite/cli/internal/validate"As per coding guidelines: "Validate paths before reading with
validate.SafeInputPathbecause CLI arguments are untrusted."Also applies to: 98-107
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sidecar/server-multi-tenant-demo/main.go` around lines 72 - 89, Validate the user-supplied path strings before any filesystem ops by calling validate.SafeInputPath on the CLI inputs (e.g., keyFile and logFile) at the start of the routine that handles key file setup; add the import for "github.com/larksuite/cli/internal/validate", call validate.SafeInputPath(keyFile) (and validate.SafeInputPath(logFile) where used), check and return the error if validation fails, then continue with the existing logic that uses keyDir, vfs.ReadFile, vfs.WriteFile, etc.; also apply the same SafeInputPath validation to the other path usage referenced around lines 98-107.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sidecar/server-multi-tenant-demo/handler_test.go`:
- Line 754: Check the return value of os.MkdirAll in the test fixture setup and
fail the test on error: replace the current unchecked call to
os.MkdirAll(filepath.Join(dir, "subdir.key"), 0755) with an error check (e.g.,
capture err := os.MkdirAll(...); if err != nil { t.Fatalf("failed to create
fixture dir %q: %v", filepath.Join(dir, "subdir.key"), err) } or use
require.NoError/alpha equivalent) so setup failures are reported immediately.
- Around line 414-432: In TestRun_RejectsSelfProxy, isolate CLI config state by
calling t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) at the start of the
test (before manipulating envvars.CliAuthProxy); keep the existing
os.LookupEnv/os.Setenv handling for envvars.CliAuthProxy and the run(...)
invocation unchanged, so the test uses a temporary CLI config directory and does
not leak or read global CLI state.
- Around line 381-409: The test depends on a real external host and an unbounded
http.Client; make it fully offline by replacing h.forwardCl with a deterministic
fake client before calling h.ServeHTTP: implement a short-timeout or a fake
RoundTripper that immediately returns a controlled response (e.g., 502) for any
request so the request passes the allowlist check but does not perform network
I/O. Update the proxyHandler instance in the test (the variable h) to set
h.forwardCl to that fake http.Client (or a client with Timeout set) so
signedReq/resign and h.ServeHTTP remain unchanged and the test is bounded and
offline.
In `@sidecar/server-multi-tenant-demo/README.md`:
- Around line 25-41: The Markdown fenced code blocks containing the ASCII
diagram and the keys file tree are missing language identifiers (triggering
MD040); update each triple-backtick fence around the diagram block (the ASCII
"Sidecar Server" diagram) and the keys list block (the keys/ tree and the
similar block at lines 76-82) to include a neutral language like text/plain or
text (e.g., change ``` to ```text) so markdownlint no longer flags them; ensure
you update all occurrences referenced in the README (the diagram block and both
keys blocks).
---
Nitpick comments:
In `@sidecar/server-multi-tenant-demo/main.go`:
- Around line 72-89: Validate the user-supplied path strings before any
filesystem ops by calling validate.SafeInputPath on the CLI inputs (e.g.,
keyFile and logFile) at the start of the routine that handles key file setup;
add the import for "github.com/larksuite/cli/internal/validate", call
validate.SafeInputPath(keyFile) (and validate.SafeInputPath(logFile) where
used), check and return the error if validation fails, then continue with the
existing logic that uses keyDir, vfs.ReadFile, vfs.WriteFile, etc.; also apply
the same SafeInputPath validation to the other path usage referenced around
lines 98-107.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 931060dc-04b6-4193-acb7-e16c585ba9b8
📒 Files selected for processing (9)
sidecar/server-demo/README.mdsidecar/server-multi-tenant-demo/README.mdsidecar/server-multi-tenant-demo/allowlist.gosidecar/server-multi-tenant-demo/audit.gosidecar/server-multi-tenant-demo/auth_bridge.gosidecar/server-multi-tenant-demo/forward.gosidecar/server-multi-tenant-demo/handler.gosidecar/server-multi-tenant-demo/handler_test.gosidecar/server-multi-tenant-demo/main.go
✅ Files skipped from review due to trivial changes (1)
- sidecar/server-demo/README.md
…t and client guide - Explain the multi-app credential isolation problem (app_secret must not be exposed to client environments) - Document typical deployment topology with multiple sidecar instances - Add complete client setup guide: env vars, multi-app switching, login flow, and end-to-end workflow example - Document design decisions and management endpoint details Signed-off-by: Gao Yang <grany@yeah.net> (topwin.tech)
- Make TestProxyHandler_AcceptsAllowedAuthHeaders fully offline by using httptest.NewTLSServer instead of depending on open.feishu.cn - Isolate TestRun_RejectsSelfProxy config state with t.Setenv and temp dirs - Check os.MkdirAll error in test fixture setup - Add language identifiers to fenced code blocks (MD040) - Validate user-supplied CLI paths with validate.SafeInputPath Signed-off-by: Gao Yang <grany@yeah.net> (topwin.tech)
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Collaborator
|
Thanks @hGrany — extraction to Before I can sign off though, the CodeRabbit findings need attention — I'd treat the following two as merge blockers: 1. DoS surface on the management plane — 2. Path traversal via The remaining items are smaller but worth folding into the same pass:
Once those are pushed I'll do another pass. Overall direction (HMAC-key-as-identity, no-fallback for unmapped clients, |
Author
|
Thanks @sang-neo03 — all seven items were already addressed in the last two commits (
Waiting for your next look. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When multiple CLI sandbox environments share a single sidecar instance, user tokens (UAT) are not isolated — the last user to log in overwrites previous users' tokens, causing identity cross-contamination (user A operates Feishu as user B).
This PR adds per-client HMAC key isolation to
server-demo, enabling each client environment to maintain its own Feishu identity.Changes
auth_bridge.gologin/poll/status) with explicitclientName → feishuOpenIdbinding and persistent mappinghandler.gomain.go--keys-dirflag for client key directory; reuses existingproxy.keyon restart instead of regenerating (fixes a race condition with multiple instances)Design Decisions
Why use HMAC key for client identification instead of a header field?
Why separate management-plane and data-plane keys?
proxy.key.Why no fallback when a client has no user mapping?
Why reuse
proxy.keyinstead of regenerating on each start?proxy.key. The last writer wins, leaving other instances with an in-memory key that doesn't match the file. Reusing the existing key eliminates this race.Backward Compatibility
sidecarpackage) is unchanged — no client-side (lark-cli-sidecar) changes needed.Risk Assessment
Signed-off-by: Gao Yang grany@yeah.net (topwin.tech)
Summary by CodeRabbit
New Features
Documentation
Tests