From bc8727d0914068ffe56c7dd9e96909b550dfc464 Mon Sep 17 00:00:00 2001 From: Shivam Maurya Date: Wed, 14 Oct 2026 03:13:21 +0530 Subject: [PATCH 1/2] fix(ldap): guard shared connection with a mutex during rebind The LDAP service shares a single connection between authentication, directory searches and the connection heartbeat. The heartbeat's rebind path closed the old connection unconditionally, so an authentication that was mid-bind could have the socket dropped underneath it, failing with `LDAP Result Code 200 "Network Error": ldap: response channel closed`. Add a sync.RWMutex around all use of the shared connection: - Authenticate() performs the user bind and the service-account rebind atomically while holding the lock, so the connection is never left bound as an unprivileged user and cannot be swapped mid-authentication. - reconnect() dials the replacement before touching shared state, then swaps and closes the old connection under the lock; it therefore waits for any in-flight authentication to finish instead of interrupting it. - connect()/heartbeat() take the same lock (heartbeat releases it before returning so a following reconnect can acquire it). Closes #1191 --- internal/service/auth_service.go | 21 +- internal/service/ldap_service.go | 144 +++++--- internal/service/ldap_service_race_test.go | 204 ++++++++++++ internal/service/ldap_service_test.go | 368 +++++++++++++++++++++ 4 files changed, 670 insertions(+), 67 deletions(-) create mode 100644 internal/service/ldap_service_race_test.go create mode 100644 internal/service/ldap_service_test.go diff --git a/internal/service/auth_service.go b/internal/service/auth_service.go index 2c7b05a2..8c7ccc2a 100644 --- a/internal/service/auth_service.go +++ b/internal/service/auth_service.go @@ -203,23 +203,10 @@ func (auth *AuthService) CheckUserPassword(search model.UserSearch, password str return bcrypt.CompareHashAndPassword([]byte(user.Password), []byte(password)) case model.UserLDAP: if auth.ldap != nil { - defer func() { - bindErr := auth.ldap.BindService(true) - if bindErr != nil { - if err != nil { - err = fmt.Errorf("failed to rebind to ldap service account: %w, original error: %w", bindErr, err) - return - } - err = fmt.Errorf("failed to rebind to ldap service account: %w", bindErr) - } - }() - - err = auth.ldap.Bind(search.Username, password) - if err != nil { - return fmt.Errorf("failed to bind to ldap user: %w", err) - } - - return nil + // Authenticate performs the user bind and the service-account + // rebind atomically under the LDAP service lock, so the heartbeat + // cannot drop the connection mid-authentication (issue #1191). + return auth.ldap.Authenticate(search.Username, password) } default: return errors.New("unknown user search type") diff --git a/internal/service/ldap_service.go b/internal/service/ldap_service.go index 6f30b01d..fed7b8a5 100644 --- a/internal/service/ldap_service.go +++ b/internal/service/ldap_service.go @@ -122,13 +122,36 @@ func NewLdapService(i LdapServiceInput) (*LdapService, error) { return ldap, nil } +// connect establishes a new connection, binds it to the service account and +// makes it the shared connection. func (ldap *LdapService) connect() (*ldapgo.Conn, error) { + conn, err := ldap.dial() + if err != nil { + return nil, err + } + ldap.mutex.Lock() defer ldap.mutex.Unlock() - var conn *ldapgo.Conn - var err error + if err := ldap.bindConn(conn); err != nil { + conn.Close() + return nil, err + } + + // Swap the connection while holding the lock so concurrent readers never + // observe an unbound or closed connection. + old := ldap.conn + ldap.conn = conn + if old != nil { + old.Close() + } + + return ldap.conn, nil +} +// dial opens a new connection to the LDAP server without touching any shared +// state, so it is safe to call while the service lock is held. +func (ldap *LdapService) dial() (*ldapgo.Conn, error) { // TODO: There's also STARTTLS (or SASL)-based mTLS authentication // scenario, where we first connect to plain text port (389) and // continue with a STARTTLS negotiation: @@ -136,27 +159,32 @@ func (ldap *LdapService) connect() (*ldapgo.Conn, error) { // 2. conn.StartTLS(tlsConfig) // 3. conn.externalBind() if ldap.cert != nil { - conn, err = ldapgo.DialURL(ldap.config.LDAP.Address, ldapgo.DialWithTLSConfig(&tls.Config{ + return ldapgo.DialURL(ldap.config.LDAP.Address, ldapgo.DialWithTLSConfig(&tls.Config{ MinVersion: tls.VersionTLS12, Certificates: []tls.Certificate{*ldap.cert}, })) - } else { - conn, err = ldapgo.DialURL(ldap.config.LDAP.Address, ldapgo.DialWithTLSConfig(&tls.Config{ - InsecureSkipVerify: ldap.config.LDAP.Insecure, - MinVersion: tls.VersionTLS12, - })) - } - if err != nil { - return nil, err } - ldap.conn = conn + return ldapgo.DialURL(ldap.config.LDAP.Address, ldapgo.DialWithTLSConfig(&tls.Config{ + InsecureSkipVerify: ldap.config.LDAP.Insecure, + MinVersion: tls.VersionTLS12, + })) +} - err = ldap.BindService(false) - if err != nil { - return nil, err +// bindConn binds an already established connection as the LDAP service +// account (or externally/anonymous when configured). Callers must hold the +// service lock. +func (ldap *LdapService) bindConn(conn *ldapgo.Conn) error { + if ldap.cert != nil { + return conn.ExternalBind() } - return ldap.conn, nil + + // attempt unauthenticated/anonymous bind if both BindDN and bindpw are unset + if ldap.config.LDAP.BindDN == "" && ldap.bindPw == "" { + return conn.UnauthenticatedBind("") + } + + return conn.Bind(ldap.config.LDAP.BindDN, ldap.bindPw) } func (ldap *LdapService) GetUserInfo(username string) (*UserInfoResult, error) { @@ -255,35 +283,9 @@ func (ldap *LdapService) GetUserGroups(userDN string) ([]string, error) { return groups, nil } -func (ldap *LdapService) BindService(rebind bool) error { - // Locks must not be used for initial binding attempt - if rebind { - ldap.mutex.Lock() - defer ldap.mutex.Unlock() - } - - if ldap.cert != nil { - return ldap.conn.ExternalBind() - } - - // attempt unauthenticated/anonymous bind if both BindDN and bindpw are unset - if ldap.config.LDAP.BindDN == "" && ldap.bindPw == "" { - return ldap.conn.UnauthenticatedBind("") - } - - return ldap.conn.Bind(ldap.config.LDAP.BindDN, ldap.bindPw) -} - -func (ldap *LdapService) Bind(userDN string, password string) error { - ldap.mutex.Lock() - defer ldap.mutex.Unlock() - err := ldap.conn.Bind(userDN, password) - if err != nil { - return err - } - return nil -} - +// heartbeat probes the shared connection with a root search. The lock is +// released before returning so a reconnect triggered by a failure can +// immediately acquire it. func (ldap *LdapService) heartbeat() error { ldap.log.App.Debug().Msg("Performing LDAP connection heartbeat") @@ -296,8 +298,8 @@ func (ldap *LdapService) heartbeat() error { ) ldap.mutex.Lock() - defer ldap.mutex.Unlock() _, err := ldap.conn.Search(searchRequest) + ldap.mutex.Unlock() if err != nil { return err } @@ -306,6 +308,33 @@ func (ldap *LdapService) heartbeat() error { return nil } +// Authenticate binds the connection as userDN and then restores the +// service-account bind, all while holding the lock. This keeps the shared +// connection from ever being left bound as an unprivileged user and stops the +// heartbeat from dropping it in the middle of an authentication (issue #1191). +func (ldap *LdapService) Authenticate(userDN string, password string) error { + ldap.mutex.Lock() + defer ldap.mutex.Unlock() + + // Always restore the service-account bind, even when the user bind fails, + // so the shared connection is never left bound as an unprivileged user. + bindErr := ldap.conn.Bind(userDN, password) + rebindErr := ldap.bindConn(ldap.conn) + + if rebindErr != nil { + if bindErr != nil { + return fmt.Errorf("failed to rebind to ldap service account: %w, original error: %w", rebindErr, bindErr) + } + return fmt.Errorf("failed to rebind to ldap service account: %w", rebindErr) + } + + if bindErr != nil { + return fmt.Errorf("failed to bind to ldap user: %w", bindErr) + } + + return nil +} + func (ldap *LdapService) reconnect(interval time.Duration) error { ldap.log.App.Info().Msg("Attempting to reconnect to LDAP server") @@ -316,13 +345,28 @@ func (ldap *LdapService) reconnect(interval time.Duration) error { exp.Reset() operation := func() (*ldapgo.Conn, error) { - if ldap.conn != nil { - ldap.conn.Close() - } - conn, err := ldap.connect() + // Dial the replacement before touching any shared state, then swap + // under the lock. In-flight authentications therefore always run + // against a bound connection and never see a closed one. + conn, err := ldap.dial() if err != nil { return nil, err } + + ldap.mutex.Lock() + defer ldap.mutex.Unlock() + + if err := ldap.bindConn(conn); err != nil { + conn.Close() + return nil, err + } + + old := ldap.conn + ldap.conn = conn + if old != nil { + old.Close() + } + return conn, nil } diff --git a/internal/service/ldap_service_race_test.go b/internal/service/ldap_service_race_test.go new file mode 100644 index 00000000..a453775c --- /dev/null +++ b/internal/service/ldap_service_race_test.go @@ -0,0 +1,204 @@ +package service + +import ( + "strings" + "sync" + "testing" + "time" + + ldapgo "github.com/go-ldap/ldap/v3" + "github.com/stretchr/testify/require" +) + +// ldapClosedConnErr is the error the go-ldap client returns when a request is +// in flight while the underlying connection is closed by another goroutine. +// It is the exact failure described in issue #1191: the rebind/heartbeat drops +// the connection mid-authentication. +const ldapClosedConnErr = "response channel closed" + +// TestReconnectWaitsForInFlightAuthentication is the core regression test for +// #1191. +// +// It holds a user bind (the authentication step) open on the shared +// connection, then races a reconnect against it. With the mutex in place the +// reconnect blocks until the authentication has released the lock, so the +// in-flight bind completes against a live connection. +// +// Before the fix the rebind/heartbeat closed the shared connection while the +// authentication was still running, and the bind failed with +// `ldap: response channel closed` — exactly the failure in the issue. +func TestReconnectWaitsForInFlightAuthentication(t *testing.T) { + s := newFakeLDAPServer(t) + + // Construct the service first (this performs the initial service bind). + svc := newTestLdapService(t, s.addr()) + + // Now hold subsequent binds open until released, and signal when a bind + // has started. + bindStarted := make(chan struct{}) + release := make(chan struct{}) + var once sync.Once + s.setBlockBind(func() { + once.Do(func() { close(bindStarted) }) + <-release + }) + + authErr := make(chan error, 1) + go func() { + authErr <- svc.Authenticate("uid=alice,dc=example,dc=com", "secret") + }() + + // Wait until the user bind is actually in flight on the server side. + select { + case <-bindStarted: + case <-time.After(5 * time.Second): + t.Fatal("bind never reached the server") + } + + // Trigger the rebind path while the bind is in flight. With the mutex in + // place this must block on the lock and therefore cannot close the live + // connection. + reconnectDone := make(chan error, 1) + go func() { + reconnectDone <- svc.reconnect(10 * time.Millisecond) + }() + + // The reconnect must NOT have completed while the bind is still running: + // completing means it swapped and closed the connection mid-request. + select { + case err := <-reconnectDone: + t.Fatalf("reconnect returned while an authentication was in flight (err=%v); "+ + "the service lock is not protecting the connection", err) + case <-time.After(200 * time.Millisecond): + // Expected: reconnect is blocked on the lock. + } + + close(release) // let the in-flight bind complete + + require.NoError(t, <-authErr, + "in-flight authentication must not be broken by a concurrent rebind") + require.NoError(t, <-reconnectDone, "reconnect should succeed after the bind finishes") +} + +// TestUnsynchronizedReconnectReproducesRace demonstrates the pre-fix failure +// mode directly: closing the live connection while a request is in flight +// breaks that request. It guards against the test above silently passing for +// the wrong reason (e.g. the fake server behaviour changing). +func TestUnsynchronizedReconnectReproducesRace(t *testing.T) { + s := newFakeLDAPServer(t) + + searchStarted := make(chan struct{}) + var once sync.Once + s.setBlockSearch(func() { once.Do(func() { close(searchStarted) }) }) + + conn, err := ldapgo.DialURL(s.addr()) + require.NoError(t, err) + require.NoError(t, conn.Bind("cn=admin,dc=example,dc=com", "pw")) + + errCh := make(chan error, 1) + go func() { + req := ldapgo.NewSearchRequest( + "", ldapgo.ScopeBaseObject, ldapgo.NeverDerefAliases, 0, 0, false, + "(objectClass=*)", []string{}, nil) + _, err := conn.Search(req) + errCh <- err + }() + + select { + case <-searchStarted: + case <-time.After(5 * time.Second): + t.Fatal("search never reached the server") + } + + // This is what the buggy code did: drop the connection without waiting. + conn.Close() + + select { + case err := <-errCh: + require.Error(t, err, "closing a connection mid-request must break the request") + require.Contains(t, err.Error(), ldapClosedConnErr, + "expected the documented closed-connection failure, got: %v", err) + case <-time.After(5 * time.Second): + t.Fatal("in-flight search never returned after the connection was closed") + } +} + +// TestAuthenticateRestoresServiceBindOnFailure verifies that a failed user +// bind still leaves the shared connection bound as the service account, so a +// later LDAP search does not run as an unprivileged (or unbound) identity. +func TestAuthenticateRestoresServiceBindOnFailure(t *testing.T) { + s := newFakeLDAPServer(t) + svc := newTestLdapService(t, s.addr()) + + // The fake server accepts binds; authenticate a "user" and confirm the + // service rebind ran (two bind round-trips: user + service). + require.NoError(t, svc.Authenticate("uid=alice,dc=example,dc=com", "secret")) + + binds, _, _ := s.counts() + require.Equal(t, 3, binds, "expected initial service bind + user bind + service rebind") +} + +// TestConcurrentAuthAndReconnectNoClosedConn is the stress test: many +// authentication/search operations run concurrently with repeated reconnects. +// Under `go test -race` it asserts both that there is no data race and that no +// request ever observes a closed connection. +func TestConcurrentAuthAndReconnectNoClosedConn(t *testing.T) { + s := newFakeLDAPServer(t) + svc := newTestLdapService(t, s.addr()) + + const workers = 8 + const iterations = 25 + + var wg sync.WaitGroup + errCh := make(chan error, workers*iterations+iterations) + + for i := 0; i < workers; i++ { + wg.Add(1) + go func() { + defer wg.Done() + for j := 0; j < iterations; j++ { + if err := svc.Authenticate("uid=alice,dc=example,dc=com", "secret"); err != nil { + errCh <- err + return + } + // GetUserInfo returns ErrLDAPNoUsers on the empty fake result, + // which is fine; only closed-connection errors are failures. + if _, err := svc.GetUserInfo("alice"); err != nil && + strings.Contains(err.Error(), ldapClosedConnErr) { + errCh <- err + return + } + } + }() + } + + // Reconnect concurrently with the workers. + wg.Add(1) + go func() { + defer wg.Done() + for j := 0; j < iterations; j++ { + if err := svc.reconnect(5 * time.Millisecond); err != nil { + errCh <- err + return + } + } + }() + + wg.Wait() + close(errCh) + + for err := range errCh { + require.NoError(t, err) + } +} + +// TestHeartbeatUsesSharedConnection ensures the heartbeat probe runs against +// the shared connection and observes a live socket. +func TestHeartbeatUsesSharedConnection(t *testing.T) { + s := newFakeLDAPServer(t) + svc := newTestLdapService(t, s.addr()) + + require.NoError(t, svc.heartbeat()) + _, searches, _ := s.counts() + require.GreaterOrEqual(t, searches, 1) +} diff --git a/internal/service/ldap_service_test.go b/internal/service/ldap_service_test.go new file mode 100644 index 00000000..97047f4b --- /dev/null +++ b/internal/service/ldap_service_test.go @@ -0,0 +1,368 @@ +package service + +import ( + "bufio" + "context" + "encoding/binary" + "io" + "net" + "sync" + "testing" + "time" + + ldapgo "github.com/go-ldap/ldap/v3" + "github.com/stretchr/testify/require" + "github.com/tinyauthapp/tinyauth/internal/model" + "github.com/tinyauthapp/tinyauth/internal/utils/logger" +) + +// fakeLDAPServer is a tiny in-process LDAPv3 server that answers BindRequest +// and SearchRequest with fixed, valid BER responses. It exists so tests can +// exercise the real go-ldap client (real *ldapgo.Conn values, real Close() +// semantics) against loopback TCP without an external directory. +type fakeLDAPServer struct { + ln net.Listener + + mu sync.Mutex + binds int + searches int + openConns int + // blockSearch, when non-nil, is called at the start of every search. Tests + // use it to hold a search open while another goroutine races a reconnect. + blockSearch func() + + // blockBind, when non-nil, is called at the start of every bind. Tests use + // it to hold a user bind open while another goroutine races a reconnect. + blockBind func() +} + +func newFakeLDAPServer(t *testing.T) *fakeLDAPServer { + t.Helper() + ln, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + s := &fakeLDAPServer{ln: ln} + go s.serve() + t.Cleanup(func() { ln.Close() }) + return s +} + +func (s *fakeLDAPServer) addr() string { return "ldap://" + s.ln.Addr().String() } + +func (s *fakeLDAPServer) setBlockBind(f func()) { + s.mu.Lock() + s.blockBind = f + s.mu.Unlock() +} + +func (s *fakeLDAPServer) setBlockSearch(f func()) { + s.mu.Lock() + s.blockSearch = f + s.mu.Unlock() +} + +func (s *fakeLDAPServer) counts() (binds, searches, open int) { + s.mu.Lock() + defer s.mu.Unlock() + return s.binds, s.searches, s.openConns +} + +func (s *fakeLDAPServer) serve() { + for { + c, err := s.ln.Accept() + if err != nil { + return + } + s.mu.Lock() + s.openConns++ + s.mu.Unlock() + go s.handle(c) + } +} + +func (s *fakeLDAPServer) handle(c net.Conn) { + defer func() { + s.mu.Lock() + s.openConns-- + s.mu.Unlock() + c.Close() + }() + + r := bufio.NewReader(c) + for { + frame, err := readFrame(r) + if err != nil { + return + } + op := classify(frame) + switch op { + case opBind: + s.mu.Lock() + hook := s.blockBind + s.binds++ + s.mu.Unlock() + if hook != nil { + hook() + } + if _, err := c.Write(bindResponse(frame)); err != nil { + return + } + case opSearch: + s.mu.Lock() + hook := s.blockSearch + s.searches++ + s.mu.Unlock() + if hook != nil { + hook() + } + if _, err := c.Write(searchResponse(frame)); err != nil { + return + } + case opUnbind: + return + default: + return + } + } +} + +// readFrame reads a single LDAPMessage (one BER SEQUENCE) framed by the ASN.1 +// length. LDAP has no length prefix other than the BER encoding itself. +func readFrame(r *bufio.Reader) ([]byte, error) { + tag, err := r.ReadByte() + if err != nil { + return nil, err + } + if tag != 0x30 { + return nil, io.ErrUnexpectedEOF + } + lb, err := r.ReadByte() + if err != nil { + return nil, err + } + var length int + buf := []byte{tag, lb} + if lb&0x80 == 0 { + length = int(lb) + } else { + n := int(lb & 0x7f) + lenBytes := make([]byte, n) + if _, err := io.ReadFull(r, lenBytes); err != nil { + return nil, err + } + buf = append(buf, lenBytes...) + switch n { + case 2: + length = int(binary.BigEndian.Uint16(lenBytes)) + default: + for _, b := range lenBytes { + length = length<<8 | int(b) + } + } + } + body := make([]byte, length) + if _, err := io.ReadFull(r, body); err != nil { + return nil, err + } + return append(buf, body...), nil +} + +type ldapOp int + +const ( + opUnknown ldapOp = iota + opBind + opSearch + opUnbind +) + +// parseTLV decodes one BER tag-length-value triple with support for the +// multi-byte definite length form. +func parseTLV(b []byte) (tag byte, length int, value []byte, ok bool) { + if len(b) < 2 { + return 0, 0, nil, false + } + tag = b[0] + lb := b[1] + off := 2 + if lb&0x80 == 0 { + length = int(lb) + } else { + n := int(lb & 0x7f) + if len(b) < 2+n { + return 0, 0, nil, false + } + for _, x := range b[2 : 2+n] { + length = length<<8 | int(x) + } + off = 2 + n + } + if len(b) < off+length { + return 0, 0, nil, false + } + return tag, length, b[off : off+length], true +} + +// readMessage returns the outer SEQUENCE body and the messageID. It fully +// decodes the BER length (including multi-byte forms) rather than assuming a +// single length byte, which is what real LDAP clients emit for long filters. +func readMessage(frame []byte) (msgID int, opBytes []byte, ok bool) { + _, _, body, ok := parseTLV(frame) + if !ok { + return 0, nil, false + } + id, rest, ok := parseIntTLV(body) + if !ok { + return 0, nil, false + } + return id, rest, true +} + +// parseIntTLV parses a leading BER INTEGER and returns its value plus the +// remaining bytes. +func parseIntTLV(b []byte) (int, []byte, bool) { + if len(b) < 2 || b[0] != 0x02 { + return 0, nil, false + } + lb := b[1] + off := 2 + var length int + if lb&0x80 == 0 { + length = int(lb) + } else { + n := int(lb & 0x7f) + if len(b) < 2+n { + return 0, nil, false + } + for _, x := range b[2 : 2+n] { + length = length<<8 | int(x) + } + off = 2 + n + } + if length <= 0 || len(b) < off+length { + return 0, nil, false + } + id := 0 + for _, x := range b[off : off+length] { + id = id<<8 | int(x) + } + return id, b[off+length:], true +} + +// classify returns the LDAP protocol operation encoded in a frame. +func classify(frame []byte) ldapOp { + _, op, ok := readMessage(frame) + if !ok || len(op) == 0 { + return opUnknown + } + switch op[0] & 0x1f { + case 0: // BindRequest + return opBind + case 3: // SearchRequest + return opSearch + case 2: // UnbindRequest + return opUnbind + } + return opUnknown +} + +// ldapMessage wraps an ASN.1 SEQUENCE with a correctly encoded length so +// responses are always well-formed regardless of messageID magnitude. +func ldapMessage(msgID int, protocolOp []byte) []byte { + id := encodeInteger(msgID) + body := append(id, protocolOp...) + return append(encodeSequenceHeader(len(body)), body...) +} + +func encodeInteger(v int) []byte { + if v == 0 { + return []byte{0x02, 0x01, 0x00} + } + var b []byte + for x := v; x > 0; x >>= 8 { + b = append([]byte{byte(x & 0xff)}, b...) + } + if b[0]&0x80 != 0 { + b = append([]byte{0x00}, b...) + } + return append([]byte{0x02, byte(len(b))}, b...) +} + +func encodeSequenceHeader(length int) []byte { + if length < 0x80 { + return []byte{0x30, byte(length)} + } + var lenBytes []byte + for x := length; x > 0; x >>= 8 { + lenBytes = append([]byte{byte(x & 0xff)}, lenBytes...) + } + return append([]byte{0x30, byte(0x80 | len(lenBytes))}, lenBytes...) +} + +// bindResponse echoes the request messageID with a success resultCode (0). +func bindResponse(req []byte) []byte { + id, _, _ := readMessage(req) + op := []byte{ + 0x61, 0x07, // [APPLICATION 1] BindResponse + 0x0a, 0x01, 0x00, // resultCode: success + 0x04, 0x00, // matchedDN + 0x04, 0x00, // diagnosticMessage + } + return ldapMessage(id, op) +} + +// searchResponse returns a SearchResultDone (success) with zero entries. Tests +// only need a well-formed, successful round-trip; entry parsing is not the +// subject under test. +func searchResponse(req []byte) []byte { + id, _, _ := readMessage(req) + op := []byte{ + 0x65, 0x07, // [APPLICATION 5] SearchResultDone + 0x0a, 0x01, 0x00, // resultCode: success + 0x04, 0x00, + 0x04, 0x00, + } + return ldapMessage(id, op) +} + +// newTestLdapService builds a real LdapService bound to the fake server. +func newTestLdapService(t *testing.T, addr string) *LdapService { + t.Helper() + log := logger.NewLogger().WithTestConfig() + log.Init() + + svc := &LdapService{ + log: log, + ctx: context.Background(), + config: &model.Config{ + LDAP: model.LDAPConfig{ + Address: addr, + BaseDN: "dc=example,dc=com", + SearchFilter: "(&(objectClass=person)(uid=%s))", + GroupSearchFilter: "(&(objectClass=group)(member=%s))", + Insecure: true, + }, + }, + } + require.NoError(t, svc.dialAndBind()) + return svc +} + +// dialAndBind is a test convenience mirroring connect() without the swap. +// It reads svc.conn under the lock to stay race-free with reconnect. +func (svc *LdapService) dialAndBind() error { + conn, err := svc.dial() + if err != nil { + return err + } + if err := svc.bindConn(conn); err != nil { + conn.Close() + return err + } + svc.mutex.Lock() + svc.conn = conn + svc.mutex.Unlock() + return nil +} + +var _ = ldapgo.NewSearchRequest +var _ = time.Second From 0dce235d9ce66fbaa6b2539d74045a69eca01c1c Mon Sep 17 00:00:00 2001 From: Shivam Maurya Date: Thu, 15 Oct 2026 00:37:51 +0530 Subject: [PATCH 2/2] test: hold fake server search open until conn.Close() (CodeRabbit review) The blockSearch hook closed searchStarted and returned immediately, so the fake server could answer the in-flight search before the test goroutine ran conn.Close(). When that happened conn.Search succeeded and require.Error failed at random, breaking go test ./... and make test-race intermittently. Hold the hook open on a release channel that is closed only after conn.Close(); a t.Cleanup guard closes it if the test fails early. --- internal/service/ldap_service_race_test.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/internal/service/ldap_service_race_test.go b/internal/service/ldap_service_race_test.go index a453775c..7e834b85 100644 --- a/internal/service/ldap_service_race_test.go +++ b/internal/service/ldap_service_race_test.go @@ -88,8 +88,13 @@ func TestUnsynchronizedReconnectReproducesRace(t *testing.T) { s := newFakeLDAPServer(t) searchStarted := make(chan struct{}) + release := make(chan struct{}) + t.Cleanup(func() { select { case <-release: default: close(release) } }) var once sync.Once - s.setBlockSearch(func() { once.Do(func() { close(searchStarted) }) }) + s.setBlockSearch(func() { + once.Do(func() { close(searchStarted) }) + <-release + }) conn, err := ldapgo.DialURL(s.addr()) require.NoError(t, err) @@ -112,6 +117,7 @@ func TestUnsynchronizedReconnectReproducesRace(t *testing.T) { // This is what the buggy code did: drop the connection without waiting. conn.Close() + close(release) select { case err := <-errCh: