search

iegomez / mosquitto-go-auth

Auth plugin for mosquitto.
MIT License
522 stars 172 forks source link

Close client connection on JWT expiry #106

Open sirockin opened 3 years ago

sirockin commented 3 years ago

This issue follows and summarises a discussion in #90 about handling of JWT expiry times.

Currently the mosquitto plugin interface does not provide a mechanism for us to feed back an expiry time for a user, either when the user is first queried or when the expiry time is reached. This means that our only means to enforce the expiry time is to stop sending subscribed data and refuse further subscribe or publish requests. This is slightly messy since from the client's point of view, its subscriptions remain active.

Since the mosquitto plugin interface is currently under review (see https://github.com/eclipse/mosquitto/issues/1779) we should aim to keep a dialog open with the maintainers of that project to find a way to feed back this expiry time, then implement in this plugin.

ralight commented 3 years ago

If I understand correctly, you should be able to use the new mosquitto_kick_client_by_clientid() or mosquitto_kick_client_by_username() functions to get rid of the expired client.

You may also find the MOSQ_EVT_TICK event useful, it is called on every instance of the main event loop.

iegomez commented 3 years ago

Oh, I missed that! Thanks, @ralight, now it should be fairly easy to return a custom error from the plugin and kick out the client on auth_plugin.c based on that. And we could poll for expirations on that MOSQ_EVT_TICK event as well. @sirockin, https://github.com/iegomez/mosquitto-go-auth/pull/120 adds different responses to the plugin. I can piggy back on that after it's merged to implement on expiration disconnect.

iegomez commented 3 years ago

Update: I already started a (local) branch on this to kickoff expired users, though I'm not sure about polling client statuses. Anyway, when I have the basis for kicking out on request, I'll open a PR and let you know so we can decide if that's enough or active polling is needed (I've given Mosquitto events a quick look, but am not entirely sure how they'll work with this package since I need to add some queryable mechanism to tell who/what is to be evicted, while also exposing that to C code. It's not that hard, I'm just not sure if it's worrh it).

iegomez commented 3 years ago

It's been a while since I started and then forgot about this, please ping me if there's still interest in getting it implemented.

Cheers!

iegomez commented 1 year ago

Well, it's been a long while since my last comment asking for interest on this. There were some emoji reactions, but I don't get notified about those and might very well be as old as my response. So I'll give it a couple of days to see if there's any new comment, and hopefully a PR to review, or just close the issue.

Not that I think it's an unworthy feature, I actually like the idea a lot, but if no one contributes then it's probably not gonna happen. Sorry about that.

spencerfeng commented 11 months ago

Hi @iegomez, really love to see this feature implemented. Otherwise, there will be a security hole there.

iegomez commented 11 months ago

@spencerfeng I won't be implementing it, but a PR is very welcome.