GetUser check required for jwt auth in files mode #339
Conversation
| @@ -36,7 +36,8 @@ func NewFilesJWTChecker(authOpts map[string]string, logLevel log.Level, hasher h | |||
| } | |||
|
|
|||
| func (o *filesJWTChecker) GetUser(token string) (bool, error) { | |||
| return false, nil | |||
| _, err := getUsernameForToken(o.options, token, o.options.skipUserExpiration) | |||
There was a problem hiding this comment.
If there's an actual error maybe we should return it?
There was a problem hiding this comment.
I tried that somewhat naively - but discovered if this propagates an error by returning it, it causes a general failure in the plugin and unresponsive behaviour. As far as I can tell, getUsernameForToken returning an error just means an invalid user, for whatever reason, not a general/unexpected failure, so returning err == nil, nil seemed to be the most fitting return format.
There was a problem hiding this comment.
Oh, ok, I'll take a deeper look into then, thanks.
Will do |
|
I've added some tests. |
december1981
force-pushed
the
jwt-files-user-auth
branch
from
25da2d6 to
e751f71
Compare
|
re the latest commit I made some days ago. This is probably related to the previous comments, but I found looking at and debugging the mosquitto code that when the acl check returns an error code back, mosquitto treats this as an arbitrary "application error" and percolates an error code value The mosquitto code base operates with no consistent error code regime and at some point it conflates this with an "out of memory" error (which is assigned to Anyway, returning no error (just false) when the acl check fails (in my regression/test case because of token expiry) plays ball with mosquitto. |
We need to run a check that the user validates with the passwd in JWT token or jwt authentication in files mode is unusable if GetUser returns a hard false.
This mode doesn't support any further checks on the user (as commented in the code and README - requiring the ACL list to effectively do this) so this change does not regress or modify any behaviour according to the current spec, as far as I can tell.