Skip to content

Allow configurable clock skew for 'iat' and 'nbf' checks - #227

Open
djw8605 wants to merge 3 commits into
masterfrom
claude/iat-clock-skew-issue-q0euo2
Open

djw8605 wants to merge 3 commits into
masterfrom
claude/iat-clock-skew-issue-q0euo2

Conversation

@djw8605

@djw8605 djw8605 commented Sep 27, 2026

Copy link
Copy Markdown
Contributor

Addresses #226.

Problem

Token time claims are checked by jwt-cpp's default verifier, which uses a leeway of 0. If the resource server's clock is even 1 second behind the issuer's, a freshly issued token fails the iat check (now < iat). Issuers such as INDIGO IAM backdate nbf by 60s or more to absorb skew, but the strict iat check defeats that. On top of this, jwt-cpp reports the failure as token_expired, which scitokens-cpp surfaces as a TokenExpiredException with "token expired". That is misleading for a token that looks like it was issued in the future.

Change

  • New int config key validation.clock_skew_s (env var SCITOKEN_CONFIG_VALIDATION_CLOCK_SKEW_S), default 60 seconds. Negative values are rejected.
  • In Validator::verify_async_continue, the default jwt-cpp iat and nbf checks are replaced via with_claim with checks that allow up to that skew. They keep the jwt-cpp behavior of skipping a claim that is absent, so profiles where nbf is optional are unaffected.
  • When a claim is too far in the future, the error now names it, for example: token verification failed: 'iat' claim is 100s in the future, more than the allowed clock skew of 60s. It no longer mentions expiry, so it is not turned into a TokenExpiredException.
  • exp is still checked strictly (no leeway), so skew tolerance never extends a token's lifetime.
  • Documented the key in scitokens.h next to the other int config keys.

Open questions for review

  • Default value. 60s matches the nbf backdating IAM uses. It changes behavior for existing deployments (previously 0). Set validation.clock_skew_s=0 to restore strict checking.
  • Key name. validation.clock_skew_s is new, since the existing prefixes (keycache., tls., monitoring.) don't fit. Happy to rename.
  • Related, not changed here. Validator::m_now is set once when the object is constructed and is not refreshed on each verify() unless validator_set_time/enforcer_set_time is called. Long-lived Validator/Enforcer objects therefore check claims against a stale clock, which could cause the same symptom. That seems worth a separate change.

Testing

  • Added SerializeTest.ClockSkew. It covers: accepted within the default skew; rejected beyond it, with an error that says "in the future" and not "expired"; accepted with a larger configured skew; rejected with skew 0; exp not extended by skew; negative value rejected.
  • SerializeTest.* passes locally (18/18), and so do the monitoring and env config tests. In the full unit suite, 44 tests pass. The 3 KeycacheTest refresh tests fail only because the sandbox could not reach demo.scitokens.org ("Failure when receiving data from the peer"), unrelated to this change.
  • Formatted with clang-format 18.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Pc7RS7J9BoUNRxvG6TZuzb


Generated by Claude Code

jwt-cpp's default claim checks allow no leeway, so a resource server
whose clock is even one second behind the issuer rejects a freshly
issued token because its 'iat' is in the future. Issuers commonly
backdate 'nbf' to absorb skew, but the strict 'iat' check defeated
that. The failure was also reported as "token expired", which is
misleading.

Replace the 'iat' and 'nbf' checks with ones that allow a configurable
skew (new int config key "validation.clock_skew_s", env var
SCITOKEN_CONFIG_VALIDATION_CLOCK_SKEW_S, default 60s) and report which
claim is in the future and by how much. 'exp' is still checked with no
leeway.

Refs #226

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pc7RS7J9BoUNRxvG6TZuzb

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The diagnostic rounds offsets inconsistently, and environment-loading coverage is missing.

Review effort: Balanced
Findings: 2 Low severity

Open (2)
What changed in this PR

Adds configurable clock-skew tolerance for future iat and nbf claims while retaining strict expiration checks.

Changes:

  • Adds the validation.clock_skew_s configuration and environment mapping.
  • Applies skew-aware validation with clearer errors.
  • Adds unit coverage and public API documentation.
File Description
src/​scitokens.cpp Implements configuration storage and environment loading.
src/​scitokens.h Documents the new integer setting.
src/​scitokens_internal.h Applies skew-aware claim validation.
test/​main.cpp Tests skew boundaries, expiration, and invalid configuration.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/scitokens.cpp Resolved
Comment thread src/scitokens_internal.h Resolved
Address review feedback:
- Round the reported offset up to whole seconds so a rejection at, say,
  60.5s is not reported as "60s in the future, more than ... 60s".
  Done manually since std::chrono::ceil is C++17.
- Cover SCITOKEN_CONFIG_VALIDATION_CLOCK_SKEW_S in the startup env
  config test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pc7RS7J9BoUNRxvG6TZuzb
Replace the manual round-up with std::ceil on a fractional-seconds
duration. std::chrono::ceil would be simpler but is C++17, and the
build does not require C++17.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pc7RS7J9BoUNRxvG6TZuzb

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

The implementation matches the stated behavior and includes focused coverage for configuration, validation boundaries, errors, and strict expiration.

Review effort: Balanced
Findings: None

Resolved since last review (2)

@djw8605
djw8605 marked this pull request as ready for review September 28, 2026 14:28
@djw8605 djw8605 mentioned this pull request Sep 28, 2026

This branch has not been deployed

No deployments
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.

3 participants