Cache behavior for topics that are constantly active within the cache expiration time

[arctic-alpaca]

Hello,

I'm using version 1.0.0 of this plugin and version 1.14.15 of Mosquitto in a docker container on a Raspberry Pi with redis as cache.
The plugin is used for authentication and authorization with the ChirpStack.

I noticed, when testing with very frequent published messages, that the cache doesn't expire even though cache expiration is set.
I'm not familiar with go, but I think this is caused by this function:

mosquitto-go-auth/cache/cache.go

Line 212 in 4b20bb3

func (s *redisStore) getAndRefresh(ctx context.Context, record string, expirationTime time.Duration) (bool, bool, error) {

Every cache hit refreshes the expiration date. This leads to cache entries never getting removed even if the backend removes the entry.
Is this intended? I think the cache should expire regardless of activity.

[iegomez]

@arctic-alpaca Indeed, the cache is refreshed because it aims for the case of say having topics published with a frequency of ~15 seconds +- a couple of seconds, you can avoid having to hit your underlying store every couple of messages by setting the expiration to something slightly higher than that, ensuring quick availability for your frequent cases while not automatically granting for the less common ones.

But I see the need for a more general cache that just grants access for some period of time without refreshing, so thanks for bringing it up. Off the top of my head I'm thinking in options such as no_refresh, max_refresh_time, max_refresh_amount, etc. I'd love to know your opinion before implementing anything.

[arctic-alpaca]

Indeed, the cache is refreshed because it aims for the case of say having topics published with a frequency of ~15 seconds +- a couple of seconds, you can avoid having to hit your underlying store every couple of messages by setting the expiration to something slightly higher than that, ensuring quick availability for your frequent cases while not automatically granting for the less common ones.

While this does reduce load on the backend significantly, it also introduces a, in my opinion, quite problematic security issue. This behavior prevents an admin from revoking access to any user who publishes/receives messages with a frequency higher than the cache expiration. At least without restarting the service with cache_reset true.

I think the best approach would be to change the default behavior to let cache entries time out regardless of acivity and to introduce options to modify this behavior backend specific. For example the current behavior could be desired for ACL and password files, but is probably not desired for databases used to grant and revoke access dynamically.

[iegomez]

@arctic-alpaca Yeah, I know there's a security issue in the approach, was asking more about what you were envisioning besides that. Having per backend options is very tricky and only possible when using prefixes, otherwise we don't know which backend options we should look at to refresh the cache or not. Let's keep it simple for now and just add an auth_opt_cache_refresh option to signal whether to refresh or not, defaulting to true because it's been the normal behavior until now and I don't wanna mess with expectations. What do you think?

[arctic-alpaca]

I'm not sure having a security issue in the default settings is the right aproach. To put it bluntly, my expectations when using a cache don't include being unable to revoke access in some scenarios.
I understand that changing default behavior can be difficult but as this impacts security, I think it is warranted.

If you don't want to default auth_opt_cache_refresh to false until you release a version 2.0 or something similar, I would suggest adding a disclaimer to the readme that explains the security concerns.

[iegomez]

@arctic-alpaca After some thought, yeah, you're right: let's play it safe and default to not refreshing records.
Here's a PR in case you wanna take a look before I merge it: #101

[arctic-alpaca]

I'm glad you decided this way, thank you very much for your quick response and your work on this plugin! The PR looks good to me, but I'm not familiar with Go.