From 444c4d59991bd950d16046104a58b5e5fb248d68 Mon Sep 17 00:00:00 2001 From: Oleg Gaidarenko Date: Wed, 3 Jul 2019 17:39:54 +0300 Subject: [PATCH] LDAP: Divide the requests (#17885) * LDAP: Divide the requests Active Directory does indeed have a limitation with 1000 results per search (default of course). However, that limitation can be workaround with the pagination search feature, meaning `pagination` number is how many times LDAP compatible server will be requested by the client with specified amount of users (like 1000). That feature already embeded with LDAP compatible client (including our `go-ldap`). But slapd server has by default stricter settings. First, limitation is not 1000 but 500, second, pagination workaround presumably (information about it a bit scarce and I still not sure on some of the details from my own testing) cannot be workaround with pagination feature. See https://www.openldap.org/doc/admin24/limits.html https://serverfault.com/questions/328671/paging-using-ldapsearch hashicorp/vault#4162 - not sure why they were hitting the limit in the first place, since `go-ldap` doesn't have one by default. But, given all that, for me `ldapsearch` command with same request as with `go-ldap` still returns more then 500 results, it can even return as much as 10500 items (probably more). So either there is some differences with implementation of the LDAP search between `go-ldap` module and `ldapsearch` or I am missing a step :/. In the wild (see serverfault link), apparently, people still hitting that limitation even with `ldapsearch`, so it still seems to be an issue. But, nevertheless, I'm still confused by this incoherence. To workaround it, I divide the request by no more then 500 items per search --- pkg/services/ldap/ldap.go | 83 +++++++++++++++---- pkg/services/ldap/ldap_helpers_test.go | 79 ++++++++++++++++++ pkg/services/ldap/ldap_private_test.go | 4 +- .../ldap/{ldap_test_mocks.go => testing.go} | 0 4 files changed, 148 insertions(+), 18 deletions(-) rename pkg/services/ldap/{ldap_test_mocks.go => testing.go} (100%) diff --git a/pkg/services/ldap/ldap.go b/pkg/services/ldap/ldap.go index d53d9166602..bea06b45a2d 100644 --- a/pkg/services/ldap/ldap.go +++ b/pkg/services/ldap/ldap.go @@ -6,11 +6,12 @@ import ( "errors" "fmt" "io/ioutil" + "math" "strings" + "github.com/davecgh/go-spew/spew" "gopkg.in/ldap.v3" - "github.com/davecgh/go-spew/spew" "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/models" @@ -43,6 +44,11 @@ type Server struct { log log.Logger } +// UsersMaxRequest is a max amount of users we can request via Users(). +// Since many LDAP servers has limitations +// on how much items can we return in one request +const UsersMaxRequest = 500 + var ( // ErrInvalidCredentials is returned if username and password do not match @@ -148,10 +154,67 @@ func (server *Server) Login(query *models.LoginUserQuery) ( return user, nil } +// getUsersIteration is a helper function for Users() method. +// It divides the users by equal parts for the anticipated requests +func getUsersIteration(logins []string, fn func(int, int) error) error { + lenLogins := len(logins) + iterations := int( + math.Ceil( + float64(lenLogins) / float64(UsersMaxRequest), + ), + ) + + for i := 1; i < iterations+1; i++ { + previous := float64(UsersMaxRequest * (i - 1)) + current := math.Min(float64(i*UsersMaxRequest), float64(lenLogins)) + + err := fn(int(previous), int(current)) + if err != nil { + return err + } + } + + return nil +} + // Users gets LDAP users func (server *Server) Users(logins []string) ( []*models.ExternalUserInfo, error, +) { + var users []*ldap.Entry + err := getUsersIteration(logins, func(previous, current int) error { + entries, err := server.users(logins[previous:current]) + if err != nil { + return err + } + + users = append(users, entries...) + + return nil + }) + if err != nil { + return nil, err + } + + if len(users) == 0 { + return []*models.ExternalUserInfo{}, nil + } + + serializedUsers, err := server.serializeUsers(users) + if err != nil { + return nil, err + } + + server.log.Debug("LDAP users found", "users", spew.Sdump(serializedUsers)) + + return serializedUsers, nil +} + +// users is helper method for the Users() +func (server *Server) users(logins []string) ( + []*ldap.Entry, + error, ) { var result *ldap.SearchResult var Config = server.Config @@ -170,18 +233,7 @@ func (server *Server) Users(logins []string) ( } } - if len(result.Entries) == 0 { - return []*models.ExternalUserInfo{}, nil - } - - serializedUsers, err := server.serializeUsers(result) - if err != nil { - return nil, err - } - - server.log.Debug("LDAP users found", "users", spew.Sdump(serializedUsers)) - - return serializedUsers, nil + return result.Entries, nil } // validateGrafanaUser validates user access. @@ -261,7 +313,6 @@ func (server *Server) getSearchRequest( return &ldap.SearchRequest{ BaseDN: base, Scope: ldap.ScopeWholeSubtree, - SizeLimit: 1000, DerefAliases: ldap.NeverDerefAliases, Attributes: attributes, Filter: filter, @@ -407,11 +458,11 @@ func (server *Server) requestMemberOf(entry *ldap.Entry) ([]string, error) { // serializeUsers serializes the users // from LDAP result to ExternalInfo struct func (server *Server) serializeUsers( - users *ldap.SearchResult, + entries []*ldap.Entry, ) ([]*models.ExternalUserInfo, error) { var serialized []*models.ExternalUserInfo - for _, user := range users.Entries { + for _, user := range entries { extUser, err := server.buildGrafanaUser(user) if err != nil { return nil, err diff --git a/pkg/services/ldap/ldap_helpers_test.go b/pkg/services/ldap/ldap_helpers_test.go index 0f30f228fc4..b9f49c49644 100644 --- a/pkg/services/ldap/ldap_helpers_test.go +++ b/pkg/services/ldap/ldap_helpers_test.go @@ -25,6 +25,85 @@ func TestLDAPHelpers(t *testing.T) { }) }) + Convey("getUsersIteration()", t, func() { + Convey("it should execute twice for 600 users", func() { + logins := make([]string, 600) + i := 0 + + result := getUsersIteration(logins, func(previous, current int) error { + i++ + + if i == 1 { + So(previous, ShouldEqual, 0) + So(current, ShouldEqual, 500) + } else { + So(previous, ShouldEqual, 500) + So(current, ShouldEqual, 600) + } + + return nil + }) + + So(i, ShouldEqual, 2) + So(result, ShouldBeNil) + }) + + Convey("it should execute three times for 1500 users", func() { + logins := make([]string, 1500) + i := 0 + + result := getUsersIteration(logins, func(previous, current int) error { + i++ + if i == 1 { + So(previous, ShouldEqual, 0) + So(current, ShouldEqual, 500) + } else if i == 2 { + So(previous, ShouldEqual, 500) + So(current, ShouldEqual, 1000) + } else { + So(previous, ShouldEqual, 1000) + So(current, ShouldEqual, 1500) + } + + return nil + }) + + So(i, ShouldEqual, 3) + So(result, ShouldBeNil) + }) + + Convey("it should execute once for 400 users", func() { + logins := make([]string, 400) + i := 0 + + result := getUsersIteration(logins, func(previous, current int) error { + i++ + if i == 1 { + So(previous, ShouldEqual, 0) + So(current, ShouldEqual, 400) + } + + return nil + }) + + So(i, ShouldEqual, 1) + So(result, ShouldBeNil) + }) + + Convey("it should not execute for 0 users", func() { + logins := make([]string, 0) + i := 0 + + result := getUsersIteration(logins, func(previous, current int) error { + i++ + return nil + }) + + So(i, ShouldEqual, 0) + So(result, ShouldBeNil) + }) + }) + Convey("getAttribute()", t, func() { Convey("Should get username", func() { value := []string{"roelgerrits"} diff --git a/pkg/services/ldap/ldap_private_test.go b/pkg/services/ldap/ldap_private_test.go index 83de153ceab..d7e8c57aeb9 100644 --- a/pkg/services/ldap/ldap_private_test.go +++ b/pkg/services/ldap/ldap_private_test.go @@ -37,7 +37,7 @@ func TestLDAPPrivateMethods(t *testing.T) { {Name: "memberof", Values: []string{"admins"}}, }, } - users := &ldap.SearchResult{Entries: []*ldap.Entry{&entry}} + users := []*ldap.Entry{&entry} result, err := server.serializeUsers(users) @@ -71,7 +71,7 @@ func TestLDAPPrivateMethods(t *testing.T) { {Name: "memberof", Values: []string{"admins"}}, }, } - users := &ldap.SearchResult{Entries: []*ldap.Entry{&entry}} + users := []*ldap.Entry{&entry} result, err := server.serializeUsers(users) diff --git a/pkg/services/ldap/ldap_test_mocks.go b/pkg/services/ldap/testing.go similarity index 100% rename from pkg/services/ldap/ldap_test_mocks.go rename to pkg/services/ldap/testing.go