Fix: Gate TLS-bridge leaf cache on wall-clock validity (survive suspend)#622
Conversation
… TTL
The minter cached forged leaves on a TTL compared with time.Now(), which
carries Go's monotonic clock. Across a host suspend / VM pause the
monotonic clock freezes while wall time — and the leaf's x509 NotAfter —
keeps advancing, so the cache kept serving a leaf the client already
rejected as expired ("certificate has expired"). The bridge then read the
rejection as a forged-cert failure, auto-skipped the host, and fell back
to undecrypted passthrough — silently disabling interception (and any
inference/MCP parsing) for that host until the proxy was restarted.
- mint: populate tls.Certificate.Leaf so the cache can gate on the cert's
real NotAfter (and the TLS stack avoids re-parsing each handshake). Leaf
validity is now now+ttl+renewBefore.
- cache: store the renewal deadline monotonic-stripped (.Round(0)) so the
freshness check uses the wall clock, plus a backstop that never serves a
leaf whose real NotAfter has already passed.
- SkipSet: same .Round(0) treatment so auto-skip entries expire on real
time across a suspend instead of lingering.
Add a regression test that ages a cached leaf past its NotAfter while the
monotonic deadline still reads fresh, and asserts a re-mint.
Primarily a dev-environment robustness fix (laptop sleep / VM pause) —
production nodes do not suspend — but wall-clock gating is the correct
invariant regardless.
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
|
Warning Review limit reached
More reviews will be available in 34 minutes and 24 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe TLS bridge now stores wall-clock expiry times, rechecks cached certificates against parsed leaf ChangesTLS bridge wall-clock expiry
Sequence Diagram(s)sequenceDiagram
participant GetCertificateForHost
participant Minter
participant "x509.ParseCertificate"
participant "tls.Certificate"
GetCertificateForHost->>Minter: request certificate for host
Minter->>Minter: compare time.Now() with e.expires and Leaf.NotAfter
Minter->>x509.ParseCertificate: parse minted DER leaf
x509.ParseCertificate-->>Minter: parsed Leaf
Minter->>tls.Certificate: set Leaf before returning
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@authbridge/authlib/tlsbridge/minter.go`:
- Around line 121-124: The exported MinterOpts.LeafTTL documentation is now
inaccurate because minter.go’s leaf certificate validity uses LeafTTL plus
renewBefore rather than treating LeafTTL as both validity and cache TTL. Update
the LeafTTL comment on MinterOpts to describe the new model clearly, and make
sure the wording matches the behavior in the leaf minting logic around the
Minter and its ttl/renewBefore handling.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b15b300b-6056-4bd5-8666-571f7766efee
📒 Files selected for processing (3)
authbridge/authlib/tlsbridge/decision.goauthbridge/authlib/tlsbridge/minter.goauthbridge/authlib/tlsbridge/minter_test.go
…eadline
- gofmt: re-align the x509.Certificate{} literal. The prior edit de-aligned
NotBefore:/NotAfter: to one space; gofmt -l flagged minter.go (CI missed it
because go-fmt pre-commit only covers proxy-init/).
- test: add TestMinter_CacheDeadlineIsWallClock, asserting the cache freshness
deadline carries no monotonic reading (exp == exp.Round(0)) — directly
guarding the .Round(0) suspend fix, which the existing test did not exercise.
Clarify that TestMinter_ReMintsWallClockExpiredLeaf covers the NotAfter
backstop (a state normal operation cannot produce).
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
cwiklik
left a comment
There was a problem hiding this comment.
Correct diagnosis and minimal fix for the monotonic-vs-wall-clock cache-staleness bug. Verified the mechanism end-to-end:
.Round(0)on the cache deadline (expires) and the SkipSet entry strips the monotonic reading, sotime.Now().Before(...)falls back to the wall clock — Go's documented behavior when one operand lacks a monotonic reading — and the cache correctly expires across a suspend.TestMinter_CacheDeadlineIsWallClockguards this precisely viaexp != exp.Round(0).- The
Leaf.NotAfterbackstop gates on the value the client actually verifies;Leaf != nilfails safe (re-mint, never serve a leaf with no parsed cert). Sub-second freshness stays onexpiressince x509NotAfteris 1s-truncated — backstop, not primary. - Populating
tls.Certificate.Leafat mint also lets the TLS stack skip per-handshake re-parsing (parse-once).renewBefore(1h) equals the old inline value, so leaf validity always outlasts the cache deadline and re-mint precedes real expiry. No data race — the cert is effectively immutable post-mint and read underm.mu.
Two assertive regression tests cover both the wall-clock deadline and the NotAfter backstop. Nothing actionable found.
Areas reviewed: Go. Commits: 2, signed-off: yes. CI: all passing (Go CI ×3, CodeQL, CodeRabbit).
Problem
The TLS-bridge minter caches forged leaf certs on a TTL checked with
time.Now(), which carries Go's monotonic clock. Across a host suspend / VM pause the monotonic clock freezes while wall time — and the leaf's x509NotAfter— keeps advancing. So the cache kept serving a leaf the client already rejected ascertificate has expired.The downstream effect is severe and silent: the client's rejection looks like a forged-cert failure, so the bridge auto-skips the host and falls back to undecrypted passthrough. Interception (and all inference/MCP/A2A parsing) for that host silently stops until the proxy is restarted — even though the agent still works via the blind tunnel, so nothing looks broken.
This was hit in practice on a dev laptop that slept overnight: litellm's cached leaf expired, every inference call fell through to passthrough, and abctl showed no inference events despite the inference-parser being active.
Fix
Gate cache freshness on wall-clock deadlines and the cert's real validity, not a monotonic TTL:
tls.Certificate.Leafso the cache can read the cert's actualNotAfter(also lets the TLS stack skip re-parsing each handshake). Leaf validity isnow + ttl + renewBefore..Round(0)) so the freshness comparison falls back to the wall clock, plus a backstop that never serves a leaf whose realNotAfterhas already passed (the single value the client verifies)..Round(0)treatment so auto-skip entries expire on real time across a suspend instead of lingering.Tests
TestMinter_ReMintsWallClockExpiredLeafages a cached leaf past itsNotAfterwhile the monotonic deadline still reads fresh, and asserts a re-mint. Existing minter/skipset TTL tests still pass (sub-second precision preserved by keeping a wall-clockexpiresrather than relying on the 1s-truncated certNotAfter).Scope
Primarily a dev-environment robustness fix (laptop sleep / VM pause) — production nodes don't suspend — but wall-clock gating is the correct invariant regardless, and the failure mode (silent passthrough until restart) is bad enough to warrant the guard.
Fixes #623.
Part of rossoctl/rossoctl#2071.
Assisted-By: Claude (Anthropic AI) noreply@anthropic.com