Anonymous: Add configurable device limit - #762
malinosqui wants to merge 36 commits into
Conversation
* Anonymous: Add device limiter * break auth if limit reached * fix typo * refactored const to make it clearer with expiration * anon device limit for config --------- Co-authored-by: Eric Leijonmarck <eric.leijonmarck@gmail.com>
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
| // if device limit is reached, only update devices | ||
| if s.deviceLimit > 0 { | ||
| count, err := s.CountDevices(ctx, time.Now().UTC().Add(-anonymousDeviceExpiration), time.Now().UTC().Add(time.Minute)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if count >= s.deviceLimit { | ||
| return s.updateDevice(ctx, device) | ||
| } | ||
| } |
There was a problem hiding this comment.
Time-of-Check to Time-of-Use (TOCTOU) race condition between CountDevices and the device insert allows concurrent requests to bypass limit checks and exceed the configured device count. Enforce the limit atomically using a database constraint or lock.
Prompt for LLM
File pkg/services/anonymous/anonimpl/anonstore/database.go:
Line 108 to 118:
WHAT: A Time-of-Check to Time-of-Use (TOCTOU) race condition exists between checking the `CountDevices` result and inserting a new device.
WHY: Multiple concurrent requests from new devices can simultaneously read a device count below the limit, bypass the `updateDevice` branch, and execute inserts that cause the total count to exceed the configured device limit.
HOW: Enforce the device limit atomically using a database constraint or lock, or document that concurrent bursts may slightly exceed the configured limit.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| if err := a.anonDeviceService.TagDevice(ctx, httpReqCopy, anonymous.AnonDeviceUI); err != nil { | ||
| if errors.Is(err, anonstore.ErrDeviceLimitReached) { | ||
| return nil, err | ||
| } | ||
| }() | ||
|
|
||
| a.log.Warn("Failed to tag anonymous session", "error", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
Cache inconsistency in TagDevice persists the device ID before database verification, allowing rejected devices to bypass limits on subsequent retries. Clear the cache entry using a.localCache.Delete(key) in the tagDeviceUI error path.
Prompt for LLM
File pkg/services/anonymous/anonimpl/client.go:
Line 44 to 51:
WHAT: The authentication block relies on `TagDevice`, which incorrectly caches the device ID before verifying if the database insert succeeds and fails to remove it on error.
WHY: An attacker whose new device is correctly rejected due to the device limit can simply retry their request, which will hit the incorrectly preserved cache, return no error, and grant them access.
HOW: Clear the cache entry if the database operation fails (e.g., by adding `a.localCache.Delete(key)` to the error path in `tagDeviceUI`).
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
2 participants
This pull request introduces a configurable limit on the number of anonymous devices that can access the system.
Key Changes:
device_limitsetting under the[auth.anonymous]section in the configuration.device_limitis reached (and is greater than 0), the system will reject the creation of new anonymous devices while still allowing existing active devices to update their sessions.anonymousDeviceLimitto the frontend viaGrafanaConfigandFrontendSettingsDTO.