From d74308280b6a16905b738ed8b5effaabff6b877b Mon Sep 17 00:00:00 2001 From: Michael Aldridge Date: Mon, 2 Nov 2026 19:59:04 -0600 Subject: [PATCH] internal/ldap: Reply with the result type a request expects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 21 ++++++- internal/ldap/bind.go | 11 +++- internal/ldap/handler.go | 69 +++++++++++++++------ internal/ldap/handler_test.go | 109 +++++++++++++++++++++++++++++----- internal/ldap/hang_test.go | 92 ++++++++++++++++++++++++++++ internal/ldap/ldap.go | 22 ++++--- internal/ldap/search.go | 8 ++- 7 files changed, 280 insertions(+), 52 deletions(-) create mode 100644 internal/ldap/hang_test.go diff --git a/AGENTS.md b/AGENTS.md index 9a84863..581a874 100644 --- a/AGENTS.md +++ b/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`; 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. diff --git a/internal/ldap/bind.go b/internal/ldap/bind.go index 6146662..3e91680 100644 --- a/internal/ldap/bind.go +++ b/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") diff --git a/internal/ldap/handler.go b/internal/ldap/handler.go index 947fc0c..ae0336e 100644 --- a/internal/ldap/handler.go +++ b/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 diff --git a/internal/ldap/handler_test.go b/internal/ldap/handler_test.go index 8c3c673..df479f2 100644 --- a/internal/ldap/handler_test.go +++ b/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) + } +} diff --git a/internal/ldap/hang_test.go b/internal/ldap/hang_test.go new file mode 100644 index 0000000..ed13512 --- /dev/null +++ b/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 +} diff --git a/internal/ldap/ldap.go b/internal/ldap/ldap.go index 8947285..afb7bd8 100644 --- a/internal/ldap/ldap.go +++ b/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) diff --git a/internal/ldap/search.go b/internal/ldap/search.go index d0b33db..dc59101 100644 --- a/internal/ldap/search.go +++ b/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)