fix: constant-time auth check + uniform unknown-user handshake flow (#719) (#726)

* fix: constant-time auth + uniform unknown-user flow in standalone and child-worker paths (#719)

Two auth hardening fixes:

- server/conn.go (standalone handleStartup): the cleartext-password check
  used a plain non-constant-time string compare that also short-circuited
  on unknown users. Route it through the existing constant-time,
  existence-hiding auth.ValidateUserPassword helper, matching the
  control-plane and Flight SQL ingress paths.

- server/worker.go (process-isolation child worker): an unknown user got
  SQLSTATE 28P01 *before* the password request, while a known user got a
  password request first — a user-existence oracle visible in protocol
  flow, independent of timing. Always request the password, then validate
  via auth.ValidateUserPassword, so unknown-user and wrong-password are
  byte-for-byte indistinguishable to the client. The handshake is
  extracted into authenticateChildClient so it is unit-testable.

Defense in depth, not an emergency: the timing signal was already
practically unexploitable because the IP-ban rate limiter caps failed
attempts per source IP.

Fixes #719

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* test: use wire.MsgErrorResponse constant instead of 'E' literal

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

Author: fuziontech

server/conn.go

@@ -1269,9 +1269,8 @@ func (c *clientConn) handleStartup() error {
 	// Password is null-terminated
 	password := string(bytes.TrimRight(body, "\x00"))
 
-	// Validate password
-	expectedPassword, ok := c.server.cfg.Users[c.username]
-	if !ok || expectedPassword != password {
+	// Validate password (constant-time; does not leak whether the user exists)
+	if !auth.ValidateUserPassword(c.server.cfg.Users, c.username, password) {
 		// Record failed authentication attempt
 		banned := c.server.rateLimiter.RecordFailedAuth(c.conn.RemoteAddr())
 		if banned {

server/worker.go

@@ -127,6 +127,58 @@ func RunChildMode() {
 	os.Exit(exitCode)
 }
 
+// authenticateChildClient runs the cleartext-password handshake for a child
+// worker connection. The password is always requested before any credential
+// check, and validation is constant-time via auth.ValidateUserPassword, so an
+// unknown user and a wrong password are indistinguishable to the client in
+// both protocol flow and error shape. Returns ExitSuccess after sending
+// AuthOK, or the exit code to terminate with on failure.
+func authenticateChildClient(reader *bufio.Reader, writer *bufio.Writer, users map[string]string, username, remoteAddr string) int {
+	// Request password
+	if err := wire.WriteAuthCleartextPassword(writer); err != nil {
+		slog.Error("Failed to request password", "error", err)
+		return ExitError
+	}
+	if err := writer.Flush(); err != nil {
+		slog.Error("Failed to flush writer", "error", err)
+		return ExitError
+	}
+
+	// Read password response
+	msgType, body, err := wire.ReadMessage(reader)
+	if err != nil {
+		slog.Error("Failed to read password message", "error", err)
+		return ExitError
+	}
+
+	if msgType != wire.MsgPassword {
+		slog.Error("Expected password message", "got", string(msgType))
+		_ = wire.WriteErrorResponse(writer, "FATAL", "28000", "expected password message")
+		_ = writer.Flush()
+		return ExitError
+	}
+
+	// Password is null-terminated
+	password := string(bytes.TrimRight(body, "\x00"))
+
+	// Validate password (constant-time; does not leak whether the user exists)
+	if !auth.ValidateUserPassword(users, username, password) {
+		slog.Warn("Authentication failed", "user", username, "remote_addr", remoteAddr)
+		auth.AuthFailuresCounter.Inc()
+		_ = wire.WriteErrorResponse(writer, "FATAL", "28P01", "password authentication failed")
+		_ = writer.Flush()
+		return ExitAuthFailure
+	}
+
+	// Send auth OK
+	if err := wire.WriteAuthOK(writer); err != nil {
+		slog.Error("Failed to send auth OK", "error", err)
+		return ExitError
+	}
+
+	return ExitSuccess
+}
+
 // runChildWorker handles a single client connection in a child process.
 // Returns the appropriate exit code.
 func runChildWorker(tcpConn *net.TCPConn, cfg *ChildConfig) int {
@@ -220,54 +272,8 @@ func runChildWorker(tcpConn *net.TCPConn, cfg *ChildConfig) int {
 		return ExitError
 	}
 
-	// Look up expected password for this user
-	expectedPassword, ok := cfg.Users[username]
-	if !ok {
-		slog.Warn("Unknown user", "user", username, "remote_addr", cfg.RemoteAddr)
-		auth.AuthFailuresCounter.Inc()
-		_ = wire.WriteErrorResponse(writer, "FATAL", "28P01", "password authentication failed")
-		_ = writer.Flush()
-		return ExitAuthFailure
-	}
-
-	// Request password
-	if err := wire.WriteAuthCleartextPassword(writer); err != nil {
-		slog.Error("Failed to request password", "error", err)
-		return ExitError
-	}
-	if err := writer.Flush(); err != nil {
-		slog.Error("Failed to flush writer", "error", err)
-		return ExitError
-	}
-
-	// Read password response
-	msgType, body, err := wire.ReadMessage(reader)
-	if err != nil {
-		slog.Error("Failed to read password message", "error", err)
-		return ExitError
-	}
-
-	if msgType != wire.MsgPassword {
-		slog.Error("Expected password message", "got", string(msgType))
-		_ = wire.WriteErrorResponse(writer, "FATAL", "28000", "expected password message")
-		_ = writer.Flush()
-		return ExitError
-	}
-
-	// Password is null-terminated
-	password := string(bytes.TrimRight(body, "\x00"))
-	if password != expectedPassword {
-		slog.Warn("Authentication failed", "user", username, "remote_addr", cfg.RemoteAddr)
-		auth.AuthFailuresCounter.Inc()
-		_ = wire.WriteErrorResponse(writer, "FATAL", "28P01", "password authentication failed")
-		_ = writer.Flush()
-		return ExitAuthFailure
-	}
-
-	// Send auth OK
-	if err := wire.WriteAuthOK(writer); err != nil {
-		slog.Error("Failed to send auth OK", "error", err)
-		return ExitError
+	if exitCode := authenticateChildClient(reader, writer, cfg.Users, username, cfg.RemoteAddr); exitCode != ExitSuccess {
+		return exitCode
 	}
 
 	slog.Info("User authenticated", "user", username, "remote_addr", cfg.RemoteAddr)

server/worker_auth_test.go

@@ -0,0 +1,98 @@
+package server
+
+import (
+	"bufio"
+	"bytes"
+	"testing"
+
+	"github.com/posthog/duckgres/server/wire"
+)
+
+// passwordMessage builds a client PasswordMessage ('p') for the given password.
+func passwordMessage(t *testing.T, password string) []byte {
+	t.Helper()
+	var buf bytes.Buffer
+	if err := wire.WriteMessage(&buf, wire.MsgPassword, append([]byte(password), 0)); err != nil {
+		t.Fatalf("failed to build password message: %v", err)
+	}
+	return buf.Bytes()
+}
+
+// runChildAuth feeds clientBytes to authenticateChildClient and returns the
+// exit code plus everything the server wrote to the client.
+func runChildAuth(t *testing.T, users map[string]string, username string, clientBytes []byte) (int, []byte) {
+	t.Helper()
+	var out bytes.Buffer
+	reader := bufio.NewReader(bytes.NewReader(clientBytes))
+	writer := bufio.NewWriter(&out)
+	exitCode := authenticateChildClient(reader, writer, users, username, "test:5432")
+	if err := writer.Flush(); err != nil {
+		t.Fatalf("failed to flush writer: %v", err)
+	}
+	return exitCode, out.Bytes()
+}
+
+func TestAuthenticateChildClientSuccess(t *testing.T) {
+	users := map[string]string{"alice": "secret"}
+
+	exitCode, out := runChildAuth(t, users, "alice", passwordMessage(t, "secret"))
+	if exitCode != ExitSuccess {
+		t.Fatalf("expected ExitSuccess, got %d", exitCode)
+	}
+
+	reader := bytes.NewReader(out)
+	msgType, _, err := wire.ReadMessage(reader)
+	if err != nil || msgType != wire.MsgAuth {
+		t.Fatalf("expected cleartext password request, got %c (err %v)", msgType, err)
+	}
+	msgType, body, err := wire.ReadMessage(reader)
+	if err != nil || msgType != wire.MsgAuth {
+		t.Fatalf("expected AuthOK, got %c (err %v)", msgType, err)
+	}
+	if !bytes.Equal(body, []byte{0, 0, 0, 0}) {
+		t.Fatalf("expected AuthOK body, got %v", body)
+	}
+}
+
+// TestAuthenticateChildClientUnknownUserIndistinguishable is a regression test
+// for the user-existence oracle: the worker used to return 28P01 for unknown
+// users before requesting a password, while known users got a password request
+// first. Both failure paths must now be byte-for-byte identical to the client.
+func TestAuthenticateChildClientUnknownUserIndistinguishable(t *testing.T) {
+	users := map[string]string{"alice": "secret"}
+
+	unknownCode, unknownOut := runChildAuth(t, users, "mallory", passwordMessage(t, "secret"))
+	wrongPwCode, wrongPwOut := runChildAuth(t, users, "alice", passwordMessage(t, "wrong"))
+
+	if unknownCode != ExitAuthFailure {
+		t.Fatalf("unknown user: expected ExitAuthFailure, got %d", unknownCode)
+	}
+	if wrongPwCode != ExitAuthFailure {
+		t.Fatalf("wrong password: expected ExitAuthFailure, got %d", wrongPwCode)
+	}
+
+	// Same message flow, same error code/message shape — no existence oracle.
+	if !bytes.Equal(unknownOut, wrongPwOut) {
+		t.Fatalf("unknown-user and wrong-password byte streams differ:\nunknown:  %q\nwrong-pw: %q", unknownOut, wrongPwOut)
+	}
+
+	// Both flows: password request first, then a generic 28P01 error.
+	reader := bytes.NewReader(unknownOut)
+	msgType, _, err := wire.ReadMessage(reader)
+	if err != nil || msgType != wire.MsgAuth {
+		t.Fatalf("expected cleartext password request before failure, got %c (err %v)", msgType, err)
+	}
+	msgType, body, err := wire.ReadMessage(reader)
+	if err != nil || msgType != wire.MsgErrorResponse {
+		t.Fatalf("expected ErrorResponse, got %c (err %v)", msgType, err)
+	}
+	if !bytes.Contains(body, []byte("28P01")) {
+		t.Fatalf("expected SQLSTATE 28P01 in error, got %q", body)
+	}
+	if !bytes.Contains(body, []byte("password authentication failed")) {
+		t.Fatalf("expected generic auth failure message, got %q", body)
+	}
+	if bytes.Contains(body, []byte("mallory")) || bytes.Contains(body, []byte("alice")) {
+		t.Fatalf("error message must not echo the username, got %q", body)
+	}
+}