Harden OTP system: brute-force protection, memory cleanup, and key collision fix#224
Open
Koaio wants to merge 28 commits into
Open
Harden OTP system: brute-force protection, memory cleanup, and key collision fix#224Koaio wants to merge 28 commits into
Koaio wants to merge 28 commits into
Conversation
Koaio
commented
Mar 10, 2026
Koaio
commented
Contributor
- Fix PrivateChatOtpConstant colliding with AllChatOtpConstant (both were 0x14E05CE5)
- Add brute-force protection: keys are locked out after 5 failed attempts for 60s
- Add automatic purge of expired codes and stale lockout records to prevent memory leaks
- Replace direct std::random_device calls with a properly seeded std::mt19937 engine
- Make OTP settings configurable (TTL, max attempts, lockout duration, purge interval)
- Fix ranch OTP key to include rancherUid (grant and authorize now use matching composite keys)
- Remove consume=false on AllChat authorization so codes are single-use
- Clean up expired codes immediately on failed authorization
Refactor OTP code management and authorization logic.
Added a Settings struct to configure OTP parameters such as code TTL, max failed attempts, lockout duration, and purge interval. Updated the OtpSystem constructor to accept these settings.
rgnter
requested changes
Mar 11, 2026
| class OtpSystem | ||
| { | ||
| public: | ||
| struct Settings |
Member
There was a problem hiding this comment.
This setting struct should be defined in Settings.hpp header. See AuthenticationService for example..
| }; | ||
|
|
||
| OtpSystem(); | ||
| explicit OtpSystem(Settings settings); |
Member
There was a problem hiding this comment.
This should take forward-definition of ServerInstance and get config by accessing GetSettings and taking a reference (so reloading of configs can be implemented easily). For example see AuthenticationService.
Comment on lines
+60
to
+65
| struct AttemptRecord | ||
| { | ||
| uint32_t failedAttempts{0}; | ||
| std::chrono::steady_clock::time_point lockoutExpiry{}; | ||
| }; | ||
|
|
Member
There was a problem hiding this comment.
Please merge AttemptRecord with the Code struct.
struct Code
{
//! A time point of when the code expires.
std::chrono::steady_clock::time_point expiry{};
//! A counter of authorization attempts.
size_t authorizationAttempts{};
//! A value of the code.
uint32_t code{};
};Also if you read previous comments, reference to the ServerInstance should be taken and access to settings should be done by reference.
| std::mt19937 _rng; | ||
| std::unordered_map<size_t, Code> _codes; | ||
| std::unordered_map<size_t, AttemptRecord> _attempts; | ||
| std::chrono::steady_clock::time_point _lastPurge; |
Member
There was a problem hiding this comment.
Not necessary. Invalidate the key/code after 5 unsuccessful attempts to use it.
Co-authored-by: Maroš Prejsa <maros.prejsa1@gmail.com>
Co-authored-by: Maroš Prejsa <maros.prejsa1@gmail.com>
Co-authored-by: Maroš Prejsa <maros.prejsa1@gmail.com>
Co-authored-by: Maroš Prejsa <maros.prejsa1@gmail.com>
rgnter
requested changes
Mar 13, 2026
Co-authored-by: Maroš Prejsa <maros.prejsa1@gmail.com>
Add Config::Otp with codeTtl, maxFailedAttempts, lockoutDuration and purgeInterval so OTP parameters live in the central config like other services.
Read otp.code_ttl, otp.max_failed_attempts, otp.lockout_duration and otp.purge_interval from config YAML; keep defaults when section is absent.
- Remove inline Settings struct and default/overload constructors. - Forward-declare ServerInstance; single constructor takes - ServerInstance& so config is read via GetSettings() for reload support.
Constructor takes ServerInstance&. GrantCode, AuthorizeCode and PurgeExpired use _serverInstance.GetSettings().otp for all parameters.
Pass server instance reference to OtpSystem so it can access config via GetSettings().
Add authorizationAttempts to Code; remove AttemptRecord and _attempts. Invalidate code after max failed attempts (no lockout).
GrantCode sets authorizationAttempts to 0. AuthorizeCode increments on failure and erases code when threshold reached; PurgeExpired only purges expired codes.
Lockout removed in favor of invalidating code after max failed attempts.
Otp no longer uses lockout; only code_ttl, max_failed_attempts and purge_interval are read.
Member
|
@Koaio this PR needs to address the review comments and be rebased, if not done so within a week I'll close it as stale and unmantained |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.