perf(gateway): hash each account token with PBKDF2 once per process - #264
Conversation
Every account-token request ran PBKDF2 (120,000 iterations, about 18 ms) inside OperatorAccountService's global lock and then rewrote the accounts file, capping gateway-wide account-token throughput. After a token's first successful verification, later uses find it by SHA-256 digest (the tokens are 192-bit random secrets) and skip PBKDF2. Each use still re-checks the token's revocation and expiry and the account's enabled state and role, so none of those needs cache invalidation. Token use now updates lastLoginAtUtc at most once a minute instead of on every request. The service takes an optional TimeProvider and hash function so tests can control time and count hashes. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe operator account service now supports injected time and secret hashing. It caches verified token digests, rechecks authorization state on each use, and limits last-login persistence to once per minute. ChangesOperator token authentication
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to No unresolved issue identified here prevents merging after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Cached token checks still consult the current account and token records before granting access. No new authorization bypass was established, but the change reduces the precision of recorded token activity, and some deployment and request-transport details remain unverified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
… saved The last-login throttle set the in-memory timestamp before saving. If the save failed, that request threw, but the next minute of requests saw a recent timestamp, skipped the write, and authenticated without recording the login. Restore the previous timestamp when the save fails, so every request keeps trying, as before the throttle. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Addressed the retained concern from the CodeRabbit security review in 9e8143f: the last-login timestamp now counts toward the one-minute interval only after it is saved. If the save fails, the previous timestamp is restored, so the next request tries again (and fails the same way) instead of skipping the write. |
| Assert.True(accounts.TryAuthenticateToken(token.Token, out _)); | ||
| _clock.Now = _clock.Now.AddMinutes(2); | ||
|
|
||
| var adminDirectory = Path.Combine(_storagePath, "admin"); |
|
|
||
| private static bool DirectoryIsWritable(string directory) | ||
| { | ||
| var probe = Path.Combine(directory, $".probe-{Guid.NewGuid():N}"); |
Description
Every request that carries an account token runs
OperatorAccountService.TryAuthenticateToken, which:admin/operator-accounts.jsonto recordlastLoginAtUtc.That caps account-token throughput for the whole gateway at roughly 50 requests per second on one core, whatever the hardware. #261 removes the second verification per request that #256 added; this PR removes the per-request cost itself.
Summary
lastLoginAtUtcat most once a minute instead of rewriting the accounts file on every request. Password logins and token exchange still record every login.TimeProviderand hash function; production passes neither.Benchmarks
200 sequential verifications of one token on the same machine (temporary benchmark, not committed):
The first use of each token per process still pays for PBKDF2.
Type of Change
Validation
dotnet build OpenClaw.Net.slnx --configuration Release: 0 warnings, 0 errorsdotnet test OpenClaw.Net.slnx --configuration Release --no-build: OpenClaw.Tests 3,130 passed / 11 skipped / 0 failed; NacosLiveAcceptance 25 passed; LayaService 67 passeddotnet run --project samples/OpenClaw.HelloAgent -c Release --no-buildNew tests in
OperatorAccountServiceTests:lastLoginAtUtc.Review Notes
CHANGELOG.md)Commercial or Customer-Driven Contribution Disclosure
Found while reviewing the #256 role checks for AgentQi Mobile, which polls the gateway with an account token. Vendor-neutral gateway performance fix.
Checklist
dotnet test)🤖 Generated with Claude Code
Summary by CodeRabbit