Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
maglnet opened this issue Jan 24, 2024 · 6 comments
Open

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

maglnet opened this issue Jan 24, 2024 · 6 comments

Comments

@maglnet
Copy link

maglnet commented Jan 24, 2024

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:

  • the cache is checked for user / topic / acc
  • no cache hit
  • an acl check for user / topic / acc is made
    • internally a superuser check is made -> granted
  • the result of user / topic / acc is cached

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

  • the cache is checked for user / topic / acc
  • no cache hit
  • an acl check for user / topic / acc is made
    • internally a superuser check (http request) is made -> denied
    • an acl check (http request) is made -> denied or granted
  • the result of user / topic / acc is cached

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
Copy link
Owner

iegomez commented Jan 25, 2024

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
Copy link
Author

maglnet commented Jan 26, 2024

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
Copy link
Owner

iegomez commented Jan 26, 2024

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
Copy link
Owner

iegomez commented Jan 26, 2024

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
Copy link
Author

maglnet commented Jan 26, 2024 via email

@iegomez
Copy link
Owner

iegomez commented Mar 1, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants