internal/ldap: Only fetch group membership when it was requested

internal/ldap/attributes.go

@@ -0,0 +1,36 @@
+package ldap
+
+import (
+	"strings"
+
+	"github.com/ps78674/goldap/message"
+)
+
+// attributeRequested reports whether an attribute selection includes
+// the named attribute.
+//
+// Per RFC 4511 section 4.5.1.8 an empty selection, or one containing
+// "*", asks for all user attributes, while "1.1" asks for none at
+// all. Anything else is an explicit list. This is used to skip the
+// extra NetAuth round trip needed for group membership when the client
+// never asked for it.
+func attributeRequested(selection message.AttributeSelection, name string) bool {
+	if len(selection) == 0 {
+		return true
+	}
+	name = strings.ToLower(name)
+	for _, sel := range selection {
+		switch s := strings.ToLower(strings.TrimSpace(string(sel))); {
+		case s == "*":
+			return true
+		case s == "1.1", s == "+":
+			continue
+		case strings.HasPrefix(s, name+";"):
+			// An attribute with options, e.g. member;binary.
+			return true
+		case s == name:
+			return true
+		}
+	}
+	return false
+}

internal/ldap/attributes_test.go

@@ -0,0 +1,139 @@
+package ldap
+
+import (
+	"testing"
+
+	"github.com/ps78674/goldap/message"
+
+	pb "github.com/netauth/protocol"
+)
+
+func strptr(s string) *string { return &s }
+func intptr(i int32) *int32   { return &i }
+
+// messageSelection builds an AttributeSelection from plain strings.
+func messageSelection(vals []string) message.AttributeSelection {
+	if vals == nil {
+		return nil
+	}
+	sel := make(message.AttributeSelection, len(vals))
+	for i, v := range vals {
+		sel[i] = message.LDAPString(v)
+	}
+	return sel
+}
+
+func testEntity(id string, number int32) *pb.Entity {
+	return &pb.Entity{ID: strptr(id), Number: intptr(number)}
+}
+
+func testGroup(name string, number int32) *pb.Group {
+	return &pb.Group{Name: strptr(name), Number: intptr(number)}
+}
+
+func TestAttributeRequested(t *testing.T) {
+	cases := []struct {
+		selection []string
+		name      string
+		want      bool
+	}{
+		{nil, "memberOf", true},
+		{[]string{"*"}, "memberOf", true},
+		{[]string{"uid", "memberOf"}, "memberOf", true},
+		{[]string{"uid"}, "memberOf", false},
+		{[]string{"UID"}, "uid", true},
+		{[]string{"1.1"}, "memberOf", false},
+		{[]string{"member;binary"}, "member", true},
+		{[]string{"uid"}, "member", false},
+	}
+	for _, tc := range cases {
+		sel := messageSelection(tc.selection)
+		if got := attributeRequested(sel, tc.name); got != tc.want {
+			t.Errorf("selection %v for %q: got %t, want %t", tc.selection, tc.name, got, tc.want)
+		}
+	}
+}
+
+// bind marks the test connection as authenticated so a search is not
+// refused before it reaches the handler logic under test.
+func bind(t *testing.T, s *server) {
+	t.Helper()
+	s.handleBind(&recordingWriter{}, mustDecode(t,
+		berBindRequest("uid=jsmith,ou=entities,dc=netauth,dc=example,dc=com", "hunter2")))
+}
+
+// Membership costs one NetAuth round trip per entry, so it must only
+// happen when the client actually asked for the attribute.
+func TestEntitySearchSkipsUnrequestedGroups(t *testing.T) {
+	c := &fakeClient{
+		entitySearchResults: []*pb.Entity{testEntity("jsmith", 1000)},
+		entityGroupsResults: map[string][]*pb.Group{"jsmith": {testGroup("admins", 10)}},
+	}
+	s := boundTestServer(c)
+	bind(t, s)
+	w := &recordingWriter{}
+
+	s.handleSearchEntities(w, mustDecode(t,
+		berSearchRequestWithAttrs("ou=entities,dc=netauth,dc=example,dc=com", "uid")))
+
+	if len(w.responses) < 2 {
+		t.Fatalf("the search did not return any entries: %d responses", len(w.responses))
+	}
+	if len(c.entityGroupsCalls) != 0 {
+		t.Errorf("EntityGroups was called %d times for a search that did not request memberOf", len(c.entityGroupsCalls))
+	}
+}
+
+func TestEntitySearchFetchesRequestedGroups(t *testing.T) {
+	c := &fakeClient{
+		entitySearchResults: []*pb.Entity{testEntity("jsmith", 1000)},
+		entityGroupsResults: map[string][]*pb.Group{"jsmith": {testGroup("admins", 10)}},
+	}
+	s := boundTestServer(c)
+	bind(t, s)
+	w := &recordingWriter{}
+
+	s.handleSearchEntities(w, mustDecode(t,
+		berSearchRequestWithAttrs("ou=entities,dc=netauth,dc=example,dc=com", "uid", "memberOf")))
+
+	if len(c.entityGroupsCalls) != 1 {
+		t.Errorf("EntityGroups was called %d times, want 1", len(c.entityGroupsCalls))
+	}
+}
+
+func TestGroupSearchSkipsUnrequestedMembers(t *testing.T) {
+	c := &fakeClient{
+		groupSearchResults:  []*pb.Group{testGroup("admins", 10)},
+		groupMembersResults: map[string][]*pb.Entity{"admins": {testEntity("jsmith", 1000)}},
+	}
+	s := boundTestServer(c)
+	bind(t, s)
+	w := &recordingWriter{}
+
+	s.handleSearchGroups(w, mustDecode(t,
+		berSearchRequestWithAttrs("ou=groups,dc=netauth,dc=example,dc=com", "cn")))
+
+	if len(w.responses) < 2 {
+		t.Fatalf("the search did not return any entries: %d responses", len(w.responses))
+	}
+	if len(c.groupMembersCalls) != 0 {
+		t.Errorf("GroupMembers was called %d times for a search that did not request member", len(c.groupMembersCalls))
+	}
+}
+
+func TestGroupSearchFetchesRequestedMembers(t *testing.T) {
+	c := &fakeClient{
+		groupSearchResults:  []*pb.Group{testGroup("admins", 10)},
+		groupMembersResults: map[string][]*pb.Entity{"admins": {testEntity("jsmith", 1000)}},
+	}
+	s := boundTestServer(c)
+	bind(t, s)
+	w := &recordingWriter{}
+
+	s.handleSearchGroups(w, mustDecode(t,
+		berSearchRequestWithAttrs("ou=groups,dc=netauth,dc=example,dc=com", "cn", "member")))
+
+	if len(c.groupMembersCalls) != 1 {
+		t.Errorf("GroupMembers was called %d times, want 1", len(c.groupMembersCalls))
+	}
+}

internal/ldap/bind_test.go

@@ -15,16 +15,34 @@ var _ naClient = (*fakeClient)(nil)
 type fakeClient struct {
 	authErr   error
 	authCalls []string
+
+	entitySearchResults []*pb.Entity
+	groupSearchResults  []*pb.Group
+	entityGroupsResults map[string][]*pb.Group
+	groupMembersResults map[string][]*pb.Entity
+
+	entityGroupsCalls []string
+	groupMembersCalls []string
 }
 
 func (f *fakeClient) AuthEntity(ctx context.Context, id, password string) error {
 	f.authCalls = append(f.authCalls, id)
 	return f.authErr
 }
-func (f *fakeClient) EntitySearch(context.Context, string) ([]*pb.Entity, error) { return nil, nil }
+func (f *fakeClient) EntitySearch(context.Context, string) ([]*pb.Entity, error) {
-func (f *fakeClient) EntityGroups(context.Context, string) ([]*pb.Group, error)  { return nil, nil }
+	return f.entitySearchResults, nil
-func (f *fakeClient) GroupSearch(context.Context, string) ([]*pb.Group, error)   { return nil, nil }
+}
-func (f *fakeClient) GroupMembers(context.Context, string) ([]*pb.Entity, error) { return nil, nil }
+func (f *fakeClient) EntityGroups(_ context.Context, id string) ([]*pb.Group, error) {
+	f.entityGroupsCalls = append(f.entityGroupsCalls, id)
+	return f.entityGroupsResults[id], nil
+}
+func (f *fakeClient) GroupSearch(context.Context, string) ([]*pb.Group, error) {
+	return f.groupSearchResults, nil
+}
+func (f *fakeClient) GroupMembers(_ context.Context, name string) ([]*pb.Entity, error) {
+	f.groupMembersCalls = append(f.groupMembersCalls, name)
+	return f.groupMembersResults[name], nil
+}
 
 func TestAnonBindSingleResponse(t *testing.T) {
 	c := &fakeClient{}

internal/ldap/search.go

@@ -79,16 +79,19 @@ func (s *server) handleSearchEntities(w ldap.ResponseWriter, m *ldap.Message) {
 	w.Write(res)
 }
 
-// entitySearchResult maps an entity onto a SearchResultEntry,
+// entitySearchResult maps an entity onto a SearchResultEntry.  The
-// performing the additional lookup for groups to populate the
+// additional lookup needed to populate memberOf is only performed when
-// memberOf attribute.  Though not implemented, the attrs list is
+// the client actually asked for that attribute, since it costs one
-// plumbed down to this level to permit attribute filtering in the
+// NetAuth round trip per entity.
-// future.
 func (s *server) entitySearchResult(ctx context.Context, e *pb.Entity, dn message.LDAPDN, attrs message.AttributeSelection) (message.SearchResultEntry, error) {
 	res := ldap.NewSearchResultEntry("uid=" + e.GetID() + "," + string(dn))
 	res.AddAttribute("uid", message.AttributeValue(e.GetID()))
 	res.AddAttribute("uidNumber", message.AttributeValue(strconv.Itoa(int(e.GetNumber()))))
 
+	if !attributeRequested(attrs, "memberOf") {
+		return res, nil
+	}
+
 	grps, err := s.c.EntityGroups(ctx, e.GetID())
 	if err != nil {
 		return res, err
@@ -155,15 +158,19 @@ func (s *server) handleSearchGroups(w ldap.ResponseWriter, m *ldap.Message) {
 	w.Write(res)
 }
 
-// groupSearchResult maps a group onto a SearchResultEntry, performing
+// groupSearchResult maps a group onto a SearchResultEntry.  The
-// the additional lookup for groups to populate the member attribute.
+// additional lookup needed to populate member is only performed when
-// Though not implemented, the attrs list is plumbed down to this
+// the client actually asked for that attribute, since it costs one
-// level to permit attribute filtering in the future.
+// NetAuth round trip per group.
 func (s *server) groupSearchResult(ctx context.Context, g *pb.Group, dn message.LDAPDN, attrs message.AttributeSelection) (message.SearchResultEntry, error) {
 	res := ldap.NewSearchResultEntry("cn=" + g.GetName() + "," + string(dn))
 	res.AddAttribute("cn", message.AttributeValue(g.GetName()))
 	res.AddAttribute("gidNumber", message.AttributeValue(strconv.Itoa(int(g.GetNumber()))))
 
+	if !attributeRequested(attrs, "member") {
+		return res, nil
+	}
+
 	members, err := s.c.GroupMembers(ctx, g.GetName())
 	if err != nil {
 		return res, err

internal/ldap/testutil_test.go

@@ -39,20 +39,30 @@ func berBindRequest(dn, password string) []byte {
 	return berElement(0x30, berInteger(1), bind)
 }
 
+// berSearchRequestWithAttrs encodes a SearchRequest for the given base
+// DN with the given attribute selection.
+func berSearchRequestWithAttrs(baseDN string, attrs ...string) []byte {
+	selection := make([][]byte, 0, len(attrs))
+	for _, a := range attrs {
+		selection = append(selection, berOctetString(a))
+	}
+	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, selection...),  // attributes
+	)
+	return berElement(0x30, berInteger(2), search)
+}
+
 // 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
+	return berSearchRequestWithAttrs(baseDN)
-		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)
 }
 
 // berEqualityFilter encodes an equalityMatch [3]