From e2cf7c9698ed319c05f911ad0ad5bf75f2fbb567 Mon Sep 17 00:00:00 2001 From: Oleg Gaidarenko Date: Fri, 5 Jul 2019 17:49:00 +0300 Subject: [PATCH] LDAP: finishing touches (#17945) * LDAP:Docs: `active_sync_enabled` setting Mention `active_sync_enabled` setting and enable it by default * LDAP: move "disableExternalUser" method Idea behind new design of the LDAP module is to minimise conflation between other parts of the system, so it would decoupled as much as possible from stuff like database, HTTP transport and etc. Following "Do One Thing and Do It Well" Unix philosophy principal, other things could be better fitted on the consumer side of things. Which what this commit trying to archive * LDAP: correct user/admin binding The second binding was not happening, so if the admin login/password in LDAP configuration was correct, anyone could had login as anyone using incorrect password --- conf/defaults.ini | 2 +- conf/sample.ini | 2 +- docs/sources/auth/enhanced_ldap.md | 7 +- pkg/login/ldap_login.go | 52 +++++++++++ pkg/services/ldap/ldap.go | 118 ++++++++++++++----------- pkg/services/ldap/ldap_login_test.go | 95 ++++++++++++++++++-- pkg/services/ldap/ldap_private_test.go | 25 ++++++ pkg/services/ldap/ldap_test.go | 88 ++++++++++++------ pkg/services/ldap/testing.go | 19 ++-- 9 files changed, 311 insertions(+), 97 deletions(-) diff --git a/conf/defaults.ini b/conf/defaults.ini index c9eff7d047c..5c892840a67 100644 --- a/conf/defaults.ini +++ b/conf/defaults.ini @@ -413,7 +413,7 @@ allow_sign_up = true # LDAP backround sync (Enterprise only) # At 1 am every day sync_cron = "0 0 1 * * *" -active_sync_enabled = false +active_sync_enabled = true #################################### SMTP / Emailing ##################### [smtp] diff --git a/conf/sample.ini b/conf/sample.ini index 39b81f897aa..fc6c0b11caa 100644 --- a/conf/sample.ini +++ b/conf/sample.ini @@ -378,7 +378,7 @@ # LDAP backround sync (Enterprise only) # At 1 am every day ;sync_cron = "0 0 1 * * *" -;active_sync_enabled = false +;active_sync_enabled = true #################################### SMTP / Emailing ########################## [smtp] diff --git a/docs/sources/auth/enhanced_ldap.md b/docs/sources/auth/enhanced_ldap.md index 1a9aa9d7241..885f0478f80 100644 --- a/docs/sources/auth/enhanced_ldap.md +++ b/docs/sources/auth/enhanced_ldap.md @@ -44,7 +44,7 @@ a user as member of a team and it will not be removed when the user signs in. Th ## Active LDAP Synchronization -> Only available in Grafana Enterprise v6.3+ +> Only available in Grafana Enterprise v6.3+ In the open source version of Grafana, user data from LDAP will be synchronized only during the login process when authenticating using LDAP. @@ -60,8 +60,11 @@ With this feature you can configure Grafana to actively sync users with LDAP ser # @weekly | Run once a week, midnight between Sat/Sun | 0 0 0 * * 0 # @daily (or @midnight) | Run once a day, midnight | 0 0 0 * * * # @hourly | Run once an hour, beginning of hour | 0 0 * * * * -sync_cron = "@hourly" +sync_cron = "0 0 1 * * *" # This is default value (At 1 am every day) # This cron expression format uses 6 space-separated fields (including seconds), for example # sync_cron = "* */10 * * * *" # This will run the LDAP Synchronization every 10th minute, which is also the minimal interval between the grafana sync times i.e. you cannot set it for every 9th minute + +# You can also disable active LDAP synchronization +active_sync_enabled = true # enabled by default ``` diff --git a/pkg/login/ldap_login.go b/pkg/login/ldap_login.go index 4312355f924..daf0dfe93f6 100644 --- a/pkg/login/ldap_login.go +++ b/pkg/login/ldap_login.go @@ -2,7 +2,9 @@ package login import ( "github.com/grafana/grafana/pkg/bus" + "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/ldap" "github.com/grafana/grafana/pkg/services/multildap" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/util/errutil" @@ -17,6 +19,9 @@ var isLDAPEnabled = multildap.IsEnabled // newLDAP creates multiple LDAP instance var newLDAP = multildap.New +// logger for the LDAP auth +var logger = log.New("login.ldap") + // loginUsingLDAP logs in user using LDAP. It returns whether LDAP is enabled and optional error and query arg will be // populated with the logged in user if successful. var loginUsingLDAP = func(query *models.LoginUserQuery) (bool, error) { @@ -33,6 +38,13 @@ var loginUsingLDAP = func(query *models.LoginUserQuery) (bool, error) { externalUser, err := newLDAP(config.Servers).Login(query) if err != nil { + if err == ldap.ErrCouldNotFindUser { + // Ignore the error since user might not be present anyway + disableExternalUser(query.Username) + + return true, ldap.ErrInvalidCredentials + } + return true, err } @@ -48,3 +60,43 @@ var loginUsingLDAP = func(query *models.LoginUserQuery) (bool, error) { return true, nil } + +// disableExternalUser marks external user as disabled in Grafana db +func disableExternalUser(username string) error { + // Check if external user exist in Grafana + userQuery := &models.GetExternalUserInfoByLoginQuery{ + LoginOrEmail: username, + } + + if err := bus.Dispatch(userQuery); err != nil { + return err + } + + userInfo := userQuery.Result + if !userInfo.IsDisabled { + + logger.Debug( + "Disabling external user", + "user", + userQuery.Result.Login, + ) + + // Mark user as disabled in grafana db + disableUserCmd := &models.DisableUserCommand{ + UserId: userQuery.Result.UserId, + IsDisabled: true, + } + + if err := bus.Dispatch(disableUserCmd); err != nil { + logger.Debug( + "Error disabling external user", + "user", + userQuery.Result.Login, + "message", + err.Error(), + ) + return err + } + } + return nil +} diff --git a/pkg/services/ldap/ldap.go b/pkg/services/ldap/ldap.go index bea06b45a2d..0e4404340b5 100644 --- a/pkg/services/ldap/ldap.go +++ b/pkg/services/ldap/ldap.go @@ -12,7 +12,6 @@ import ( "github.com/davecgh/go-spew/spew" "gopkg.in/ldap.v3" - "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/models" ) @@ -53,6 +52,9 @@ var ( // ErrInvalidCredentials is returned if username and password do not match ErrInvalidCredentials = errors.New("Invalid Username or Password") + + // ErrCouldNotFindUser is returned when username hasn't been found (not username+password) + ErrCouldNotFindUser = errors.New("Can't find user in LDAP") ) // New creates the new LDAP auth @@ -123,14 +125,35 @@ func (server *Server) Close() { server.Connection.Close() } -// Login user by searching and serializing it +// Login the user. +// There is several cases - +// 1. First we check if we need to authenticate the admin user. +// That user should have search privileges. +// 2. For some configurations it is allowed to search the +// user without any authenfication, in such case we +// perform "unauthenticated bind". +// -- +// After either first or second step is done we find the user DN +// by its username, after that, we then combine it with user password and +// then try to authentificate that user func (server *Server) Login(query *models.LoginUserQuery) ( *models.ExternalUserInfo, error, ) { - // Authentication - err := server.Auth(query.Username, query.Password) - if err != nil { - return nil, err + var err error + + // Do we need to authenticate the "admin" user first? + // Admin user should have access for the user search in LDAP server + if server.shouldAuthAdmin() { + if err := server.AuthAdmin(); err != nil { + return nil, err + } + + // Or if anyone can perform the search in LDAP? + } else { + err := server.Connection.UnauthenticatedBind(server.Config.BindDN) + if err != nil { + return nil, err + } } // Find user entry & attributes @@ -142,8 +165,7 @@ func (server *Server) Login(query *models.LoginUserQuery) ( // If we couldn't find the user - // we should show incorrect credentials err if len(users) == 0 { - server.disableExternalUser(query.Username) - return nil, ErrInvalidCredentials + return nil, ErrCouldNotFindUser } user := users[0] @@ -151,6 +173,12 @@ func (server *Server) Login(query *models.LoginUserQuery) ( return nil, err } + // Authenticate user + err = server.Auth(user.AuthId, query.Password) + if err != nil { + return nil, err + } + return user, nil } @@ -252,34 +280,6 @@ func (server *Server) validateGrafanaUser(user *models.ExternalUserInfo) error { return nil } -// disableExternalUser marks external user as disabled in Grafana db -func (server *Server) disableExternalUser(username string) error { - // Check if external user exist in Grafana - userQuery := &models.GetExternalUserInfoByLoginQuery{ - LoginOrEmail: username, - } - - if err := bus.Dispatch(userQuery); err != nil { - return err - } - - userInfo := userQuery.Result - if !userInfo.IsDisabled { - server.log.Debug("Disabling external user", "user", userQuery.Result.Login) - // Mark user as disabled in grafana db - disableUserCmd := &models.DisableUserCommand{ - UserId: userQuery.Result.UserId, - IsDisabled: true, - } - - if err := bus.Dispatch(disableUserCmd); err != nil { - server.log.Debug("Error disabling external user", "user", userQuery.Result.Login, "message", err.Error()) - return err - } - } - return nil -} - // getSearchRequest returns LDAP search request for users func (server *Server) getSearchRequest( base string, @@ -360,32 +360,46 @@ func (server *Server) buildGrafanaUser(user *ldap.Entry) (*models.ExternalUserIn return extUser, nil } -// shouldBindAdmin checks if we should use +// shouldAuthAdmin checks if we should use // admin username & password for LDAP bind -func (server *Server) shouldBindAdmin() bool { +func (server *Server) shouldAuthAdmin() bool { return server.Config.BindPassword != "" } -// Auth authentificates user in LDAP. -// It might not use passed password and username, -// since they can be overwritten with admin config values - -// see "bind_dn" and "bind_password" options in LDAP config +// Auth authentificates user in LDAP func (server *Server) Auth(username, password string) error { - path := server.Config.BindDN - - if server.shouldBindAdmin() { - password = server.Config.BindPassword - } else { - path = fmt.Sprintf(path, username) + err := server.auth(username, password) + if err != nil { + server.log.Error( + fmt.Sprintf("Cannot authentificate user %s in LDAP", username), + "error", + err, + ) + return err } - bindFn := func() error { - return server.Connection.Bind(path, password) + return nil +} + +// AuthAdmin authentificates LDAP admin user +func (server *Server) AuthAdmin() error { + err := server.auth(server.Config.BindDN, server.Config.BindPassword) + if err != nil { + server.log.Error( + "Cannot authentificate admin user in LDAP", + "error", + err, + ) + return err } - if err := bindFn(); err != nil { - server.log.Error("Cannot authentificate in LDAP", "err", err) + return nil +} +// auth is helper for several types of LDAP authentification +func (server *Server) auth(path, password string) error { + err := server.Connection.Bind(path, password) + if err != nil { if ldapErr, ok := err.(*ldap.Error); ok { if ldapErr.ResultCode == 49 { return ErrInvalidCredentials diff --git a/pkg/services/ldap/ldap_login_test.go b/pkg/services/ldap/ldap_login_test.go index 162cba53c91..1828eeb9d7b 100644 --- a/pkg/services/ldap/ldap_login_test.go +++ b/pkg/services/ldap/ldap_login_test.go @@ -4,10 +4,11 @@ import ( "errors" "testing" - "github.com/grafana/grafana/pkg/infra/log" - "github.com/grafana/grafana/pkg/models" . "github.com/smartystreets/goconvey/convey" "gopkg.in/ldap.v3" + + "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/models" ) func TestLDAPLogin(t *testing.T) { @@ -24,7 +25,7 @@ func TestLDAPLogin(t *testing.T) { result := ldap.SearchResult{Entries: []*ldap.Entry{&entry}} connection.setSearchResult(&result) - connection.bindProvider = func(username, password string) error { + connection.BindProvider = func(username, password string) error { return &ldap.Error{ ResultCode: 49, } @@ -42,12 +43,12 @@ func TestLDAPLogin(t *testing.T) { So(err, ShouldEqual, ErrInvalidCredentials) }) - Convey("Returns an error when search hasn't find anything", func() { + Convey("Returns an error when search didn't find anything", func() { connection := &MockConnection{} result := ldap.SearchResult{Entries: []*ldap.Entry{}} connection.setSearchResult(&result) - connection.bindProvider = func(username, password string) error { + connection.BindProvider = func(username, password string) error { return nil } server := &Server{ @@ -60,7 +61,7 @@ func TestLDAPLogin(t *testing.T) { _, err := server.Login(defaultLogin) - So(err, ShouldEqual, ErrInvalidCredentials) + So(err, ShouldEqual, ErrCouldNotFindUser) }) Convey("When search returns an error", func() { @@ -68,7 +69,7 @@ func TestLDAPLogin(t *testing.T) { expected := errors.New("Killa-gorilla") connection.setSearchError(expected) - connection.bindProvider = func(username, password string) error { + connection.BindProvider = func(username, password string) error { return nil } server := &Server{ @@ -98,7 +99,7 @@ func TestLDAPLogin(t *testing.T) { result := ldap.SearchResult{Entries: []*ldap.Entry{&entry}} connection.setSearchResult(&result) - connection.bindProvider = func(username, password string) error { + connection.BindProvider = func(username, password string) error { return nil } server := &Server{ @@ -119,5 +120,83 @@ func TestLDAPLogin(t *testing.T) { So(err, ShouldBeNil) So(resp.Login, ShouldEqual, "markelog") }) + + Convey("Should perform unauthentificate bind without admin", func() { + connection := &MockConnection{} + entry := ldap.Entry{ + DN: "test", + } + result := ldap.SearchResult{Entries: []*ldap.Entry{&entry}} + connection.setSearchResult(&result) + + connection.UnauthenticatedBindProvider = func() error { + return nil + } + server := &Server{ + Config: &ServerConfig{ + SearchBaseDNs: []string{"BaseDNHere"}, + }, + Connection: connection, + log: log.New("test-logger"), + } + + user, err := server.Login(defaultLogin) + + So(err, ShouldBeNil) + So(user.AuthId, ShouldEqual, "test") + So(connection.UnauthenticatedBindCalled, ShouldBeTrue) + }) + + Convey("Should perform authentificate binds", func() { + connection := &MockConnection{} + entry := ldap.Entry{ + DN: "test", + } + result := ldap.SearchResult{Entries: []*ldap.Entry{&entry}} + connection.setSearchResult(&result) + + adminUsername := "" + adminPassword := "" + username := "" + password := "" + + i := 0 + connection.BindProvider = func(name, pass string) error { + i++ + if i == 1 { + adminUsername = name + adminPassword = pass + } + + if i == 2 { + username = name + password = pass + } + + return nil + } + server := &Server{ + Config: &ServerConfig{ + BindDN: "killa", + BindPassword: "gorilla", + SearchBaseDNs: []string{"BaseDNHere"}, + }, + Connection: connection, + log: log.New("test-logger"), + } + + user, err := server.Login(defaultLogin) + + So(err, ShouldBeNil) + + So(user.AuthId, ShouldEqual, "test") + So(connection.BindCalled, ShouldBeTrue) + + So(adminUsername, ShouldEqual, "killa") + So(adminPassword, ShouldEqual, "gorilla") + + So(username, ShouldEqual, "test") + So(password, ShouldEqual, "pwd") + }) }) } diff --git a/pkg/services/ldap/ldap_private_test.go b/pkg/services/ldap/ldap_private_test.go index d7e8c57aeb9..f1ec97c176b 100644 --- a/pkg/services/ldap/ldap_private_test.go +++ b/pkg/services/ldap/ldap_private_test.go @@ -143,4 +143,29 @@ func TestLDAPPrivateMethods(t *testing.T) { So(result, ShouldBeNil) }) }) + + Convey("shouldAuthAdmin()", t, func() { + Convey("it should require admin auth", func() { + server := &Server{ + Config: &ServerConfig{ + BindPassword: "test", + }, + } + + result := server.shouldAuthAdmin() + So(result, ShouldBeTrue) + }) + + Convey("it should not require admin auth", func() { + server := &Server{ + Config: &ServerConfig{ + BindPassword: "", + }, + } + + result := server.shouldAuthAdmin() + So(result, ShouldBeFalse) + }) + }) + } diff --git a/pkg/services/ldap/ldap_test.go b/pkg/services/ldap/ldap_test.go index 3aca78f2e9e..8207e0d3187 100644 --- a/pkg/services/ldap/ldap_test.go +++ b/pkg/services/ldap/ldap_test.go @@ -103,10 +103,10 @@ func TestPublicAPI(t *testing.T) { }) Convey("Auth()", t, func() { - Convey("Should ignore passsed username and password", func() { + Convey("Should use provided DN and password", func() { connection := &MockConnection{} var actualUsername, actualPassword string - connection.bindProvider = func(username, password string) error { + connection.BindProvider = func(username, password string) error { actualUsername = username actualPassword = password return nil @@ -114,33 +114,15 @@ func TestPublicAPI(t *testing.T) { server := &Server{ Connection: connection, Config: &ServerConfig{ - BindDN: "cn=admin,dc=grafana,dc=org", - BindPassword: "bindpwd", + BindDN: "cn=admin,dc=grafana,dc=org", }, } - err := server.Auth("user", "pwd") - So(err, ShouldBeNil) - So(actualUsername, ShouldEqual, "cn=admin,dc=grafana,dc=org") - So(actualPassword, ShouldEqual, "bindpwd") - }) - Convey("Given bind dn configured", func() { - connection := &MockConnection{} - var actualUsername, actualPassword string - connection.bindProvider = func(username, password string) error { - actualUsername = username - actualPassword = password - return nil - } - server := &Server{ - Connection: connection, - Config: &ServerConfig{ - BindDN: "cn=%s,o=users,dc=grafana,dc=org", - }, - } - err := server.Auth("user", "pwd") + dn := "cn=user,ou=users,dc=grafana,dc=org" + err := server.Auth(dn, "pwd") + So(err, ShouldBeNil) - So(actualUsername, ShouldEqual, "cn=user,o=users,dc=grafana,dc=org") + So(actualUsername, ShouldEqual, dn) So(actualPassword, ShouldEqual, "pwd") }) @@ -149,13 +131,13 @@ func TestPublicAPI(t *testing.T) { expected := &ldap.Error{ ResultCode: uint16(25), } - connection.bindProvider = func(username, password string) error { + connection.BindProvider = func(username, password string) error { return expected } server := &Server{ Connection: connection, Config: &ServerConfig{ - BindDN: "cn=%s,o=users,dc=grafana,dc=org", + BindDN: "cn=%s,ou=users,dc=grafana,dc=org", }, log: log.New("test-logger"), } @@ -163,4 +145,56 @@ func TestPublicAPI(t *testing.T) { So(err, ShouldEqual, expected) }) }) + + Convey("AuthAdmin()", t, func() { + Convey("Should use admin DN and password", func() { + connection := &MockConnection{} + var actualUsername, actualPassword string + connection.BindProvider = func(username, password string) error { + actualUsername = username + actualPassword = password + return nil + } + + dn := "cn=admin,dc=grafana,dc=org" + + server := &Server{ + Connection: connection, + Config: &ServerConfig{ + BindPassword: "pwd", + BindDN: dn, + }, + } + + err := server.AuthAdmin() + + So(err, ShouldBeNil) + So(actualUsername, ShouldEqual, dn) + So(actualPassword, ShouldEqual, "pwd") + }) + + Convey("Should handle an error", func() { + connection := &MockConnection{} + expected := &ldap.Error{ + ResultCode: uint16(25), + } + connection.BindProvider = func(username, password string) error { + return expected + } + + dn := "cn=admin,dc=grafana,dc=org" + + server := &Server{ + Connection: connection, + Config: &ServerConfig{ + BindPassword: "pwd", + BindDN: dn, + }, + log: log.New("test-logger"), + } + + err := server.AuthAdmin() + So(err, ShouldEqual, expected) + }) + }) } diff --git a/pkg/services/ldap/testing.go b/pkg/services/ldap/testing.go index fda9fd95a8c..7e0b33fcbd5 100644 --- a/pkg/services/ldap/testing.go +++ b/pkg/services/ldap/testing.go @@ -19,14 +19,19 @@ type MockConnection struct { DelParams *ldap.DelRequest DelCalled bool - bindProvider func(username, password string) error - unauthenticatedBindProvider func(username string) error + UnauthenticatedBindCalled bool + BindCalled bool + + BindProvider func(username, password string) error + UnauthenticatedBindProvider func() error } // Bind mocks Bind connection function func (c *MockConnection) Bind(username, password string) error { - if c.bindProvider != nil { - return c.bindProvider(username, password) + c.BindCalled = true + + if c.BindProvider != nil { + return c.BindProvider(username, password) } return nil @@ -34,8 +39,10 @@ func (c *MockConnection) Bind(username, password string) error { // UnauthenticatedBind mocks UnauthenticatedBind connection function func (c *MockConnection) UnauthenticatedBind(username string) error { - if c.unauthenticatedBindProvider != nil { - return c.unauthenticatedBindProvider(username) + c.UnauthenticatedBindCalled = true + + if c.UnauthenticatedBindProvider != nil { + return c.UnauthenticatedBindProvider() } return nil