feat(network): #8 add AUTH handshake for binary protocol - #18
Conversation
Implements connection-level authentication for the binary protocol (port 9988) using the same --require-pass / TSD_REQUIRE_PASS config as the RESP AUTH command. - Add MsgAuth/MsgAuthOk/MsgAuthErr wire types with username+password payload format - bcrypt password hashing at startup, comparison on first AUTH only - connState gains authenticated flag (true when no password set) - Auth guard rejects non-MsgPing commands before authentication - Client.Auth() method for both internal/network and public client - Full test coverage: auth flow, wrong/correct password, username, no-password no-op, malformed payload, per-connection isolation - Add bench:go task to Taskfile for Go benchmark runs Signed-off-by: Maximilian Hagen <git@saxy.dev>
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds binary-protocol password authentication across protocol constants, clients, server connection handling, configuration wiring, and integration tests. It also adds a configurable Go benchmark task and updates constructor call sites. ChangesBinary protocol authentication
Go benchmark task
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ClientAPI
participant NetworkClient
participant NetworkServer
participant AuthWorker
participant ApplicationHandler
ClientAPI->>NetworkClient: Auth(password, scratchBuf)
NetworkClient->>NetworkServer: Send MsgAuth credentials
NetworkServer->>AuthWorker: Dispatch bcrypt verification
AuthWorker-->>NetworkServer: Write MsgAuthOk or MsgAuthErr
ClientAPI->>NetworkClient: Send application request
NetworkClient->>NetworkServer: Send authenticated frame
NetworkServer->>ApplicationHandler: Forward request
ApplicationHandler-->>ClientAPI: Return protocol response
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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/network/server_test.go (1)
168-326: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd end-to-end coverage for TLS authentication.
These tests exercise only plaintext connections, while TLS uses an independent authentication-gating branch. Add a TLS case covering pre-auth rejection and successful AUTH.
🤖 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/network/server_test.go` around lines 168 - 326, Extend the authentication tests with a TLS end-to-end case using the existing startAuthServer and connection helpers, configuring TLS and dialing through a TLS client. Verify a protected request before AUTH returns MsgAuthErr, then authenticate with the correct password and verify MsgAuthOk followed by a successful request, covering the TLS-specific authentication gate.
🤖 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/network/server_test.go`:
- Around line 56-58: Update the fake DELETE handler’s OpDelete case to remove
the key from the store rather than storing nil. Use the store’s existing
deletion operation, preserving the current successful response behavior.
In `@internal/network/server.go`:
- Around line 247-248: Update the MsgAuth handling in OnTraffic and processAuth
so bcrypt verification never runs synchronously on the gnet event loop; dispatch
authentication checks through a bounded worker pool or rate limiter, then return
results to the connection safely. Track failed AUTH attempts and enforce a limit
by throttling or closing clients after repeated failures, while preserving
successful authentication behavior for both plaintext and TLS paths.
---
Nitpick comments:
In `@internal/network/server_test.go`:
- Around line 168-326: Extend the authentication tests with a TLS end-to-end
case using the existing startAuthServer and connection helpers, configuring TLS
and dialing through a TLS client. Verify a protected request before AUTH returns
MsgAuthErr, then authenticate with the correct password and verify MsgAuthOk
followed by a successful request, covering the TLS-specific authentication gate.
🪄 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 Plus
Run ID: d2bcc9e3-a203-45cd-8d1b-78c122e81dde
📒 Files selected for processing (10)
Taskfile.ymlclient/client.gointernal/metrics/metrics_test.gointernal/network/benchmark_tls_test.gointernal/network/client.gointernal/network/protocol.gointernal/network/protocol_test.gointernal/network/server.gointernal/network/server_test.goserver/server.go
Move bcrypt.CompareHashAndPassword off the gnet event loop by dispatching AUTH verification through a bounded goroutine pool, preventing bcrypt's slow path from blocking all connections on the event-loop goroutine. Signed-off-by: Maximilian Hagen <git@saxy.dev>
…re closing channel Signed-off-by: Maximilian Hagen <git@saxy.dev>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/network/server.go (1)
272-301: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate AUTH-handling logic between TLS and plaintext paths.
The
msg.Type == MsgAuthblock (fast-OK on no password, username fast-reject,dispatchAuth+ pending handling) is duplicated almost verbatim inhandleDecryptedFrames(Lines 272-301) andonTrafficPlaintext(Lines 383-412), differing only in how the "pending" outcome is signaled to the caller. Extracting a shared helper (e.g.s.handleAuthMessage(msg) (payload []byte, respType MessageType, pending bool)) would prevent the two copies drifting apart on future auth changes (e.g. one path getting a fix the other misses).♻️ Suggested extraction shape
func (s *Server) tryAuth(c gnet.Conn, st *connState, msg *Message) (payload []byte, respType MessageType, pending, skipHandler bool) { if s.requirePassHash == nil { return ResponseOK, MsgAuthOk, false, true } username, password := parseAuthPayload(msg.Value) if len(username) > 0 && string(username) != "default" { return s.authFailed(st), MsgAuthErr, false, true } passwordCopy := make([]byte, len(password)) copy(passwordCopy, password) if s.dispatchAuth(c, passwordCopy) { st.authPending = true return nil, 0, true, true } return s.authFailed(st), MsgAuthErr, false, true }Both call sites then branch on
pendingto decide whether to advance/discard orbreak/return gnet.None.Also applies to: 383-412
🤖 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/network/server.go` around lines 272 - 301, Extract the duplicated AUTH processing from handleDecryptedFrames and onTrafficPlaintext into a shared Server helper that handles password bypass, username rejection, password copying, dispatchAuth, and authPending state, returning response values plus a pending indicator. Replace both inline msg.Type == MsgAuth branches with calls to this helper, preserving each caller’s existing pending control flow and skip-handler behavior.
🤖 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/network/server.go`:
- Around line 494-518: Update the AUTH success and failure response writes in
the surrounding authentication flow to capture and handle Write errors
consistently with the other response paths, closing the connection when a write
fails. Also record the bytes written in s.bytesWritten and the per-shard
AddBytesWritten metric for both MsgAuthOk and MsgAuthErr writes, preserving the
TLS-versus-plain connection selection.
- Around line 525-534: Update both authentication call sites using dispatchAuth
so a false result from worker-pool saturation is not passed to authFailed or
counted as a credential failure; instead use the existing retry/rejection
behavior appropriate for unavailable capacity. Also update parseAuthPayload to
distinguish malformed payloads from valid empty/default credentials, and
fast-reject malformed AUTH frames before dispatching bcrypt work while
preserving normal handling for valid credentials.
- Around line 500-519: The failed AUTH response path in the auth worker must
wake the connection so buffered frames are processed immediately. In the branch
handling failed attempts around st.authFails and st.closeAfterReply, call
c.Wake(nil) after writing the failure response when the connection is not being
closed; preserve the existing close behavior for st.closeAfterReply and match
the successful AUTH response path.
---
Nitpick comments:
In `@internal/network/server.go`:
- Around line 272-301: Extract the duplicated AUTH processing from
handleDecryptedFrames and onTrafficPlaintext into a shared Server helper that
handles password bypass, username rejection, password copying, dispatchAuth, and
authPending state, returning response values plus a pending indicator. Replace
both inline msg.Type == MsgAuth branches with calls to this helper, preserving
each caller’s existing pending control flow and skip-handler behavior.
🪄 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 Plus
Run ID: 7c61d266-4924-4e92-86dc-0bf71e3ca7b6
📒 Files selected for processing (1)
internal/network/server.go
Signed-off-by: Maximilian Hagen <git@saxy.dev>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/network/server.go (1)
486-488: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winGate the auth worker pool on
requirePass.
NewServercreatesauthJobsand starts the fourauthWorkergoroutines even whenrequirePassis empty, so an auth-disabled server still owns idle goroutines/channel infrastructure for its lifetime. Skip both allocations/spawns unlessrequirePass != "".🤖 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/network/server.go` around lines 486 - 488, Update NewServer and the authWorker startup path so authJobs is allocated and authWorker goroutines are spawned only when requirePass is non-empty; leave the existing worker behavior unchanged when password authentication is enabled.
🧹 Nitpick comments (1)
internal/network/server.go (1)
486-548: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrite-error handling, byte metrics, and post-failure Wake now resolve prior feedback.
authWorkernow checkswriteErrand closes on failure, updatess.bytesWritten/per-shard metrics for the AUTH response, and callsc.Wake(nil)uniformly after both success and failure paths (guarded bycloseAfterReply) — this addresses the previously flagged missing metrics/write-error handling and the missing wake-on-failure re-trigger.Separately: the failure branch (lines 507-518) re-implements
authFails++/ warn-log /closeAfterReplythreshold logic inline, whilehandleAuthMessage(Line 472) calls a separates.authFailed(st)helper that — per the change description — does the same accounting. Having two independent implementations of the same failure-counting/threshold policy risks them drifting (e.g., ifmaxAuthFailshandling or log fields are updated in one but not the other).♻️ Suggested consolidation
} else { - st.authFails++ - respPayload, respType = ResponseAuthErr, MsgAuthErr - if s.logger.Enabled(log.LevelWarn) { - s.logger.Log(log.LevelWarn, "network: failed AUTH attempt", - log.String("remote_addr", st.remoteAddr), - log.Int("attempts", st.authFails), - ) - } - if st.authFails >= maxAuthFails { - st.closeAfterReply = true - } + respPayload = s.authFailed(st) + respType = MsgAuthErr }🤖 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/network/server.go` around lines 486 - 548, Consolidate the failed-authentication handling in authWorker by reusing the existing authFailed helper instead of duplicating authFails incrementing, warning logs, and maxAuthFails closeAfterReply logic inline. Preserve the existing failure response selection and ensure authFailed remains the single source of truth for failure accounting and threshold behavior.
🤖 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/network/server.go`:
- Around line 375-382: Handle the error returned by c.Discard in the MsgAuth
dispatched branch of the server message loop, matching the existing
discard-error handling used near the other loop path: increment s.protocolErrors
and emit the corresponding warning when Discard fails, while preserving the
current return behavior.
---
Outside diff comments:
In `@internal/network/server.go`:
- Around line 486-488: Update NewServer and the authWorker startup path so
authJobs is allocated and authWorker goroutines are spawned only when
requirePass is non-empty; leave the existing worker behavior unchanged when
password authentication is enabled.
---
Nitpick comments:
In `@internal/network/server.go`:
- Around line 486-548: Consolidate the failed-authentication handling in
authWorker by reusing the existing authFailed helper instead of duplicating
authFails incrementing, warning logs, and maxAuthFails closeAfterReply logic
inline. Preserve the existing failure response selection and ensure authFailed
remains the single source of truth for failure accounting and threshold
behavior.
🪄 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 Plus
Run ID: 89ca737d-d155-4760-8dde-9714178a00a0
📒 Files selected for processing (1)
internal/network/server.go
Signed-off-by: Maximilian Hagen <git@saxy.dev>
1 participant
Component: Networking/RESP, Client SDK, Build/CI
Type of Change: New feature
Related Issue: Closes #8
Technical Deep Dive & Context:
The binary protocol AUTH handshake follows the same design as the RESP AUTH from #17:
--require-passis unset,requirePassHashisnilandconnState.authenticatedstarts astrue. The auth guard is a single boolean check on the hot path — no bcrypt, no allocations.NewServerhashes the password once withbcrypt.GenerateFromPassword. The hash is stored, never the plaintext.bcrypt.CompareHashAndPasswordruns only onMsgAuth, never on data commands.MsgAuthpayload is[2B usernameLen][username][2B passwordLen][password]. Parsed inprocessAuthby slicing directly into the gnet read buffer — zero copy, zero allocation.handleDecryptedFrames/onTrafficPlaintextbefore the handler callback, so the handler never sees unauthenticated requests (exceptMsgPing, which is allow-listed like Redis).AUTH <username> <password>is supported; only"default"user is accepted until an ACL system lands.Performance & Benchmarks:
No measurable change on the hot path. The
!st.authenticatedcheck is a single boolean load;MsgAuthis never sent on the hot path.How Has This Been Tested:
go test ./...— all packages passgo test -race ./...— all packages passtask bench:go PKG=./internal/network/...— benchmarks passTestServerAuthFlow,TestServerAuthNoPassword,TestServerAuthWithUsername,TestServerAuthMalformedPayloadChecklist:
go test ./...andgo test -race ./...)go vetwarningsSummary by CodeRabbit
Authflow that works for both plaintext and TLS.bench:gotask to run Go benchmarks with configurable target packages, benchmark pattern, and iteration count.