From 12d3a2a6b65efc7ff8960a7a263a2f46e98b72fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ignacio=20G=C3=B3mez?= Date: Thu, 19 Sep 2024 14:56:47 -0300 Subject: [PATCH] Allow to check backends for superuser and ACL in order instead of checking them all for superuser and then their ACLs. --- README.md | 16 ++++++ backends/backends.go | 118 +++++++++++++++++++++++++++----------- backends/backends_test.go | 101 ++++++++++++++++++++++++++++++++ 3 files changed, 201 insertions(+), 34 deletions(-) diff --git a/README.md b/README.md index 4f1dbe61..7c02bd63 100644 --- a/README.md +++ b/README.md @@ -245,6 +245,22 @@ auth_opt_backends files, postgres, jwt Set all other plugin options below in the same file. +#### Backends order + +By default, the plugin won't establish any order for checks (Go maps guarantee no order). +In particular and importantly, when running ACL checks with `superuser` checks enabled, +the plugin will first check them all for `superuser` and then check ACLs for all of them, +*in any given order* as mentioned. + +You can override this behaviour by setting `exhaust_backend_first` option to `true`: +``` +auth_opt_exhaust_backend_first true +``` + +When set, ACL checks will first try to check for `superuser` (if possible and enabled) in the backend, +and then run an ACL check against the same backend before moving to the next one. + + #### Using clientid as username You may choose to override chedk against `username` to be done against `clientid` by setting this option: diff --git a/backends/backends.go b/backends/backends.go index a2fa9e32..1e0c149d 100644 --- a/backends/backends.go +++ b/backends/backends.go @@ -28,7 +28,9 @@ type Backends struct { stripPrefix bool prefixes map[string]string - disableSuperuser bool + disableSuperuser bool + exhaustBackendFirst bool + sortedBackends []string } const ( @@ -80,12 +82,18 @@ func Initialize(authOpts map[string]string, logLevel log.Level, version string) prefixes: make(map[string]string), } - //Disable superusers for all backends if option is set. + // Disable superusers for all backends if option is set. if authOpts["disable_superuser"] == "true" { b.disableSuperuser = true } + // When set, a backend will be checked for superuser (if enabled) and ACL before checking another backend. + if authOpts["exhaust_backend_first"] == "true" { + b.exhaustBackendFirst = true + + } + backendsOpt, ok := authOpts["backends"] if !ok || backendsOpt == "" { return nil, fmt.Errorf("missing or blank option backends") @@ -118,6 +126,16 @@ func Initialize(authOpts map[string]string, logLevel log.Level, version string) } func (b *Backends) addBackends(authOpts map[string]string, logLevel log.Level, backends []string, version string) error { + // Store given backends as given to order them when checking. + // + // This allows to sort user checking, and first exhaust superuser/acl checks of a given backend before checking the next one, + // instead of the default superuser of all backends before checking them again for ACLs. + // + // Neither option is a silver bullet, but at least give some more grained control when paired with + // checkers registering. + b.sortedBackends = make([]string, len(backends)) + copy(b.sortedBackends, backends) + for _, bename := range backends { var beIface Backend var err error @@ -224,7 +242,8 @@ func (b *Backends) setCheckers(authOpts map[string]string) error { // We'll register which plugins will perform checks for user, superuser and acls. // At least one backend must be registered for user and acl checks. // When option auth_opt_backend_register is missing for the backend, we register all checks. - for name := range b.backends { + for _, name := range b.sortedBackends { + opt := fmt.Sprintf("%s_register", allowedBackendsOptsPrefix[name]) options, ok := authOpts[opt] @@ -381,7 +400,6 @@ func (b *Backends) AuthUnpwdCheck(username, password, clientid string) (bool, er func (b *Backends) checkAuth(username, password, clientid string) (bool, error) { var err error - authenticated := false for _, bename := range b.userCheckers { var backend = b.backends[bename] @@ -389,21 +407,15 @@ func (b *Backends) checkAuth(username, password, clientid string) (bool, error) log.Debugf("checking user %s with backend %s", username, backend.GetName()) if ok, getUserErr := backend.GetUser(username, password, clientid); ok && getUserErr == nil { - authenticated = true log.Debugf("user %s authenticated with backend %s", username, backend.GetName()) - break + + return true, nil } else if getUserErr != nil && err == nil { err = getUserErr } } - // If authenticated is true, it means at least one backend didn't fail and - // accepted the user. If so, honor the backend and clear the error. - if authenticated { - err = nil - } - - return authenticated, err + return false, err } // AuthAclCheck checks user/topic/acc authorization. @@ -462,46 +474,84 @@ func (b *Backends) AuthAclCheck(clientid, username, topic string, acc int) (bool } func (b *Backends) checkAcl(username, topic, clientid string, acc int) (bool, error) { - // Check superusers first + // Historically, the plugin checked all backends for superuser first (without order), + // and only then it checked for ACLs. + // If exhaust_backend_first is set, we check backends for both first following order. + if b.exhaustBackendFirst { + return b.exhaustBackendsInOrder(username, topic, clientid, acc) + } + + return b.checkSuperuserThenACL(username, topic, clientid, acc) +} + +func (b *Backends) exhaustBackendsInOrder(username, topic, clientid string, acc int) (bool, error) { + // Check every backend, in order, for superuser and ACL. var err error - aclCheck := false - if !b.disableSuperuser { - for _, bename := range b.superuserCheckers { - var backend = b.backends[bename] - log.Debugf("Superuser check with backend %s", backend.GetName()) + for _, bename := range b.sortedBackends { + var backend = b.backends[bename] + + if !b.disableSuperuser && checkRegistered(bename, b.superuserCheckers) { + log.Debugf("superuser check with backend %s", backend.GetName()) if ok, getSuperuserErr := backend.GetSuperuser(username); ok && getSuperuserErr == nil { log.Debugf("superuser %s acl authenticated with backend %s", username, backend.GetName()) - aclCheck = true - break + + return true, nil } else if getSuperuserErr != nil && err == nil { err = getSuperuserErr } } - } - if !aclCheck { - for _, bename := range b.aclCheckers { - var backend = b.backends[bename] - - log.Debugf("Acl check with backend %s", backend.GetName()) + if checkRegistered(bename, b.aclCheckers) { + log.Debugf("acl check with backend %s", backend.GetName()) if ok, checkACLErr := backend.CheckAcl(username, topic, clientid, int32(acc)); ok && checkACLErr == nil { log.Debugf("user %s acl authenticated with backend %s", username, backend.GetName()) - aclCheck = true - break + + return true, nil } else if checkACLErr != nil && err == nil { err = checkACLErr } } } - // If aclCheck is true, it means at least one backend didn't fail and - // accepted the access. In this case trust this backend and clear the error. - if aclCheck { - err = nil + // No backend authorized access. + return false, err +} + +func (b *Backends) checkSuperuserThenACL(username, topic, clientid string, acc int) (bool, error) { + // Check superusers first + var err error + + if !b.disableSuperuser { + for _, bename := range b.superuserCheckers { + var backend = b.backends[bename] + + log.Debugf("superuser check with backend %s", backend.GetName()) + if ok, getSuperuserErr := backend.GetSuperuser(username); ok && getSuperuserErr == nil { + log.Debugf("superuser %s acl authenticated with backend %s", username, backend.GetName()) + + return true, nil + } else if getSuperuserErr != nil && err == nil { + err = getSuperuserErr + } + } } - return aclCheck, err + for _, bename := range b.aclCheckers { + var backend = b.backends[bename] + + log.Debugf("Acl check with backend %s", backend.GetName()) + if ok, checkACLErr := backend.CheckAcl(username, topic, clientid, int32(acc)); ok && checkACLErr == nil { + log.Debugf("user %s acl authenticated with backend %s", username, backend.GetName()) + + return true, nil + } else if checkACLErr != nil && err == nil { + err = checkACLErr + } + } + + // No backend authorized access. + return false, err } func (b *Backends) Halt() { diff --git a/backends/backends_test.go b/backends/backends_test.go index 4480602b..e4b1033b 100644 --- a/backends/backends_test.go +++ b/backends/backends_test.go @@ -8,10 +8,111 @@ import ( "github.com/iegomez/mosquitto-go-auth/hashing" log "github.com/sirupsen/logrus" + log_test "github.com/sirupsen/logrus/hooks/test" . "github.com/smartystreets/goconvey/convey" "github.com/stretchr/testify/assert" ) +func TestBackendsOrder(t *testing.T) { + authOpts := make(map[string]string) + + pwPath, _ := filepath.Abs("../test-files/passwords") + aclPath, _ := filepath.Abs("../test-files/acls") + + authOpts["files_password_path"] = pwPath + authOpts["files_acl_path"] = aclPath + + authOpts["redis_host"] = "localhost" + authOpts["redis_port"] = "6379" + authOpts["redis_db"] = "2" + authOpts["redis_password"] = "" + + username := "test1" + password := "test1" + passwordHash := "PBKDF2$sha512$100000$2WQHK5rjNN+oOT+TZAsWAw==$TDf4Y6J+9BdnjucFQ0ZUWlTwzncTjOOeE00W4Qm8lfPQyPCZACCjgfdK353jdGFwJjAf6vPAYaba9+z4GWK7Gg==" + clientid := "clientid" + + version := "2.0.0" + + Convey("When exhaust_backend_first is set, backends should be fully checked for superuser and acl first", t, func() { + authOpts["backends"] = "files, redis" + authOpts["exhaust_backend_first"] = "true" + + hook := log_test.NewGlobal() + + redis, err := NewRedis(authOpts, log.DebugLevel, hashing.NewHasher(authOpts, "redis")) + assert.Nil(t, err) + + ctx := context.Background() + + // Make one of files users a Redis superuser and assert ACL is granted by Files because of order. + username = "test1" + redis.conn.Set(ctx, username, passwordHash, 0) + + b, err := Initialize(authOpts, log.DebugLevel, version) + So(err, ShouldBeNil) + + tt1, err1 := b.AuthUnpwdCheck(username, password, clientid) + + So(err1, ShouldBeNil) + So(tt1, ShouldBeTrue) + + redis.conn.Set(ctx, "test1:su", "true", 0) + + aclCheck, err := b.AuthAclCheck(clientid, username, "test/topic/1", 2) + + // Files should be checked for ACL first even if it's a superuser in Redis. + lastEntry := hook.LastEntry() + + So(lastEntry.Level, ShouldEqual, log.DebugLevel) + So(lastEntry.Message, ShouldEqual, "user test1 acl authenticated with backend Files") + + So(err, ShouldBeNil) + So(aclCheck, ShouldBeTrue) + + redis.Halt() + }) + + Convey("When exhaust_backend_first is not set, all backends should be checked for superuser first and acl later", t, func() { + authOpts["backends"] = "files, redis" + authOpts["exhaust_backend_first"] = "false" + + hook := log_test.NewGlobal() + + redis, err := NewRedis(authOpts, log.DebugLevel, hashing.NewHasher(authOpts, "redis")) + assert.Nil(t, err) + + ctx := context.Background() + + // Make one of files users a Redis superuser and assert ACL is granted by Files because of order. + username = "test1" + redis.conn.Set(ctx, username, passwordHash, 0) + + b, err := Initialize(authOpts, log.DebugLevel, version) + So(err, ShouldBeNil) + + tt1, err1 := b.AuthUnpwdCheck(username, password, clientid) + + So(err1, ShouldBeNil) + So(tt1, ShouldBeTrue) + + redis.conn.Set(ctx, "test1:su", "true", 0) + + aclCheck, err := b.AuthAclCheck(clientid, username, "test/topic/1", 2) + + // Redis should auth the superuser. + lastEntry := hook.LastEntry() + + So(lastEntry.Level, ShouldEqual, log.DebugLevel) + So(lastEntry.Message, ShouldEqual, "superuser test1 acl authenticated with backend Redis") + + So(err, ShouldBeNil) + So(aclCheck, ShouldBeTrue) + + redis.Halt() + }) +} + func TestBackends(t *testing.T) { /* No way we're gonna test every possibility given the amount of backends,