feat(jans-cedarling): add CEDARLING_JWT_STATUS_LIST_REFRESH_INTERVAL_MAX bootstrap property#14106
Merged
Conversation
Member
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds CEDARLING_JWT_STATUS_LIST_REFRESH_INTERVAL_MAX and related runtime wiring: config schema, serde deserializer and defaults, status-list cache uses min(jwt.ttl, max) or max when ttl is absent, tests for parsing/clamping/refresh behavior, and documentation updates. ChangesStatus List Refresh Interval Max
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
mo-auto
added
area-documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/cedarling/reference/cedarling-jwt-validation.md`:
- Around line 201-206: Add a sentence to the "Status List Refresh Cadence"
paragraph explicitly stating the clamping rule: if the bootstrap env/property
CEDARLING_JWT_STATUS_LIST_REFRESH_INTERVAL_FALLBACK is set to a non-zero value
less than 5 seconds it will be clamped up to 5 seconds; mention this applies
only to the fallback value (when the Status List JWT's ttl is absent) and is
ignored when a ttl claim is present. Reference the terms "Status List JWT",
"ttl", and CEDARLING_JWT_STATUS_LIST_REFRESH_INTERVAL_FALLBACK so readers can
connect this note to the properties reference.
In `@jans-cedarling/cedarling/src/bootstrap_config/raw_config/config.rs`:
- Line 820: Add a descriptive failure message to the assert_eq! that compares
config.status_list_refresh_interval_fallback to 120 so test failures explain
intent; locate the assertion referencing
config.status_list_refresh_interval_fallback and update the assert_eq! call to
include a string describing what is being validated (e.g., that the fallback
refresh interval defaults to 120 seconds) so the message appears on test
failure.
In `@jans-cedarling/cedarling/src/jwt/status_list/cache.rs`:
- Around line 68-77: The scheduler currently uses
InitForIssArgs.refresh_interval_fallback directly, which allows zero or
too-small values to create a tight refresh loop; normalize
refresh_interval_fallback to a safe minimum before any scheduling by computing a
sanitized interval (e.g., let effective_refresh = if refresh_interval_fallback <
MIN_REFRESH_INTERVAL { MIN_REFRESH_INTERVAL } else { refresh_interval_fallback
}) and use effective_refresh everywhere the refresh loop/scheduler is started
(locations where refresh_interval_fallback is currently read in the refresh
loop/scheduler). Ensure the normalization is done once after destructuring
InitForIssArgs so all subsequent uses (the refresh loop and any retry/schedule
calls) reference the normalized value.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: e71ff336-3d63-428b-94b8-935addb3af49
📒 Files selected for processing (10)
docs/cedarling/reference/cedarling-jwt-validation.mddocs/cedarling/reference/cedarling-properties.mdjans-cedarling/cedarling/src/bootstrap_config/decode.rsjans-cedarling/cedarling/src/bootstrap_config/jwt_config.rsjans-cedarling/cedarling/src/bootstrap_config/raw_config/config.rsjans-cedarling/cedarling/src/bootstrap_config/raw_config/default_values.rsjans-cedarling/cedarling/src/bootstrap_config/raw_config/json_util.rsjans-cedarling/cedarling/src/jwt/status_list/cache.rsjans-cedarling/cedarling/src/jwt/test_utils.rsjans-cedarling/cedarling/src/jwt/trusted_issuers_loader.rs
…ck interval Previously, the Status List JWT was refreshed only when its payload carried a `ttl` claim. If the issuer omitted `ttl`, the list was fetched once at startup and never refreshed, so revoked tokens stayed valid in the cache for the lifetime of the process. The in-loop fallback was also a hardcoded `600s`. Replace that magic number with a new bootstrap property `CEDARLING_JWT_STATUS_LIST_REFRESH_INTERVAL_FALLBACK` (default 300 s). - 0 resolves to the default; non-zero values below 5 s are clamped - Background task always spawns, regardless of whether `ttl` is present - Refactor `StatusListCache::init_for_iss` to take an args struct Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Add `CEDARLING_JWT_STATUS_LIST_REFRESH_INTERVAL_FALLBACK` to the bootstrap properties reference and a new "Status List Refresh Cadence" section in the JWT validation reference explaining the JWT-`ttl` vs. bootstrap-fallback precedence. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Spell out that non-zero CEDARLING_JWT_STATUS_LIST_REFRESH_INTERVAL_FALLBACK values below 5 seconds are clamped up to 5 seconds, and that the clamp only applies to the fallback path (Status List JWT without a `ttl` claim). Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
… test The `from_env` assertion was bare while the sibling assertions in the same module carry descriptive messages. Add one here so a regression surfaces the intent of the check rather than just `120 != actual`. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
… call site The bootstrap deserializer already normalizes CEDARLING_JWT_STATUS_LIST_REFRESH_INTERVAL_FALLBACK, but `JwtConfig` is a public struct that callers can construct programmatically and bypass that normalization, which could feed a `0` or sub-floor value into the refresh loop and create a tight scheduling cycle. Clamp the value to MIN_STATUS_LIST_REFRESH_SECS where the loader hands it to `StatusListCache::init_for_iss`, so the production path is always safe while internal callers (tests) can still pass small values directly to the cache for fast verification. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
olehbozhok
force-pushed
the
jans-cedarling-14105
branch
from
10a17c2 to
efa43de
Compare
Contributor
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 `@jans-cedarling/cedarling/src/jwt/trusted_issuers_loader.rs`:
- Around line 242-245: The current normalization for refresh_interval_fallback
turns an explicit 0 into MIN_STATUS_LIST_REFRESH_SECS by using .max(...); change
this so you check loader.jwt_config.status_list_refresh_interval_fallback first
and if it is exactly 0 you set refresh_interval_fallback to the documented
default (300s), otherwise apply .max(MIN_STATUS_LIST_REFRESH_SECS) to clamp
nonzero values — update the assignment of refresh_interval_fallback to perform
this explicit 0 branch before calling .max.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 43360695-1fb5-403c-80b9-9e1b41b3fc98
📒 Files selected for processing (10)
docs/cedarling/reference/cedarling-jwt-validation.mddocs/cedarling/reference/cedarling-properties.mdjans-cedarling/cedarling/src/bootstrap_config/decode.rsjans-cedarling/cedarling/src/bootstrap_config/jwt_config.rsjans-cedarling/cedarling/src/bootstrap_config/raw_config/config.rsjans-cedarling/cedarling/src/bootstrap_config/raw_config/default_values.rsjans-cedarling/cedarling/src/bootstrap_config/raw_config/json_util.rsjans-cedarling/cedarling/src/jwt/status_list/cache.rsjans-cedarling/cedarling/src/jwt/test_utils.rsjans-cedarling/cedarling/src/jwt/trusted_issuers_loader.rs
The "0 -> default, sub-floor -> floor" rule for CEDARLING_JWT_STATUS_LIST_REFRESH_INTERVAL_FALLBACK lived in two places: the bootstrap deserializer and the trusted-issuer-loader call site. They agreed today, but were easy to drift apart on the next change. Move the rule into a single `normalize_status_list_refresh_interval_fallback` helper and expose a `JwtConfig::normalize` method that applies all such invariants in place. `JwtService::new` now calls `normalize` once on the incoming config so programmatic callers cannot bypass the bootstrap deserializer's normalization. Downstream call sites (loader, cache) simply trust the already-normalized value. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
dagregi
previously approved these changes
May 19, 2026
haileyesus2433
previously approved these changes
May 19, 2026
Rename CEDARLING_JWT_STATUS_LIST_REFRESH_INTERVAL_FALLBACK to CEDARLING_JWT_STATUS_LIST_REFRESH_INTERVAL_MAX and flip its semantics from "fallback when JWT has no ttl" to "upper bound on the refresh interval". When the Status List JWT carries a ttl, the effective refresh interval is now min(jwt_ttl, MAX) so issuers can request more frequent refreshes but never a less frequent one. When ttl is absent, the MAX value is used directly. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Reflect the rename from CEDARLING_JWT_STATUS_LIST_REFRESH_INTERVAL_FALLBACK to CEDARLING_JWT_STATUS_LIST_REFRESH_INTERVAL_MAX and the new capping semantics: the effective refresh interval is min(jwt_ttl, MAX). Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
olehbozhok
dismissed stale reviews from haileyesus2433, dagregi, and coderabbitai[bot]
via
bf07ff9
olehbozhok
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
jans-cedarling/cedarling/src/jwt/status_list/cache.rs (1)
49-56:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize
refresh_interval_maxinside this cache boundary.Line 125 and Line 228 still schedule with the raw
InitForIssArgs.refresh_interval_max. That means any crate-internal caller can pass0or<5and drive a zero/too-tight refresh loop, even though the documented contract says0 -> 300and non-zero<5 -> 5. The new test at Line 454 is already relying on an impossible production value, so this sink is still missing its own guardrail.Also applies to: 71-78, 123-125, 166-176, 224-228, 448-455
🤖 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 `@jans-cedarling/cedarling/src/jwt/status_list/cache.rs` around lines 49 - 56, The cache is using the raw InitForIssArgs.refresh_interval_max when scheduling refresh loops, allowing callers to pass 0 or <5 and force invalid intervals; normalize this inside the cache boundary by computing let refresh_interval_max = if args.refresh_interval_max == 0 { 300 } else { args.refresh_interval_max.max(5) } (or equivalent) at the start of JwtStatusListCache initialization and use that normalized value for any scheduling/spawn calls (references: InitForIssArgs::refresh_interval_max, JwtStatusListCache::init_for_issuer and any functions that currently read args.refresh_interval_max to schedule loops); update the affected call sites to use this local normalized variable and adjust the test that passes an impossible production value to use the normalized semantics.
🤖 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 `@jans-cedarling/cedarling/src/jwt/status_list/cache.rs`:
- Around line 396-483: Add a new async test (e.g.,
refresh_capped_when_jwt_ttl_exceeds_max) modeled on
refresh_max_used_when_jwt_has_no_ttl that registers an issuer via
JwtValidatorCache::init_for_iss and StatusListCache::init_for_iss but has the
mock server return a status list JWT containing a ttl greater than the provided
refresh_interval_max (use MockServer::generate_status_list_endpoint_with_ttl or
equivalent to set ttl large, e.g., 10s, and pass refresh_interval_max = 1 to
init_for_iss). After updating the server-side list to a different bit pattern,
sleep for a duration slightly greater than refresh_interval_max but less than
the JWT ttl (e.g., 3s) and then assert the StatusListCache.status_lists entry
for mock_server.status_list_endpoint() was refreshed to the new list, proving
the loop used min(ttl, refresh_interval_max) and honored the cap. Ensure you
reference the same types used in the existing test (TrustedIssuer, IssuerConfig,
JwtConfig, StatusList) and reuse the HttpClient/key_service setup to keep the
test consistent.
---
Duplicate comments:
In `@jans-cedarling/cedarling/src/jwt/status_list/cache.rs`:
- Around line 49-56: The cache is using the raw
InitForIssArgs.refresh_interval_max when scheduling refresh loops, allowing
callers to pass 0 or <5 and force invalid intervals; normalize this inside the
cache boundary by computing let refresh_interval_max = if
args.refresh_interval_max == 0 { 300 } else { args.refresh_interval_max.max(5) }
(or equivalent) at the start of JwtStatusListCache initialization and use that
normalized value for any scheduling/spawn calls (references:
InitForIssArgs::refresh_interval_max, JwtStatusListCache::init_for_issuer and
any functions that currently read args.refresh_interval_max to schedule loops);
update the affected call sites to use this local normalized variable and adjust
the test that passes an impossible production value to use the normalized
semantics.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 8c0ded87-224a-429d-b441-5e0f13d0913c
📒 Files selected for processing (3)
docs/cedarling/reference/cedarling-properties.mdjans-cedarling/cedarling/src/jwt/mod.rsjans-cedarling/cedarling/src/jwt/status_list/cache.rs
Internal StatusListUpdateCtx and InitForIssArgs now carry the refresh interval and effective sleep as Duration instead of raw u64 seconds. A helper effective_refresh_interval centralizes the min(jwt_ttl, refresh_interval_max) cap, with refresh_interval_max used directly when the JWT omits ttl. Status list cache tests use sub-second Duration caps so the refresh loop drives ticks at 200ms instead of 1s, cutting cache suite from ~3.0s to ~0.7s. Adds refresh_capped_when_jwt_ttl_exceeds_max covering the cap path where JWT ttl exceeds the configured max. The bootstrap config field remains seconds; the loader converts at the call site, so user-facing configuration is unchanged. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
dagregi
previously approved these changes
May 20, 2026
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Contributor
Author
|
@coderabbitai approve |
Contributor
✅ Actions performedComments resolved and changes approved. |
dagregi
approved these changes
May 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prepare
Description
Target issue
closes #14105
Implementation Details
Problem. Cedarling refreshes the Status List JWT only when the JWT carries a
ttlclaim. Two gaps:ttl, the list is fetched once at startup and never refreshed, so revoked tokens remain valid in the cache for the lifetime of the process.ttl(e.g.86400), Cedarling silently honors it and revocation propagation can lag a full day with no operator override.There was no bootstrap property to guarantee or cap the refresh cadence.
Approach. Added a single bootstrap property
CEDARLING_JWT_STATUS_LIST_REFRESH_INTERVAL_MAXthat acts as an upper bound on the refresh interval. The Status List JWT'sttlis still honored when present (per the IETFoauth-status-listspec) — the issuer can always request a more frequent refresh — but the bootstrap value caps how infrequent the refresh can be. When the JWT omitsttl, the max value is used directly, closing the "nottl→ no refresh" gap.Semantics.
ttl→ effective interval ismin(jwt_ttl, INTERVAL_MAX). Issuer can refresh faster, never slower.ttl→ useCEDARLING_JWT_STATUS_LIST_REFRESH_INTERVAL_MAXdirectly.0or unset → resolves to the built-in default (300seconds). There is intentionally no "disable" mode, so the cache cannot silently go stale forever.5are clamped to5to protect the IDP from misconfiguration.Key changes.
cedarling/src/bootstrap_config/jwt_config.rs— newstatus_list_refresh_interval_max: u64field onJwtConfig, withJwtConfig::DEFAULT_STATUS_LIST_REFRESH_INTERVAL_MAX_SECS = 300as an associated constant andMIN_STATUS_LIST_REFRESH_SECS = 5as the floor. Shared normalizernormalize_status_list_refresh_interval_maxis invoked by both the bootstrap deserializer andJwtConfig::normalizeso programmatic callers cannot bypass clamping.cedarling/src/bootstrap_config/raw_config/{config,default_values,json_util}.rs— env-var binding forCEDARLING_JWT_STATUS_LIST_REFRESH_INTERVAL_MAX. The deserializer mapsNone/0to the default and clamps non-zero values below the minimum.cedarling/src/bootstrap_config/decode.rs— propagates the value from raw config intoJwtConfig.cedarling/src/jwt/status_list/cache.rs—StatusListCache::init_for_issnow takes the max interval viaInitForIssArgs. The background refresh task is always spawned:ttl = jwt.ttl.map_or(MAX, |t| t.min(MAX)). The samemin(jwt_ttl, MAX)rule is applied inside the refresh loop, replacing the hardcoded600sfallback, so init and in-loop paths behave consistently.cedarling/src/jwt/trusted_issuers_loader.rs— passesloader.jwt_config.status_list_refresh_interval_maxintoinit_for_iss.Docs. Updated
docs/cedarling/reference/cedarling-properties.md(property reference) anddocs/cedarling/reference/cedarling-jwt-validation.md(new "Status List Refresh Cadence" section explaining themin(jwt_ttl, INTERVAL_MAX)precedence).Backward compatibility. The new field has a sane default (
300s), so:ttl(and therefore never refreshed) automatically start refreshing every 5 minutes after upgrade.ttl ≤ 300sare unaffected.ttl > 300swill now refresh every 300s instead of the issuer-requested interval. Operators who relied on a longer issuer-driven interval can raiseCEDARLING_JWT_STATUS_LIST_REFRESH_INTERVAL_MAXto restore the previous cadence.Test and Document the changes
Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with
docs:to indicate documentation changes or if the below checklist is not selected.Summary by CodeRabbit
New Features
Behavior
Documentation
Tests