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. |
|
I'm terribly sorry I didn't get to this earlier, you know, holidays with the family and whatnot. Apologies apart, I'm not convinced that an alleged flaw in Mosquitto itself should be "fixed" in the plugin, maybe we need @ralight to chime in here. |
|
At this point, then, I'll just the summarize the situation as I found it. mosquitto interprets an error code as an error in the auth plugin itself, and propagates that up as an "application error" which seems to be treated or considered as a type of fatal error at the plugin level. This would appear to be by design - there is no indication of a set of error codes which a plugin can return to indicate the plugin itself is behaving as expected but that the information it is dealing with is problematic in this way or that (ie JWT token expiry). It seems that to return true/false to say that authentication passed/failed is all mosquitto wants to interpret from it (at that level). Hence my last commit to return no error (but return code false to indicate failure to authenticate). Also for the return code re GetUser in the original PR commits in September. However, mosquitto code, with its current design/expectation on the plugin response, has the unfortunate issue that, supposing there were a reason for the auth plugin itself (having failed unexpectedly) to return an error code to indicate application error, it doesn't seem to handle well the propagation of this "application error": it assigns it a hard coded value of 1 (not specifically to any of its defined list of error codes) ... and as it propagates this error up the stack,mosquitto goes a bit wild, interpreting the error as an out of memory issue (which is what 1 corresponds to in its list of defined error codes) and also (for some reason which I didn't investigate further) disconnecting other connected clients. |
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.