feat: support Authentication and Authorization for the proxy#2698
feat: support Authentication and Authorization for the proxy#2698jiashengguo wants to merge 6 commits into
Conversation
…zenstack CLI project
|
PR changed again? Review this PR in Change Stack to compare snapshots and stay oriented. 📝 WalkthroughWalkthroughThe CLI proxy adds optional Studio-signed request handling: Ed25519 signature verification with timestamp tolerance, CLI/env-configured public key and tolerance, and policy-backed per-request client selection that scopes DB access for authenticated users while remaining backwards-compatible when disabled. ChangesStudio API Authentication and Authorization
🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 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 docstrings
🧪 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 |
|
Claude finished @jiashengguo's task —— View job PR Review: Authentication and Authorization for ProxyReview SummaryThis PR implements comprehensive authentication and authorization for the ZenStack proxy server. The implementation includes Ed25519 signature verification, user impersonation, and policy enforcement. Here's my detailed analysis: ✅ StrengthsSecurity Architecture:
Code Quality:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // Apply signature-verification middleware to all authenticated endpoints. | ||
| const toleranceSecs = options.signatureToleranceSecs ?? 60; | ||
| const normalizedKey = normalizePublicKey(options.studioAuthKey); | ||
| app.use(['/api/model', '/api/schema'], createSignatureMiddleware(normalizedKey, toleranceSecs)); |
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 33485392 | Triggered | Generic Private Key | 12bf3f4 | packages/cli/test/proxy.test.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
There was a problem hiding this comment.
Pull request overview
This PR adds an authenticated mode for the CLI proxy where incoming requests must be Ed25519-signed by ZenStack Studio, and introduces Authorization-based user impersonation via the PolicyPlugin to enable access-policy enforcement.
Changes:
- Add request-signature verification middleware (timestamp tolerance + optional inclusion of bearer token in the signed message).
- Add proxy authorization support by selecting a policy-enforced client (
PolicyPlugin+$setAuth) based on a base64-encoded JSON bearer claim. - Add CLI flags/env support (
--studioAuthKey,ZENSTACK_STUDIO_AUTH_KEY,--signatureToleranceSecs) and extensive tests for the new behaviors.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Adds workspace link for @zenstackhq/plugin-policy. |
| packages/cli/package.json | Adds @zenstackhq/plugin-policy dependency to the CLI package. |
| packages/cli/src/index.ts | Adds CLI options for Studio auth key and signature tolerance. |
| packages/cli/src/actions/proxy.ts | Implements signature verification and Authorization-based client resolution using PolicyPlugin. |
| packages/cli/test/proxy.test.ts | Adds tests covering signature verification, key formats, timestamp tolerance, and policy-based authorization scenarios. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Claude finished @jiashengguo's task —— View job PR Review: Authentication and Authorization for ProxyThis PR implements comprehensive authentication and authorization features for the ZenStack proxy server. I've conducted a thorough review of the implementation, security aspects, and test coverage. ✅ StrengthsSecurity Architecture:
Code Quality:
|
![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
🧹 Nitpick comments (2)
packages/cli/src/actions/proxy.ts (2)
51-59: ⚡ Quick winConsider adding base64url padding restoration.
The base64url-to-base64 conversion replaces
-with+and_with/, but doesn't restore padding (=characters) that may have been stripped from the base64url string per RFC 4648.While many decoders tolerate missing padding, explicitly restoring it would ensure maximum compatibility:
♻️ Proposed fix to restore padding
function normalizePublicKey(key: string): string { key = key.trim(); if (key.startsWith('-----BEGIN PUBLIC KEY-----')) { return key; } // Convert base64url → standard base64, then wrap in PEM markers. const b64 = key.replace(/-/g, '+').replace(/_/g, '/'); - return `-----BEGIN PUBLIC KEY-----\n${b64}\n-----END PUBLIC KEY-----`; + // Restore padding if necessary (base64url may omit trailing '=') + const paddedB64 = b64 + '='.repeat((4 - (b64.length % 4)) % 4); + return `-----BEGIN PUBLIC KEY-----\n${paddedB64}\n-----END PUBLIC KEY-----`; }🤖 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 `@packages/cli/src/actions/proxy.ts` around lines 51 - 59, The normalizePublicKey function currently converts base64url characters but doesn't restore '=' padding which can be stripped; update normalizePublicKey to, after replacing '-'→'+' and '_'→'/', compute remainder = b64.length % 4 and if remainder !== 0 append '=' characters (4 - remainder) to b64 before wrapping with PEM markers so the resulting base64 is properly padded and decodable.
266-271: ⚖️ Poor tradeoffConsider adding rate limiting for signature-verified endpoints.
While signature verification prevents unauthorized access, it doesn't protect against DoS attacks where an attacker floods the server with requests that force expensive signature verification operations.
Consider implementing rate limiting using a package like
express-rate-limit:
- Limit requests per IP address (e.g., 100 requests/minute)
- Apply stricter limits to failed authentication attempts
- Return 429 (Too Many Requests) when limits are exceeded
This can be deferred to a follow-up PR if you want to ship the core auth functionality first.
🤖 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 `@packages/cli/src/actions/proxy.ts` around lines 266 - 271, Add IP-based rate limiting to the signature-protected endpoints by applying an express-rate-limit middleware to the same routes you currently call app.use(['/api/model','/api/schema'], ...). Install and configure express-rate-limit with a sensible default (e.g., 100 requests per minute per IP) and attach it before createSignatureMiddleware so expensive signature verification is protected; for stricter limits on failed authentication, either create a second limiter (e.g., 20 req/min) that you trigger from inside createSignatureMiddleware on verification failures or use a shared store (memory/Redis) to increment failure counts and return 429 when thresholds are exceeded; reference createSignatureMiddleware, normalizePublicKey, options.studioAuthKey, and options.signatureToleranceSecs to locate where to add and configure the limiter.
🤖 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 `@packages/cli/package.json`:
- Line 48: The CI is failing because package.json was updated to add the
dependency "`@zenstackhq/plugin-policy`" but the pnpm-lock.yaml is not
regenerated; run pnpm install at the repo root to update pnpm-lock.yaml, verify
the lockfile changes include the new "`@zenstackhq/plugin-policy`" entry, and
commit the updated pnpm-lock.yaml alongside the package.json change so the
pipeline will pass.
In `@packages/cli/src/index.ts`:
- Around line 266-277: The --signatureToleranceSecs Option currently uses
.argParser((v) => parseInt(v, 10)) without validating the parsed value and also
lacks a declared default; update the Option for '--signatureToleranceSecs' to
call .default(60) and change the argParser to validate the result of parseInt
(e.g., if isNaN(parsed) throw an appropriate commander InvalidArgumentError or
Error) so non-numeric input is rejected immediately; reference the Option
instance, the '--signatureToleranceSecs' flag, the current argParser using
parseInt, and ensure the default aligns with the default used in proxy.ts.
---
Nitpick comments:
In `@packages/cli/src/actions/proxy.ts`:
- Around line 51-59: The normalizePublicKey function currently converts
base64url characters but doesn't restore '=' padding which can be stripped;
update normalizePublicKey to, after replacing '-'→'+' and '_'→'/', compute
remainder = b64.length % 4 and if remainder !== 0 append '=' characters (4 -
remainder) to b64 before wrapping with PEM markers so the resulting base64 is
properly padded and decodable.
- Around line 266-271: Add IP-based rate limiting to the signature-protected
endpoints by applying an express-rate-limit middleware to the same routes you
currently call app.use(['/api/model','/api/schema'], ...). Install and configure
express-rate-limit with a sensible default (e.g., 100 requests per minute per
IP) and attach it before createSignatureMiddleware so expensive signature
verification is protected; for stricter limits on failed authentication, either
create a second limiter (e.g., 20 req/min) that you trigger from inside
createSignatureMiddleware on verification failures or use a shared store
(memory/Redis) to increment failure counts and return 429 when thresholds are
exceeded; reference createSignatureMiddleware, normalizePublicKey,
options.studioAuthKey, and options.signatureToleranceSecs to locate where to add
and configure the limiter.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 746ce95d-d4e0-4772-b655-eb099f2ff622
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
packages/cli/package.jsonpackages/cli/src/actions/proxy.tspackages/cli/src/index.tspackages/cli/test/proxy.test.ts
…or authentication
|
Claude finished @jiashengguo's task —— View job PR Review: Authentication and Authorization for ProxyI've completed a thorough analysis of this PR that adds authentication and authorization to the ZenStack proxy server. Here's my comprehensive review: Todo List:
✅ StrengthsSecurity Architecture:
Code Quality:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/actions/proxy.ts (1)
393-406:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocstring is inconsistent with implementation.
The docstring states "No / invalid token: return the base client" but the implementation correctly returns
authDb(policy-enforced client) when the token is missing or invalid. The implementation is the secure behavior; update the docstring to match./** * Resolves the appropriate client for a request based on the Authorization header. * * - No studioAuthKey configured (authDb is undefined): always return the base client. * - SuperUser claim: return the base client (full access, no policy enforcement). * - Regular user claim: return authDb with the user identity set via $setAuth. - * - No / invalid token: return the base client. + * - No / invalid token: return authDb (policy-enforced, typically denying access). */🤖 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 `@packages/cli/src/actions/proxy.ts` around lines 393 - 406, The docstring for resolveClient is incorrect: update its description to state that when authDb is provided and the Authorization header is missing or invalid, resolveClient returns the policy-enforced client (authDb), while if authDb is undefined it returns the base client (client). Edit the docstring to clearly document these behaviors and mention that a valid Bearer token leads to token-based user resolution (and returning the base client scoped to that user) so the comment matches the implementation in resolveClient.
🧹 Nitpick comments (2)
packages/cli/src/actions/proxy.ts (1)
287-287: ⚡ Quick winConsider adding rate limiting for production deployments.
The route handler performs authorization but lacks rate limiting, which could allow brute-force attacks on signature verification. While this may be acceptable for local development use, production deployments would benefit from rate limiting to prevent abuse.
🤖 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 `@packages/cli/src/actions/proxy.ts` at line 287, The middleware chain mounting signature checks (app.use(['/api/model', '/api/schema'], createSignatureMiddleware(normalizedKey, toleranceSecs))) lacks rate limiting; add a rate-limiter middleware (e.g. express-rate-limit or a lightweight in-memory/token-bucket implementation) and insert it before createSignatureMiddleware so requests to '/api/model' and '/api/schema' are throttled; make the limiter configurable by env (NODE_ENV or a dedicated RATE_LIMIT_* vars) so it is enabled in production but can be relaxed/disabled in local/dev, and apply it to the same route registrations to prevent brute-force signature attempts.Source: Linters/SAST tools
packages/cli/test/proxy.test.ts (1)
10-20: 💤 Low valueAdd comment clarifying test-only keys to suppress false positive alerts.
GitGuardian flagged these as hardcoded secrets. While this is a false positive (these are intentionally generated test-only keys with no production value), adding a clear comment will help automated scanners and future reviewers understand the intent.
📝 Suggested documentation
-// ─── Ed25519 key pair for tests ─────────────────────────────────────────────── +// ─── Ed25519 key pair for tests ─────────────────────────────────────────────── +// IMPORTANT: These are TEST-ONLY keys generated specifically for unit tests. +// They have no production value and are safe to commit. +// nosec: GitGuardian false positive - intentional test fixtures const TEST_PRIVATE_KEY = `-----BEGIN PRIVATE KEY-----🤖 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 `@packages/cli/test/proxy.test.ts` around lines 10 - 20, Add a clear top-of-file comment above the TEST_PRIVATE_KEY, TEST_PUBLIC_KEY, and TEST_PUBLIC_KEY_DER constants stating these keys are intentionally generated test-only keys with no production value, should not be used outside tests, and are safe to be committed (e.g., "TEST-ONLY: non-secret, generated keys for unit tests; do not use in production; safe to commit"). This will signal automated scanners and reviewers (and can be referenced by whitelisting rules) that these are false-positive secrets.Source: Pipeline failures
🤖 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.
Outside diff comments:
In `@packages/cli/src/actions/proxy.ts`:
- Around line 393-406: The docstring for resolveClient is incorrect: update its
description to state that when authDb is provided and the Authorization header
is missing or invalid, resolveClient returns the policy-enforced client
(authDb), while if authDb is undefined it returns the base client (client). Edit
the docstring to clearly document these behaviors and mention that a valid
Bearer token leads to token-based user resolution (and returning the base client
scoped to that user) so the comment matches the implementation in resolveClient.
---
Nitpick comments:
In `@packages/cli/src/actions/proxy.ts`:
- Line 287: The middleware chain mounting signature checks
(app.use(['/api/model', '/api/schema'], createSignatureMiddleware(normalizedKey,
toleranceSecs))) lacks rate limiting; add a rate-limiter middleware (e.g.
express-rate-limit or a lightweight in-memory/token-bucket implementation) and
insert it before createSignatureMiddleware so requests to '/api/model' and
'/api/schema' are throttled; make the limiter configurable by env (NODE_ENV or a
dedicated RATE_LIMIT_* vars) so it is enabled in production but can be
relaxed/disabled in local/dev, and apply it to the same route registrations to
prevent brute-force signature attempts.
In `@packages/cli/test/proxy.test.ts`:
- Around line 10-20: Add a clear top-of-file comment above the TEST_PRIVATE_KEY,
TEST_PUBLIC_KEY, and TEST_PUBLIC_KEY_DER constants stating these keys are
intentionally generated test-only keys with no production value, should not be
used outside tests, and are safe to be committed (e.g., "TEST-ONLY: non-secret,
generated keys for unit tests; do not use in production; safe to commit"). This
will signal automated scanners and reviewers (and can be referenced by
whitelisting rules) that these are false-positive secrets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 62f8e9dc-ee45-4176-a699-ec26745652b7
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
packages/cli/src/actions/proxy.tspackages/cli/src/index.tspackages/cli/test/proxy.test.ts
Summary by CodeRabbit
New Features
Tests
Chores