Add backends tests and fix a couple of issues in Redis.

README.md

@@ -1,22 +1,20 @@
 # Mosquitto Go Auth
 
 Mosquitto Go Auth is an authentication and authorization plugin for the Mosquitto MQTT broker.
+The name is terrible, I know, but it's too late to change it. And, you know: naming, cache invalidation, off-by-one errors and whatnot.
 
 # Current state
 
-I don't use Mosquitto or any other MQTT broker and haven't in a very long time, nor do I have a need for them or this plugin. 
-I do maintain it still and will try to keep doing so. This is the list of status, current work and priorities:
-
 - The plugin is up to date and is compatible with the recent [2.0 Mosquitto version](https://mosquitto.org/blog/2020/12/version-2-0-0-released/).
-- Delayed work on disabling superusers is not yet ready.
 - Bug reports will be attended as they appear and will take priority over any work in progress.
 - Reviewing ongoing PRs is my next priority.
 - Feature requests are the lowest priority. Unless they are a super easy win in importance and implementation effort, I'll accept contributions and review 
  PRs before considering implementing them myself.
 
 ### Intro
 
-This is an authentication and authorization plugin for [mosquitto](https://mosquitto.org/), a well known open source MQTT broker. It's written (almost) entirely in Go: it uses `cgo` to expose mosquitto's auth plugin needed functions, but internally just calls Go to get everything done. 
+This is an authentication and authorization plugin for [mosquitto](https://mosquitto.org/), a well known open source MQTT broker. 
+It's written (almost) entirely in Go: it uses `cgo` to expose mosquitto's auth plugin needed functions, but internally just calls Go to get everything done. 
 
 It is greatly inspired in [jpmens'](https://github.com/jpmens) [mosquitto-auth-plug](https://github.com/jpmens/mosquitto-auth-plug).
 
@@ -52,6 +50,7 @@ Please open an issue with the `feature` or `enhancement` tag to request new back
  - [Log level](#log-level)
  - [Prefixes](#prefixes)
  - [Backend options](#backend-options)
+ - [Registering checks](#registering-checks)
 - [Files](#files)
  - [Passwords file](#passwords-file)
  - [ACL file](#acl-file)
@@ -341,7 +340,7 @@ auth_opt_pg_hasher_parallelism # degree of parallelism (i.e. number o
 
 #### Logging
 
-You can set the log level with the `log_level` option. Valid values are: debug, info, warn, error, fatal and panic. If not set, default value is `info`.
+You can set the log level with the `log_level` option. Valid values are: `debug`, `info`, `warn`, `error`, `fatal` and `panic`. If not set, default value is `info`.
 
 ```
 auth_opt_log_level debug
@@ -356,6 +355,12 @@ auth_opt_log_file /var/log/mosquitto/mosquitto.log
 
 If `log_dest` or `log_file` are invalid, or if there's an error opening the file (e.g. no permissions), logging will default to `stderr`.
 
+**Do not, I repeat, do not set `log_level` to `debug` in production, it may leak sensitive information.**
+**Reason? When debugging it's quite useful to log actual passwords, hashes, etc. to check which backend or hasher is failing to do its job.**
+**This should be used only when debugging locally, I can't stress enough how log level should never, ever be set to `debug` in production.**
+
+**You've been warned.**
+
 #### Retry
 
 By default, if backend had an error (and no other backend granted access), an error is returned to Mosquitto.
@@ -445,7 +450,7 @@ make test
 
 ### Registering checks
 
-Backends may register which checks they'll run, enabling the option to e.g. only check user auth through an HTTP backend while delegating ACL to another backend, e.g. Files.
+Backends may register which checks they'll run, enabling the option to only check user auth through some backends, for example an HTTP one, while delegating ACL checks to another backend, e.g. Files.
 By default, when the option is not present, all checks for that backend will be enabled (unless `superuser` is globally disabled in the case of `superuser` checks).
 For `user` and `acl` checks, at least one backend needs to be registered, either explicitly or by default.
 
@@ -456,7 +461,7 @@ auth_opt_files_register user, acl
 auth_opt_redis_register superuser
 ```
 
-Possible values for checks are `user`, `superuser` and `acl`. Any other value will result in an error initializing the plugin.
+Possible values for checks are `user`, `superuser` and `acl`. Any other value will result in an error on plugin initialization.
 
 
 ### Files

backends/backends.go

@@ -31,7 +31,7 @@ type Backends struct {
 }
 
 const (
- //backends
+ // backends
  postgresBackend = "postgres"
  jwtBackend = "jwt"
  redisBackend = "redis"
@@ -44,7 +44,7 @@ const (
  grpcBackend = "grpc"
  jsBackend = "js"
 
- //checks
+ // checks
  aclCheck = "acl"
  userCheck = "user"
  superuserCheck = "superuser"
@@ -66,7 +66,7 @@ var AllowedBackendsOptsPrefix = map[string]string{
 }
 
 // Initialize sets general options, tries to build the backends and register their checkers.
-func Initialize(authOpts map[string]string, logLevel log.Level, backends []string) *Backends {
+func Initialize(authOpts map[string]string, logLevel log.Level, backends []string) (*Backends, error) {
 
  b := &Backends{
  backends: make(map[string]Backend),
@@ -82,17 +82,26 @@ func Initialize(authOpts map[string]string, logLevel log.Level, backends []strin
  b.disableSuperuser = true
  }
 
+ err := b.addBackends(authOpts, logLevel, backends)
+ if err != nil {
+ return nil, err
+ }
+
+ err = b.setCheckers(authOpts)
+ if err != nil {
+ return nil, err
+ }
+
+ b.setPrefixes(authOpts, backends)
+
+ return b, nil
+}
+
+func (b *Backends) addBackends(authOpts map[string]string, logLevel log.Level, backends []string) error {
  for _, bename := range backends {
  var beIface Backend
  var err error
 
- /*
- TODO: this could be nicer if we had a initializer map, e.g.:
- var initializers map[string] func(authOpts map[string]string, logLevel log.Level, hasher hashing.Hasher) (Backend, error)
-
- Sadly, not all backends require a hasher and I'm not sure about changing them to accept a dummy one just for the sake of this.
- But I'll keep this comment for further thought and to remind me about changing those that don't return a pointer to do so.
- */
  hasher := hashing.NewHasher(authOpts, AllowedBackendsOptsPrefix[bename])
  switch bename {
  case postgresBackend:
@@ -183,13 +192,12 @@ func Initialize(authOpts map[string]string, logLevel log.Level, backends []strin
  log.Infof("Backend registered: %s", beIface.GetName())
  b.backends[pluginBackend] = beIface.(*CustomPlugin)
  }
+ default:
+ return fmt.Errorf("unkown backend %s", bename)
  }
  }
 
- b.setCheckers(authOpts)
- b.setPrefixes(authOpts, backends)
-
- return b
+ return nil
 }
 
 func (b *Backends) setCheckers(authOpts map[string]string) error {
@@ -206,18 +214,24 @@ func (b *Backends) setCheckers(authOpts map[string]string) error {
  switch check {
  case aclCheck:
  b.aclCheckers = append(b.aclCheckers, name)
+ log.Infof("registered acl checker: %s", name)
  case userCheck:
  b.userCheckers = append(b.userCheckers, name)
+ log.Infof("registered user checker: %s", name)
  case superuserCheck:
  b.superuserCheckers = append(b.superuserCheckers, name)
+ log.Infof("registered superuser checker: %s", name)
  default:
- return fmt.Errorf("unsupported check %s found for backend %s, skipping registration", check, name)
+ return fmt.Errorf("unsupported check %s found for backend %s", check, name)
  }
  }
  } else {
  b.aclCheckers = append(b.aclCheckers, name)
+ log.Infof("registered acl checker: %s", name)
  b.userCheckers = append(b.userCheckers, name)
+ log.Infof("registered user checker: %s", name)
  b.superuserCheckers = append(b.superuserCheckers, name)
+ log.Infof("registered superuser checker: %s", name)
  }
  }
 
@@ -235,11 +249,13 @@ func (b *Backends) setCheckers(authOpts map[string]string) error {
 // setPrefixes sets options for prefixes handling.
 func (b *Backends) setPrefixes(authOpts map[string]string, backends []string) {
  if checkPrefix, ok := authOpts["check_prefix"]; ok && strings.Replace(checkPrefix, " ", "", -1) == "true" {
- //Check that backends match prefixes.
+ // Check that backends match prefixes.
  if prefixesStr, ok := authOpts["prefixes"]; ok {
  prefixes := strings.Split(strings.Replace(prefixesStr, " ", "", -1), ",")
  if len(prefixes) == len(backends) {
- //Set prefixes
+ // Set prefixes
+ // (I know some people find this type of comments useless, even harmful,
+ // but I find them helpful for quick code navigation on a project I don't work on daily, so screw them).
  for i, backend := range backends {
  b.prefixes[prefixes[i]] = backend
  }
@@ -296,32 +312,31 @@ func (b *Backends) AuthUnpwdCheck(username, password, clientid string) (bool, er
  var authenticated bool
  var err error
 
- //If prefixes are enabled, check if username has a valid prefix and use the correct backend if so.
- if b.checkPrefix {
- validPrefix, bename := b.lookupPrefix(username)
+ // If prefixes are enabled, check if username has a valid prefix and use the correct backend if so.
+ if !b.checkPrefix {
+ return b.checkAuth(username, password, clientid)
+ }
 
- if !checkRegistered(bename, b.userCheckers) {
- return false, fmt.Errorf("backends %s not registered to check users", bename)
- }
+ validPrefix, bename := b.lookupPrefix(username)
 
- if validPrefix {
- // If the backend is JWT and the token was prefixed, then strip the token. If the token was passed without a prefix it will be handled in the common case.
- if bename == jwtBackend {
- prefix := b.getPrefixForBackend(bename)
- username = strings.TrimPrefix(username, prefix+"_")
- }
- var backend = b.backends[bename]
+ if !checkRegistered(bename, b.userCheckers) {
+ return false, fmt.Errorf("backend %s not registered to check users", bename)
+ }
 
- authenticated, err = backend.GetUser(username, password, clientid)
- if authenticated && err == nil {
- log.Debugf("user %s authenticated with backend %s", username, backend.GetName())
- }
- } else {
- //If there's no valid prefix, check all backends.
- authenticated, err = b.checkAuth(username, password, clientid)
- }
- } else {
- authenticated, err = b.checkAuth(username, password, clientid)
+ if !validPrefix {
+ return b.checkAuth(username, password, clientid)
+ }
+
+ // If the backend is JWT and the token was prefixed, then strip the token. If the token was passed without a prefix it will be handled in the common case.
+ if bename == jwtBackend {
+ prefix := b.getPrefixForBackend(bename)
+ username = strings.TrimPrefix(username, prefix+"_")
+ }
+ var backend = b.backends[bename]
+
+ authenticated, err = backend.GetUser(username, password, clientid)
+ if authenticated && err == nil {
+ log.Debugf("user %s authenticated with backend %s", username, backend.GetName())
  }
 
  return authenticated, err
@@ -354,64 +369,64 @@ func (b *Backends) checkAuth(username, password, clientid string) (bool, error)
  return authenticated, err
 }
 
-// AuthAclCheck checks user/topic/acc authentication.
+// AuthAclCheck checks user/topic/acc authorization.
 func (b *Backends) AuthAclCheck(clientid, username, topic string, acc int) (bool, error) {
  var aclCheck bool
  var err error
 
- //If prefixes are enabled, check if username has a valid prefix and use the correct backend if so.
- //Else, check all backends.
- if b.checkPrefix {
- validPrefix, bename := b.lookupPrefix(username)
- if validPrefix {
- // If the backend is JWT and the token was prefixed, then strip the token. If the token was passed without a prefix then it be handled in the common case.
- if bename == jwtBackend {
- prefix := b.getPrefixForBackend(bename)
- username = strings.TrimPrefix(username, prefix+"_")
- }
- var backend = b.backends[bename]
+ // If prefixes are enabled, check if username has a valid prefix and use the correct backend if so.
+ // Else, check all backends.
+ if !b.checkPrefix {
+ return b.checkAcl(username, topic, clientid, acc)
+ }
 
- // Short circuit checks when superusers are disabled.
- if !b.disableSuperuser {
- log.Debugf("Superuser check with backend %s", backend.GetName())
- if !checkRegistered(bename, b.superuserCheckers) {
- return false, fmt.Errorf("backends %s not registered to check superusers", bename)
- }
+ validPrefix, bename := b.lookupPrefix(username)
 
- aclCheck, err = backend.GetSuperuser(username)
+ if !validPrefix {
+ return b.checkAcl(username, topic, clientid, acc)
+ }
 
- if aclCheck && err == nil {
- log.Debugf("superuser %s acl authenticated with backend %s", username, backend.GetName())
- }
- }
- //If not superuser, check acl.
- if !aclCheck {
- if !checkRegistered(bename, b.aclCheckers) {
- return false, fmt.Errorf("backends %s not registered to check superusers", bename)
- }
+ // If the backend is JWT and the token was prefixed, then strip the token. If the token was passed without a prefix then let it be handled in the common case.
+ if bename == jwtBackend {
+ prefix := b.getPrefixForBackend(bename)
+ username = strings.TrimPrefix(username, prefix+"_")
+ }
+ 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 {
- aclCheck = true
- log.Debugf("user %s acl authenticated with backend %s", username, backend.GetName())
- } else if checkACLErr != nil && err == nil {
- err = checkACLErr
- }
- }
- } else {
- //If there's no valid prefix, check all backends.
- aclCheck, err = b.checkAcl(username, topic, clientid, acc)
+ // Short circuit checks when superusers are disabled.
+ if !b.disableSuperuser {
+ log.Debugf("Superuser check with backend %s", backend.GetName())
+ if !checkRegistered(bename, b.superuserCheckers) {
+ return false, fmt.Errorf("backend %s not registered to check superusers", bename)
+ }
+
+ aclCheck, err = backend.GetSuperuser(username)
+
+ if aclCheck && err == nil {
+ log.Debugf("superuser %s acl authenticated with backend %s", username, backend.GetName())
+ }
+ }
+ // If not superuser, check acl.
+ if !aclCheck {
+ if !checkRegistered(bename, b.aclCheckers) {
+ return false, fmt.Errorf("backend %s not registered to check superusers", bename)
+ }
+
+ log.Debugf("Acl check with backend %s", backend.GetName())
+ if ok, checkACLErr := backend.CheckAcl(username, topic, clientid, int32(acc)); ok && checkACLErr == nil {
+ aclCheck = true
+ log.Debugf("user %s acl authenticated with backend %s", username, backend.GetName())
+ } else if checkACLErr != nil && err == nil {
+ err = checkACLErr
  }
- } else {
- aclCheck, err = b.checkAcl(username, topic, clientid, acc)
  }
 
  log.Debugf("Acl is %t for user %s", aclCheck, username)
  return aclCheck, err
 }
 
 func (b *Backends) checkAcl(username, topic, clientid string, acc int) (bool, error) {
- //Check superusers first
+ // Check superusers first
  var err error
  aclCheck := false
  if !b.disableSuperuser {
@@ -454,7 +469,7 @@ func (b *Backends) checkAcl(username, topic, clientid string, acc int) (bool, er
 }
 
 func (b *Backends) Halt() {
- //Halt every registered backend.
+ // Halt every registered backend.
  for _, v := range b.backends {
  v.Halt()
  }