feat: implemented dual authentication support for /satellites/sync endpoint#319
feat: implemented dual authentication support for /satellites/sync endpoint#319mahil-2040 wants to merge 2 commits intocontainer-registry:mainfrom
Conversation
…dpoint Signed-off-by: Mahil Patel <mahilpatel0808@gmail.com>
📝 WalkthroughWalkthroughAdds satellite authentication for POST /satellites/sync (SPIFFE/mTLS and Robot Account Basic Auth), a DB query to lookup robot accounts by name, middleware to validate and attach satellite identity to request context, route changes to apply middleware, handler checks to enforce identity matching, test helper, and conditional Basic Auth in the reporter. Changes
Sequence Diagram(s)sequenceDiagram
participant Satellite
participant GC_Router as GroundControl Router
participant Middleware as SatelliteAuthMiddleware
participant DB as Database
participant Handler as syncHandler
Satellite->>GC_Router: POST /satellites/sync (mTLS or Authorization header)
GC_Router->>Middleware: Pass request
alt SPIFFE/mTLS
Middleware->>Middleware: Extract SPIFFE ID from TLS/JWT
Middleware->>DB: Lookup satellite by SPIFFE ID
DB-->>Middleware: Satellite record
else Robot Basic Auth
Middleware->>Middleware: Parse Basic Auth (robotName, secret)
Middleware->>DB: GetRobotAccByName(robotName)
DB-->>Middleware: RobotAccount
Middleware->>Middleware: Verify secret hash
end
alt Auth success
Middleware->>Middleware: Attach satellite_name & satellite_id to context
Middleware->>Handler: Forward request
Handler->>Handler: GetSatelliteNameFromContext()
Handler->>Handler: Compare req.Name (if provided) to authenticated name
alt Names match
Handler->>DB: Fetch satellite details / process sync
DB-->>Handler: OK
Handler->>Satellite: 200 OK
else Name mismatch
Handler->>Satellite: 403 Forbidden
end
else Auth failure
Middleware->>Satellite: 401 Unauthorized
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
Codacy's Analysis Summary2 new issues (≤ 0 issue) Review Pull Request in Codacy →
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ground-control/internal/server/middleware.go`:
- Around line 117-138: The sync endpoint is missing rate limiting which allows
repeated Basic Auth attempts that trigger crypto.VerifySecret and DB lookups via
GetRobotAccByName; update the router setup so the syncRouter (in routes.go where
/sync is mounted) applies the existing RateLimitMiddleware (the same middleware
used for /ztr and /spiffe-ztr) so requests to the sync subrouter are
rate-limited, preventing brute-force CPU/DB exhaustion—ensure you attach
RateLimitMiddleware to the syncRouter definition before registering its
handlers.
🧹 Nitpick comments (4)
internal/state/reporting_process.go (1)
126-133: Log a warning when credentials are absent in non-SPIFFE mode.If
GetSourceRegistryCredentials()returns empty credentials, the request silently proceeds without authentication and will be rejected with 401 by the server. Adding a warning log here would help operators diagnose authentication failures.💡 Proposed improvement
// Add authentication header for non-SPIFFE mode // SPIFFE mode uses mTLS for authentication, so no header is needed if s.spiffeClient == nil { creds := s.cm.GetSourceRegistryCredentials() if creds.Username != "" && creds.Password != "" { httpReq.SetBasicAuth(creds.Username, creds.Password) + } else { + logger.FromContext(ctx).Warn().Msg("No credentials available for satellite authentication, request may be rejected") } }ground-control/internal/server/satellite_handlers.go (1)
570-595: RedundantGetSatelliteByNamelookup — the middleware already resolved the satellite.
SatelliteAuthMiddlewarealready verifies the satellite exists and stores its ID viasatelliteIDKeyin the context. The handler re-queries the same satellite by name on Line 587, producing an unnecessary DB round-trip on every sync request. You can use the ID already in context:♻️ Suggested refactor
// Get authenticated satellite name from middleware context satelliteName, ok := GetSatelliteNameFromContext(r.Context()) if !ok { // This should never happen if middleware is properly applied log.Println("syncHandler: no authenticated satellite in context") HandleAppError(w, &AppError{Message: "authentication required", Code: http.StatusUnauthorized}) return } + satelliteID, ok := GetSatelliteIDFromContext(r.Context()) + if !ok { + log.Println("syncHandler: no authenticated satellite ID in context") + HandleAppError(w, &AppError{Message: "authentication required", Code: http.StatusUnauthorized}) + return + } // Validate that request body name matches authenticated identity (if provided) // This prevents a satellite from submitting status on behalf of another satellite if req.Name != "" && req.Name != satelliteName { log.Printf("syncHandler: name mismatch - authenticated=%s, requested=%s", satelliteName, req.Name) HandleAppError(w, &AppError{Message: "satellite name mismatch", Code: http.StatusForbidden}) return } - sat, err := s.dbQueries.GetSatelliteByName(r.Context(), satelliteName) - if err != nil { - log.Printf("Unknown satellite: %s", satelliteName) - HandleAppError(w, &AppError{ - Message: "unknown satellite entity", - Code: http.StatusForbidden, - }) - return - }Then replace
sat.IDwithsatelliteIDin the downstream calls (Lines 638, 656).ground-control/internal/server/middleware.go (2)
88-99: SPIFFE auth path: a DB-registered satellite name that later gets deleted could cause a brief inconsistency.If a satellite is deleted from the database between the middleware's
GetSatelliteByNamecheck (here) and the handler's use of the satellite ID from context, the handler could fail with a confusing error. This is a narrow TOCTOU window and unlikely in practice — just worth noting for awareness. The defensiveGetSatelliteByNamein the handler (which I've suggested removing in my other comment) actually provides a secondary check against this.
118-137: Minor: failed auth attempts log the robot username, which is acceptable but consider log volume.Lines 132 and 135 log every failed robot credential attempt including the username. Under sustained attack, this could generate significant log volume. If log ingestion costs are a concern, consider rate-limiting these log lines or downgrading to debug level.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 20 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="ground-control/internal/server/routes.go">
<violation number="1" location="ground-control/internal/server/routes.go:90">
P1: The sync endpoint should have rate limiting applied to prevent brute-force attacks on robot credentials. The `SatelliteAuthMiddleware` performs CPU-intensive argon2 password verification via `crypto.VerifySecret`, and without rate limiting, an attacker could exhaust server resources by repeatedly sending invalid credentials. Consider adding `RateLimitMiddleware` before `SatelliteAuthMiddleware`, consistent with how `/ztr` and `/spiffe-ztr` endpoints are protected.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Signed-off-by: Mahil Patel <mahilpatel0808@gmail.com>
|
@bupd Can you please review this PR? |
1 participant
Description
This PR secures the
/satellites/syncendpoint by implementing dual authentication support:Previously, this endpoint was publicly accessible, allowing any client to submit status reports for any satellite name—a security vulnerability.
Changes
Ground Control
middleware.goSatelliteAuthMiddlewarewith dual auth logicroutes.go/satellites/syncendpointsatellite_handlers.gosyncHandlerto use authenticated contextrobot_accounts.sqlGetRobotAccByNamequerycached_images_test.goSatellite
reporting_process.goBreaking Change
/satellites/syncnow require authentication.Existing satellites will continue to work if they have valid robot credentials from ZTR or SPIFFE configured.
Testing
Screenshot
Unauthenticated Request Rejected (401)
The endpoint now correctly rejects unauthenticated requests:
A 200 OK response requires a registered satellite with valid robot credentials from ZTR, which can be verified in a full e2e test environment.
Additional context
Summary by cubic
Secured the /satellites/sync endpoint with dual auth (SPIFFE/mTLS or robot Basic Auth) and added rate limiting to prevent abuse. The satellite client now sends Basic Auth when SPIFFE isn’t used.
New Features
Migration
Written for commit d10dd6f. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests
Chores