Enhancement: Option to validate JWT before passing to remote backend

[sirockin]

Currently auth_opt_jwt_secret is ignored when auth_opt_jwt_remote is set to true so it is left to the remote endpoint to validate the token. The expiry time is also not checked. It is left to the implementer of the remote interface to carry out this validation.

I understand that under some circumstances that may be useful, but since the JWT validation code is already included in the plugin, could we enable this option and jwt_skip_user_expiration, jwt_skip_acl_expiration for remote backends? This would allow the plugin to prevalidate the token before passing to the back end, with the advantages of:

• Reducing the amount of code required to implement the backend
• Reducing the number of http requests required since invalid tokens would be rejected in advance

It should be a non-breaking change.

I'd be happy to submit a PR if you agree.

[iegomez]

Hey, @sirockin! Thanks for opening the issue. I'll start with the bad news and then elaborate, so please go all the way before judging the answer. 😅

Sadly, I don't agree with the approach. There are two main points to the remote option, which are delegating both the secret and all the logic to the third service: they may skip or do whatever they want. I do understand the need and the benefits of having more options that solve some of the pain points, but I believe the custom plugin is the place for this.
The same goes for #114: returning ACLs from a user check breaks the contract. Again, the benefits are clear, but it doesn't follow the general backend interface.

I believe what you and Paul want is a rich JWT plugin for Mosquitto, and that'd be cool. It may be achieved by leveraging the custom backend interface, or I'll gladly join you in writing a specialized JWT plugin, but tweaking this one to accommodate for all those situations doesn't seem the right way.

I've already started adding an embedded JS interpreter to allow for custom processing of the requests as I do agree with you that JWT is in some way different from the other backends because it can carry custom claims. It has a long way to go, but it looks like a nice solution with a low level of effort.

That said, if you have working and tested solutions for this or the other issue, by all means do share and open a PR: maybe they make a whole lot of sense and I'm just being a bit too defensive here. 🤷
Anyway, thanks for bringing up these points and your will to contribute. This is not a definitive response at all, just my initial thoughts on the issue. As said, maybe looking at the PR will clear up everything, while I'd also love to keep discussing it and know what you think.

[pmous]

@iegomez I do appreciate your efforts, but for me a JS interpreter is a bit far from the point. I do get the advantage: javascript is for a lot more people understandable. However, the things we want to do, just having some simple placeholders and generic claim handling, is not that big a deal and are perfectly writable in go-lang. I feel strongly that the last idea, extending the acl-syntax (see my last remarks on #111), is a good one. I would think that this is a perfect generic addition to the existing plugin. If you, as maintainer, would opt for a specialised jwt-plugin for this, I can understand. Then we could separate some more, just throw out all database-code for example. Thing would become cleaner for sure. But suggesting a js interpreter for this, feels like slamming a mosquito with an elephant (sorry, this is a Dutch expression, but it fitted so good). I fear for the performance as well. Mosquitto is fast, plugin in go-lang too. But relying for all those getuser and acl-checks on javascript?

[iegomez]

@pmous Sure, I feel you and loved that elephant expression. 🐘

But I'm not actually suggesting the JS interpreter as a solution to the particular issues of skipping expiration or extending the syntax, it's just something that occurred to me when you mentioned it might be hard for people to fill in the blanks in the custom plugin interface that's available, and thought I'd mention it here. I'd say is more of an alternative to the remote mode for JWT.

I really need to go through the current discussion you've been having, I'll admit to just glancing very quickly at it. This week is a bit tight, but I'll do my best to engage in the convo as soon as possible.

Cheers!

[pmous]

Cheers, no hurries for me. Take your time.

[iegomez]

@pmous @sirockin I went along adding the JS alternative for 3 reasons:

• It was bloody easy to add a runtime package to just execute scripts, so why not (with the caveat that I need to add a VM pool to improve it after some testing).
• It forced me to refactor the JWT package to create a checker interface that makes it a whole lot cleaner. Now instead of checking a binary remote option, we can just switch over it, instantiate a checker and delegate all the work. This reduces the JWT backend to ~80 lines of code and enables to add any new sub-backends(for lack of a better word) for JWT, such as the one you've been discussing, a breeze. Now I'm much more inclined to add said alternative because maintaining it is much easier.
• Adding a new JS backend, for anyone that's not comfortable with filling the Go based custom plugin, is now trivial.

It's still a WIP, it doesn't even compile because it's late and I'm too tired to fix errors right now, but go take a look and give me your thoughts on this. I believe it'll make it super easy to add the JWT checks you've discussed as just another sub-backendfor it: https://github.com/iegomez/mosquitto-go-auth/pull/116/files

Side note: Holy shit, there's so much to refactor in this plugin, present-me wants to hit past-me in the face so hard. But hey, that's the cost of writing a package/library/program/whatever when you're just learning the language and then have to live with your initial screw ups. Refactoring JWT tests is gonna be a major pain 😭 , but it'll surely be worth it.

Cheers!

[sirockin]

Hi @iegomez

Thank you again for your quick responses and sorry for my slow one. The JS backend sounds like a great addition - I'll try to find some time this weekend to have a look at it.

Regarding this particular issue, you say:

Sadly, I don't agree with the approach. There are two main points to the remote option, which are delegating both the secret and all the logic to the third service: they may skip or do whatever they want.

I'm curious about why you think enabling the auth_opt_jwt_secret as an option for implementers of the remote backend would work against this. It's not removing any of the existing flexibility, just providing the option of some functionality which is already provided for the db option. I'm also fairly sure it wouldn't significantly increase the size of the codebase.

I'm not hung up on this - I've implemented a working remote back end for my own use case, I just think it would be a useful addition to an already very useful project.

Cheers!

[pmous]

@pmous @sirockin I went along adding the JS alternative for 3 reasons:

Great! I think it is a great option for a lot of people to quickly fix a particular problem. Specially when I hear your refactoring makes it easy to implement, I say go for it!
For me it is nog very interesting. In our application mqtt is a very crucial part and for me writing some go-lang code sounds more reliable and better in terms of performance.

It forced me to refactor the JWT package to create a checker interface that makes it a whole lot cleaner. Now instead of checking a binary remote option, we can just switch over it, instantiate a checker and delegate all the work. This reduces the JWT backend to ~80 lines of code and enables to add any new sub-backends(for lack of a better word) for JWT, such as the one you've been discussing, a breeze. Now I'm much more inclined to add said alternative because maintaining it is much easier.

That is great news! It makes me even more convinced that making another sub-backend for the power-ACL is the way to go for me.

It's still a WIP, it doesn't even compile because it's late and I'm too tired to fix errors right now, but go take a look and give me your thoughts on this. I believe it'll make it super easy to add the JWT checks you've discussed as just another sub-backendfor it: https://github.com/iegomez/mosquitto-go-auth/pull/116/files

Not right now, I'm afraid. I have to finish a big pile of work this week and the beginning of next week. Perhaps I can in the weekend.

Side note: Holy shit, there's so much to refactor in this plugin, present-me wants to hit past-me in the face so hard. But hey, that's the cost of writing a package/library/program/whatever when you're just learning the language and then have to live with your initial screw ups. Refactoring JWT tests is gonna be a major pain 😭 , but it'll surely be worth it.

Haha, I've been there! Mostly I see it as prove that I've grown. It would be bad if you look at code from the past and you think "well, that is perfect as it is!"

Cheers!

[iegomez]

@sirockin It's mostly because it means sharing responsibilities, e.g. now you need to get secrets synced or you end up with clients circumventing any remote change. I'm not against having some local default and a remote fallback (or vice versa), but that needs some more thinking and work than just adding the option.

Glad to hear it's not a blocker though, that gives us time to find a good solution.

[sirockin]

It's mostly because it means sharing responsibilities, e.g. now you need to get secrets synced or you end up with clients circumventing any remote change

Sorry I'm not really clear what you mean by this, or how my proposal alters any of the current design philosophy. With the new option, I'd have the choice whether to:

1. As currently, not specify the option, so carry out validation on the backend
2. Implement the option so let the validation be carried out in the plugin (as currently for the DB side)

I really don't see where a conflict arises. It seems a shame (and fairly arbitrary) to provide this functionality to one jwt backend and not to the other.

[iegomez]

Hey, @sirockin, thanks again for your comment! So OK, let's say we add the option, what's the API now? Before we just sent the token and Mosquitto provided values, should we now send all the claims or should there be an option to set which get sent? Encoded how? If the plugin is set to skip expiration, does expiry need to be forwarded to the remote service? And so on...

My point was it's not as obvious as adding the option and calling it a day, thus not trivial nor arbitrary. Again, I may have been too quick to answer before when saying the custom backend was the place for this, it does sound like a nice addition. And sure, those questions are not very good ones, but there may be others that arise and we should answer them before effectively making the option available: it's easy to add options, removing or modifying them is highly problematic.

[sirockin]

Hi @iegomez, thanks for your reply.

So OK, let's say we add the option, what's the API now?

Exactly the same as previously, why would it need to change? Sooo...

Before we just sent the token and Mosquitto provided values, should we now send all the claims

Yes, why not?

or should there be an option to set which get sent?

I don't see why this would be desirable or necessary.

Encoded how?

Exactly as previously

If the plugin is set to skip expiration, does expiry need to be forwarded to the remote service?

Do you mean should the expiry time still be included in the token forwarded to the API? Yes, why not?

And so on...

I understand your worries about over-complicating things and making unnecessary changes to the API. I honestly don't think anything would need to change, other than enabling the option I described.

Thanks again for taking the time to reply and discuss. Like I say, I'm not hung up on this so if you're really uneasy about adding the option, I'm happy to close the issue.

[iegomez]

Nah, no worries, just wanted to clear possible issues out. It looks you were aiming for a solution as simple as possible, e.g. just validate the token and forward whatever was parsed from it, no fancy processing options. I think I can throw that option along with #116 to leverage the clean up it brings.

[iegomez]

@sirockin This is now implemented and available to both remote and JS modes by passing jwt_parse_token and jwt_secret options.

Cheers!

[abh]

For anyone else coming across this trying to figure out how to configure mosquito-go-auto without a database or remote calls:

For JWT-only use cases (without having to implement the checkers in JS) I found https://github.com/wiomoc/mosquitto-jwt-auth met my needs better.