fix(wallet): tolerate client clock skew in credential expiry checks#377
Open
noevidence1017 wants to merge 1 commit into
Open
fix(wallet): tolerate client clock skew in credential expiry checks#377noevidence1017 wants to merge 1 commit into
noevidence1017 wants to merge 1 commit into
Conversation
Session-key and allowance expiry were compared against the consensus ledger timestamp with a strict `now > expiry`. Clients pick that expiry from their local wall clock (and, for ledger-bound lifetimes, an assumed ~5s/ledger rate), so client clock drift and ledger-rate rounding could place the stored expiry a few seconds early and cause spurious SessionKeyExpired / AllowanceExpired rejections at the boundary. Add a dedicated `auth::expiration` module that centralises the comparison and applies a small, fixed clock-skew grace window (CLOCK_SKEW_TOLERANCE_SECS = 60). The on-chain ledger timestamp is part of consensus and identical across validators, so a strict comparison is correct on the contract side; the grace window only ever extends validity, by a bounded constant, to absorb client-side estimation error. Session-key enforcement and the allowance check now route through this module. Adds unit tests for the boundary (at expiry, within grace, one second past grace, overflow safety) and end-to-end boundary tests through `session_key::enforce`. Closes Miracle656#365
|
@noevidence1017 is attempting to deploy a commit to the miracle656's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@noevidence1017 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
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
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.
Summary
Audits and hardens clock-skew handling for time-bound credentials in
__check_auth.Session-key (
SessionKeyAcl::expiry) and allowance (Allowance::expiry) expiry were compared against the consensus ledger timestamp with a strictnow > expiry. Clients choose that expiry from their local wall clock and, for ledger-bound lifetimes, an assumed ledger rate (~5 s/ledger). Local clock drift and ledger-rate rounding can therefore place the stored expiry a few seconds earlier than the user expects, producing spuriousSessionKeyExpired/AllowanceExpiredrejections right at the boundary.Behaviour (documented)
env.ledger().timestamp(), which is part of consensus and identical for every validator in a given ledger. There is no validator-side skew, so a strict comparison is correct on the contract side.CLOCK_SKEW_TOLERANCE_SECS = 60, applied to every expiry comparison. The grace window only ever extends validity, by a bounded constant, so it cannot cause an unexpired credential to be rejected and cannot weaken any check other than expiry.Changes
contracts/invisible_wallet/src/auth/expiration.rscentralising expiry logic:is_expired(now, expiry)— tolerance-aware, with saturating add for overflow safety.is_expired_strict(now, expiry)— exact boundary, for tests/docs.ensure_not_expired(now, expiry, err)—Resulthelper letting each caller surface its own error variant.session_key::enforceand the allowance check in__check_authnow route through this module instead of inlinenow > expiry.Tests
u64::MAXoverflow safety and zero-expiry cases.session_key::enforce(at expiry, within grace, past grace).cargo test -p invisible-wallet→ 62 passed; 0 failed.Closes #365