feat(resp): add STARTTLS connection upgrades - #24
Conversation
Keep implicit TLS as the default and reject plaintext pipelining across the RESP transport upgrade. Closes Saxy#15 Signed-off-by: Oghenefega Daniel Omajene <omajeneoghenefega11@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded opt-in RESP STARTTLS configuration. RESP connections can upgrade from plaintext to TLS 1.3 before authentication. The server rejects pipelined upgrades, preserves implicit TLS behavior, loads current TLS configuration, and includes end-to-end tests and documentation. ChangesRESP STARTTLS
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RESPClient
participant RESPServer
participant TLSConfigStore
RESPClient->>RESPServer: Send STARTTLS
RESPServer-->>RESPClient: Return plaintext +OK
RESPServer->>TLSConfigStore: Load current TLS configuration
RESPServer-->>RESPClient: Start TLS 1.3 handshake
RESPClient->>RESPServer: Complete handshake
RESPClient->>RESPServer: Send AUTH and commands
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 (3)
internal/resp/starttls_test.go (1)
119-152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for certificate rotation during the plaintext STARTTLS window.
upgradeToTLSininternal/resp/server.godeliberately callss.tlsConfigs.Load()at upgrade time rather than caching the value fromOnOpen, so that an idle plaintext connection observes a rotated certificate. This is called out explicitly inARCHITECTURE.mdand the PR objectives, but no test in this file connects, rotates theConfigStoreto a second certificate, then issuesSTARTTLSand verifies the client sees the new certificate. Add a test that swaps the store's config between connect and upgrade.🤖 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/starttls_test.go` around lines 119 - 152, Add a STARTTLS certificate-rotation test using startRESPTLSServer and its returned ConfigStore, keeping a plaintext connection open, replacing the store configuration with a second certificate before issuing STARTTLS, and asserting the client receives the rotated certificate rather than the original one.internal/resp/server.go (2)
381-415: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the handshake deadline into a shared constant.
upgradeToTLSrepeats the same10 * time.Secondliteral already used for implicit TLS inOnOpen(line 167). Extract one named constant so both call sites stay in sync if the timeout changes.♻️ Proposed fix
+const tlsHandshakeTimeout = 10 * time.Second + func (s *Server) upgradeToTLS(c gnet.Conn, st *connState, consumed int) gnet.Action { ... adapter := tlslib.NewGnetConnAdapter(c) st.tlsConn = tlslib.Server(adapter, tlsCfg) st.readBuf = make([]byte, 0, 4096) - st.handshakeDeadline = time.Now().Add(10 * time.Second) + st.handshakeDeadline = time.Now().Add(tlsHandshakeTimeout) st.upgradeTLS = false return gnet.None }Also update the implicit-TLS setup outside this range:
// OnOpen, line ~167 st.handshakeDeadline = time.Now().Add(tlsHandshakeTimeout)🤖 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.go` around lines 381 - 415, Define a shared named constant for the TLS handshake timeout and replace the 10-second literal in upgradeToTLS when assigning st.handshakeDeadline. Also update the implicit-TLS setup in OnOpen to use the same constant, keeping both handshake deadline paths synchronized.
421-500: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHoist the STARTTLS check to remove duplication.
s.startTLS && EqualFold(cmd, "STARTTLS")is checked twice: once pre-auth and once in thedefaultcase. SincedispatchSTARTTLSdoes not depend onst.authenticated, move the check above the!st.authenticatedgate and drop thedefault-case copy.♻️ Proposed fix
cmd := args[0] if EqualFold(cmd, shard.CmdAuth) { return s.auth(st, args, out) } // STARTTLS precedes the authentication gate so credentials can remain encrypted. + if s.startTLS && EqualFold(cmd, "STARTTLS") { + return s.dispatchSTARTTLS(st, args, out) + } if !st.authenticated { - if s.startTLS && EqualFold(cmd, "STARTTLS") { - return s.dispatchSTARTTLS(st, args, out) - } // Unauthenticated connections may only issue AUTH, PING, and QUIT (Redis semantics). if !EqualFold(cmd, shard.CmdPing) && !EqualFold(cmd, "QUIT") { return AppendError(out, "NOAUTH Authentication required") } } switch { ... default: - if s.startTLS && EqualFold(cmd, "STARTTLS") { - return s.dispatchSTARTTLS(st, args, out) - } return AppendError(out, "ERR unknown command '"+string(cmd)+"'") } }🤖 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.go` around lines 421 - 500, Update Server.dispatch to handle the s.startTLS && EqualFold(cmd, "STARTTLS") condition once before the !st.authenticated gate, then remove the duplicate STARTTLS branch from the switch default case while preserving dispatchSTARTTLS behavior for both authenticated and unauthenticated connections.
🤖 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 `@ARCHITECTURE.md`:
- Around line 160-173: Update the TLS rotation description in the “TLS Transport
and Certificate Rotation” section so RESP STARTTLS loads the shared TLS
configuration before flushing the plaintext +OK response, matching the ordering
in upgradeToTLS. Preserve the surrounding handshake and established-connection
behavior.
---
Nitpick comments:
In `@internal/resp/server.go`:
- Around line 381-415: Define a shared named constant for the TLS handshake
timeout and replace the 10-second literal in upgradeToTLS when assigning
st.handshakeDeadline. Also update the implicit-TLS setup in OnOpen to use the
same constant, keeping both handshake deadline paths synchronized.
- Around line 421-500: Update Server.dispatch to handle the s.startTLS &&
EqualFold(cmd, "STARTTLS") condition once before the !st.authenticated gate,
then remove the duplicate STARTTLS branch from the switch default case while
preserving dispatchSTARTTLS behavior for both authenticated and unauthenticated
connections.
In `@internal/resp/starttls_test.go`:
- Around line 119-152: Add a STARTTLS certificate-rotation test using
startRESPTLSServer and its returned ConfigStore, keeping a plaintext connection
open, replacing the store configuration with a second certificate before issuing
STARTTLS, and asserting the client receives the rotated certificate rather than
the original one.
🪄 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: a0a99f55-f70f-4b16-a43f-e8a7af16f2c1
⛔ Files ignored due to path filters (1)
.DS_Storeis excluded by!**/.DS_Store
📒 Files selected for processing (9)
ARCHITECTURE.mdREADME.mdROADMAP.mdconfig/config.goconfig/config_test.gointernal/resp/server.gointernal/resp/server_test.gointernal/resp/starttls_test.goserver/server.go
Share the TLS handshake timeout and correct the documented upgrade order. Signed-off-by: Oghenefega Daniel Omajene <omajeneoghenefega11@gmail.com>
Saxy
left a comment
Saxy
left a comment
There was a problem hiding this comment.
LGTM
just remove the DS_STORE
some nitpick:
The pipelining-guard edge case: a trailing incomplete command after STARTTLS (e.g. STARTTLS\r\n + *1\r\n$4\r\nPIN) isn't rejected by hasPipelinedSTARTTLS — the bytes cross into the TLS layer, the handshake fails, and the connection closes.
"ERR TLS not configured" is technically due to panic not reachable
not blocking
|
created follow up #25 |
Signed-off-by: Oghenefega Daniel Omajene <omajeneoghenefega11@gmail.com>
Signed-off-by: Oghenefega Daniel Omajene <omajeneoghenefega11@gmail.com>
2 participants
Description
Adds an explicit, opt-in STARTTLS upgrade path for RESP connections. When
--resp-starttls(orTSD_RESP_STARTTLS) is enabled, the RESP listener begins in plaintext, acceptsSTARTTLS, flushes a plaintext+OK, and then performs a TLS 1.3 handshake on the same socket.Existing behavior remains secure and backward compatible: implicit TLS is still the default when TLS material is configured, and the binary protocol continues to require TLS from the first byte. STARTTLS requires both
--tls-certand--tls-key, so an invalid deployment fails during configuration loading rather than accepting plaintext without an available upgrade.Component: Networking/RESP
Type of Change:
Related Issue
Closes #15
Technical Deep Dive & Context
Configuration and compatibility
--resp-starttlsandTSD_RESP_STARTTLS; both default to disabled.Connection upgrade
A successful
STARTTLScommand follows this sequence on the gnet event loop:+OKresponse.ConfigStorefor this upgrade.+OKto the socket.Loading
ConfigStorebefore writing+OK, rather than when the plaintext connection is accepted, means the server fails closed if no TLS configuration is available and an idle client receives the latest successfully rotated certificate when it eventually upgrades. Established TLS sessions retain their existing TLS state. Implicit TLS and STARTTLS share the same named 10-second handshake timeout so their deadline behavior cannot drift.STARTTLSis handled before the authentication gate, allowing clients to encrypt the connection before sending credentials. Authentication state is preserved across the transport transition. A repeated STARTTLS command on an encrypted connection returns-ERR connection is already encrypted.Plaintext boundary and pipelining
STARTTLS must be the only plaintext command in the current inbound buffer. The server preflights the buffer before dispatching any command and closes the connection when a valid STARTTLS frame has preceding or trailing plaintext bytes. This prevents commands in the same buffer from executing across the transport-security boundary. Rejected transitions increment the existing RESP protocol-error counter.
The preflight is guarded by the opt-in feature flag, so disabled deployments do not parse requests a second time. The normal authenticated
GETandSETdispatch path does not evaluate STARTTLS handling.Performance & Benchmarks
Workload:
Server.dispatchmicrobenchmarks for a GET hit and plain SET,darwin/arm64, Apple M3, five runs per benchmark. Baseline is merged upstream commitd51e187; before and after were run back-to-back from separate worktrees.No new allocations were introduced. GET was effectively unchanged; SET measured approximately 1 ns/op slower in this five-run microbenchmark. No end-to-end throughput or latency benchmark was run.
How Has This Been Tested?
The following commands passed locally:
The optional
taskCLI was not installed, so the documented direct Go equivalents were used for formatting, vetting, and race testing.Integration coverage uses a real standard-library TLS 1.3 client and generated test certificate. It verifies:
Checklist
go test ./...andgo test -race ./...)go vetwarningsThis change is opt-in and introduces no breaking behavior changes.
Summary by CodeRabbit
New Features
--resp-starttlsorTSD_RESP_STARTTLS.Documentation