internal/ldap: Reply with the result type a request expects

An LDAP client only accepts the result that pairs with the request it
sent, and silently ignores anything else. Unrouted operations were
answered with a generic result, which encodes with the wrong tag, so a
search with an unexpected base DN (for example the empty base that
ldapsearch uses when -b is omitted) left the client waiting forever
instead of reporting an error.

Every failure path now terminates the request with the matching result
type, panics included, so a single wrapper covers all operations rather
than one responder per operation. NetAuth calls carry a 30s deadline
because the gRPC client dials lazily and applies none of its own, and
refused operations and failed authentication are logged, which is what
an operator needs when a client appears to hang.

💘 Generated with Crush

Assisted-by: Crush:qwen38_flash_next_mtp

AGENTS.md

@@ -36,7 +36,7 @@ actually used — the real one is `.goreleaser.yml` at the repo root.
 main.go                    # wiring only: config, logging, client, serve
 internal/ldap/             # the LDAP server (package ldap)
   ldap.go                  # New(), route (re)building, Serve/ServeTLS, SetDomain
-  handler.go               # panic guard wrapper + per-op failure responders
+  handler.go               # panic guard wrapper + request-paired result writer
   conn.go                  # per-connection bind state (bound/markBound/markUnbound)
   type.go                  # server struct + naClient interface (the seam for fakes)
   option.go                # functional options: WithLogger, WithNetAuth, WithAnonBind
@@ -78,6 +78,20 @@ goroutines with no recovery of its own).
   itself; the library does not enforce a minimum version.
 - **Per-connection state**: use `GetData`/`SetData` on the connection
   (the `connection` interface in `conn.go`).
+- **Result messages must pair with the request**: an LDAP client
+  silently ignores any result whose application tag does not pair
+  with the request it sent (a search is only completed by a
+  searchResDone), so a mismatch hangs the client forever. Never write
+  a bare `ldap.NewResponse(...)` as a reply; use
+  `writeResult(w, m, code, msg)` (handler.go), which picks the right
+  result type via `resultForRequest`. `handleNotFound` is the
+  user-visible case: any search with an unexpected base DN lands
+  there.
+- **NetAuth calls are deadline-bound**: every handler puts a
+  `context.WithTimeout` (`rpcTimeout`, 30s, bind.go) on its context
+  before calling `naClient`; the gRPC client dials lazily with no
+  deadline of its own, so an unbounded context hangs the handler and
+  the client behind it.
 - **The server struct is its own Handler**: `New()` registers `*server` once
   (the fork forbids re-registration), and `ServeLDAP` delegates to whatever
   `s.routes` mux is current. `SetDomain` rebuilds the mux in place, which is
@@ -156,7 +170,10 @@ startup exits with 3 if invalid).
 - Commit message convention: `component: Capitalized description`
   (e.g. `internal/ldap: Add option to bind anonmyously`).
 - Handlers are methods on `*server` named `handle<X>`; response pattern is
-  construct response → `w.Write(res)` on every exit path.
+  construct response → `w.Write(res)` on every exit path. Panic failures
+  are handled centrally: `guard(h)` (single arg) writes an
+  `operationsError` result paired with the request type via
+  `writeResult`, so handlers do not each need a failure responder.
 - Loop style favors `for i := range x` throughout.
 - Errors returned to LDAP clients go in diagnostic messages; keep them
   non-technical, they are shown verbatim by LDAP browsers.

internal/ldap/bind.go

@@ -2,12 +2,20 @@ package ldap
 
 import (
 	"context"
+	"time"
 
 	ldap "github.com/vjeantet/ldapserver"
 )
 
+// rpcTimeout bounds every NetAuth call made on behalf of one client
+// request. The gRPC client dials lazily and never applies its own
+// deadline, so without this a stalled server socket hangs the handler
+// goroutine and the LDAP client behind it forever.
+const rpcTimeout = 30 * time.Second
+
 func (s *server) handleBind(w ldap.ResponseWriter, m *ldap.Message) {
-	ctx := context.Background()
+	ctx, cancel := context.WithTimeout(context.Background(), rpcTimeout)
+	defer cancel()
 
 	r := m.GetBindRequest()
 	// The server only supports simple auth, no SASL or anything
@@ -41,6 +49,7 @@ func (s *server) handleBind(w ldap.ResponseWriter, m *ldap.Message) {
 	}
 
 	if err := s.c.AuthEntity(ctx, entityID, string(r.AuthenticationSimple())); err != nil {
+		s.l.Warn("NetAuth authentication failed", "id", entityID, "error", err)
 		s.markUnbound(m)
 		res := ldap.NewBindResponse(ldap.LDAPResultInvalidCredentials)
 		res.SetDiagnosticMessage("invalid credentials")

internal/ldap/handler.go

@@ -14,14 +14,14 @@ import (
 // Requests that already wrote something cannot be recovered cleanly,
 // since the client is expecting whatever it received to be terminated
 // by the matching done message, so those are only logged.
-func (s *server) guard(h ldap.HandlerFunc, fail func(ldap.ResponseWriter)) ldap.HandlerFunc {
+func (s *server) guard(h ldap.HandlerFunc) ldap.HandlerFunc {
 	return func(w ldap.ResponseWriter, m *ldap.Message) {
 		tw := &trackingWriter{ResponseWriter: w}
 		defer func() {
 			if r := recover(); r != nil {
 				s.l.Error("Recovered from panic in handler", "panic", r)
-				if !tw.written && fail != nil {
-					fail(tw.ResponseWriter)
+				if !tw.written {
+					writeResult(w, m, ldap.LDAPResultOperationsError, "internal server error")
 				}
 			}
 		}()
@@ -29,25 +29,56 @@ func (s *server) guard(h ldap.HandlerFunc, fail func(ldap.ResponseWriter)) ldap.
 	}
 }
 
-// failBindError, failSearchError and failOperationError write the
-// terminating error response for each kind of operation and are used
-// by guard when a handler dies before it started its reply.
-func failBindError(w ldap.ResponseWriter) {
-	res := ldap.NewBindResponse(ldap.LDAPResultOperationsError)
-	res.SetDiagnosticMessage("internal server error")
+// writeResult sends the result message that terminates a request,
+// using the response type that pairs with the request type. It does
+// nothing for operations that are never answered, such as abandon.
+func writeResult(w ldap.ResponseWriter, m *ldap.Message, code int, diagnostic string) {
+	if m == nil {
+		return
+	}
+	res, ok := resultForRequest(m.ProtocolOp(), code, diagnostic)
+	if !ok {
+		return
+	}
 	w.Write(res)
 }
 
-func failSearchError(w ldap.ResponseWriter) {
-	res := ldap.NewSearchResultDoneResponse(ldap.LDAPResultOperationsError)
-	res.SetDiagnosticMessage("internal server error")
-	w.Write(res)
-}
-
-func failOperationError(w ldap.ResponseWriter) {
-	res := ldap.NewResponse(ldap.LDAPResultOperationsError)
-	res.SetDiagnosticMessage("internal server error")
-	w.Write(res)
+// resultForRequest builds the result message that terminates a
+// request. An LDAP client only accepts the response type that pairs
+// with the request it sent (a search for instance is only completed
+// by a searchResDone) and silently ignores anything else, so answering
+// a search with a bare LDAPResult leaves the client waiting forever
+// rather than reporting the error. The boolean reports whether a
+// response is expected for the operation at all.
+//
+// Diagnostic messages are only attached where goldap exposes a setter
+// for the type; the modify family carries the result code alone.
+func resultForRequest(op message.ProtocolOp, code int, diagnostic string) (message.ProtocolOp, bool) {
+	switch op.(type) {
+	case message.SearchRequest:
+		res := ldap.NewSearchResultDoneResponse(code)
+		res.SetDiagnosticMessage(diagnostic)
+		return res, true
+	case message.BindRequest:
+		res := ldap.NewBindResponse(code)
+		res.SetDiagnosticMessage(diagnostic)
+		return res, true
+	case message.ExtendedRequest:
+		res := ldap.NewExtendedResponse(code)
+		res.SetDiagnosticMessage(diagnostic)
+		return res, true
+	case message.CompareRequest:
+		return message.CompareResponse(ldap.NewResponse(code)), true
+	case message.AddRequest:
+		return message.AddResponse(ldap.NewResponse(code)), true
+	case message.DelRequest:
+		return message.DelResponse(ldap.NewResponse(code)), true
+	case message.ModifyRequest:
+		return message.ModifyResponse(ldap.NewResponse(code)), true
+	case message.ModifyDNRequest:
+		return message.ModifyDNResponse(ldap.NewResponse(code)), true
+	}
+	return nil, false
 }
 
 // trackingWriter records whether anything has been written so that

internal/ldap/handler_test.go

@@ -1,6 +1,7 @@
 package ldap
 
 import (
+	"reflect"
 	"testing"
 
 	"github.com/hashicorp/go-hclog"
@@ -23,41 +24,41 @@ func testLogger() hclog.Logger { return hclog.NewNullLogger() }
 func TestGuardRecoversPanic(t *testing.T) {
 	s := &server{l: testLogger()}
 
-	failed := false
-	fail := func(ldap.ResponseWriter) { failed = true }
-
 	h := s.guard(func(w ldap.ResponseWriter, m *ldap.Message) {
 		panic("boom")
-	}, fail)
+	})
 
 	w := &recordingWriter{}
-	h(w, &ldap.Message{})
+	h(w, mustDecode(t, berSearchRequest("ou=entities,dc=netauth,dc=example,dc=com")))
 
-	if !failed {
-		t.Error("the failure handler was not called after a panic")
+	if len(w.responses) != 1 {
+		t.Fatalf("got %d responses, want 1", len(w.responses))
+	}
+	if _, ok := w.responses[0].(message.SearchResultDone); !ok {
+		t.Errorf("response is %T, want message.SearchResultDone", w.responses[0])
+	}
+	if code := resultCode(t, w.responses[0]); code != ldap.LDAPResultOperationsError {
+		t.Errorf("result code is %d, want %d", code, ldap.LDAPResultOperationsError)
 	}
 }
 
 func TestGuardSilentAfterPartialWrite(t *testing.T) {
 	s := &server{l: testLogger()}
 
-	failed := false
-	fail := func(ldap.ResponseWriter) { failed = true }
-
 	h := s.guard(func(w ldap.ResponseWriter, m *ldap.Message) {
 		w.Write(ldap.NewSearchResultEntry("cn=test"))
 		panic("boom")
-	}, fail)
+	})
 
 	w := &recordingWriter{}
-	h(w, &ldap.Message{})
+	h(w, mustDecode(t, berSearchRequest("ou=entities,dc=netauth,dc=example,dc=com")))
 
-	if failed {
-		t.Error("the failure handler ran even though a response was already sent")
-	}
 	if len(w.responses) != 1 {
 		t.Errorf("got %d responses, want 1", len(w.responses))
 	}
+	if _, ok := w.responses[0].(message.SearchResultEntry); !ok {
+		t.Errorf("response is %T, want the partial entry only", w.responses[0])
+	}
 }
 
 func TestGuardPassesThrough(t *testing.T) {
@@ -65,7 +66,7 @@ func TestGuardPassesThrough(t *testing.T) {
 
 	h := s.guard(func(w ldap.ResponseWriter, m *ldap.Message) {
 		w.Write(ldap.NewSearchResultDoneResponse(ldap.LDAPResultSuccess))
-	}, failSearchError)
+	})
 
 	w := &recordingWriter{}
 	h(w, &ldap.Message{})
@@ -77,3 +78,79 @@ func TestGuardPassesThrough(t *testing.T) {
 		t.Errorf("response is %T, want message.SearchResultDone", w.responses[0])
 	}
 }
+
+// TestResultForRequestPairing is the regression test for the client
+// hang: an LDAP client only accepts the result type that pairs with
+// the request it sent and silently discards anything else, so a
+// mismatched type leaves it waiting for a done message forever.
+func TestResultForRequestPairing(t *testing.T) {
+	tests := []struct {
+		name string
+		op   message.ProtocolOp
+		want string
+	}{
+		{"search", message.SearchRequest{}, "SearchResultDone"},
+		{"bind", message.BindRequest{}, "BindResponse"},
+		{"extended", message.ExtendedRequest{}, "ExtendedResponse"},
+		{"compare", message.CompareRequest{}, "CompareResponse"},
+		{"add", message.AddRequest{}, "AddResponse"},
+		{"delete", message.DelRequest("cn=x,dc=example,dc=com"), "DelResponse"},
+		{"modify", message.ModifyRequest{}, "ModifyResponse"},
+		{"modifyDN", message.ModifyDNRequest{}, "ModifyDNResponse"},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			res, ok := resultForRequest(tc.op, ldap.LDAPResultUnwillingToPerform, "nope")
+			if !ok {
+				t.Fatal("no result produced for a request that needs one")
+			}
+			if got := reflect.TypeOf(res).Name(); got != tc.want {
+				t.Errorf("result type is %s, want %s", got, tc.want)
+			}
+		})
+	}
+
+	// Abandon is never answered, so it must produce nothing rather
+	// than an unsolicited result.
+	if res, ok := resultForRequest(message.AbandonRequest(1), ldap.LDAPResultSuccess, ""); ok {
+		t.Errorf("abandon produced %T, want no response", res)
+	}
+}
+
+// TestSearchResultDoneWireTag pins the search reply to the
+// searchResDone application tag (5). A reply carrying any other tag,
+// including the bare LDAPResult universal SEQUENCE, is ignored by
+// clients and hangs them.
+func TestSearchResultDoneWireTag(t *testing.T) {
+	res, ok := resultForRequest(message.SearchRequest{}, ldap.LDAPResultUnwillingToPerform, "nope")
+	if !ok {
+		t.Fatal("no result produced for a search request")
+	}
+
+	m := message.NewLDAPMessageWithProtocolOp(res)
+	m.SetMessageID(2)
+	b, err := m.Write()
+	if err != nil {
+		t.Fatalf("encoding response: %s", err)
+	}
+	raw := b.Bytes()
+
+	// Descend from the LDAPMessage SEQUENCE into the protocolOp.
+	off := 0
+	if _, off, err = message.ParseTagAndLength(raw, off); err != nil {
+		t.Fatalf("message tag: %s", err)
+	}
+	var id message.TagAndLength
+	if id, off, err = message.ParseTagAndLength(raw, off); err != nil {
+		t.Fatalf("messageID tag: %s", err)
+	}
+	off += id.Length
+	op, _, err := message.ParseTagAndLength(raw, off)
+	if err != nil {
+		t.Fatalf("protocolOp tag: %s", err)
+	}
+	if op.Class != 1 || op.Tag != 5 {
+		t.Errorf("protocolOp is class %d tag %d, want APPLICATION (1) tag 5", op.Class, op.Tag)
+	}
+}

internal/ldap/hang_test.go

@@ -0,0 +1,92 @@
+package ldap
+
+import (
+	"errors"
+	"io"
+	"net"
+	"testing"
+	"time"
+
+	"github.com/vjeantet/goldap/message"
+)
+
+// TestUnroutedSearchTerminatesOverTheWire is the end-to-end
+// regression test for the hang reported in the field: a search whose
+// base DN matches no route (for example the empty base an LDAP client
+// uses when no -b flag is given) must still be answered with a
+// searchResDone, otherwise the client waits for one forever.
+func TestUnroutedSearchTerminatesOverTheWire(t *testing.T) {
+	s := New(WithLogger(testLogger()), WithNetAuth(&fakeClient{}))
+	if err := s.SetDomain("example.com"); err != nil {
+		t.Fatalf("SetDomain: %s", err)
+	}
+	addr := freeAddr(t)
+	if err := s.Serve(addr); err != nil {
+		t.Fatalf("Serve: %s", err)
+	}
+	t.Cleanup(func() { s.Listener.Close() })
+
+	conn, err := net.DialTimeout("tcp", addr, 2*time.Second)
+	if err != nil {
+		t.Fatalf("dial: %s", err)
+	}
+	t.Cleanup(func() { conn.Close() })
+
+	// A search with an empty base DN and subtree scope, exactly what
+	// an LDAP client sends when no -b flag is given. It matches no
+	// route and must be answered, not silently dropped.
+	conn.SetWriteDeadline(time.Now().Add(2 * time.Second))
+	if _, err := conn.Write(berSearchRequest("")); err != nil {
+		t.Fatalf("sending search: %s", err)
+	}
+
+	// Read responses until a searchResDone arrives or the deadline
+	// passes; a client would block here forever if none is sent.
+	deadline := time.Now().Add(5 * time.Second)
+	for time.Now().Before(deadline) {
+		conn.SetReadDeadline(deadline)
+		raw, err := readLDAPPacket(conn)
+		if err != nil {
+			t.Fatalf("reading response: %s", err)
+		}
+		m, err := message.ReadLDAPMessage(message.NewBytes(0, raw))
+		if err != nil {
+			t.Fatalf("decoding response: %s", err)
+		}
+		if _, ok := m.ProtocolOp().(message.SearchResultDone); ok {
+			return
+		}
+	}
+	t.Error("no searchResDone received; an unrouted search hangs the client")
+}
+
+// readLDAPPacket reads one BER TLV (the top-level LDAPMessage
+// SEQUENCE) and returns its full encoding.
+func readLDAPPacket(r io.Reader) ([]byte, error) {
+	head := make([]byte, 2)
+	if _, err := io.ReadFull(r, head); err != nil {
+		return nil, err
+	}
+	packet := head
+	length := int(head[1])
+	if head[1]&0x80 != 0 {
+		nb := int(head[1] & 0x7f)
+		if nb == 0 || nb > 4 {
+			return nil, errors.New("unsupported length encoding")
+		}
+		ext := make([]byte, nb)
+		if _, err := io.ReadFull(r, ext); err != nil {
+			return nil, err
+		}
+		length = 0
+		for _, b := range ext {
+			length = length<<8 | int(b)
+		}
+		packet = append(packet, ext...)
+	}
+	body := make([]byte, length)
+	if _, err := io.ReadFull(r, body); err != nil {
+		return nil, err
+	}
+	return append(packet, body...), nil
+}

internal/ldap/ldap.go

@@ -33,11 +33,11 @@ func New(opts ...Option) *server {
 // reflect the current domain without ever leaving duplicates behind.
 func (s *server) buildRoutes() {
 	routes := ldap.NewRouteMux()
-	routes.NotFound(s.guard(s.handleNotFound, failOperationError))
-	routes.Abandon(s.guard(s.handleAbandon, nil))
-	routes.Bind(s.guard(s.handleBind, failBindError))
+	routes.NotFound(s.guard(s.handleNotFound))
+	routes.Abandon(s.guard(s.handleAbandon))
+	routes.Bind(s.guard(s.handleBind))
 
-	routes.Search(s.guard(s.handleSearchDSE, failSearchError)).
+	routes.Search(s.guard(s.handleSearchDSE)).
 		BaseDn("").
 		Scope(ldap.SearchRequestScopeBaseObject).
 		Filter("(objectclass=*)").
@@ -46,12 +46,12 @@ func (s *server) buildRoutes() {
 	if len(s.nc) > 0 {
 		base := strings.Join(s.nc, ",")
 
-		routes.Search(s.guard(s.handleSearchEntities, failSearchError)).
+		routes.Search(s.guard(s.handleSearchEntities)).
 			BaseDn("ou=entities," + base).
 			Scope(ldap.SearchRequestHomeSubtree).
 			Label("Search - Entities")
 
-		routes.Search(s.guard(s.handleSearchGroups, failSearchError)).
+		routes.Search(s.guard(s.handleSearchGroups)).
 			BaseDn("ou=groups," + base).
 			Scope(ldap.SearchRequestHomeSubtree).
 			Label("Search - Groups")
@@ -114,9 +114,9 @@ func (s *server) listenAndServe(bind string, opts ...func(*ldap.Server)) error {
 }
 
 func (s *server) handleNotFound(w ldap.ResponseWriter, r *ldap.Message) {
-	res := ldap.NewResponse(ldap.LDAPResultUnwillingToPerform)
-	res.SetDiagnosticMessage("Operation not implemented by server")
-	w.Write(res)
+	s.l.Warn("Refusing unrouted operation", "op", r.ProtocolOpName(), "message", r.String())
+	writeResult(w, r, ldap.LDAPResultUnwillingToPerform,
+		"Operation not implemented by server, searches must use a base DN of ou=entities or ou=groups underneath the naming context")
 }
 
 func (s *server) handleAbandon(w ldap.ResponseWriter, m *ldap.Message) {
@@ -159,9 +159,7 @@ func (s *server) SetDomain(domain string) error {
 // registration).
 func (s *server) ServeLDAP(w ldap.ResponseWriter, r *ldap.Message) {
 	if s.routes == nil {
-		res := ldap.NewResponse(ldap.LDAPResultUnavailable)
-		res.SetDiagnosticMessage("server is not configured with a domain")
-		w.Write(res)
+		writeResult(w, r, ldap.LDAPResultUnavailable, "server is not configured with a domain")
 		return
 	}
 	s.routes.ServeLDAP(w, r)

internal/ldap/search.go

@@ -29,7 +29,8 @@ func (s *server) handleSearchDSE(w ldap.ResponseWriter, m *ldap.Message) {
 }
 
 func (s *server) handleSearchEntities(w ldap.ResponseWriter, m *ldap.Message) {
-	ctx := context.Background()
+	ctx, cancel := context.WithTimeout(context.Background(), rpcTimeout)
+	defer cancel()
 	s.l.Debug("Search Entities")
 
 	if !s.bound(m) {
@@ -56,6 +57,7 @@ func (s *server) handleSearchEntities(w ldap.ResponseWriter, m *ldap.Message) {
 
 	ents, err := s.c.EntitySearch(ctx, expr)
 	if err != nil {
+		s.l.Warn("NetAuth entity search failed", "query", expr, "error", err)
 		res := ldap.NewSearchResultDoneResponse(ldap.LDAPResultOperationsError)
 		res.SetDiagnosticMessage(err.Error())
 		w.Write(res)
@@ -108,7 +110,8 @@ func (s *server) entitySearchResult(ctx context.Context, e *pb.Entity, dn messag
 }
 
 func (s *server) handleSearchGroups(w ldap.ResponseWriter, m *ldap.Message) {
-	ctx := context.Background()
+	ctx, cancel := context.WithTimeout(context.Background(), rpcTimeout)
+	defer cancel()
 	s.l.Debug("Search Groups")
 
 	if !s.bound(m) {
@@ -136,6 +139,7 @@ func (s *server) handleSearchGroups(w ldap.ResponseWriter, m *ldap.Message) {
 
 	groups, err := s.c.GroupSearch(ctx, expr)
 	if err != nil {
+		s.l.Warn("NetAuth group search failed", "query", expr, "error", err)
 		res := ldap.NewSearchResultDoneResponse(ldap.LDAPResultOperationsError)
 		res.SetDiagnosticMessage(err.Error())
 		w.Write(res)