feat(sidecar): add client key self-registration API#1357
Conversation
|
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 (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds per-client HMAC key self-registration to the multi-tenant demo server. Clients can register data-plane keys via a signed POST to ChangesPer-Client Key Registration System
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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 |
|
|
github-actions
Bot
added
the
size/L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.go`:
- Around line 149-162: Replace the read-then-write race with an atomic
create-or-check pattern: attempt to create the key file with exclusive create
semantics (O_CREATE|O_EXCL) via the VFS open API (use vfs.OpenFile with
os.O_CREATE|os.O_EXCL and mode 0600) and write keyHex; if the create succeeds,
hot-load the key (h.hotLoadClientKey) and return "registered"; if the create
fails with EEXIST, read the existing file (vfs.ReadFile) and compare to
keyHex—if equal hot-load and return "already_registered", otherwise return the
key_conflict error; ensure partial writes are cleaned up on failure and wrap
errors with context.
In `@sidecar/server-multi-tenant-demo/README.md`:
- Around line 261-264: The loop attempts to register with two sidecar instances
but call_sidecar reads only global SIDECAR_HOST/SIDECAR_PORT, so it ignores the
extra host/port args; update call_sidecar (the function named call_sidecar in
the README script) to accept and use positional parameters for host and port
when provided (falling back to global SIDECAR_HOST/SIDECAR_PORT if not passed),
or alternatively change the loop to export/override SIDECAR_HOST and
SIDECAR_PORT per iteration before calling call_sidecar; ensure the
implementation uses the passed host/port variables when constructing the request
so the POST targets both ports 16384 and 16385 as intended.
🪄 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: ba596bab-733e-4eb1-80bc-29a9d0e573fd
📒 Files selected for processing (5)
sidecar/server-multi-tenant-demo/README.mdsidecar/server-multi-tenant-demo/auth_bridge.gosidecar/server-multi-tenant-demo/handler.gosidecar/server-multi-tenant-demo/handler_test.gosidecar/server-multi-tenant-demo/main.go
When the sidecar's keys/ directory is a read-only host mount (common in container orchestrators and CI environments), operators cannot pre-create per-client key files for every new sandbox. This blocks fully automated provisioning and elastic scaling. Add a `/_sidecar/keys/register` management-plane endpoint so clients can dynamically register their own per-client HMAC keys. The client generates a random 64-char hex key locally (in a writable path such as ~/.lark-sidecar/client.key), then registers it with the sidecar via a single POST signed with the shared proxy.key. The server persists the key to disk and hot-loads it into memory — no restart needed. Changes: - auth_bridge.go: add keyRegistrar interface + handleRegisterKey handler + /_sidecar/keys/register route - handler.go: add validateClientID, registerClientKey (persist + hot- load), hotLoadClientKey; add regexp and encoding/hex imports - main.go: replace validate.SafeInputPath with charcheck-based validateSidecarPath that allows absolute server paths; reorder init so handler is created before authBridge (circular dep: authBridge needs keyRegistrar which is the handler) - handler_test.go: 4 new tests covering registration, idempotency, conflict detection, input validation, and full HTTP round-trip - README.md: document self-registration flow, add client-init.sh reference script, update architecture and end-to-end workflow sections; clarify multi-organization use case Signed-off-by: Gao Yang <grany@yeah.net> (topwin.tech)
01fff73 to
f02f411
Compare
- handler.go: replace read-then-write with atomic O_CREATE|O_EXCL to prevent TOCTOU race between concurrent key registrations; clean up partial writes on failure - README.md: fix multi-organization registration loop — call_sidecar reads SIDECAR_PORT from env, so override it per iteration instead of passing extra positional args that were silently ignored Signed-off-by: Gao Yang <gaoyang@topwin.tech> (topwin.tech)
|
@hGrany That said, this directory is currently a demo. It is better kept focused on I would prefer to keep the demo minimal and limited to the core example logic. So my suggestion is to close this PR and keep this capability out of the current demo. |
|
Thanks for the review @sang-neo03 — I understand the concern about keeping the demo minimal and focused. That said, I'd like to point out that this demo directory is, in practice, the only implementation of the multi-tenant sidecar. There's no separate "production" version — everyone deploying multi-tenant setups builds from I'd also appreciate your thoughts on #1367, which addresses a more fundamental issue: the demo silently falls back to Both PRs came from running the demo in production (Coder workspaces, 90+ containers). The demo's current state requires significant setup knowledge and manual pre-configuration to work correctly. These patches aim to make it more self-contained — closer to "clone, configure, run" rather than "clone, read source code, understand scope caching internals, manually create cache files, then run." If you'd prefer to keep #1357 out, I can accept that — key distribution is indeed deployment-specific. But #1367 is fixing a correctness issue in the demo itself: the |
4 participants
Summary
Adds
/_sidecar/keys/registermanagement-plane endpoint so clients can dynamically register their own per-client HMAC keys without server-side pre-provisioning.Problem
In containerized / sandboxed environments (e.g. Coder workspaces, CI runners), the sidecar's
keys/directory is typically a read-only host mount. Operators had to manually create per-client.keyfiles for every new sandbox before it could connect — this doesn't scale and blocks fully automated provisioning.Solution
Clients generate a random 64-char hex HMAC key locally (in their writable home directory), then register it with the sidecar via a single
POST /_sidecar/keys/registercall signed with the sharedproxy.key. The server:keys/{client_id}.keyon disk"registered"on first call,"already_registered"on idempotent re-registration, or409 Conflictif the client already has a different keyChanges
auth_bridge.gokeyRegistrarinterface +handleRegisterKeyhandler +/_sidecar/keys/registerroutehandler.govalidateClientID,registerClientKey(persist + hot-load),hotLoadClientKeymain.govalidate.SafeInputPathwithcharcheck-basedvalidateSidecarPaththat allows absolute server paths; reorder init to resolve circular dependency (handler ↔ authBridge)handler_test.goREADME.mdclient-init.shexample script, update architecture diagram and end-to-end workflow sectionsDesign decisions
proxy.keysigning scheme — no new auth mechanism neededloadClientKeys) continues to work for pre-provisioned keysTesting
Test plan
go test -tags authsidecar_multi_tenant_demo ./sidecar/server-multi-tenant-demo/ -v— all tests passgo build -tags authsidecar_multi_tenant_demo ./sidecar/server-multi-tenant-demo/— builds cleanlyMade with Cursor
Summary by CodeRabbit
New Features
Documentation
Tests
Bug Fixes / Validation