Trigger Noise session rekey on absolute age, not just idle time (#1675)

* Trigger Noise session rekey on absolute age, not just idle time

needsRenegotiation() had two triggers: message count nearing the 1
billion-message ceiling, and lastActivityTime idle for longer than
sessionTimeout (24h). Neither fires for a session with steady traffic:
message count realistically never approaches a billion, and
lastActivityTime resets on every encrypt/decrypt, so a continuously
active conversation never looks idle.

Meanwhile encrypt/decrypt both hard-fail with sessionExpired once
sessionStartTime crosses the same 24h sessionTimeout, unconditionally,
with no rekey ever having been scheduled to prevent it. Two peers who
keep a conversation open past 24h (a stationary mesh node, or two
phones left open overnight) hit this simultaneously: every message
throws sessionExpired on both sides, isEstablished() still reports true
so the UI keeps showing the session as secured, MessageRouter never
falls back off the transport because canDeliverSecurely never goes
false, and nothing re-handshakes because the one self-heal path
(BLENoisePacketHandler's decrypt-failure recovery) requires an inbound
ciphertext to fail to decrypt for a session-establishment reason, not
sessionExpired specifically -- so it's excluded. The conversation is
silently and permanently dead until an app restart or a BLE
disconnect/reconnect forces revalidation.

Adds a third needsRenegotiation() trigger on sessionStartTime age
directly, firing at 90% of sessionTimeout -- the rekey timer polls
every 60s (NoiseEncryptionService.rekeyCheckInterval), leaving over two
hours of margin to complete a full handshake before the hard cutoff.

No Xcode/Swift toolchain available here, so this is verified by
tracing, not compiling: manually walked the new
continuouslyActiveSessionStillNeedsRenegotiationBeforeExpiry test's
inputs through both the pre-fix and post-fix needsRenegotiation() logic
(confirmed the pre-fix code returns false for a session with fresh
lastActivityTime and old sessionStartTime, and the fix flips it to
true), checked every other call site of needsRenegotiation() and
getSessionsNeedingRekey() in the test suite to confirm none depend on
the old always-false behavior at this age, and confirmed via
git apply -R / git apply that the source diff is exactly the traced
change with no other drift.

* Fix stale comment in needsRenegotiation idle time check
This commit is contained in:
krish rathi 2026-09-24 13:04:14 +05:30 committed by GitHub
parent 25a3219a53
commit 5a10ba6bf9
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 53 additions and 3 deletions

View File

@ -66,12 +66,24 @@ final class SecureNoiseSession: NoiseSession {
if messageCount >= messageThreshold {
return true
}
// Check if last activity was more than 30 minutes ago
// Check if last activity exceeded sessionTimeout
if Date().timeIntervalSince(lastActivityTime) > NoiseSecurityConstants.sessionTimeout {
return true
}
// A session that never goes idle (a long-lived conversation with
// steady traffic) keeps refreshing lastActivityTime forever, so the
// check above never fires -- only sessionStartTime tracks how close
// encrypt/decrypt are to the hard sessionExpired cutoff above. Firing
// at 90% of sessionTimeout leaves the rekey timer's polling interval
// (checked every 60s, see NoiseEncryptionService.rekeyCheckInterval)
// room to complete a full handshake well before the old session
// starts throwing sessionExpired on every call.
if Date().timeIntervalSince(sessionStartTime) > NoiseSecurityConstants.sessionTimeout * 0.9 {
return true
}
return false
}

View File

@ -893,6 +893,44 @@ struct NoiseCoverageTests {
}
}
@Test("A continuously active session still schedules a rekey before it hard-expires")
func continuouslyActiveSessionStillNeedsRenegotiationBeforeExpiry() throws {
// A session with steady traffic never goes idle, so lastActivityTime
// stays fresh right up to the moment sessionStartTime crosses the
// hard sessionTimeout cutoff above and encrypt/decrypt start throwing
// sessionExpired on every call. needsRenegotiation() has to notice
// the session is old on its own -- it can't rely on the idle check,
// because there is no idle time to find.
let session = SecureNoiseSession(
peerID: alicePeerID,
role: .initiator,
keychain: keychain,
localStaticKey: aliceStaticKey
)
let responder = SecureNoiseSession(
peerID: bobPeerID,
role: .responder,
keychain: keychain,
localStaticKey: bobStaticKey
)
try establishSessions(initiator: session, responder: responder)
// Just under the hard cutoff, but past the 90% rekey threshold.
session.setSessionStartTimeForTesting(
Date().addingTimeInterval(-(NoiseSecurityConstants.sessionTimeout * 0.95))
)
session.setLastActivityTimeForTesting(Date())
session.setMessageCountForTesting(0)
#expect(session.needsRenegotiation())
// The session is still within its hard cutoff, so it must keep
// working right up until a rekey actually replaces it -- the fix is
// to schedule the rekey earlier, not to expire the session sooner.
let ciphertext = try session.encrypt(Data("still working".utf8))
#expect(try responder.decrypt(ciphertext) == Data("still working".utf8))
}
@Test("Rate limiter handles global message caps and per-peer resets")
func rateLimiterGlobalMessageCapAndReset() async throws {
let globalLimiter = NoiseRateLimiter()