feat(relay): close sessions when the token/cert expires#1789
Conversation
Auth was a one-time gate: the JWT `exp` was checked only in Key::decode at connect time, the relay's AuthToken dropped claims.expires, and the session was held open indefinitely. mTLS client certs were validated only at the TLS handshake. So a credential that expired mid-session kept working until the client disconnected. Carry the expiry through AuthToken (JWT `exp` via finalize, mTLS `notAfter` parsed from the peer certificate) and, in Connection::run, race session.closed() against a sleep until that deadline, closing with Unauthorized once it passes. - moq-relay: add AuthToken.expires, populate it in finalize() and on the mTLS path; close the session on expiry in connection.rs. - moq-native: peer_certificate_expiry() helper (x509-parser) exposed on the Quinn/noq requests and the Request wrapper; other backends return None. - Tests for exp carry-through and cert notAfter parsing; doc/bin/relay/auth.md notes the session-lifetime enforcement. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
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)
WalkthroughThe relay gains continuous credential-lifetime enforcement for both JWT and mTLS sessions. A new 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
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 |
Address review: replace has_peer_certificate() -> bool and peer_certificate_expiry() -> Option<SystemTime> with a single, more generic peer_identity() -> Option<PeerIdentity>. PeerIdentity owns the validated client cert chain (leaf first) and offers chain() and expiry() so callers can inspect it without re-parsing the type-erased QUIC identity. has_peer_certificate becomes peer_identity().is_some() at call sites. Also simplify Connection::run: use tokio::time::timeout instead of an async block plus tokio::select! to close on credential expiry. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rs/moq-native/src/tls.rs (1)
369-370: Document therustlssemver coupling forPeerIdentity::chain().
PeerIdentity::chain()exposesrustls::CertificateDerin the public signature. Sincerustlsis already re-exported in the public API (pub use rustlsin lib.rs), this crate acknowledges the coupling—but lacks documentation that a majorrustlsversion bump is a breaking change for moq-native consumers.Add a doc comment on
PeerIdentity::chain()or the struct itself clarifying this dependency relationship, or migrate to a crate-owned wrapper type to fully decouple the semver boundary.🤖 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 `@rs/moq-native/src/tls.rs` around lines 369 - 370, The chain() method on the PeerIdentity struct exposes the rustls::CertificateDer type in its public signature, but lacks documentation about the semver coupling this creates with the rustls crate. Add a doc comment to the chain() method (or to the PeerIdentity struct itself) that clearly documents that this function exposes the rustls::CertificateDer type and warns that major version bumps of rustls constitute breaking changes for moq-native consumers, since rustls is already part of the public API through pub use rustls in lib.rs.Source: Coding guidelines
🤖 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 `@rs/moq-native/src/server.rs`:
- Around line 621-623: The peer_identity() method in Request is gated behind a
feature flag `#[cfg(any(feature = "quinn", feature = "noq"))]` but the
authenticate() function in connection.rs calls it unconditionally without the
same guard. To fix this, either add the matching `#[cfg(any(feature = "quinn",
feature = "noq"))]` attribute to guard the authenticate() function in moq-relay,
or add a `#[cfg]` compile_error directive in moq-native to prevent compilation
when neither the quinn nor noq features are enabled, ensuring only supported
feature combinations can build.
---
Nitpick comments:
In `@rs/moq-native/src/tls.rs`:
- Around line 369-370: The chain() method on the PeerIdentity struct exposes the
rustls::CertificateDer type in its public signature, but lacks documentation
about the semver coupling this creates with the rustls crate. Add a doc comment
to the chain() method (or to the PeerIdentity struct itself) that clearly
documents that this function exposes the rustls::CertificateDer type and warns
that major version bumps of rustls constitute breaking changes for moq-native
consumers, since rustls is already part of the public API through pub use rustls
in lib.rs.
🪄 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: 7e697304-514b-406e-9190-00ecc879773c
📒 Files selected for processing (7)
rs/moq-native/src/noq.rsrs/moq-native/src/quinn.rsrs/moq-native/src/server.rsrs/moq-native/src/tls.rsrs/moq-native/tests/backend.rsrs/moq-relay/src/connection.rsrs/moq-relay/src/web.rs
✅ Files skipped from review due to trivial changes (1)
- rs/moq-relay/src/web.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rs/moq-relay/src/connection.rs
The previous refactor gated Request::peer_identity() behind the quinn/noq features and dropped the no-backend fallback arm that has_peer_certificate used to have. moq-relay calls peer_identity() unconditionally, so a backend-less build (e.g. `--no-default-features --features websocket`) failed to compile. Restore the always-available semantics: PeerIdentity and peer_identity() are no longer feature-gated (x509-parser becomes a normal dependency), and the `_ => None` fallback is back. Only PeerIdentity::from_any stays gated to quinn/noq since that is the only place it is constructed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
chain() returns rustls::pki_types::CertificateDer in its public signature. rustls is already re-exported (pub use rustls), so document that a major rustls bump is a breaking change for consumers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Authentication was a one-time gate. The JWT
expclaim was checked only inKey::decodeat connect time, the relay'sAuthTokenthen droppedclaims.expires, and the session was held open indefinitely via a baresession.closed().await?. mTLS client certificates were validated only during the TLS handshake and never re-checked. So a credential that expired mid-session kept working until the client disconnected.This enforces the credential's expiry for the whole session: the relay now closes the connection once the JWT
exp(or the mTLS certificate'snotAfter) passes, forcing the client to reconnect with a fresh credential.What changed
rs/moq-relay/src/auth.rs— addedexpires: Option<SystemTime>toAuthToken(the struct is#[non_exhaustive], so this is additive). Populated fromclaims.expiresinfinalize(), so both the JWT path and the auth-API path carry it.rs/moq-relay/src/connection.rs—Connection::runracessession.closed()againsttokio::time::sleepuntiltoken.expires(sleeps forever viapending()when there's no expiry), closing withError::Unauthorizedwhen the deadline passes. The mTLS branch setstoken.expiresfrom the peer certificate.rs/moq-native— newpeer_certificate_expiry()helper that downcasts the backend'speer_identity()toVec<CertificateDer>and parses the leaf'snotAfterwithx509-parser(already in the lockfile). Exposed onQuinnRequest,NoqRequest, and theRequestwrapper; other backends returnNone.doc/bin/relay/auth.md— notes thatexp(and mTLSnotAfter) is enforced for the session lifetime, not just at connect.Reviewer notes
AuthToken.expiresfield (on a#[non_exhaustive]struct); newpub fn peer_certificate_expiry()onmoq_native::Request,QuinnRequest,NoqRequest. New optional depx509-parser, gated behind thequinn/noqfeatures. Targetsmainsince there are no wire or breaking changes.SystemTimebuttokio::time::sleepruns on the monotonic clock, so a large wall-clock jump after connect isn't tracked. This matches howexpwas originally validated (SystemTime::now()). Exactness under clock skew would need a periodic re-check instead; happy to switch if preferred.Test plan
expsurvivesfinalize; certnotAfterparses correctly;None/wrong-type identity yieldsNonewithout panicking.cargo fmt --check,clippy(both crates,noqfeature on), and all lib tests pass via nix.(Written by Claude)