diff --git a/internal/ldap/attributes.go b/internal/ldap/attributes.go new file mode 100644 index 0000000..a8a7359 --- /dev/null +++ b/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 +} diff --git a/internal/ldap/attributes_test.go b/internal/ldap/attributes_test.go new file mode 100644 index 0000000..cab78dc --- /dev/null +++ b/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)) + } +} diff --git a/internal/ldap/bind_test.go b/internal/ldap/bind_test.go index 3213a85..1d0df53 100644 --- a/internal/ldap/bind_test.go +++ b/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) EntityGroups(context.Context, string) ([]*pb.Group, error) { return nil, 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) EntitySearch(context.Context, string) ([]*pb.Entity, error) { + return f.entitySearchResults, 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{} diff --git a/internal/ldap/search.go b/internal/ldap/search.go index 21e20f9..fdefc07 100644 --- a/internal/ldap/search.go +++ b/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, -// performing the additional lookup for groups to populate the -// memberOf attribute. Though not implemented, the attrs list is -// plumbed down to this level to permit attribute filtering in the -// future. +// entitySearchResult maps an entity onto a SearchResultEntry. The +// additional lookup needed to populate memberOf is only performed when +// the client actually asked for that attribute, since it costs one +// NetAuth round trip per entity. 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 -// the additional lookup for groups to populate the member attribute. -// Though not implemented, the attrs list is plumbed down to this -// level to permit attribute filtering in the future. +// groupSearchResult maps a group onto a SearchResultEntry. The +// additional lookup needed to populate member is only performed when +// the client actually asked for that attribute, since it costs one +// 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 diff --git a/internal/ldap/testutil_test.go b/internal/ldap/testutil_test.go index ab291dc..5054f1d 100644 --- a/internal/ldap/testutil_test.go +++ b/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 - 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) + return berSearchRequestWithAttrs(baseDN) } // berEqualityFilter encodes an equalityMatch [3]