diff --git a/internal/ldap/bind.go b/internal/ldap/bind.go index 3244461..d1e2dae 100644 --- a/internal/ldap/bind.go +++ b/internal/ldap/bind.go @@ -14,6 +14,7 @@ func (s *server) handleBind(w ldap.ResponseWriter, m *ldap.Message) { // fancy because we are after all just fronting another // protocol. if r.AuthenticationChoice() != "simple" { + s.markUnbound(m) res := ldap.NewBindResponse(ldap.LDAPResultUnwillingToPerform) res.SetDiagnosticMessage("Authentication choice not supported") w.Write(res) @@ -23,6 +24,7 @@ func (s *server) handleBind(w ldap.ResponseWriter, m *ldap.Message) { s.l.Debug("Bind from dn", "dn", r.Name()) if s.allowAnon && r.Name() == "" { + s.markBound(m) res := ldap.NewBindResponse(ldap.LDAPResultSuccess) w.Write(res) return @@ -30,6 +32,7 @@ func (s *server) handleBind(w ldap.ResponseWriter, m *ldap.Message) { entityID, err := s.entityIDFromDN(r.Name()) if err != nil { + s.markUnbound(m) res := ldap.NewBindResponse(ldap.LDAPResultInvalidDNSyntax) res.SetDiagnosticMessage(err.Error()) s.l.Warn("Request with invalid DN", "dn", r.Name()) @@ -38,12 +41,14 @@ func (s *server) handleBind(w ldap.ResponseWriter, m *ldap.Message) { } if err := s.c.AuthEntity(ctx, entityID, string(r.AuthenticationSimple())); err != nil { + s.markUnbound(m) res := ldap.NewBindResponse(ldap.LDAPResultInvalidCredentials) res.SetDiagnosticMessage("invalid credentials") w.Write(res) return } + s.markBound(m) res := ldap.NewBindResponse(ldap.LDAPResultSuccess) w.Write(res) } diff --git a/internal/ldap/conn.go b/internal/ldap/conn.go new file mode 100644 index 0000000..c45aaa1 --- /dev/null +++ b/internal/ldap/conn.go @@ -0,0 +1,89 @@ +package ldap + +import ( + "sync" + + ldap "github.com/ps78674/ldapserver" +) + +// connection is the part of the underlying server's client type that +// this bridge uses for state belonging to a connection rather than to +// a single request. That type is unexported, so it is reached through +// the message and abstracted here so tests can supply their own. +type connection interface { + GetAddData() interface{} + SetAddData(interface{}) +} + +// boundState is the per-connection record stored in the client's +// additional data, which releases it together with the connection. +type boundState struct { + sync.Mutex + bound bool +} + +// messageConn returns the connection a request arrived on. +func messageConn(m *ldap.Message) connection { + if m == nil || m.Client == nil { + return nil + } + return m.Client +} + +// connOf returns the connection for a request. It is a field so that +// tests can inject a connection without the server's unexported +// client type. +func (s *server) connOf(m *ldap.Message) connection { + if s.conn == nil { + return messageConn(m) + } + return s.conn(m) +} + +// bound reports whether the request arrived on a connection that has +// already been authenticated. +func (s *server) bound(m *ldap.Message) bool { + st := existingState(s.connOf(m)) + if st == nil { + return false + } + st.Lock() + defer st.Unlock() + return st.bound +} + +// markBound records that the connection has been authenticated. +func (s *server) markBound(m *ldap.Message) { + c := s.connOf(m) + if c == nil { + return + } + if st := existingState(c); st != nil { + st.Lock() + defer st.Unlock() + st.bound = true + return + } + c.SetAddData(&boundState{bound: true}) +} + +// markUnbound clears the authentication state of a connection. RFC +// 4511 requires a failed bind to leave the connection unauthenticated +// so that a client cannot hold on to access it has lost. +func (s *server) markUnbound(m *ldap.Message) { + st := existingState(s.connOf(m)) + if st == nil { + return + } + st.Lock() + defer st.Unlock() + st.bound = false +} + +func existingState(c connection) *boundState { + if c == nil { + return nil + } + st, _ := c.GetAddData().(*boundState) + return st +} diff --git a/internal/ldap/search.go b/internal/ldap/search.go index 6fdb511..21e20f9 100644 --- a/internal/ldap/search.go +++ b/internal/ldap/search.go @@ -32,6 +32,14 @@ func (s *server) handleSearchEntities(w ldap.ResponseWriter, m *ldap.Message) { ctx := context.Background() s.l.Debug("Search Entities") + if !s.bound(m) { + s.l.Warn("Search from an unauthenticated connection") + res := ldap.NewSearchResultDoneResponse(ldap.LDAPResultInsufficientAccessRights) + res.SetDiagnosticMessage("a bind is required before searching") + w.Write(res) + return + } + r := m.GetSearchRequest() expr, err := s.buildBleveQuery(r.Filter()) if err != nil { @@ -100,6 +108,14 @@ func (s *server) handleSearchGroups(w ldap.ResponseWriter, m *ldap.Message) { ctx := context.Background() s.l.Debug("Search Groups") + if !s.bound(m) { + s.l.Warn("Search from an unauthenticated connection") + res := ldap.NewSearchResultDoneResponse(ldap.LDAPResultInsufficientAccessRights) + res.SetDiagnosticMessage("a bind is required before searching") + w.Write(res) + return + } + r := m.GetSearchRequest() expr, err := s.buildBleveQuery(r.Filter()) diff --git a/internal/ldap/search_test.go b/internal/ldap/search_test.go new file mode 100644 index 0000000..f9e709f --- /dev/null +++ b/internal/ldap/search_test.go @@ -0,0 +1,78 @@ +package ldap + +import ( + "errors" + "testing" + + ldap "github.com/ps78674/ldapserver" +) + +// fakeConn stands in for the server's unexported client type so that +// per-connection bind state can be exercised in tests. +type fakeConn struct { + data interface{} +} + +func (c *fakeConn) GetAddData() interface{} { return c.data } +func (c *fakeConn) SetAddData(d interface{}) { c.data = d } + +func boundTestServer(c naClient) *server { + fc := &fakeConn{} + s := &server{l: testLogger(), c: c} + s.nc = []string{"dc=netauth", "dc=example", "dc=com"} + s.conn = func(*ldap.Message) connection { return fc } + return s +} + +func TestSearchRequiresBind(t *testing.T) { + s := boundTestServer(&fakeClient{}) + + w := &recordingWriter{} + s.handleSearchEntities(w, mustDecode(t, berSearchRequest("ou=entities,dc=netauth,dc=example,dc=com"))) + + if len(w.responses) != 1 { + t.Fatalf("got %d responses, want exactly 1", len(w.responses)) + } + if code := resultCode(t, w.responses[0]); code != ldap.LDAPResultInsufficientAccessRights { + t.Errorf("entities: result code is %d, want %d", code, ldap.LDAPResultInsufficientAccessRights) + } + + w = &recordingWriter{} + s.handleSearchGroups(w, mustDecode(t, berSearchRequest("ou=groups,dc=netauth,dc=example,dc=com"))) + if code := resultCode(t, w.responses[0]); code != ldap.LDAPResultInsufficientAccessRights { + t.Errorf("groups: result code is %d, want %d", code, ldap.LDAPResultInsufficientAccessRights) + } +} + +func TestSearchAfterBindSucceeds(t *testing.T) { + s := boundTestServer(&fakeClient{}) + + // A successful bind marks the connection, so a follow-up search + // is no longer refused for lack of authentication. + s.handleBind(&recordingWriter{}, mustDecode(t, + berBindRequest("uid=jsmith,ou=entities,dc=netauth,dc=example,dc=com", "hunter2"))) + + w := &recordingWriter{} + s.handleSearchEntities(w, mustDecode(t, berSearchRequest("ou=entities,dc=netauth,dc=example,dc=com"))) + if code := resultCode(t, w.responses[len(w.responses)-1]); code == ldap.LDAPResultInsufficientAccessRights { + t.Errorf("a bound connection was still refused") + } +} + +func TestFailedRebindClearsConnection(t *testing.T) { + c := &fakeClient{authErr: errors.New("invalid password")} + s := boundTestServer(c) + + // Mark the connection bound, then have a rebind fail; RFC 4511 + // requires the connection to drop back to unauthenticated. + s.markBound(&ldap.Message{}) + w := &recordingWriter{} + s.handleBind(w, mustDecode(t, + berBindRequest("uid=jsmith,ou=entities,dc=netauth,dc=example,dc=com", "wrong"))) + if code := resultCode(t, w.responses[0]); code != ldap.LDAPResultInvalidCredentials { + t.Fatalf("bind result code is %d, want %d", code, ldap.LDAPResultInvalidCredentials) + } + if s.bound(&ldap.Message{}) { + t.Error("connection still marked bound after a failed bind") + } +} diff --git a/internal/ldap/testutil_test.go b/internal/ldap/testutil_test.go index 80f58fd..2c2251d 100644 --- a/internal/ldap/testutil_test.go +++ b/internal/ldap/testutil_test.go @@ -39,6 +39,22 @@ func berBindRequest(dn, password string) []byte { return berElement(0x30, berInteger(1), bind) } +// berSearchRequest encodes an LDAPMessage containing a SearchRequest +// with an attribute-present filter on uid and no attribute selection. +func berSearchRequest(baseDN string) []byte { + search := berElement(0x63, // [APPLICATION 3] constructed + berOctetString(baseDN), // baseObject + berElement(0x0a, []byte{2}), // scope: wholeSubtree + berElement(0x0a, []byte{0}), // derefAliases: never + berInteger(0), // sizeLimit + berInteger(0), // timeLimit + berElement(0x01, []byte{0x00}), // typesOnly: false + berElement(0x87, []byte("uid")), // filter: (uid=*) + berElement(0x30), // attributes: empty selection + ) + return berElement(0x30, berInteger(2), search) +} + func mustDecode(t *testing.T, raw []byte) *ldap.Message { t.Helper() m, err := message.ReadLDAPMessage(message.NewBytes(0, raw)) diff --git a/internal/ldap/type.go b/internal/ldap/type.go index d36c03f..3e8cc28 100644 --- a/internal/ldap/type.go +++ b/internal/ldap/type.go @@ -28,5 +28,10 @@ type server struct { nc []string + // conn resolves the connection a request arrived on so that + // per-connection bind state can be tracked. Tests override it + // since the server's client type is unexported. + conn func(*ldap.Message) connection + allowAnon bool }