Improve handling of backend failure #120

PierreF · 2020-11-13T15:29:17Z

This is more or less the port of jpmens/mosquitto-auth-plug#296 and jpmens/mosquitto-auth-plug#111

The issue this PR fix is is that when the HTTP (or MySQL) backend is down, ACL check return MOSQ_DENY_ACL. Which means that : 1) the message is rejected without notification for the sender 2) that rejection is cached.

This change few behavior:

On failure, ACL check will reply to Mosquitto with MOSQ_ERR_UNKNOWN which will DISCONNECT the client (previous was deny the ACL). This is because denied ACL is silent, which could lead to data-loss
HTTP (and JWT backends) will consider status code >= 500 as backend failure. An HTTP backend shouldn't use this to indicate a simple auth rejection.

Note on the PR changes:

Most of the change is just returning the error and updating tests to verify that error is nil
The C interface change, it return a uint8 instead of bool, since we need 3 states (granted, rejected, error). But this interface is only used between Go code and the auth-plugin.c from this repository I assume.
I've made two unrelated change in jwt_test.go, both in "Given a token that is admin, super user should pass" for PostgreSQL and Mysql:
- Both used jwt.GetSuperuser(username) (on line 198 and 360 on this PR), but I think jwt.GetSuperuser(token) is correct
- Both changed jwt.SuperuserUri = "", while I think jwt.SuperuserQuery = "" is correct.
- Those change were not seen, because error (username isn't a JWT token) was converted to superuser = false

iegomez · 2020-11-13T15:46:46Z

@PierreF As I mentioned in the other PR, I'll review next week and get back to you.

Cheers!

* 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.

PierreF · 2021-01-18T09:39:37Z

Hi,

is there something I can do to help this PR to get merged ?

iegomez · 2021-01-18T15:15:53Z

@PierreF Sorry for taking so long, I've been prioritizing other things lately. As I mentioned in the README, I expect to get back to reviewing and working a bit on the plugin during this month and the next one.

iegomez · 2021-01-28T04:06:43Z

@PierreF Hey again! I took care of a couple of bugs today and tried to get back to my JWT refactor, but in a very short span of time couldn't figure out exactly what I was doing a couple of months ago (yup, stupid me), just got to fix a couple of bugs in that branch and call it a day. I'm mentioning this because it is a major refactor in the JWT backend that could affect your PRs, so heads-up I guess.

Besides the anecdote, I'm really struggling to maintain the plugin lately and your PRs show both knowledge and will. I'm going on vacation in about a week, which may mean I finally get to review everything, or it may mean I don't have a proper connection and, once again, won't respond for a couple of weeks.

If it were the case I don't do anything for 3 weeks (and that's putting aside the month and a half of absence that already happened), is that critical to you? I gotta be honest: maintaining something that I haven't used for a really long time, is a burden. I've been busy with other things, and I'd be happy if you wanted to take over... or, if you want to, give a major help with the project. The JWT refactor is the only major thing I want to merge before being comfortable with the state of the plugin (with all its quirks and shit that need redoing, or, at the very least, rethinking) and being happy to give all the power to a peer to carry on.

Just shoot your thoughts and I'll gladly read them.

Cheers!

PierreF · 2021-01-28T12:58:22Z

Thanks for keeping us informed.

At my company we need a Mosquitto authentication plugin. Since https://github.com/jpmens/mosquitto-auth-plug is archived (and I prefer writing Go than C :)), I'm willing to help maintaining this project.
While I think it's better to not be alone maintaining a software, I understand that without using a software its hard to find time for it (even when using it, it's not easy).

So to be clear, I'm open to both major help/being co-maintainer or taking alone the project.

Just tell me what you prefer and what are the next step.

iegomez · 2021-01-28T18:38:25Z

Thanks, @PierreF! I'll keep you posted.

PierreF · 2021-02-11T14:25:02Z

I'm fixing conflict generated by last merge. Should be pushed in the hour.

PierreF · 2021-02-11T15:09:59Z

Conflict fixed. There is still one unrelated change in jwt_test.go: for test "But disabling superusers by removing superuri should now return false", the settings changed was "jwt_superquery" but I think the correct value is "jwt_pg_superquery" or "jwt_mysql_superquery".

iegomez · 2021-02-11T15:11:56Z

You're absolutely right, missed that when changing general queries to DB specific ones. Nice catch!

iegomez

I left some comments for discussion, I still need to go through go-auth.go though.
I'll try to get to it today or tomorrow morning.

Also, I don't see any changes for the gRPC backend.

auth-plugin.c

iegomez · 2021-02-11T15:47:50Z

backends/files.go

 	}

 	if o.hasher.Compare(password, fileUser.Password) {
-		return true
+		return true, nil
 	}

 	log.Warnf("wrong password for user %s", username)


This is on me, but we should really get rid of these warns that leak the username was correct.

I'm going to push another PR with those fix (or did you started any works on this ?) to avoid unrelated change in this (already big) PR.

No problem, I'll open one later today.

backends/http.go

backends/jwt_remote.go

backends/mongo.go

iegomez · 2021-02-11T16:00:05Z

backends/mysql.go


 	var pwHash sql.NullString
 	err := o.DB.Get(&pwHash, o.UserQuery, username)

 	if err != nil {
 		log.Debugf("MySql get user error: %s", err)
-		return false
+		return false, err
 	}

 	if !pwHash.Valid {
 		log.Debugf("MySql get user error: user %s not found", username)


Again, not on you, but this leaks. OTOH, this one is only in debug level, so maybe not that bad.

iegomez · 2021-02-11T16:00:35Z

backends/mysql.go

 	}

 	var count sql.NullInt64
 	err := o.DB.Get(&count, o.SuperuserQuery, username)

 	if err != nil {
 		log.Debugf("MySql get superuser error: %s", err)
-		return false
+		return false, err
 	}

 	if !count.Valid {
 		log.Debugf("MySql get superuser error: user %s not found", username)


Same story, I'll have to go through all these cases.

iegomez · 2021-02-11T16:01:59Z

backends/sqlite.go


 	var pwHash sql.NullString
 	err := o.DB.Get(&pwHash, o.UserQuery, username)

 	if err != nil {
 		log.Debugf("SQlite get user error: %s", err)
-		return false
+		return false, err
 	}

 	if !pwHash.Valid {
 		log.Debugf("SQlite get user error: user %s not found.", username)


Leak reminder.

PierreF · 2021-02-13T12:52:05Z

Also, I don't see any changes for the gRPC backend.

Didn't you looked at a partial diff ? There are change to grpc.go file (and grpc_test.go).

iegomez · 2021-02-13T12:53:57Z

Also, I don't see any changes for the gRPC backend.

Didn't you looked at a partial diff ? There are change to grpc.go file (and grpc_test.go).

Oops, sorry, I don't know how but I totally missed them.

Apply suggestions from code review Co-authored-by: Ignacio Gómez <[email protected]>

go-auth.go

iegomez

Thanks for dealing with non present users errors!
Remaining comments are only aesthetics related, but functionally this PR looks good to merge.

Co-authored-by: Ignacio Gómez <[email protected]>

iegomez · 2021-02-17T15:36:35Z

@PierreF feel free to merge when you're ready and I'll go on to review the other PRs.

PierreF requested a review from iegomez as a code owner November 13, 2020 15:29

Improve handling of backend failure

dc1edd8

* 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.

PierreF force-pushed the backend-error branch from 012ed11 to dc1edd8 Compare November 13, 2020 16:04

PierreF mentioned this pull request Nov 13, 2020

Retry on backend error #124

Merged

Merge branch 'master' into backend-error

4be4a08

iegomez reviewed Feb 11, 2021

View reviewed changes

iegomez mentioned this pull request Feb 12, 2021

Close client connection on JWT expiry #106

Open

Merge branch 'master' into backend-error

250485f

PierreF and others added 2 commits February 13, 2021 13:59

Use switch and cover all plugin possible reply

e2f3849

Improve log messages

f1a3fef

Apply suggestions from code review Co-authored-by: Ignacio Gómez <[email protected]>

iegomez reviewed Feb 13, 2021

View reviewed changes

Fix leaking existence of username from error

90a24b5

iegomez approved these changes Feb 13, 2021

View reviewed changes

PierreF and others added 2 commits February 13, 2021 15:56

Rename "firstError" in "err"

84620d8

Fix typo

04ca042

Co-authored-by: Ignacio Gómez <[email protected]>

PierreF merged commit c0667a4 into iegomez:master Feb 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve handling of backend failure #120

Improve handling of backend failure #120

PierreF commented Nov 13, 2020

iegomez commented Nov 13, 2020

PierreF commented Jan 18, 2021

iegomez commented Jan 18, 2021

iegomez commented Jan 28, 2021

PierreF commented Jan 28, 2021

iegomez commented Jan 28, 2021

PierreF commented Feb 11, 2021

PierreF commented Feb 11, 2021

iegomez commented Feb 11, 2021

iegomez left a comment •

edited

Loading

iegomez Feb 11, 2021

PierreF Feb 13, 2021

iegomez Feb 13, 2021

iegomez Feb 11, 2021

iegomez Feb 11, 2021

iegomez Feb 11, 2021

PierreF commented Feb 13, 2021

iegomez commented Feb 13, 2021

iegomez left a comment

iegomez commented Feb 17, 2021

Improve handling of backend failure #120

Improve handling of backend failure #120

Conversation

PierreF commented Nov 13, 2020

iegomez commented Nov 13, 2020

PierreF commented Jan 18, 2021

iegomez commented Jan 18, 2021

iegomez commented Jan 28, 2021

PierreF commented Jan 28, 2021

iegomez commented Jan 28, 2021

PierreF commented Feb 11, 2021

PierreF commented Feb 11, 2021

iegomez commented Feb 11, 2021

iegomez left a comment • edited Loading

Choose a reason for hiding this comment

iegomez Feb 11, 2021

Choose a reason for hiding this comment

PierreF Feb 13, 2021

Choose a reason for hiding this comment

iegomez Feb 13, 2021

Choose a reason for hiding this comment

iegomez Feb 11, 2021

Choose a reason for hiding this comment

iegomez Feb 11, 2021

Choose a reason for hiding this comment

iegomez Feb 11, 2021

Choose a reason for hiding this comment

PierreF commented Feb 13, 2021

iegomez commented Feb 13, 2021

iegomez left a comment

Choose a reason for hiding this comment

iegomez commented Feb 17, 2021

iegomez left a comment •

edited

Loading