iegomez / mosquitto-go-auth

Auth plugin for mosquitto.
MIT License
495 stars 165 forks source link

http backend: superuser check is made for every single acl check #310

Open maglnet opened 6 months ago

maglnet commented 6 months ago

Hi,

I'm currently using a http backend with caching and superuser check enabled and observed the following behavior:

As a superuser, for every subscribed topic the following happens:

As a normal user, for every subscribed topic the following happens:

Speaking of 50 subscribed topics, 1 superuser account and 1 normal account, this results in 150 requests. (50 superuser checks for superuser + 50 superuser checks for normal user + 50 acl checks for normal user)

If the superuser check would be cached, this would reduce the requests to 52 requests. (1 superuser check per user + 50 acl checks for the normal user)

When scaling to more users and more topics this may result in an large amount of requests made for superuser and acl checks.

My configuration is like the following

auth_plugin /mosquitto/go-auth.so

auth_opt_backends http
auth_opt_http_host mymqttserver
auth_opt_http_port 443
auth_opt_http_getuser_uri /auth/check_user
auth_opt_http_superuser_uri /auth/check_superuser
auth_opt_http_aclcheck_uri /auth/check_acl
auth_opt_http_with_tls true
auth_opt_http_response_mode json

auth_opt_cache true
auth_opt_auth_cache_seconds 120
auth_opt_acl_cache_seconds 120
auth_opt_auth_jitter_seconds 3
auth_opt_acl_jitter_seconds 3

Maybe I did something wrong? If not, is there anything that may improve this behavior / reduce requests? I'm not sure if this happens for other remote backends, too.

Best regards, Matthias

iegomez commented 6 months ago

This seems odd, if an ACL check passed because the user was a superuser or because it wasn't but had right access, then an ACL cache record should be persisted, the cache is agnostic of the reason.

I may be wrong though, but could you check that your cache records are not actually expiring and that's why the superuser check gets conducted?

maglnet commented 6 months ago

Hi,

I checked and my cache is stored for 2 minutes, so there are no requests within these 2 minutes, when topics get written, that's fine, but after that, a request for every written topic is made against the superuser endpoint again.

After a bit more analysis, I think the problem is here, where the result of the acl check is written to cache and the superuser info is not available: https://github.com/iegomez/mosquitto-go-auth/blob/master/go-auth.go#L363 https://github.com/iegomez/mosquitto-go-auth/blob/master/go-auth.go#L378

The cache key also has the topic as a part of it (which it has to have, that's fine) but it doesn't save the information of superuser within the cache, because it doesn't have the information here..

So while writing this, I'm wondering if the caching layer should be implemented within the backends.AuthAclCheck function here, where the superuser information is available and could be cached, too: https://github.com/iegomez/mosquitto-go-auth/blob/master/backends/backends.go#L410

Please let me know if I can do anything to help fix this :-)

best regards, Matthias

iegomez commented 6 months ago

Ah, I understand now: after the cache expires you have a rapid burst of reads/writes against 50 topics, and they all conduct a superuser check prior to the ACL one. So yeah, we could cache being a superuser and then the first call has to make the request but the other 49 don't (well, kind of depending on timing, but that's the idea).

I like it, let me find some time to work on that and I'll let you know when it's ready. In the meantime I can only recommend increasing cache expiration to mitigate the problem.

iegomez commented 6 months ago

As you noticed, it's not straightforward since the caching is done at an upper level, so this will need some non trivial refactoring.

maglnet commented 6 months ago

Thanks for looking and in general thanks for this great plugin. 👍

For now I'll stick with a very long cache time and if I've some spare time, I'll see if I'm able to set up a dev environment for this, but I'm not very familiar with go.

Best regards.

Am 26. Januar 2024 19:39:13 MEZ schrieb "Ignacio Gómez" @.***>:

As you noticed, it's not straightforward since the caching is done at an upper level, so this will need some non trivial refactoring.

-- Reply to this email directly or view it on GitHub: https://github.com/iegomez/mosquitto-go-auth/issues/310#issuecomment-1912517155 You are receiving this because you authored the thread.

Message ID: @.***>

iegomez commented 5 months ago

Hey, I started working on this a couple of weeks ago but then got derailed by some other stuff. I'll let you know when I resume it.