From 853916d4ea85c8c7aaaae20d2f52b83f30798ec3 Mon Sep 17 00:00:00 2001 From: sea-snake Date: Tue, 21 Oct 2025 11:15:07 +0200 Subject: [PATCH 1/6] Fix fetch certs interval, earlier retries were only done once on the first http outcall error instead of any failed future outcalls. --- src/internet_identity/src/openid/generic.rs | 37 ++++++++++++--------- src/internet_identity/src/openid/google.rs | 37 ++++++++++++--------- 2 files changed, 44 insertions(+), 30 deletions(-) diff --git a/src/internet_identity/src/openid/generic.rs b/src/internet_identity/src/openid/generic.rs index b372bb8f1c..b276ec5af4 100644 --- a/src/internet_identity/src/openid/generic.rs +++ b/src/internet_identity/src/openid/generic.rs @@ -26,6 +26,7 @@ use sha2::{Digest, Sha256}; #[cfg(test)] use std::cell::Cell; use std::cell::RefCell; +use std::cmp::max; use std::collections::HashMap; use std::convert::Into; use std::rc::Rc; @@ -167,7 +168,7 @@ impl Provider { let certs: Rc>> = Rc::new(RefCell::new(vec![])); #[cfg(not(test))] - schedule_fetch_certs(config.jwks_uri, Rc::clone(&certs), None); + schedule_fetch_certs(config.jwks_uri, Rc::clone(&certs), Some(0)); Provider { client_id: config.client_id, @@ -188,20 +189,26 @@ fn schedule_fetch_certs( use std::cmp::min; use std::time::Duration; - set_timer(Duration::from_secs(delay.unwrap_or(0)), move || { - spawn(async move { - let new_delay = match fetch_certs(jwks_uri.clone()).await { - Ok(certs) => { - certs_reference.replace(certs); - FETCH_CERTS_INTERVAL - } - // Try again earlier with backoff if fetch failed, the HTTP outcall responses - // aren't the same across nodes when we fetch at the moment of key rotation. - Err(_) => min(FETCH_CERTS_INTERVAL, delay.unwrap_or(60) * 2), - }; - schedule_fetch_certs(jwks_uri, certs_reference, Some(new_delay)); - }); - }); + set_timer( + Duration::from_secs(delay.unwrap_or(FETCH_CERTS_INTERVAL)), + move || { + spawn(async move { + let new_delay = match fetch_certs(jwks_uri.clone()).await { + Ok(certs) => { + certs_reference.replace(certs); + // Reset delay to None so default `FETCH_CERTS_INTERVAL` delay is used. + None + } + // Try again earlier with backoff if fetch failed, the HTTP outcall responses + // aren't the same across nodes when we fetch at the moment of key rotation. + // + // The delay should be at most `FETCH_CERTS_INTERVAL` and at minimum 60 * 2. + Err(_) => Some(min(FETCH_CERTS_INTERVAL, max(60, delay.unwrap_or(60)) * 2)), + }; + schedule_fetch_certs(jwks_uri, certs_reference, new_delay); + }); + }, + ); } #[cfg(not(test))] diff --git a/src/internet_identity/src/openid/google.rs b/src/internet_identity/src/openid/google.rs index ed2454d8bc..707b30b161 100644 --- a/src/internet_identity/src/openid/google.rs +++ b/src/internet_identity/src/openid/google.rs @@ -22,6 +22,7 @@ use sha2::{Digest, Sha256}; #[cfg(test)] use std::cell::Cell; use std::cell::RefCell; +use std::cmp::max; use std::collections::HashMap; use std::convert::Into; use std::rc::Rc; @@ -158,7 +159,7 @@ impl Provider { let certs: Rc>> = Rc::new(RefCell::new(vec![])); #[cfg(not(test))] - schedule_fetch_certs(Rc::clone(&certs), None); + schedule_fetch_certs(Rc::clone(&certs), Some(0)); Provider { client_id: config.client_id, @@ -174,20 +175,26 @@ fn schedule_fetch_certs(certs_reference: Rc>>, delay: Option { - certs_reference.replace(google_certs); - FETCH_CERTS_INTERVAL - } - // Try again earlier with backoff if fetch failed, the HTTP outcall responses - // aren't the same across nodes when we fetch at the moment of key rotation. - Err(_) => min(FETCH_CERTS_INTERVAL, delay.unwrap_or(60) * 2), - }; - schedule_fetch_certs(certs_reference, Some(new_delay)); - }); - }); + set_timer( + Duration::from_secs(delay.unwrap_or(FETCH_CERTS_INTERVAL)), + move || { + spawn(async move { + let new_delay = match fetch_certs().await { + Ok(google_certs) => { + certs_reference.replace(google_certs); + // Reset delay to None so default `FETCH_CERTS_INTERVAL` delay is used. + None + } + // Try again earlier with backoff if fetch failed, the HTTP outcall responses + // aren't the same across nodes when we fetch at the moment of key rotation. + // + // The delay should be at most `FETCH_CERTS_INTERVAL` and at minimum 60 * 2. + Err(_) => Some(min(FETCH_CERTS_INTERVAL, max(60, delay.unwrap_or(60)) * 2)), + }; + schedule_fetch_certs(certs_reference, new_delay); + }); + }, + ); } #[cfg(not(test))] From 2498b9646766ca273f8c3031c606beb3c45d1526 Mon Sep 17 00:00:00 2001 From: sea-snake Date: Tue, 21 Oct 2025 11:19:20 +0200 Subject: [PATCH 2/6] Fix fetch certs interval, earlier retries were only done once on the first http outcall error instead of any failed future outcalls. --- src/internet_identity/src/openid/generic.rs | 3 +-- src/internet_identity/src/openid/google.rs | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/internet_identity/src/openid/generic.rs b/src/internet_identity/src/openid/generic.rs index b276ec5af4..3c9e3a5af1 100644 --- a/src/internet_identity/src/openid/generic.rs +++ b/src/internet_identity/src/openid/generic.rs @@ -26,7 +26,6 @@ use sha2::{Digest, Sha256}; #[cfg(test)] use std::cell::Cell; use std::cell::RefCell; -use std::cmp::max; use std::collections::HashMap; use std::convert::Into; use std::rc::Rc; @@ -186,7 +185,7 @@ fn schedule_fetch_certs( ) { use ic_cdk::spawn; use ic_cdk_timers::set_timer; - use std::cmp::min; + use std::cmp::{max, min}; use std::time::Duration; set_timer( diff --git a/src/internet_identity/src/openid/google.rs b/src/internet_identity/src/openid/google.rs index 707b30b161..5f34f2dffe 100644 --- a/src/internet_identity/src/openid/google.rs +++ b/src/internet_identity/src/openid/google.rs @@ -22,7 +22,6 @@ use sha2::{Digest, Sha256}; #[cfg(test)] use std::cell::Cell; use std::cell::RefCell; -use std::cmp::max; use std::collections::HashMap; use std::convert::Into; use std::rc::Rc; @@ -172,7 +171,7 @@ impl Provider { fn schedule_fetch_certs(certs_reference: Rc>>, delay: Option) { use ic_cdk::spawn; use ic_cdk_timers::set_timer; - use std::cmp::min; + use std::cmp::{max, min}; use std::time::Duration; set_timer( From 736c18c27bcad2830b2161378f2acdd02aa7dc60 Mon Sep 17 00:00:00 2001 From: sea-snake Date: Tue, 28 Oct 2025 11:47:50 +0100 Subject: [PATCH 3/6] Split delay computation into its own method and add test. --- src/internet_identity/src/openid/generic.rs | 67 ++++++++++++++++----- 1 file changed, 52 insertions(+), 15 deletions(-) diff --git a/src/internet_identity/src/openid/generic.rs b/src/internet_identity/src/openid/generic.rs index 3c9e3a5af1..60d9d16ccc 100644 --- a/src/internet_identity/src/openid/generic.rs +++ b/src/internet_identity/src/openid/generic.rs @@ -38,7 +38,6 @@ const HTTP_STATUS_OK: u8 = 200; // Fetch the certs every fifteen minutes, the responses are always // valid for a couple of hours so that should be enough margin. -#[cfg(not(test))] const FETCH_CERTS_INTERVAL: u64 = 60 * 15; // 15 minutes in seconds // A JWT is only valid for a very small window, even if the JWT itself says it's valid for longer, @@ -177,6 +176,29 @@ impl Provider { } } +fn compute_next_certs_fetch_delay( + result: &Result, + current_delay: Option, +) -> Option { + use std::cmp::{max, min}; + + const MAX_DELAY: u64 = FETCH_CERTS_INTERVAL; + const MIN_DELAY: u64 = 60; + + match result { + // Reset delay to None so default (`FETCH_CERTS_INTERVAL`) delay is used. + Ok(_) => None, + // Try again earlier with backoff if fetch failed, the HTTP outcall responses + // aren't the same across nodes when we fetch at the moment of key rotation. + // + // The delay should be at most `MAX_DELAY` and at minimum `MIN_DELAY` * 2. + Err(_) => Some(min( + MAX_DELAY, + max(MIN_DELAY, current_delay.unwrap_or(MIN_DELAY)) * 2, + )), + } +} + #[cfg(not(test))] fn schedule_fetch_certs( jwks_uri: String, @@ -185,26 +207,18 @@ fn schedule_fetch_certs( ) { use ic_cdk::spawn; use ic_cdk_timers::set_timer; - use std::cmp::{max, min}; use std::time::Duration; set_timer( Duration::from_secs(delay.unwrap_or(FETCH_CERTS_INTERVAL)), move || { spawn(async move { - let new_delay = match fetch_certs(jwks_uri.clone()).await { - Ok(certs) => { - certs_reference.replace(certs); - // Reset delay to None so default `FETCH_CERTS_INTERVAL` delay is used. - None - } - // Try again earlier with backoff if fetch failed, the HTTP outcall responses - // aren't the same across nodes when we fetch at the moment of key rotation. - // - // The delay should be at most `FETCH_CERTS_INTERVAL` and at minimum 60 * 2. - Err(_) => Some(min(FETCH_CERTS_INTERVAL, max(60, delay.unwrap_or(60)) * 2)), - }; - schedule_fetch_certs(jwks_uri, certs_reference, new_delay); + let result = fetch_certs(jwks_uri.clone()).await; + let next_delay = compute_next_certs_fetch_delay(&result, delay); + if let Ok(certs) = result { + certs_reference.replace(certs); + } + schedule_fetch_certs(jwks_uri, certs_reference, next_delay); }); }, ); @@ -647,3 +661,26 @@ fn should_return_error_when_name_too_long() { )) ); } +#[test] +fn should_compute_next_certs_fetch_delay() { + let success: Result<(), ()> = Ok(()); + let error: Result<(), ()> = Err(()); + + for (current_delay, expected_next_delay_on_error) in [ + (None, Some(120)), + (Some(0), Some(120)), + (Some(120), Some(240)), + (Some(240), Some(480)), + (Some(480), Some(FETCH_CERTS_INTERVAL)), + (Some(FETCH_CERTS_INTERVAL * 2), Some(FETCH_CERTS_INTERVAL)), + ] { + assert_eq!( + compute_next_certs_fetch_delay(&success, current_delay), + None + ); + assert_eq!( + compute_next_certs_fetch_delay(&error, current_delay), + expected_next_delay_on_error + ); + } +} From e3170449a62ce0420d7a7a4367a1589b22140cd0 Mon Sep 17 00:00:00 2001 From: sea-snake Date: Tue, 28 Oct 2025 12:05:43 +0100 Subject: [PATCH 4/6] Split delay computation into its own method and add test. --- src/internet_identity/src/openid/generic.rs | 36 ++++++++++++++------- 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/src/internet_identity/src/openid/generic.rs b/src/internet_identity/src/openid/generic.rs index 60d9d16ccc..d0ba1f2d82 100644 --- a/src/internet_identity/src/openid/generic.rs +++ b/src/internet_identity/src/openid/generic.rs @@ -180,10 +180,9 @@ fn compute_next_certs_fetch_delay( result: &Result, current_delay: Option, ) -> Option { - use std::cmp::{max, min}; - - const MAX_DELAY: u64 = FETCH_CERTS_INTERVAL; const MIN_DELAY: u64 = 60; + const MAX_DELAY: u64 = FETCH_CERTS_INTERVAL; + const MULTIPLIER: u64 = 2; match result { // Reset delay to None so default (`FETCH_CERTS_INTERVAL`) delay is used. @@ -191,11 +190,12 @@ fn compute_next_certs_fetch_delay( // Try again earlier with backoff if fetch failed, the HTTP outcall responses // aren't the same across nodes when we fetch at the moment of key rotation. // - // The delay should be at most `MAX_DELAY` and at minimum `MIN_DELAY` * 2. - Err(_) => Some(min( - MAX_DELAY, - max(MIN_DELAY, current_delay.unwrap_or(MIN_DELAY)) * 2, - )), + // The delay should be at most `MAX_DELAY` and at minimum `MIN_DELAY`. + Err(_) => Some( + current_delay + .map_or(MIN_DELAY, |d| d * MULTIPLIER) + .clamp(MIN_DELAY, MAX_DELAY), + ), } } @@ -663,16 +663,28 @@ fn should_return_error_when_name_too_long() { } #[test] fn should_compute_next_certs_fetch_delay() { + const MIN_DELAY: u64 = 60; + const MAX_DELAY: u64 = FETCH_CERTS_INTERVAL; + let success: Result<(), ()> = Ok(()); let error: Result<(), ()> = Err(()); for (current_delay, expected_next_delay_on_error) in [ - (None, Some(120)), - (Some(0), Some(120)), + // Should be at least `MIN_DELAY` (1 minute) + (None, Some(MIN_DELAY)), + (Some(0), Some(MIN_DELAY)), + (Some(1), Some(MIN_DELAY)), + (Some(MIN_DELAY / 2 - 1), Some(MIN_DELAY)), + // Should be multiplied by two + (Some(MIN_DELAY / 2), Some(MIN_DELAY)), + (Some(MIN_DELAY / 2 + 1), Some(MIN_DELAY + 2)), + (Some(120), Some(240)), (Some(120), Some(240)), (Some(240), Some(480)), - (Some(480), Some(FETCH_CERTS_INTERVAL)), - (Some(FETCH_CERTS_INTERVAL * 2), Some(FETCH_CERTS_INTERVAL)), + // Should be at most `MAX_DELAY` (15 minutes) + (Some(480), Some(MAX_DELAY)), + (Some(MAX_DELAY / 2 + 1), Some(MAX_DELAY)), + (Some(MAX_DELAY * 2), Some(MAX_DELAY)), ] { assert_eq!( compute_next_certs_fetch_delay(&success, current_delay), From c0f2f791e5038234b549a9e9bc19d324c7fd7592 Mon Sep 17 00:00:00 2001 From: sea-snake Date: Tue, 28 Oct 2025 12:07:22 +0100 Subject: [PATCH 5/6] Split delay computation into its own method and add test. --- src/internet_identity/src/openid/generic.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/internet_identity/src/openid/generic.rs b/src/internet_identity/src/openid/generic.rs index d0ba1f2d82..e5876d947c 100644 --- a/src/internet_identity/src/openid/generic.rs +++ b/src/internet_identity/src/openid/generic.rs @@ -686,10 +686,12 @@ fn should_compute_next_certs_fetch_delay() { (Some(MAX_DELAY / 2 + 1), Some(MAX_DELAY)), (Some(MAX_DELAY * 2), Some(MAX_DELAY)), ] { + // Should return `None` on success so default (`FETCH_CERTS_INTERVAL`) delay is used. assert_eq!( compute_next_certs_fetch_delay(&success, current_delay), None ); + // Should return `expected_next_delay_on_error` on error as specified above. assert_eq!( compute_next_certs_fetch_delay(&error, current_delay), expected_next_delay_on_error From 2003d5cadd53bc7242086c1c1e9ea159236116d2 Mon Sep 17 00:00:00 2001 From: sea-snake Date: Tue, 28 Oct 2025 12:18:18 +0100 Subject: [PATCH 6/6] Split delay computation into its own method and add test. --- src/internet_identity/src/openid/generic.rs | 43 ++++++++++----------- 1 file changed, 21 insertions(+), 22 deletions(-) diff --git a/src/internet_identity/src/openid/generic.rs b/src/internet_identity/src/openid/generic.rs index e5876d947c..63a6635bc1 100644 --- a/src/internet_identity/src/openid/generic.rs +++ b/src/internet_identity/src/openid/generic.rs @@ -4,7 +4,6 @@ use super::{ }; use crate::openid::OpenIdCredential; use crate::openid::OpenIdProvider; -use crate::openid::MINUTE_NS; use crate::secs_to_nanos; use base64::prelude::BASE64_URL_SAFE_NO_PAD; use base64::Engine; @@ -38,13 +37,13 @@ const HTTP_STATUS_OK: u8 = 200; // Fetch the certs every fifteen minutes, the responses are always // valid for a couple of hours so that should be enough margin. -const FETCH_CERTS_INTERVAL: u64 = 60 * 15; // 15 minutes in seconds +const FETCH_CERTS_INTERVAL_SECONDS: u64 = 60 * 15; // 15 minutes in seconds // A JWT is only valid for a very small window, even if the JWT itself says it's valid for longer, // we only need it right after it's being issued to create a JWT delegation with its own expiry. // As the JWT is also used for registration, which may include longer user interaction, // we are using 10 minutes to account for potential clock offsets as well as users. -const MAX_VALIDITY_WINDOW: u64 = 10 * MINUTE_NS; // Same as ingress expiry +const MAX_VALIDITY_WINDOW_SECONDS: u64 = 10 * 60; // Same as ingress expiry // Maximum length of the email claim in the JWT, in practice we expect the identity provider to // already validate it on their end for a sane maximum length. This is an additional sanity check. @@ -180,9 +179,9 @@ fn compute_next_certs_fetch_delay( result: &Result, current_delay: Option, ) -> Option { - const MIN_DELAY: u64 = 60; - const MAX_DELAY: u64 = FETCH_CERTS_INTERVAL; - const MULTIPLIER: u64 = 2; + const MIN_DELAY_SECONDS: u64 = 60; + const MAX_DELAY_SECONDS: u64 = FETCH_CERTS_INTERVAL_SECONDS; + const BACKOFF_MULTIPLIER: u64 = 2; match result { // Reset delay to None so default (`FETCH_CERTS_INTERVAL`) delay is used. @@ -193,8 +192,8 @@ fn compute_next_certs_fetch_delay( // The delay should be at most `MAX_DELAY` and at minimum `MIN_DELAY`. Err(_) => Some( current_delay - .map_or(MIN_DELAY, |d| d * MULTIPLIER) - .clamp(MIN_DELAY, MAX_DELAY), + .map_or(MIN_DELAY_SECONDS, |d| d * BACKOFF_MULTIPLIER) + .clamp(MIN_DELAY_SECONDS, MAX_DELAY_SECONDS), ), } } @@ -210,7 +209,7 @@ fn schedule_fetch_certs( use std::time::Duration; set_timer( - Duration::from_secs(delay.unwrap_or(FETCH_CERTS_INTERVAL)), + Duration::from_secs(delay.unwrap_or(FETCH_CERTS_INTERVAL_SECONDS)), move || { spawn(async move { let result = fetch_certs(jwks_uri.clone()).await; @@ -378,7 +377,7 @@ fn verify_claims( if now > secs_to_nanos(claims.exp) { return Err(OpenIDJWTVerificationError::JWTExpired); } - if now > secs_to_nanos(claims.iat) + MAX_VALIDITY_WINDOW { + if now > secs_to_nanos(claims.iat + MAX_VALIDITY_WINDOW_SECONDS) { return Err(OpenIDJWTVerificationError::JWTExpired); } if now < secs_to_nanos(claims.iat) { @@ -612,7 +611,7 @@ fn should_return_error_when_invalid_caller() { #[test] fn should_return_error_when_no_longer_valid() { - TEST_TIME.replace(time() + MAX_VALIDITY_WINDOW + 1); + TEST_TIME.replace(time() + secs_to_nanos(MAX_VALIDITY_WINDOW_SECONDS) + 1); let (_, salt, config, claims) = test_data(); assert_eq!( @@ -663,28 +662,28 @@ fn should_return_error_when_name_too_long() { } #[test] fn should_compute_next_certs_fetch_delay() { - const MIN_DELAY: u64 = 60; - const MAX_DELAY: u64 = FETCH_CERTS_INTERVAL; + const MIN_DELAY_SECONDS: u64 = 60; + const MAX_DELAY_SECONDS: u64 = FETCH_CERTS_INTERVAL_SECONDS; let success: Result<(), ()> = Ok(()); let error: Result<(), ()> = Err(()); for (current_delay, expected_next_delay_on_error) in [ // Should be at least `MIN_DELAY` (1 minute) - (None, Some(MIN_DELAY)), - (Some(0), Some(MIN_DELAY)), - (Some(1), Some(MIN_DELAY)), - (Some(MIN_DELAY / 2 - 1), Some(MIN_DELAY)), + (None, Some(MIN_DELAY_SECONDS)), + (Some(0), Some(MIN_DELAY_SECONDS)), + (Some(1), Some(MIN_DELAY_SECONDS)), + (Some(MIN_DELAY_SECONDS / 2 - 1), Some(MIN_DELAY_SECONDS)), // Should be multiplied by two - (Some(MIN_DELAY / 2), Some(MIN_DELAY)), - (Some(MIN_DELAY / 2 + 1), Some(MIN_DELAY + 2)), + (Some(MIN_DELAY_SECONDS / 2), Some(MIN_DELAY_SECONDS)), + (Some(MIN_DELAY_SECONDS / 2 + 1), Some(MIN_DELAY_SECONDS + 2)), (Some(120), Some(240)), (Some(120), Some(240)), (Some(240), Some(480)), // Should be at most `MAX_DELAY` (15 minutes) - (Some(480), Some(MAX_DELAY)), - (Some(MAX_DELAY / 2 + 1), Some(MAX_DELAY)), - (Some(MAX_DELAY * 2), Some(MAX_DELAY)), + (Some(480), Some(MAX_DELAY_SECONDS)), + (Some(MAX_DELAY_SECONDS / 2 + 1), Some(MAX_DELAY_SECONDS)), + (Some(MAX_DELAY_SECONDS * 2), Some(MAX_DELAY_SECONDS)), ] { // Should return `None` on success so default (`FETCH_CERTS_INTERVAL`) delay is used. assert_eq!(