internal/ldap: Recover from panics in request handlers

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)
+}

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])
+	}
+}

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")
 
 }