Improve handling of backend failure

* No longer cache response from backend when the backend fail.
* Reply to Mosquitto using "MOSQ_ERR_UNKNOWN" which will disconnect
  client and avoid silent data loss when the error occure for ACL
  checks.

auth-plugin.c

@@ -18,6 +18,11 @@
 
 #include "go-auth.h"
 
+// Same constant as one in go-auth.go.
+#define AuthRejected 0
+#define AuthGranted 1
+#define AuthError 2
+
 int mosquitto_auth_plugin_version(void) {
  #ifdef MOSQ_AUTH_PLUGIN_VERSION
  #if MOSQ_AUTH_PLUGIN_VERSION == 5
@@ -91,10 +96,16 @@ int mosquitto_auth_unpwd_check(void *userdata, const char *username, const char
  GoString go_password = {password, strlen(password)};
  GoString go_clientid = {clientid, strlen(clientid)};
 
- if(AuthUnpwdCheck(go_username, go_password, go_clientid)){
+ GoUint8 ret = AuthUnpwdCheck(go_username, go_password, go_clientid);
+
+ if(ret == AuthGranted){
  return MOSQ_ERR_SUCCESS;
  }
 
+ if (ret == AuthError){
+ return MOSQ_ERR_UNKNOWN;
+ }
+
  return MOSQ_ERR_AUTH;
 }
 
@@ -122,10 +133,16 @@ int mosquitto_auth_acl_check(void *userdata, const char *clientid, const char *u
  GoString go_topic = {topic, strlen(topic)};
  GoInt32 go_access = access;
 
- if(AuthAclCheck(go_clientid, go_username, go_topic, go_access)){
+ GoUint8 ret = AuthAclCheck(go_clientid, go_username, go_topic, go_access);
+
+ if(ret == AuthGranted){
  return MOSQ_ERR_SUCCESS;
  }
 
+ if(ret == AuthError){
+ return MOSQ_ERR_UNKNOWN;
+ }
+
  return MOSQ_ERR_ACL_DENIED;
 }
 

backends/files.go

@@ -285,34 +285,34 @@ func checkCommentOrEmpty(line string) bool {
 }
 
 //GetUser checks that user exists and password is correct.
-func (o Files) GetUser(username, password, clientid string) bool {
+func (o Files) GetUser(username, password, clientid string) (bool, error) {
 
  fileUser, ok := o.Users[username]
  if !ok {
- return false
+ return false, nil
  }
 
  if o.hasher.Compare(password, fileUser.Password) {
- return true
+ return true, nil
  }
 
  log.Warnf("wrong password for user %s", username)
 
- return false
+ return false, nil
 
 }
 
 //GetSuperuser returns false for files backend.
-func (o Files) GetSuperuser(username string) bool {
- return false
+func (o Files) GetSuperuser(username string) (bool, error) {
+ return false, nil
 }
 
 //CheckAcl checks that the topic may be read/written by the given user/clientid.
-func (o Files) CheckAcl(username, topic, clientid string, acc int32) bool {
+func (o Files) CheckAcl(username, topic, clientid string, acc int32) (bool, error) {
  //If there are no acls and Files is the only backend, all access is allowed.
  //If there are other backends, then we can't blindly grant access.
  if !o.CheckAcls {
- return o.filesOnly
+ return o.filesOnly, nil
  }
 
  fileUser, ok := o.Users[username]
@@ -321,7 +321,7 @@ func (o Files) CheckAcl(username, topic, clientid string, acc int32) bool {
  if ok {
  for _, aclRecord := range fileUser.AclRecords {
  if TopicsMatch(aclRecord.Topic, topic) && (acc == int32(aclRecord.Acc) || int32(aclRecord.Acc) == MOSQ_ACL_READWRITE || (acc == MOSQ_ACL_SUBSCRIBE && topic != "#" && (int32(aclRecord.Acc) == MOSQ_ACL_READ || int32(aclRecord.Acc) == MOSQ_ACL_SUBSCRIBE))) {
- return true
+ return true, nil
  }
  }
  }
@@ -330,11 +330,11 @@ func (o Files) CheckAcl(username, topic, clientid string, acc int32) bool {
  aclTopic := strings.Replace(aclRecord.Topic, "%c", clientid, -1)
  aclTopic = strings.Replace(aclTopic, "%u", username, -1)
  if TopicsMatch(aclTopic, topic) && (acc == int32(aclRecord.Acc) || int32(aclRecord.Acc) == MOSQ_ACL_READWRITE || (acc == MOSQ_ACL_SUBSCRIBE && topic != "#" && (int32(aclRecord.Acc) == MOSQ_ACL_READ || int32(aclRecord.Acc) == MOSQ_ACL_SUBSCRIBE))) {
- return true
+ return true, nil
  }
  }
 
- return false
+ return false, nil
 
 }
 

backends/files_test.go

@@ -71,18 +71,21 @@ func TestFiles(t *testing.T) {
  })
 
  Convey("Given a username and a correct password, it should correctly authenticate it", func() {
- authenticated := files.GetUser(user1, user1, clientID)
+ authenticated, err := files.GetUser(user1, user1, clientID)
+ So(err, ShouldBeNil)
  So(authenticated, ShouldBeTrue)
  })
 
  Convey("Given a username and an incorrect password, it should not authenticate it", func() {
- authenticated := files.GetUser(user1, user2, clientID)
+ authenticated, err := files.GetUser(user1, user2, clientID)
+ So(err, ShouldBeNil)
  So(authenticated, ShouldBeFalse)
  })
 
  //There are no superusers for files
  Convey("For any user superuser should return false", func() {
- superuser := files.GetSuperuser(user1)
+ superuser, err := files.GetSuperuser(user1)
+ So(err, ShouldBeNil)
  So(superuser, ShouldBeFalse)
  })
 
@@ -93,61 +96,77 @@ func TestFiles(t *testing.T) {
  readWriteTopic := "readwrite/topic"
 
  Convey("User 1 should be able to publish and not subscribe to test topic 1, and only subscribe but not publish to topic 2", func() {
- tt1 := files.CheckAcl(user1, testTopic1, clientID, 2)
- tt2 := files.CheckAcl(user1, testTopic1, clientID, 1)
- tt3 := files.CheckAcl(user1, testTopic2, clientID, 2)
- tt4 := files.CheckAcl(user1, testTopic2, clientID, 1)
-
+ tt1, err1 := files.CheckAcl(user1, testTopic1, clientID, 2)
+ tt2, err2 := files.CheckAcl(user1, testTopic1, clientID, 1)
+ tt3, err3 := files.CheckAcl(user1, testTopic2, clientID, 2)
+ tt4, err4 := files.CheckAcl(user1, testTopic2, clientID, 1)
+
+ So(err1, ShouldBeNil)
+ So(err2, ShouldBeNil)
+ So(err3, ShouldBeNil)
+ So(err4, ShouldBeNil)
  So(tt1, ShouldBeTrue)
  So(tt2, ShouldBeFalse)
  So(tt3, ShouldBeFalse)
  So(tt4, ShouldBeTrue)
  })
 
  Convey("User 1 should be able to subscribe or publish to a readwrite topic rule", func() {
- tt1 := files.CheckAcl(user1, readWriteTopic, clientID, 2)
- tt2 := files.CheckAcl(user1, readWriteTopic, clientID, 1)
+ tt1, err1 := files.CheckAcl(user1, readWriteTopic, clientID, 2)
+ tt2, err2 := files.CheckAcl(user1, readWriteTopic, clientID, 1)
+ So(err1, ShouldBeNil)
+ So(err2, ShouldBeNil)
  So(tt1, ShouldBeTrue)
  So(tt2, ShouldBeTrue)
  })
 
  Convey("User 2 should be able to read any test/topic/X but not any/other", func() {
- tt1 := files.CheckAcl(user2, testTopic1, clientID, 1)
- tt2 := files.CheckAcl(user2, testTopic2, clientID, 1)
- tt3 := files.CheckAcl(user2, testTopic3, clientID, 1)
+ tt1, err1 := files.CheckAcl(user2, testTopic1, clientID, 1)
+ tt2, err2 := files.CheckAcl(user2, testTopic2, clientID, 1)
+ tt3, err3 := files.CheckAcl(user2, testTopic3, clientID, 1)
 
+ So(err1, ShouldBeNil)
+ So(err2, ShouldBeNil)
+ So(err3, ShouldBeNil)
  So(tt1, ShouldBeTrue)
  So(tt2, ShouldBeTrue)
  So(tt3, ShouldBeFalse)
  })
 
  Convey("User 3 should be able to read any test/X but not other/...", func() {
- tt1 := files.CheckAcl(user3, testTopic1, clientID, 1)
- tt2 := files.CheckAcl(user3, testTopic2, clientID, 1)
- tt3 := files.CheckAcl(user3, testTopic3, clientID, 1)
- tt4 := files.CheckAcl(user3, testTopic4, clientID, 1)
-
+ tt1, err1 := files.CheckAcl(user3, testTopic1, clientID, 1)
+ tt2, err2 := files.CheckAcl(user3, testTopic2, clientID, 1)
+ tt3, err3 := files.CheckAcl(user3, testTopic3, clientID, 1)
+ tt4, err4 := files.CheckAcl(user3, testTopic4, clientID, 1)
+
+ So(err1, ShouldBeNil)
+ So(err2, ShouldBeNil)
+ So(err3, ShouldBeNil)
+ So(err4, ShouldBeNil)
  So(tt1, ShouldBeTrue)
  So(tt2, ShouldBeTrue)
  So(tt3, ShouldBeTrue)
  So(tt4, ShouldBeFalse)
  })
 
  Convey("User 4 should not be able to read since it's not in the passwords file", func() {
- tt1 := files.CheckAcl(user4, testTopic1, clientID, 1)
+ tt1, err1 := files.CheckAcl(user4, testTopic1, clientID, 1)
 
+ So(err1, ShouldBeNil)
  So(tt1, ShouldBeFalse)
  })
 
  //Now check against patterns.
 
  Convey("Given a topic that mentions username, acl check should pass", func() {
- tt1 := files.CheckAcl(user1, "test/test1", clientID, 1)
+ tt1, err1 := files.CheckAcl(user1, "test/test1", clientID, 1)
+ So(err1, ShouldBeNil)
  So(tt1, ShouldBeTrue)
  })
 
  Convey("Given a topic that mentions clientid, acl check should pass", func() {
- tt1 := files.CheckAcl(user1, "test/test_client", clientID, 1)
+ tt1, err1 := files.CheckAcl(user1, "test/test_client", clientID, 1)
+ So(err1, ShouldBeNil)
  So(tt1, ShouldBeTrue)
  })
 

backends/grpc.go

@@ -52,7 +52,7 @@ func NewGRPC(authOpts map[string]string, logLevel log.Level) (GRPC, error) {
 }
 
 // GetUser checks that the username exists and the given password hashes to the same password.
-func (o GRPC) GetUser(username, password, clientid string) bool {
+func (o GRPC) GetUser(username, password, clientid string) (bool, error) {
 
  req := gs.GetUserRequest{
  Username: username,
@@ -64,18 +64,18 @@ func (o GRPC) GetUser(username, password, clientid string) bool {
 
  if err != nil {
  log.Errorf("grpc get user error: %s", err)
- return false
+ return false, err
  }
 
- return resp.Ok
+ return resp.Ok, nil
 
 }
 
 // GetSuperuser checks that the user is a superuser.
-func (o GRPC) GetSuperuser(username string) bool {
+func (o GRPC) GetSuperuser(username string) (bool, error) {
 
  if o.disableSuperuser {
- return false
+ return false, nil
  }
 
  req := gs.GetSuperuserRequest{
@@ -86,15 +86,15 @@ func (o GRPC) GetSuperuser(username string) bool {
 
  if err != nil {
  log.Errorf("grpc get superuser error: %s", err)
- return false
+ return false, err
  }
 
- return resp.Ok
+ return resp.Ok, nil
 
 }
 
 // CheckAcl checks if the user has access to the given topic.
-func (o GRPC) CheckAcl(username, topic, clientid string, acc int32) bool {
+func (o GRPC) CheckAcl(username, topic, clientid string, acc int32) (bool, error) {
 
  req := gs.CheckAclRequest{
  Username: username,
@@ -107,10 +107,10 @@ func (o GRPC) CheckAcl(username, topic, clientid string, acc int32) bool {
 
  if err != nil {
  log.Errorf("grpc check acl error: %s", err)
- return false
+ return false, err
  }
 
- return resp.Ok
+ return resp.Ok, nil
 
 }
 

backends/grpc_test.go

@@ -92,36 +92,43 @@ func TestGRPC(t *testing.T) {
 
  Convey("given incorrect credentials user should not be authenticated", func(c C) {
 
- auth := g.GetUser(grpcUsername, "wrong", grpcClientId)
+ auth, err := g.GetUser(grpcUsername, "wrong", grpcClientId)
+ So(err, ShouldBeNil)
  c.So(auth, ShouldBeFalse)
  Convey("given correct credential user should be authenticated", func(c C) {
 
- auth := g.GetUser(grpcUsername, grpcPassword, grpcClientId)
+ auth, err := g.GetUser(grpcUsername, grpcPassword, grpcClientId)
+ So(err, ShouldBeNil)
  c.So(auth, ShouldBeTrue)
 
  Convey("given a non superuser user the service should respond false", func(c C) {
- auth = g.GetSuperuser(grpcUsername)
+ auth, err = g.GetSuperuser(grpcUsername)
+ So(err, ShouldBeNil)
  So(auth, ShouldBeFalse)
 
  Convey("switching to a superuser should return true", func(c C) {
- auth = g.GetSuperuser(grpcSuperuser)
+ auth, err = g.GetSuperuser(grpcSuperuser)
+ So(err, ShouldBeNil)
  So(auth, ShouldBeTrue)
 
  Convey("but if we disable superuser checks it should return false", func(c C) {
  authOpts["grpc_disable_superuser"] = "true"
  g, err = NewGRPC(authOpts, log.DebugLevel)
  c.So(err, ShouldBeNil)
 
- auth = g.GetSuperuser(grpcSuperuser)
+ auth, err = g.GetSuperuser(grpcSuperuser)
+ So(err, ShouldBeNil)
  So(auth, ShouldBeFalse)
  })
 
  Convey("authorizing a wrong topic should fail", func(c C) {
- auth = g.CheckAcl(grpcUsername, "wrong/topic", grpcClientId, grpcAcc)
+ auth, err = g.CheckAcl(grpcUsername, "wrong/topic", grpcClientId, grpcAcc)
+ So(err, ShouldBeNil)
  So(auth, ShouldBeFalse)
 
  Convey("switching to a correct one should succedd", func(c C) {
- auth = g.CheckAcl(grpcUsername, grpcTopic, grpcClientId, grpcAcc)
+ auth, err = g.CheckAcl(grpcUsername, grpcTopic, grpcClientId, grpcAcc)
+ So(err, ShouldBeNil)
  So(auth, ShouldBeTrue)
 
  })