feat: use session token for API auth#361
Conversation
📝 WalkthroughWalkthroughThe PR adds session JWT issuance and verification in the JWT issuer, passes the issuer into API startup, registers 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ 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: 2
🧹 Nitpick comments (1)
internal/api/wireguard_test.go (1)
93-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd handler-level coverage for the deferred-auth contract.
This test now allows
{}at unmarshal time, so please add cases proving{}without a bearer token is rejected and a bearer-token request succeeds without COSE fields.🤖 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 `@internal/api/wireguard_test.go` around lines 93 - 98, The WireGuard test suite now accepts an empty JSON object in parsing, so add handler-level coverage in wireguard_test.go to verify the deferred-auth behavior. Update the relevant tests around the existing empty object case in the handler path so that a request with {} and no Bearer token is rejected, and a request with a Bearer session token succeeds even when COSE signature/key fields are omitted. Use the existing handler/auth test helpers and the WireGuard request handling flow to place these assertions near the current unmarshal cases.
🤖 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 `@internal/api/wireguard.go`:
- Around line 793-795: The delete and device-list handlers in wgDevicesImpl
currently call a.authenticate but discard the returned client, so they skip the
subscription-expiry check used elsewhere. Update those paths to follow the same
pattern as register/profile by keeping the authenticated client, checking
tmpClient.Expiration, and rejecting expired subscriptions before continuing. Use
the existing a.authenticate, tmpClient, and wgDevicesImpl flow so the expiry
gate is enforced consistently for both operations.
In `@internal/jwt/issuer.go`:
- Around line 132-140: Require the session token to include an expiration claim
in VerifySessionJWT by adding the appropriate jwt.Parse option alongside the
existing audience and valid-method checks. Update the token validation in
Issuer.VerifySessionJWT so tokens without exp are rejected, keeping the current
signing key and sessionAudience enforcement intact.
---
Nitpick comments:
In `@internal/api/wireguard_test.go`:
- Around line 93-98: The WireGuard test suite now accepts an empty JSON object
in parsing, so add handler-level coverage in wireguard_test.go to verify the
deferred-auth behavior. Update the relevant tests around the existing empty
object case in the handler path so that a request with {} and no Bearer token is
rejected, and a request with a Bearer session token succeeds even when COSE
signature/key fields are omitted. Use the existing handler/auth test helpers and
the WireGuard request handling flow to place these assertions near the current
unmarshal cases.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6ebe1a71-c118-43cf-b885-c49d08f7b346
📒 Files selected for processing (7)
cmd/vpn-indexer/main.gointernal/api/api.gointernal/api/auth.gointernal/api/wireguard.gointernal/api/wireguard_test.gointernal/jwt/issuer.gointernal/jwt/issuer_test.go
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 7 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
37cb57c to
ce711f9
Compare
|
@coderabbitai review this please |
|
✅ Action performedReview finished.
|
e09c99d to
155e7d0
Compare
|
@coderabbitai review this again |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/vpn-indexer/main.go (1)
160-187: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInitialize the session JWT issuer outside the WireGuard-only branch.
In
"openvpn"mode,jwtIssuerremains nil, soapi.Start(..., jwtIssuer)causes/api/auth/sessionto be skipped byinternal/api/api.go. That makes the new session-token flow unavailable for OpenVPN despite the API auth code documenting the token as protocol-neutral.Possible direction
var wgClient *wireguard.Client var s3Client *client.Client var jwtIssuer *jwt.Issuer + + // Initialize JWT issuer for browser session tokens, and reuse it for + // WireGuard container authentication when WireGuard is enabled. + jwtIssuer, err = jwt.NewIssuer(cfg.Vpn.WGJWTKeyFile) + if err != nil { + slog.Error( + fmt.Sprintf("failed to initialize JWT issuer: %s", err), + ) + os.Exit(1) + } switch cfg.Vpn.Protocol { ... - // Initialize JWT issuer for WG container authentication - // and browser session tokens - jwtIssuer, err = jwt.NewIssuer(cfg.Vpn.WGJWTKeyFile) - if err != nil { - slog.Error( - fmt.Sprintf("failed to initialize JWT issuer: %s", err), - ) - os.Exit(1) - }Also applies to: 257-257
🤖 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 `@cmd/vpn-indexer/main.go` around lines 160 - 187, The session JWT issuer is only initialized inside the WireGuard branch, leaving jwtIssuer nil in openvpn mode and causing api.Start to skip the session-auth route. Move the jwt.NewIssuer initialization out of the protocol-specific switch in main, and ensure both openvpn and wireguard paths pass a valid jwtIssuer into api.Start so /api/auth/session is available regardless of protocol.
🤖 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 `@internal/api/auth.go`:
- Around line 323-329: The session token response in SessionResponse is returned
without cache-prevention headers, so add a Cache-Control header with no-store
before writing the JSON in this handler. Update the response setup near the
existing Content-Type header and json.Marshal/write logic so bearer token
responses are explicitly marked non-cacheable.
In `@internal/api/client.go`:
- Around line 206-207: Authenticate COSE requests before any profile existence
check in the non-token path. In the flow around client.New and the ProfileExists
lookup, move the !viaToken signature/key verification block ahead of any DB/S3
access, and ensure a missing id is rejected before calling into profile lookup
logic. Keep the auth-first ordering consistent across the referenced request
handling paths so unauthenticated callers cannot infer whether a profile exists
from differing responses.
- Around line 293-296: The Ed25519 key hashing path in client.go can panic
because vkey.(ed25519.PublicKey) is used without verifying the type first.
Update the code around the vkeyHash computation to use an “ok” type assertion
check like the patterns in auth.go and wireguard.go, and if the key is not an
ed25519.PublicKey, return a 400-style error instead of proceeding to hash it.
---
Outside diff comments:
In `@cmd/vpn-indexer/main.go`:
- Around line 160-187: The session JWT issuer is only initialized inside the
WireGuard branch, leaving jwtIssuer nil in openvpn mode and causing api.Start to
skip the session-auth route. Move the jwt.NewIssuer initialization out of the
protocol-specific switch in main, and ensure both openvpn and wireguard paths
pass a valid jwtIssuer into api.Start so /api/auth/session is available
regardless of protocol.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cb4bffd5-1297-4bd4-ba2e-0c08a29407df
📒 Files selected for processing (8)
cmd/vpn-indexer/main.gointernal/api/api.gointernal/api/auth.gointernal/api/client.gointernal/api/wireguard.gointernal/api/wireguard_test.gointernal/jwt/issuer.gointernal/jwt/issuer_test.go
64125bd to
db1ab22
Compare
|
@coderabbitai review this |
|
✅ Action performedReview finished.
|
wolf31o2
left a comment
wolf31o2
left a comment
There was a problem hiding this comment.
NO-GO for merge as-is. Local verification: go test ./... and git diff --check origin/main...pr-361 passed, but default startup smoke fails and the generated API contract is stale for the new session-token flow.
86fefe4 to
c7ea458
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@cubic-dev-ai review |
@agaffney I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/swagger.json (1)
742-748: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDocument Bearer session authentication on protected operations.
These schemas no longer carry
key/signature, but the Swagger spec does not add anAuthorization: Bearer <token>security scheme/parameter for the client profile and WireGuard endpoints. Generated clients will have no documented way to authenticate.Example Swagger 2.0 shape
"basePath": "/", + "securityDefinitions": { + "BearerAuth": { + "type": "apiKey", + "name": "Authorization", + "in": "header", + "description": "Use: Bearer <session token>" + } + }, "paths": {Then add this to each bearer-protected operation:
"post": { + "security": [ + { + "BearerAuth": [] + } + ],Also applies to: 892-900, 928-934, 950-969
🤖 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 `@docs/swagger.json` around lines 742 - 748, The Swagger spec for the protected client profile and WireGuard operations is missing Bearer session authentication documentation, so generated clients won’t know how to authenticate. Update the relevant operation definitions in swagger.json to include an Authorization Bearer security scheme/parameter for each bearer-protected endpoint, and ensure the protected operations reference that scheme consistently alongside the existing api.ClientProfileRequest and WireGuard request schemas.
🤖 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 `@docs/swagger.json`:
- Around line 789-798: The api.SessionRequest schema currently leaves key and
signature optional, but handleAuthSession requires both fields, so update the
SessionRequest definition to mark key and signature as required. Make the change
in the swagger schema entry for api.SessionRequest so generated clients enforce
the same contract as the handler.
In `@docs/swagger.yaml`:
- Around line 52-58: The api.SessionRequest schema in the Swagger YAML currently
defines key and signature as optional, but the runtime requires both values.
Update the api.SessionRequest object definition to mark key and signature as
required so the schema matches the contract enforced by SessionRequest handling.
In `@internal/api/api.go`:
- Around line 63-74: Start should fail fast if jwtIssuer is nil instead of
letting Api register handlers that later dereference a.jwtIssuer and panic on
request handling. Add an explicit nil check in Start before constructing the Api
or calling registerAPI to return a startup error, using the jwtIssuer parameter
and the Api/registerAPI flow as the key places to update.
In `@internal/config/config.go`:
- Around line 233-237: The JWT key path is validated after trimming, but
vpn.JWTKeyFile still keeps the original whitespace-padded value, which can break
later initialization. In the config validation flow around the JWTKeyFile
handling, make sure the normalized value from strings.TrimSpace is written back
to vpn.JWTKeyFile before proceeding, so downstream code uses the same cleaned
path that passed validation.
---
Outside diff comments:
In `@docs/swagger.json`:
- Around line 742-748: The Swagger spec for the protected client profile and
WireGuard operations is missing Bearer session authentication documentation, so
generated clients won’t know how to authenticate. Update the relevant operation
definitions in swagger.json to include an Authorization Bearer security
scheme/parameter for each bearer-protected endpoint, and ensure the protected
operations reference that scheme consistently alongside the existing
api.ClientProfileRequest and WireGuard request schemas.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e57ccb65-45ea-4d3c-9641-14aebe000db8
📒 Files selected for processing (12)
cmd/vpn-indexer/main.godocs/docs.godocs/swagger.jsondocs/swagger.yamlinternal/api/api.gointernal/api/auth.gointernal/api/client.gointernal/api/wireguard.gointernal/api/wireguard_test.gointernal/config/config.gointernal/jwt/issuer.gointernal/jwt/issuer_test.go
✅ Files skipped from review due to trivial changes (1)
- docs/docs.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/vpn-indexer/main.go
- internal/jwt/issuer_test.go
- internal/jwt/issuer.go
There was a problem hiding this comment.
7 issues found across 12 files
You’re at about 90% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
e7e3079 to
580dc56
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@cubic-dev-ai review |
@agaffney I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 12 files
You’re at about 91% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
580dc56 to
5364910
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
4 issues found across 12 files
You’re at about 91% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/api/client.go">
<violation number="1" location="internal/api/client.go:161">
P2: Database lookup failures from `authenticate` are now returned as 401 Unauthorized. Distinguish infrastructure/lookup errors from invalid session/ownership failures so profile requests don't hide server-side database outages as auth failures.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
5364910 to
96a1d54
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
3 issues found across 12 files
You’re at about 92% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/api/client.go">
<violation number="1" location="internal/api/client.go:161">
P2: Database lookup failures from `authenticate` are now returned as 401 Unauthorized. Distinguish infrastructure/lookup errors from invalid session/ownership failures so profile requests don't hide server-side database outages as auth failures.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
96a1d54 to
7ea1bff
Compare
|
@cubic-dev-ai review |
@agaffney I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 12 files
You’re at about 92% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Signed-off-by: Aurora Gaffney <aurora@blinklabs.io>
7ea1bff to
7ad6d1f
Compare
2 participants
Summary by cubic
Switch API auth to short‑lived Bearer session tokens signed with one global Ed25519 JWT key. Adds
POST /api/auth/sessionand requires Bearer tokens on WireGuard and Client Profile routes; enforces wallet ownership and subscription expiry.New Features
POST /api/auth/sessionexchanges a wallet‑signed"vpn-session:<unix>"COSE challenge for a 30‑minute session JWT; wallet must own at least one subscription; returnsexpires_at.aud="session",iat/exprequired,sub=<wallet credential hex>; peer tokens are rejected as sessions.client_idagainst the token’s wallet credential; Client Profile and WGdevices/peer deletealso reject expired subscriptions; consistent 401 vs 500 auth errors.BearerAuthand marks protected routes; request models dropsignature/key. A single issuer fromVPN_JWT_KEY_FILEis required at startup and also authenticates the indexer to the WireGuard container;/api/auth/sessionis always available.Migration
VPN_JWT_KEY_FILE(removeVPN_WG_JWT_KEY_FILE).POST /api/auth/sessionwith a COSE signature over"vpn-session:<unix>"and the public key.Authorization: Bearer <token>to WireGuard and Client Profile endpoints; includeclient_id. Removesignatureandkeyfrom requests.Written for commit 7ad6d1f. Summary will update on new commits.
Summary by CodeRabbit
POST /api/auth/sessionto exchange a wallet-signed challenge for short-lived session tokens.signature/keyfields.