From 09fe187bcef94eb8e8e5edbdbfcc5dc880354e9e Mon Sep 17 00:00:00 2001 From: Michael Aldridge Date: Sun, 1 Nov 2026 18:06:55 -0600 Subject: [PATCH] internal/ldap: Recover from panics in request handlers --- internal/ldap/handler.go | 68 ++++++++++++++++++++++++++++ internal/ldap/handler_test.go | 83 +++++++++++++++++++++++++++++++++++ internal/ldap/ldap.go | 14 +++--- 3 files changed, 158 insertions(+), 7 deletions(-) create mode 100644 internal/ldap/handler.go create mode 100644 internal/ldap/handler_test.go diff --git a/internal/ldap/handler.go b/internal/ldap/handler.go new file mode 100644 index 0000000..b380ea0 --- /dev/null +++ b/internal/ldap/handler.go @@ -0,0 +1,68 @@ +package ldap + +import ( + "github.com/ps78674/goldap/message" + ldap "github.com/ps78674/ldapserver" +) + +// guard wraps a handler so that a panic in handler code is logged and +// converted into an error response instead of killing the process. The +// underlying server runs each request in its own goroutine with no +// recovery of its own, so a single panic anywhere in a handler takes +// the whole daemon down with it. +// +// 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 { + 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) + } + } + }() + h(tw, m) + } +} + +// 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") + 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) +} + +// trackingWriter records whether anything has been written so that +// guard can decide if an error response is still appropriate. +type trackingWriter struct { + ldap.ResponseWriter + written bool +} + +func (w *trackingWriter) Write(po message.ProtocolOp) { + w.written = true + w.ResponseWriter.Write(po) +} + +func (w *trackingWriter) WriteMessage(m *message.LDAPMessage) { + w.written = true + w.ResponseWriter.WriteMessage(m) +} diff --git a/internal/ldap/handler_test.go b/internal/ldap/handler_test.go new file mode 100644 index 0000000..ce4a7b0 --- /dev/null +++ b/internal/ldap/handler_test.go @@ -0,0 +1,83 @@ +package ldap + +import ( + "testing" + + "github.com/hashicorp/go-hclog" + "github.com/ps78674/goldap/message" + ldap "github.com/ps78674/ldapserver" +) + +// recordingWriter captures everything written to it so tests can +// assert on the exact sequence of responses a handler produces. +type recordingWriter struct { + responses []message.ProtocolOp +} + +func (w *recordingWriter) Write(po message.ProtocolOp) { + w.responses = append(w.responses, po) +} + +func (w *recordingWriter) WriteMessage(m *message.LDAPMessage) { + w.responses = append(w.responses, m.ProtocolOp()) +} + +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{}) + + if !failed { + t.Error("the failure handler was not called after a panic") + } +} + +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{}) + + 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)) + } +} + +func TestGuardPassesThrough(t *testing.T) { + s := &server{l: testLogger()} + + h := s.guard(func(w ldap.ResponseWriter, m *ldap.Message) { + w.Write(ldap.NewSearchResultDoneResponse(ldap.LDAPResultSuccess)) + }, failSearchError) + + w := &recordingWriter{} + h(w, &ldap.Message{}) + + 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]) + } +} diff --git a/internal/ldap/ldap.go b/internal/ldap/ldap.go index 0527c1c..3b3924b 100644 --- a/internal/ldap/ldap.go +++ b/internal/ldap/ldap.go @@ -15,11 +15,11 @@ func New(opts ...Option) *server { x.Server = ldap.NewServer() x.routes = ldap.NewRouteMux() - x.routes.NotFound(x.handleNotFound) - x.routes.Abandon(x.handleAbandon) - x.routes.Bind(x.handleBind) + x.routes.NotFound(x.guard(x.handleNotFound, failOperationError)) + x.routes.Abandon(x.guard(x.handleAbandon, nil)) + x.routes.Bind(x.guard(x.handleBind, failBindError)) - x.routes.Search(x.handleSearchDSE). + x.routes.Search(x.guard(x.handleSearchDSE, failSearchError)). BaseDn(""). Scope(ldap.SearchRequestScopeBaseObject). Filter("(objectclass=*)"). @@ -92,15 +92,15 @@ func (s *server) SetDomain(domain string) { // Register routes that are dependent on the namingConvention entitySearchDN := "ou=entities," + strings.Join(s.nc, ",") - s.routes.Search(s.handleSearchEntities). + s.routes.Search(s.guard(s.handleSearchEntities, failSearchError)). BaseDn(entitySearchDN). Scope(ldap.SearchRequestHomeSubtree). Label("Search - Entities") groupSearchDN := "ou=groups," + strings.Join(s.nc, ",") - s.routes.Search(s.handleSearchGroups). + s.routes.Search(s.guard(s.handleSearchGroups, failSearchError)). BaseDn(groupSearchDN). Scope(ldap.SearchRequestHomeSubtree). - Label("Search - Entities") + Label("Search - Groups") }