feat(resp): add AUTH command with --require-pass server password - #17
Conversation
|
Warning Review limit reached
Next review available in: 41 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 (7)
📝 WalkthroughWalkthroughChangesRESP authentication
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant RESPServer
participant connState
participant bcrypt
Client->>RESPServer: AUTH password
RESPServer->>connState: Check authentication state
RESPServer->>bcrypt: Verify password hash
bcrypt-->>RESPServer: Verification result
RESPServer->>connState: Mark connection authenticated
RESPServer-->>Client: +OK or authentication error
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
🧹 Nitpick comments (1)
internal/resp/server_test.go (1)
109-120: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
Engine.Stopinstead of deprecatedgnet.Stop.golangci-lint flags this call as SA1019 deprecated;
gnet.Stoponly shuts down the last-registered Engine for the address/protocol and can leak Engines whenWithReuseAddr/WithReusePortare enabled elsewhere. This mirrors an existing pattern already in the file, so no new risk is introduced here, but since the linter flags this specific new call, using thegnet.Enginereturned fromOnBootwould be more future-proof.🤖 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/resp/server_test.go` around lines 109 - 120, Update startServer to capture the gnet.Engine returned through the server’s OnBoot callback, then call that engine’s Stop method during cleanup instead of the deprecated gnet.Stop function. Preserve the existing timeout context and cleanup behavior.Source: Linters/SAST tools
🤖 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/resp/server.go`:
- Around line 331-342: Handle QUIT explicitly in the switch within
Server.dispatch, returning a successful Redis response instead of falling
through to the unknown-command default. Preserve its existing pre-auth allowance
and limit this change to the reply; do not add connection-closing behavior.
---
Nitpick comments:
In `@internal/resp/server_test.go`:
- Around line 109-120: Update startServer to capture the gnet.Engine returned
through the server’s OnBoot callback, then call that engine’s Stop method during
cleanup instead of the deprecated gnet.Stop function. Preserve the existing
timeout context and cleanup 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: eee48b22-9c97-43a6-a07a-59f7bdfe8c88
📒 Files selected for processing (7)
config/config.goconfig/config_test.gointernal/resp/benchmark_test.gointernal/resp/server.gointernal/resp/server_test.gointernal/shard/runner.goserver/server.go
Implements Redis-compatible AUTH for the RESP listener: - AUTH <password> and AUTH <username> <password> (only the implicit "default" user exists until ACLs land) - New --require-pass flag / TSD_REQUIRE_PASS env var; the password is bcrypt-hashed once at startup and compared only on AUTH, never on the hot path - Unauthenticated connections may only issue AUTH, PING, and QUIT; everything else returns -NOAUTH Authentication required - When no password is configured, AUTH replies +OK as a no-op and all connections start authenticated (zero overhead, backward compatible) - Failed AUTH attempts are logged at warn level with the remote address Closes Saxy#7 Signed-off-by: Faruk Can Özkan <ozkanfarukcan@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/resp/server_test.go (3)
126-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnchecked
Close()errors flagged by errcheck.Static analysis flags the unchecked return of
conn.Close()/conn2.Close()at these three new sites.🔧 Proposed fix
- defer conn.Close() + defer func() { _ = conn.Close() }()(apply analogously to
conn2.Close()at Line 147)Also applies to: 147-147, 156-156
🤖 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/resp/server_test.go` at line 126, Handle the return values from conn.Close() and conn2.Close() in the affected test cleanup sites, including the close at the later third location, so errcheck no longer reports unchecked errors. Use the existing test error-handling convention rather than silently discarding the results.Source: Linters/SAST tools
55-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOld pipeline test duplicates the new
startServer/expectReplyhelpers.
TestRESPServer_GetSetPingPipelinestill rolls its own inlineexpectclosure and manualgo ListenAndServe/defer gnet.Stopteardown (Lines 58-63, 68-81), duplicating the logic just extracted intostartServer/expectReply. Worth converging on the shared helpers now that they exist.♻️ Suggested refactor
func TestRESPServer_GetSetPingPipeline(t *testing.T) { - addr := freeAddr(t) - srv := NewServer(addr, newFakeStore(), nil, log.NewNoOpLogger(), nil, "") - go func() { _ = srv.ListenAndServe() }() - defer func() { - ctx, cancel := context.WithTimeout(context.Background(), time.Second) - defer cancel() - _ = gnet.Stop(ctx, "tcp://"+addr) - }() - + addr := startServer(t, "") conn := dialWithRetry(t, addr) defer conn.Close() - - expect := func(name, send, want string) { - t.Helper() - if _, err := conn.Write([]byte(send)); err != nil { - t.Fatalf("%s write: %v", name, err) - } - got := make([]byte, len(want)) - _ = conn.SetReadDeadline(time.Now().Add(2 * time.Second)) - if _, err := io.ReadFull(conn, got); err != nil { - t.Fatalf("%s read: %v", name, err) - } - if string(got) != want { - t.Fatalf("%s: got %q want %q", name, got, want) - } - } + expect := func(name, send, want string) { expectReply(t, conn, name, send, want) }Also applies to: 109-120
🤖 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/resp/server_test.go` around lines 55 - 91, Refactor TestRESPServer_GetSetPingPipeline to use the shared startServer setup and expectReply assertion helpers instead of its inline ListenAndServe/gnet.Stop lifecycle and expect closure. Preserve all existing PING, SET, GET hit/miss, and pipelined reply assertions and ordering.
109-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the server’s
Shutdownpath instead ofgnet.Stopin tests.
Serveralready captures thegnet.EngineinOnBootand definesShutdown(ctx)to callEngine.Stop(ctx), whilegnet.Stopis deprecated as a global shutdown API. Have the test helpers callsrv.Shutdowninstead of adding another global shutdown point, even though the currentListenAndServeoptions don’t trigger theWithReuseAddr/WithReusePortleak.🤖 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/resp/server_test.go` around lines 109 - 120, Update the startServer cleanup to call the server instance’s Shutdown method with the existing timeout context instead of gnet.Stop, using srv as the shutdown owner and preserving the current cleanup timing and error handling.Source: Linters/SAST tools
🤖 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/resp/server_test.go`:
- Around line 122-150: The TestRESPServer_AuthRequired coverage should also
verify unauthenticated QUIT handling. Add a QUIT request on an unauthenticated
connection and assert the documented Redis-style teardown response and
connection closure, using the existing dispatch/expectReply test helpers without
altering authentication coverage.
---
Nitpick comments:
In `@internal/resp/server_test.go`:
- Line 126: Handle the return values from conn.Close() and conn2.Close() in the
affected test cleanup sites, including the close at the later third location, so
errcheck no longer reports unchecked errors. Use the existing test
error-handling convention rather than silently discarding the results.
- Around line 55-91: Refactor TestRESPServer_GetSetPingPipeline to use the
shared startServer setup and expectReply assertion helpers instead of its inline
ListenAndServe/gnet.Stop lifecycle and expect closure. Preserve all existing
PING, SET, GET hit/miss, and pipelined reply assertions and ordering.
- Around line 109-120: Update the startServer cleanup to call the server
instance’s Shutdown method with the existing timeout context instead of
gnet.Stop, using srv as the shutdown owner and preserving the current cleanup
timing and error handling.
🪄 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: bfcd9b65-ceba-44a7-96bf-3b5d641ea5b7
📒 Files selected for processing (7)
config/config.goconfig/config_test.gointernal/resp/benchmark_test.gointernal/resp/server.gointernal/resp/server_test.gointernal/shard/runner.goserver/server.go
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/shard/runner.go
- config/config.go
- internal/resp/benchmark_test.go
- config/config_test.go
- server/server.go
- internal/resp/server.go
QUIT was allow-listed for unauthenticated connections but had no dispatch case, so it fell through to the unknown-command error. Implement it per Redis semantics: reply +OK, stop parsing pipelined commands, flush pending replies, and close the connection. Also replace the deprecated gnet.Stop with Server.Shutdown in the auth test helper. Addresses review feedback on Saxy#17. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Faruk Can Özkan <ozkanfarukcan@gmail.com>
QUIT was allow-listed for unauthenticated connections but had no dispatch case, so it fell through to the unknown-command error. Implement it per Redis semantics: reply +OK, stop parsing pipelined commands, flush pending replies, and close the connection. Also replace the deprecated gnet.Stop with Server.Shutdown in the auth test helper. Addresses review feedback on Saxy#17. Signed-off-by: Faruk Can Özkan <ozkanfarukcan@gmail.com>
|
Understood — removed the Co-authored-by trailers from both commits (history rewritten and force-pushed); DCO sign-offs are unchanged. Also applied the SetReadDeadline error-check suggestion in the QUIT test. |
2 participants
Description
Implements the Redis-compatible
AUTHcommand for the RESP listener, with a new--require-pass/TSD_REQUIRE_PASSserver password option.AUTH <password>(single-password mode, Redis < 6.0 compatible) andAUTH <username> <password>(ACL form — only the implicitdefaultuser exists until API Key System with Per-Key ACLs #9 lands)+OKon success,-ERR invalid passwordon failure, per the Redis AUTH specAUTH,PING, andQUIT; everything else gets-NOAUTH Authentication requiredAUTHis a+OKno-op and every connection starts authenticated — fully backward compatibleComponent: Networking/RESP, CLI
Type of Change:
Related Issue
Closes #7
Technical Deep Dive & Context
The implementation follows the touch points laid out in the issue:
bcrypt.GenerateFromPassword(stored as[]byteonServer);bcrypt.CompareHashAndPasswordruns only inside theAUTHhandler. GET/SET/DEL never touch bcrypt. bcrypt's cost factor also acts as a natural brute-force rate limit on AUTH attempts. No new dependency —golang.org/x/cryptois already a direct dependency (TLS, ChaCha20).connStategainsauthenticated bool(plusremoteAddr, captured once inOnOpenso the failed-AUTH log path allocates nothing per attempt). State initializes totruewhen no password is configured,falseotherwise. Works identically on the plaintext and TLS traffic paths since both funnel throughdispatch.--require-passunset the per-command cost is oneEqualFold(cmd, "AUTH")length-check-first comparison and a single bool test that short-circuits. Dispatch benchmarks confirm no measurable change and no new allocations (table below).AUTHis checked before the auth guard so unauthenticated clients can always authenticate; the guard then allow-listsPING/QUITper Redis semantics before the command switch.dispatchsignature changed fromdispatch(args, out)todispatch(st, args, out)to carry connection state; benchmarks updated accordingly.default(byte-equal, case-sensitive like Redis) with the same-ERR invalid passwordreply, so usernames aren't oracle-able. A real ACL system is deferred to API Key System with Per-Key ACLs #9.Performance & Benchmarks (If Applicable)
Workload:
go test -bench BenchmarkDispatch -benchmem -benchtime 3000000x -count 3(Apple M4 Pro, best of 3)No measurable change; zero new allocations on the hot path. (The 1 alloc on Set is the pre-existing fakeStore key copy in the benchmark harness, unchanged.)
How Has This Been Tested?
Written test-first (tests were red before the implementation existed):
TestRESPServer_AuthRequired— NOAUTH before auth, PING allowed pre-auth, wrong password rejected, correct password unlocks the connection, and auth state is per-connection (a second connection must re-authenticate)TestRESPServer_AuthWithUsername—AUTH default <pass>accepted, unknown username rejectedTestRESPServer_AuthNoPasswordConfigured— no-op+OKpath, commands work without authTestRESPServer_AuthWrongArity— arity errors for 1 and 4 args--require-passflag,TSD_REQUIRE_PASSenv var, and empty defaultgo test ./...,go test -race ./..., andgo vet ./...all pass locally with zero warnings.Checklist
go test ./...andgo test -race ./...)go vetwarnings🤖 Generated with Claude Code
Summary by CodeRabbit
--require-passorTSD_REQUIRE_PASS; when enabled, clients must authenticate withAUTHbefore other commands.AUTH <password>andAUTH default <password>, with appropriate handling for wrong credentials, wrong argument counts, and username-basedAUTH.PINGandQUITremain available before authentication.