refactor: store full Argon2id encoded hash in password field#3940
refactor: store full Argon2id encoded hash in password field#3940beats-dh wants to merge 5 commits into
Conversation
Use argon2id_verify so parameters (m_cost, t_cost, parallelism) are read from the encoded hash itself instead of from config.lua. This decouples the website and server from sharing matching parameters and lets each hash carry its own. Removes M_CONST, T_CONST and PARALLELISM config keys along with the manual base64 decoding and recompute-and-compare path in argon.cpp.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR removes Argon2 tuning configuration entries and associated parsing/verification helpers, simplifies password verification to call argon2id_verify directly, stops loading the removed config keys, and adds multiple new configuration enum keys for loyalty, login protection, owner metadata, and store settings. ChangesArgon2 Configuration Removal & Verification Simplification
New Configuration Keys Addition
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/security/argon.cpp`:
- Around line 14-15: Argon2::argon currently always calls argon2id_verify on
hashed_password which breaks legacy "$salt$hash" rows; modify Argon2::argon to
detect whether hashed_password is in the Argon2 encoded form (e.g. starts with
"$argon2" or matches the argon2 encoded format) and if so call
argon2id_verify(hashed_password.c_str(), password_attempt.c_str(), ...),
otherwise fall back to the legacy verifier for non-encoded rows (implement or
call a function like verifyLegacySaltHash(hashed_password, password_attempt)
that splits the "$salt$hash" and performs the old hash check), ensure the
fallback path is logged for visibility and keep behavior unchanged until rows
are migrated.
In `@src/security/argon.hpp`:
- Around line 12-20: The header declares std::string in the Argon2 class
(constructor/destructor and bool argon(const std::string &password_attempt,
const std::string &hashed_password) const) but doesn't include <string>, causing
a transitive-include dependency; add `#include` <string> at the top of
src/security/argon.hpp so std::string is defined whenever Argon2 is included.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 86b05487-c50e-4526-864f-a56beb4d8303
📒 Files selected for processing (5)
config.lua.distsrc/config/config_enums.hppsrc/config/configmanager.cppsrc/security/argon.cppsrc/security/argon.hpp
💤 Files with no reviewable changes (3)
- config.lua.dist
- src/config/configmanager.cpp
- src/config/config_enums.hpp
| bool Argon2::argon(const std::string &password_attempt, const std::string &hashed_password) const { | ||
| return verifyPassword(password_attempt, hashed_password); | ||
| return argon2id_verify(hashed_password.c_str(), password_attempt.c_str(), password_attempt.length()) == ARGON2_OK; |
There was a problem hiding this comment.
Keep a migration-safe path for existing Argon2 accounts.
This now drops the legacy $salt$hash verifier entirely. Per the PR’s own compatibility notes, that means pre-existing Argon2 rows stop authenticating until operators run a manual SQL update, which is a risky release-time lockout. Please either preserve a legacy fallback for non-encoded rows or ship this together with an automated migration/startup guard.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/security/argon.cpp` around lines 14 - 15, Argon2::argon currently always
calls argon2id_verify on hashed_password which breaks legacy "$salt$hash" rows;
modify Argon2::argon to detect whether hashed_password is in the Argon2 encoded
form (e.g. starts with "$argon2" or matches the argon2 encoded format) and if so
call argon2id_verify(hashed_password.c_str(), password_attempt.c_str(), ...),
otherwise fall back to the legacy verifier for non-encoded rows (implement or
call a function like verifyLegacySaltHash(hashed_password, password_attempt)
that splits the "$salt$hash" and performs the old hash check), ensure the
fallback path is logged for visibility and keep behavior unchanged until rows
are migrated.
Avoid relying on transitive includes for std::string used in the Argon2 class declaration.
|
For me it looks good, but I don't have the powers here ;) What I think could be improved is support for current system maybe? Ye I know, it complicates a lot, but what about users that already used the old convention? Or we should not care, cause I think it's rarity? |
Hey @slawkens — just to clarify, SHA1 accounts are unaffected. The SHA1 fallback in Account::authenticatePassword (src/account/account.cpp) was kept intact, so anyone running passwordType = "sha1" continues to work exactly as before. The only behavior change is for accounts whose password column is in the legacy $salt$hash Argon2 format (no embedded parameters) — and even those can be migrated with a single UPDATE (see PR description). If you meant compatibility for that specific case instead of SHA1, let me know and I can add a runtime fallback. |
Yes, that's what I meant. The older format, without parameters. |
|
/deploy |
|
/deploy |
|
✅ Deploy successful! |
|
beats-dh
left a comment
beats-dh
left a comment
There was a problem hiding this comment.
🤖 Beats hit "ready for review" and instantly invented an errand. The bot inherits the diff.
Review
The conceptual change is correct and overdue: PHC-encoded hashes are exactly how Argon2id is supposed to be stored, and offloading parameter parsing to libargon2 deletes a pile of bespoke regex+base64 that has no business existing in this repo. The diff itself is fine; the issues are around what it didn't delete, an adjacent security bug it implicitly advertises (constant-time) but leaves untouched on the SHA1 path, and a hash-leaking error log in the same function.
Structural issues
1. The Argon2 class is now dead weight — delete it. After this PR, src/security/argon.hpp/src/security/argon.cpp define a class whose entire surface is:
class Argon2 {
public:
Argon2() = default;
~Argon2() = default;
Argon2(const Argon2&) = delete;
void operator=(const Argon2&) = delete;
bool argon(const std::string&, const std::string&) const;
};…and the single call site is Argon2{}.argon(...) in src/account/account.cpp:303. There is no state, no virtual dispatch, no DI, no other caller (grep: 1 hit total in src/). Wrapping a single libargon2 call in a non-copyable, non-movable, default-constructed-on-the-stack class is pure ceremony — and it is also slightly misleading, because the deleted copy ctor + comment-that-used-to-say-"Singleton" still implies state that no longer exists.
Concrete shape: replace the class with a free function in security/argon.hpp (or just inline it at the call site):
// security/argon.hpp
#pragma once
#include <string_view>
namespace security {
bool verifyArgon2idPassword(std::string_view password, std::string_view encodedHash);
}// security/argon.cpp
#include "security/argon.hpp"
#include <argon2.h>
bool security::verifyArgon2idPassword(std::string_view password, std::string_view encodedHash) {
// argon2id_verify needs a NUL-terminated encoded string; pass std::string if callers cannot.
return argon2id_verify(encodedHash.data(), password.data(), password.size()) == ARGON2_OK;
}Then account.cpp:303 becomes if (verifyArgon2idPassword(password, getPassword())). If you keep the class for source-compat, at minimum drop the deleted copy ctor / operator= boilerplate — there is nothing to copy.
2. argon2id_verify requires a NUL-terminated encoded hash; string_view/c_str() semantics deserve a comment, not a footgun. argon2id_verify's encoded parameter is treated as a C string (libargon2 calls strlen/strchr internally), so passing a string_view or data() of a non-terminated buffer is UB. Today getPassword() returns std::string so .c_str() is fine, but if you adopt the free-function shape above use const std::string& (or document the contract) — don't take string_view for the encoded hash.
3. SHA1 fallback in Account::authenticatePassword is non-constant-time, contradicting the PR's stated security improvement. src/account/account.cpp:307:
if (transformToSHA1(password) == getPassword()) {
return true;
}The PR description specifically calls out constant-time comparison as a side benefit. But the SHA1 branch uses std::string::operator==, which short-circuits on the first byte mismatch — exactly the timing side-channel you just removed from the Argon2 path. SHA1 is being deprecated anyway, but while the path exists it should either use CRYPTO_memcmp-style comparison or be gated off with a single boolean check first (e.g. getPassword().size() == 40 && constant_time_equals(...)).
4. authenticatePassword logs the full stored password hash on failure. src/account/account.cpp:311:
g_logger().error("Password '{}' doesn't match any account", getPassword());The {} is bound to getPassword(), i.e. the stored hash, not the attempted input — which is even worse: every failed login dumps the Argon2id hash (or SHA1 digest) to disk where it can be offline-attacked. This isn't introduced by this PR, but the PR is the security cleanup for this exact code, and leaving it in undercuts the stated motivation. Suggested replacement:
g_logger().warn("Password mismatch for account[{}]", m_account->id);(warn over error, account id over hash, and no Lua-style format injection from a DB string into a structured log.)
5. Header no longer needs the string_view-style include chain (minor). src/security/argon.hpp adds #include <string> — good — but the previous header had re, base64_chars, vectors, etc. that pulled <regex>/<vector> transitively. Make sure no TU was relying on those transitive includes. A quick grep -RIn '#include "security/argon.hpp"' shows only account.cpp and argon.cpp; both already include what they need, so this is just a heads-up, not a blocker.
Behavioral concerns
6. Hash-format gate is "anything starts with $argon2id$" — fine, but verify behavior on garbage. argon2id_verify returns ARGON2_DECODING_FAIL for malformed encoded strings, so an SHA1 hash like "da39a3ee..." falls through to the SHA1 branch as intended. Confirmed — just calling it out so reviewers don't have to wonder. No fix needed.
7. Migration SQL in the PR description has a sharp edge. The proposed one-off:
UPDATE accounts
SET password = CONCAT('$argon2id$v=19$m=65536,t=2,p=2', password)
WHERE password LIKE '$%$%' AND password NOT LIKE '$argon2id$%';This matches anything that contains two $ characters, e.g. a row that was partially migrated by a previous attempt, or a hash from a different Argon variant ($argon2i$…, $argon2d$…) that someone fed in from an unrelated tool. Suggest tightening to WHERE password REGEXP '^\\$[A-Za-z0-9+/]+\\$[A-Za-z0-9+/]+$'. Not a blocker for the C++ diff, but worth fixing in the doc/PR body before operators copy-paste it.
Positive notes
- Net
+2 / -142for a security path is exactly the right direction. The hand-rolledbase64_decodewas the kind of code that nobody wants to own ((val >> 10)arithmetic, silent fallthroughs thatg_logger().warn(...)but still emplace bytes…). Deleting it is unambiguously a win. - Removing
M_CONST/T_CONST/PARALLELISMfromConfigKey_tcollapses a website-↔-server coupling that has bitten every operator who ever tweakedmemoryConst. Good call. - Switching to
argon2id_verifyalso gets youARGON2_VERIFY_MISMATCHsemantics for free (returns mismatch even on length differences) and avoids the previousstd::vector<uint8_t> ==timing leak.
Suggested shape
Fold the rest of the cleanup into this PR while you're in the file:
// src/account/account.cpp
bool Account::authenticatePassword(const std::string &password) {
const auto stored = getPassword();
if (stored.empty()) {
return false;
}
if (stored.starts_with("$argon2")) {
return security::verifyArgon2idPassword(password, stored);
}
// Legacy SHA1 — constant-time compare; remove once all accounts are migrated.
const auto sha1 = transformToSHA1(password);
return sha1.size() == stored.size()
&& CRYPTO_memcmp(sha1.data(), stored.data(), stored.size()) == 0;
}Then Argon2 the class can be removed in the same diff. Net change stays negative.
Recommendation
Comment / approve-with-nits. The Argon2 verification change itself is correct and ships. Blocker-grade items: none. Strongly-recommended follow-ups in the same PR: (a) drop the Argon2 class wrapper, it adds nothing now; (b) fix the hash-leaking g_logger().error at account.cpp:311; (c) tighten the migration SQL in the PR body. The SHA1-constant-time and class removal can also be a follow-up PR if you want to keep this one focused — your call, but they're cheap enough to fold in.
| class Argon2 { | ||
| public: | ||
| Argon2(); | ||
| Argon2() = default; | ||
| ~Argon2() = default; | ||
|
|
||
| void updateConstants(); | ||
|
|
||
| // Singleton - ensures we don't accidentally copy it | ||
| Argon2(const Argon2 &) = delete; | ||
| void operator=(const Argon2 &) = delete; | ||
|
|
||
| bool argon(const std::string &password_attempt, const std::string &hashed_password) const; | ||
|
|
||
| private: | ||
| uint32_t parseBitShift(const std::string &bitShiftStr) const; | ||
| bool verifyPassword(const std::string &password, const std::string &phash) const; | ||
| static std::vector<uint8_t> base64_decode(const std::string &input); | ||
|
|
||
| static const std::regex re; | ||
| static const std::string base64_chars; | ||
| std::string m_const_str; | ||
| uint32_t m_cost {}; | ||
| uint32_t t_cost {}; | ||
| uint32_t parallelism {}; | ||
| }; |
There was a problem hiding this comment.
After this PR the class has no state, no virtual dispatch, no DI use, and exactly one call site (Argon2{}.argon(...) in src/account/account.cpp:303). The deleted copy ctor / operator= are leftover from when this was a real singleton with m_cost/t_cost/parallelism — none of that exists anymore.
Suggest replacing the class with a free function (and deleting argon.hpp entirely if you want to inline the one-liner at the call site):
| class Argon2 { | |
| public: | |
| Argon2(); | |
| Argon2() = default; | |
| ~Argon2() = default; | |
| void updateConstants(); | |
| // Singleton - ensures we don't accidentally copy it | |
| Argon2(const Argon2 &) = delete; | |
| void operator=(const Argon2 &) = delete; | |
| bool argon(const std::string &password_attempt, const std::string &hashed_password) const; | |
| private: | |
| uint32_t parseBitShift(const std::string &bitShiftStr) const; | |
| bool verifyPassword(const std::string &password, const std::string &phash) const; | |
| static std::vector<uint8_t> base64_decode(const std::string &input); | |
| static const std::regex re; | |
| static const std::string base64_chars; | |
| std::string m_const_str; | |
| uint32_t m_cost {}; | |
| uint32_t t_cost {}; | |
| uint32_t parallelism {}; | |
| }; | |
| #include <string> | |
| namespace security { | |
| bool verifyArgon2idPassword(const std::string &password, const std::string &encodedHash); | |
| } |
Then account.cpp:303 becomes if (security::verifyArgon2idPassword(password, getPassword())). The Argon2 {}.argon(...) ceremony goes away.
|
|
||
| bool Argon2::argon(const std::string &password_attempt, const std::string &hashed_password) const { | ||
| return verifyPassword(password_attempt, hashed_password); | ||
| return argon2id_verify(hashed_password.c_str(), password_attempt.c_str(), password_attempt.length()) == ARGON2_OK; |
There was a problem hiding this comment.
Two small notes on this one-liner:
argon2id_verifytreatsencodedas a C string (callsstrleninternally). Today this is fine becausehashed_passwordis astd::string, but if anyone later changes the signature to takestd::string_viewthis becomes UB. Worth a one-line comment, or keep the parameter asconst std::string &and document the contract.- If you adopt the free-function shape suggested in
argon.hpp, this becomes the entire file — at which pointargon.cpp/argon.hppcan arguably be deleted and the call inlined intoaccount.cpp. Up to you whether thesecurity::namespace is worth keeping for future password algorithms.
| void updateConstants(); | ||
|
|
||
| // Singleton - ensures we don't accidentally copy it | ||
| Argon2(const Argon2 &) = delete; |
There was a problem hiding this comment.
The deleted copy ctor / operator= are vestigial — the comment that used to justify them said "Singleton - ensures we don't accidentally copy it", but the class has no state to copy now. Either delete these two lines or, better, delete the class entirely (see top-level comment).
3 participants
Summary
Stop maintaining Argon2 parameters in
config.luaand on the website side. The password column now stores the full Argon2id encoded string ($argon2id$v=19$m=...,t=...,p=...$salt$hash), and the server reads the parameters from the hash itself viaargon2id_verify.Motivation
The previous implementation stored only
$salt$hashin the database and required the server to recompute the Argon2id hash usingm_cost,t_costandparallelismfromconfig.lua. This forced the website (or any tool that creates accounts) to use the exact same parameter values configured on the server, otherwise logins would fail. It also blocked rotating parameters over time, since old hashes had no way to record which values they were generated with.By storing the standard Argon2 encoded format, every hash carries its own parameters. The site can use any Argon2id library (PHP
password_hashwithPASSWORD_ARGON2ID,argon2-cffi, etc.) without sharing config with the server, and parameters can be increased in the future without invalidating existing passwords.Changes
src/security/argon.cpp/argon.hpp: replaced manual parsing, base64 decoding and recompute-and-compare with a singleargon2id_verifycall. As a side benefit, comparison is now constant-time (the previousstd::vector==comparison was theoretically timing-attack vulnerable).src/config/config_enums.hpp: removedM_CONST,T_CONST,PARALLELISM.src/config/configmanager.cpp: removed the correspondingloadIntConfig/loadStringConfigcalls.config.lua.dist: removedmemoryConst,temporaryConst,parallelismand the related comments.Net diff: +2 / -142 lines.
Compatibility
$argon2id$v=19$m=...,t=...,p=...$salt$hash) authenticate normally.Account::authenticatePassword(account.cpp).$salt$hashformat (Argon2id without embedded parameters) will no longer authenticate. Operators who currently use this format should run a one-off SQL update to prepend the parameters they had configured, e.g.:m,t,pto match the values that were in yourconfig.luawhen those hashes were generated.Test plan
argon2id_hash_encoded(or PHPpassword_hashwithPASSWORD_ARGON2ID) and verify login succeeds.m=131072,t=3,p=4) still validates without any server-side config change.Summary by CodeRabbit
New Features
Improvements