Skip to content

Conversation

@AnthonyRonning
Copy link
Contributor

@AnthonyRonning AnthonyRonning commented Nov 23, 2025

Backdates JWT issuance time (iat) by 60 seconds to prevent verification failures due to clock drift between servers. Ensures 'exp' and 'nbf' are calculated relative to the adjusted 'iat'.

Summary by CodeRabbit

  • Bug Fixes

    • Authentication token generation now compensates for clock drift so sign-ins are more reliable across systems with differing clocks.
  • Data

    • Appended new device attestation/history entries to both development and production PCR history logs.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Nov 23, 2025

Walkthrough

Backdated JWT issued-at values by 60 seconds in Apple client-secret and internal JWT creation to tolerate clock skew; appended new PCR entries to both pcrDevHistory.json and pcrProdHistory.json. No public API signatures were changed.

Changes

Cohort / File(s) Summary
Token clock-drift compensation
src/apple_signin.rs, src/jwt.rs
Set iat to now −60s (backdated by 60 seconds); align exp and nbf around the backdated iat in Apple client-secret generation and JWT claim construction. Internal behavior only; public signatures unchanged.
PCR history append
pcrDevHistory.json, pcrProdHistory.json
Appended a new PCR entry object (fields include PCR0, PCR1, PCR2, timestamp, signature) to each JSON history array, increasing array length by one.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Client
    participant TokenSvc as Token Service
    participant Apple as Apple Sign-in

    Client->>TokenSvc: request token
    alt creating Apple client-secret
        TokenSvc->>TokenSvc: now = Utc::now()
        Note over TokenSvc: issued_at = now - 60s\nexp/nbf aligned to issued_at
        TokenSvc->>Apple: sign JWT (header, claims, signature)
        Apple-->>TokenSvc: signed client-secret
    else creating internal JWT
        TokenSvc->>TokenSvc: now = Utc::now()
        Note over TokenSvc: issued_at = now - 60s\nexp/nbf aligned to issued_at
        TokenSvc->>TokenSvc: sign JWT
    end
    TokenSvc-->>Client: return token
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Pattern is repeated in two modules (homogeneous change).
  • Check correctness of backdate across all token-creation paths and any time-based tests.
  • Verify JSON append structure and signature field formats in the two PCR files.

Possibly related PRs

Poem

🐰
Sixty seconds I leap back in time,
So tokens align and clocks rhyme.
I hop through claims with gentle cheer,
New PCR notes tucked safely near.
Hooray — synchronized and clear! 🥕

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: account for clock drift in JWT issuance' accurately summarizes the main change across multiple files (apple_signin.rs, jwt.rs) which implement 60-second backdating of JWT issuance timestamps to prevent clock-drift verification failures.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/jwt-clock-drift

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d994651 and ec4f45c.

⛔ Files ignored due to path filters (2)
  • pcrDev.json is excluded by !pcrDev.json
  • pcrProd.json is excluded by !pcrProd.json
📒 Files selected for processing (4)
  • pcrDevHistory.json (1 hunks)
  • pcrProdHistory.json (1 hunks)
  • src/apple_signin.rs (1 hunks)
  • src/jwt.rs (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/apple_signin.rs
🔇 Additional comments (5)
pcrDevHistory.json (1)

450-456: PCR history entry appended successfully.

The new PCR history entry follows the established format and appears to be part of routine history tracking for the development environment.

pcrProdHistory.json (1)

450-456: PCR history entry appended successfully.

The new PCR history entry follows the established format and appears to be part of routine history tracking for the production environment.

src/jwt.rs (3)

13-13: LGTM! Import addition supports clock drift fix.

The Utc import is correctly added to support the clock-drift compensation logic implemented below.


295-303: Excellent clock drift compensation implementation!

The 60-second backdate for the iat claim is a well-established practice for handling clock skew between servers. The implementation correctly:

  • Calculates exp relative to the adjusted iat (preserving intended token lifetime)
  • Sets not_before to the adjusted iat for consistency
  • Applies the fix before token signing

This approach prevents legitimate tokens from being rejected due to minor clock differences while maintaining proper token lifetime.


365-373: Consistent clock drift fix applied for platform users!

The clock drift compensation is correctly and consistently applied in the platform user token creation path, matching the implementation in NewToken::new. This ensures all token types (user tokens and platform tokens) benefit from the same clock skew protection.


Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps
Copy link

greptile-apps bot commented Nov 23, 2025

Greptile Overview

Greptile Summary

This PR mitigates clock drift issues in JWT token generation by backdating the issued-at timestamp by 60 seconds. This prevents verification failures when the issuing server's clock is slightly ahead of the validating server.

Key changes:

  • Backdates token issuance time by 1 minute in user and platform token generation
  • Applies same 60-second backdate to Apple signin flow
  • Ensures expiration and not-before claims are calculated relative to the adjusted issuance time
  • Updates PCR measurements for dev and prod environments

The 60-second buffer provides reasonable tolerance for typical clock skew scenarios between distributed servers.

Confidence Score: 4/5

  • This PR is safe to merge with minimal risk once token lifetime validation is verified
  • The implementation is straightforward and addresses a real production issue. However, the code doesn't validate that tokens won't immediately fail nbf checks on systems with normal clocks, and there are no tests covering the clock drift scenarios. The score reflects solid implementation with room for validation improvements.
  • Pay attention to src/jwt.rs to ensure the validation logic properly handles backdated tokens

Important Files Changed

File Analysis

Filename Score Overview
src/jwt.rs 4/5 Clock drift mitigation implemented by backdating iat by 60 seconds in two token generation functions, with exp and nbf calculated relative to adjusted iat
src/apple_signin.rs 5/5 Backdates Apple client secret JWT iat by 60 seconds to match clock drift fix

Sequence Diagram

sequenceDiagram
    participant Client
    participant Server
    participant JWT as JWT Generation
    participant Validator as JWT Validation
    
    Note over Server,JWT: Token Generation with Clock Drift Fix
    Client->>Server: Request authentication
    Server->>JWT: Generate token (NewToken::new)
    JWT->>JWT: now = Utc::now()
    JWT->>JWT: iat = now - 60 seconds
    JWT->>JWT: exp = iat + duration
    JWT->>JWT: nbf = iat
    JWT->>Server: Return signed JWT
    Server->>Client: Return JWT token
    
    Note over Client,Validator: Token Validation
    Client->>Server: Request with JWT (Bearer token)
    Server->>Validator: Validate token
    Validator->>Validator: Check expiration (exp)
    Validator->>Validator: Check audience (aud)
    Validator->>Validator: Verify signature
    Validator->>Server: Return claims
    Server->>Client: Authorized response
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@AnthonyRonning
Copy link
Contributor Author

@claude what did TimeOptions::default() do and set? what is in that struct? could it have been better to create our own TimeOptions or did it not have the "iat - 1 minute" logic that we needed?

@claude
Copy link
Contributor

claude bot commented Nov 24, 2025

Claude finished @AnthonyRonning's task —— View job


Analysis of TimeOptions::default() and Implementation Choice

  • Read the current JWT implementation
  • Find the TimeOptions struct definition
  • Analyze whether TimeOptions could be customized for clock drift
  • Provide comprehensive answer

What TimeOptions::default() Does

TimeOptions is a struct from the jwt-compact crate that controls time-based validation behavior. The struct has two fields:

pub struct TimeOptions<F = fn() -> DateTime<Utc>> {
    pub leeway: Duration,
    pub clock_fn: F,
}

TimeOptions::default() creates:

  • leeway: 60 seconds (for clock skew tolerance during validation)
  • clock_fn: Utc::now() (uses the system clock)

The leeway field is used only during validation to tolerate clock differences. When validating a token, the validator checks:

  • For exp (expiration): allows the token to be valid even if current time is up to 60s past the expiration
  • For nbf (not before): allows the token to be valid even if current time is up to 60s before the nbf time

Why Creating a Custom TimeOptions Wouldn't Work

The current implementation manually sets the time claims during token creation (src/jwt.rs:295-303):

// Account for clock drift by setting issued_at 1 minute in the past
let now = Utc::now();
let iat = now - Duration::minutes(1);
let exp = iat + duration;

let mut claims = Claims::new(custom_claims);
claims.issued_at = Some(iat);
claims.expiration = Some(exp);
claims.not_before = Some(iat);

The problem: TimeOptions is only used during validation, not during token creation. The jwt-compact crate's Claims::new() and related creation methods don't accept TimeOptions as a parameter. They use the real current time (Utc::now()) directly when you call methods like set_duration() or set_duration_and_issuance().

Why the Manual Approach is Correct

The PR needed to backdate the iat timestamp to handle the following scenario:

  1. Server A (token issuer) clock shows 12:00:00
  2. Server B (token validator) clock shows 11:59:30 (30 seconds behind)
  3. Without backdating: Server A issues token with iat=12:00:00, nbf=12:00:00
  4. Server B validates immediately and sees nbf=12:00:00 but its clock says 11:59:30
  5. Even with 60s leeway, the validation would fail because Server B thinks "this token is not valid yet" (30s in the future)

By backdating iat by 60 seconds during creation, the token becomes immediately usable even when the validator's clock is behind the issuer's clock by up to 60 seconds.

Custom TimeOptions can't solve this because:

  • It only affects validation logic (checking if tokens are expired/not-yet-valid)
  • It doesn't affect token creation (setting the iat, exp, nbf values)
  • The jwt-compact library doesn't provide a way to pass custom time functions during token creation

Validation at src/jwt.rs:531

The code uses TimeOptions::default() during validation, which provides the standard 60-second leeway on the validation side. This works in conjunction with the backdated issuance time to provide protection against clock drift in both directions.


Sources:


AnthonyRonning and others added 2 commits November 24, 2025 10:46
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

6 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@AnthonyRonning AnthonyRonning merged commit 9bff2f3 into master Nov 24, 2025
7 checks passed
@AnthonyRonning AnthonyRonning deleted the fix/jwt-clock-drift branch November 24, 2025 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants