From a8b510a03ec39d2263021e6f9681c1d2b9fd66d7 Mon Sep 17 00:00:00 2001 From: cduvray Date: Sun, 12 Feb 2023 09:06:18 +0100 Subject: [PATCH] refactor: refresh simplification - difference between refresh_interval and minimal_refresh_interval was no clear, - therfore they were merged --- jwt-authorizer/src/jwks/key_store_manager.rs | 262 ++++++++++--------- jwt-authorizer/tests/integration_tests.rs | 6 +- 2 files changed, 142 insertions(+), 126 deletions(-) diff --git a/jwt-authorizer/src/jwks/key_store_manager.rs b/jwt-authorizer/src/jwks/key_store_manager.rs index 4e77e79..27975bb 100644 --- a/jwt-authorizer/src/jwks/key_store_manager.rs +++ b/jwt-authorizer/src/jwks/key_store_manager.rs @@ -11,15 +11,16 @@ use tokio::sync::Mutex; use crate::error::AuthError; +/// Defines the strategy for the JWKS refresh. #[derive(Clone, Copy)] pub enum RefreshStrategy { /// refresh periodicaly Interval, - /// when kid not found in the store + /// refresh when kid not found in the store KeyNotFound, - /// load once triggered by the first use + /// loading is triggered only once by the first use NoRefresh, } @@ -27,11 +28,15 @@ pub enum RefreshStrategy { #[derive(Clone, Copy)] pub struct Refresh { pub strategy: RefreshStrategy, - // after the interval the store will be refreshed (before getting a new key - lazy behaviour) + /// After the refresh interval the store will/can be refreshed. + /// + /// - RefreshStrategy::KeyNotFound - refresh will be performed only if a kid is not found in the store + /// (if no kid is in the token header the alg is looked up) + /// - RefreshStrategy::Interval - refresh will be performed each time the refresh interval has elapsed + /// (before checking a new token -> lazy behaviour) pub refresh_interval: Duration, - // don't refresh before (counting from the last refresh, when the kid not found) - pub minimal_refresh_interval: Duration, - // don't refresh before (after an error or jwks unawailable) + /// don't refresh before (after an error or jwks is unawailable) + /// (we let a little bit of time to the jwks endpoint to recover) pub retry_interval: Duration, } @@ -40,7 +45,6 @@ impl Default for Refresh { Self { strategy: RefreshStrategy::KeyNotFound, refresh_interval: Duration::from_secs(600), - minimal_refresh_interval: Duration::from_secs(30), retry_interval: Duration::from_secs(10), } } @@ -81,9 +85,7 @@ impl KeyStoreManager { let mut ks_gard = kstore.lock().await; let key = match self.refresh.strategy { RefreshStrategy::Interval => { - if ks_gard.should_refresh(self.refresh.refresh_interval) - && ks_gard.can_refresh(self.refresh.minimal_refresh_interval, self.refresh.retry_interval) - { + if ks_gard.can_refresh(self.refresh.refresh_interval, self.refresh.retry_interval) { ks_gard.refresh(&self.key_url, &[]).await?; } if let Some(ref kid) = header.kid { @@ -97,7 +99,7 @@ impl KeyStoreManager { let jwk_opt = ks_gard.find_kid(kid); if let Some(jwk) = jwk_opt { jwk - } else if ks_gard.can_refresh(self.refresh.minimal_refresh_interval, self.refresh.retry_interval) { + } else if ks_gard.can_refresh(self.refresh.refresh_interval, self.refresh.retry_interval) { ks_gard.refresh(&self.key_url, &[("kid", kid)]).await?; ks_gard.find_kid(kid).ok_or_else(|| AuthError::InvalidKid(kid.to_owned()))? } else { @@ -105,10 +107,9 @@ impl KeyStoreManager { } } else { let jwk_opt = ks_gard.find_alg(&header.alg); - // .ok_or(AuthError::InvalidKeyAlg(header.alg))? if let Some(jwk) = jwk_opt { jwk - } else if ks_gard.can_refresh(self.refresh.minimal_refresh_interval, self.refresh.retry_interval) { + } else if ks_gard.can_refresh(self.refresh.refresh_interval, self.refresh.retry_interval) { ks_gard .refresh( &self.key_url, @@ -127,7 +128,10 @@ impl KeyStoreManager { } } RefreshStrategy::NoRefresh => { - if ks_gard.load_time.is_none() { + if ks_gard.load_time.is_none() + // if jwks endpoint is down for the loading, respect retry_interval + && ks_gard.can_refresh(self.refresh.refresh_interval, self.refresh.retry_interval) + { ks_gard.refresh(&self.key_url, &[]).await?; } if let Some(ref kid) = header.kid { @@ -143,23 +147,15 @@ impl KeyStoreManager { } impl KeyStore { - fn should_refresh(&self, refresh_interval: Duration) -> bool { - if let Some(t) = self.load_time { - t.elapsed() > refresh_interval - } else { - true - } - } - - fn can_refresh(&self, minimal_refresh_interval: Duration, minimal_retry: Duration) -> bool { + fn can_refresh(&self, refresh_interval: Duration, minimal_retry: Duration) -> bool { if let Some(fail_tm) = self.fail_time { if let Some(load_tm) = self.load_time { - fail_tm.elapsed() > minimal_retry && load_tm.elapsed() > minimal_refresh_interval + fail_tm.elapsed() > minimal_retry && load_tm.elapsed() > refresh_interval } else { fail_tm.elapsed() > minimal_retry } } else if let Some(load_tm) = self.load_time { - load_tm.elapsed() > minimal_refresh_interval + load_tm.elapsed() > refresh_interval } else { true } @@ -171,12 +167,16 @@ impl KeyStore { .query(qparam) .send() .await - .map_err(AuthError::JwksRefreshError)? + .map_err(|e| { + self.fail_time = Some(Instant::now()); + AuthError::JwksRefreshError(e) + })? .json::() .await .map(|jwks| { self.load_time = Some(Instant::now()); self.jwks = jwks; + self.fail_time = None; Ok(()) }) .map_err(|e| { @@ -222,24 +222,43 @@ mod tests { use crate::jwks::key_store_manager::{KeyStore, KeyStoreManager}; use crate::{Refresh, RefreshStrategy}; - #[test] - fn keystore_should_refresh() { - let ks = KeyStore { - jwks: jsonwebtoken::jwk::JwkSet { keys: vec![] }, - fail_time: None, - load_time: Some(Instant::now()), - }; + const JWK_ED01: &str = r#"{ + "kty": "OKP", + "use": "sig", + "crv": "Ed25519", + "x": "uWtSkE-I9aTMYTTvuTE1rtu0rNdxp3DU33cJ_ksL1Gk", + "kid": "ed01", + "alg": "EdDSA" + }"#; - assert!(!ks.should_refresh(Duration::from_secs(5))); + const JWK_ED02: &str = r#"{ + "kty": "OKP", + "use": "sig", + "crv": "Ed25519", + "x": "uWtSkE-I9aTMYTTvuTE1rtu0rNdxp3DU33cJ_ksL1Gk", + "kid": "ed02", + "alg": "EdDSA" + }"#; - let ks = KeyStore { - jwks: jsonwebtoken::jwk::JwkSet { keys: vec![] }, - fail_time: None, - load_time: Some(Instant::now() - Duration::from_secs(6)), - }; + const JWK_EC01: &str = r#"{ + "kty": "EC", + "crv": "P-256", + "x": "w7JAoU_gJbZJvV-zCOvU9yFJq0FNC_edCMRM78P8eQQ", + "y": "wQg1EytcsEmGrM70Gb53oluoDbVhCZ3Uq3hHMslHVb4", + "kid": "ec01", + "alg": "ES256", + "use": "sig" + }"#; - assert!(ks.should_refresh(Duration::from_secs(5))); - } + const JWK_EC02: &str = r#"{ + "kty": "EC", + "crv": "P-256", + "x": "w7JAoU_gJbZJvV-zCOvU9yFJq0FNC_edCMRM78P8eQQ", + "y": "wQg1EytcsEmGrM70Gb53oluoDbVhCZ3Uq3hHMslHVb4", + "kid": "ec02", + "alg": "ES256", + "use": "sig" + }"#; #[test] fn keystore_can_refresh() { @@ -249,9 +268,8 @@ mod tests { fail_time: Some(Instant::now() - Duration::from_secs(5)), load_time: None, }; - assert!(ks.can_refresh(Duration::from_secs(4), Duration::from_secs(4))); - assert!(ks.can_refresh(Duration::from_secs(6), Duration::from_secs(4))); - assert!(!ks.can_refresh(Duration::from_secs(6), Duration::from_secs(6))); + assert!(ks.can_refresh(Duration::from_secs(0), Duration::from_secs(4))); + assert!(!ks.can_refresh(Duration::from_secs(0), Duration::from_secs(6))); // NO FAIL, LOAD let ks = KeyStore { @@ -259,8 +277,8 @@ mod tests { fail_time: None, load_time: Some(Instant::now() - Duration::from_secs(5)), }; - assert!(ks.can_refresh(Duration::from_secs(4), Duration::from_secs(4))); - assert!(!ks.can_refresh(Duration::from_secs(6), Duration::from_secs(6))); + assert!(ks.can_refresh(Duration::from_secs(4), Duration::from_secs(0))); + assert!(!ks.can_refresh(Duration::from_secs(6), Duration::from_secs(0))); // FAIL, LOAD let ks = KeyStore { @@ -269,6 +287,7 @@ mod tests { load_time: Some(Instant::now() - Duration::from_secs(10)), }; assert!(ks.can_refresh(Duration::from_secs(6), Duration::from_secs(4))); + assert!(!ks.can_refresh(Duration::from_secs(11), Duration::from_secs(4))); assert!(!ks.can_refresh(Duration::from_secs(6), Duration::from_secs(6))); } @@ -309,6 +328,15 @@ mod tests { .await; } + async fn mock_jwks_response_fail_once(mock_server: &MockServer) { + Mock::given(method("GET")) + .and(path("/")) + .respond_with(ResponseTemplate::new(500)) + .expect(1) + .mount(&mock_server) + .await; + } + fn build_header(kid: &str, alg: Algorithm) -> Header { let mut header = Header::new(alg); header.kid = Some(kid.to_owned()); @@ -316,125 +344,115 @@ mod tests { } #[tokio::test] - async fn keystore_manager_find_key_with_refresh_interval() { + async fn strategy_interval() { let mock_server = MockServer::start().await; - mock_jwks_response_once( - &mock_server, - r#"{ - "kty": "OKP", - "use": "sig", - "crv": "Ed25519", - "x": "uWtSkE-I9aTMYTTvuTE1rtu0rNdxp3DU33cJ_ksL1Gk", - "kid": "key-ed", - "alg": "EdDSA" - }"#, - ) - .await; + mock_jwks_response_once(&mock_server, JWK_ED01).await; let ksm = KeyStoreManager::new( Url::parse(&mock_server.uri()).unwrap(), Refresh { strategy: RefreshStrategy::Interval, - refresh_interval: Duration::from_secs(3000), - ..Default::default() + refresh_interval: Duration::from_millis(10), + retry_interval: Duration::from_millis(5), }, ); + + // 1st RELOAD let r = ksm.get_key(&Header::new(Algorithm::EdDSA)).await; assert!(r.is_ok()); mock_server.verify().await; + + // NO RELOAD - inteval not elapsed + assert!(ksm.get_key(&Header::new(Algorithm::EdDSA)).await.is_ok()); + + // RELOAD - interval elapsed + mock_server.reset().await; + tokio::time::sleep(Duration::from_millis(11)).await; + mock_jwks_response_once(&mock_server, JWK_ED01).await; + assert!(ksm.get_key(&Header::new(Algorithm::EdDSA)).await.is_ok()); + mock_server.verify().await; + + // RELOAD - with fail + mock_server.reset().await; + tokio::time::sleep(Duration::from_millis(11)).await; + mock_jwks_response_fail_once(&mock_server).await; + assert!(ksm.get_key(&Header::new(Algorithm::EdDSA)).await.is_err()); + mock_server.verify().await; + + // NO RELOAD - retry not ellapsed + assert!(ksm.get_key(&Header::new(Algorithm::EdDSA)).await.is_ok()); + + // RELOAD - retry elapsed + mock_server.reset().await; + tokio::time::sleep(Duration::from_millis(6)).await; + mock_jwks_response_once(&mock_server, JWK_ED01).await; + assert!(ksm.get_key(&Header::new(Algorithm::EdDSA)).await.is_ok()); + mock_server.verify().await; } #[tokio::test] - async fn keystore_manager_find_key_with_refresh() { + async fn strategy_key_not_found_with_refresh() { let mock_server = MockServer::start().await; - mock_jwks_response_once( - &mock_server, - r#"{ - "kty": "OKP", - "use": "sig", - "crv": "Ed25519", - "x": "uWtSkE-I9aTMYTTvuTE1rtu0rNdxp3DU33cJ_ksL1Gk", - "kid": "key-ed", - "alg": "EdDSA" - }"#, - ) - .await; + mock_jwks_response_once(&mock_server, JWK_ED01).await; - let mut ksm = KeyStoreManager::new( + let ksm = KeyStoreManager::new( Url::parse(&mock_server.uri()).unwrap(), Refresh { strategy: RefreshStrategy::KeyNotFound, - ..Default::default() + refresh_interval: Duration::from_millis(10), + retry_interval: Duration::from_millis(5), }, ); // STEP 1: initial (lazy) reloading - let r = ksm.get_key(&build_header("key-ed", Algorithm::EdDSA)).await; + let r = ksm.get_key(&build_header("ed01", Algorithm::EdDSA)).await; assert!(r.is_ok()); mock_server.verify().await; - // STEP2: new kid -> reloading ksm + // STEP2: new kid, < refresh_interval -> reloading ksm mock_server.reset().await; - mock_jwks_response_once( - &mock_server, - r#"{ - "kty": "OKP", - "use": "sig", - "crv": "Ed25519", - "x": "uWtSkE-I9aTMYTTvuTE1rtu0rNdxp3DU33cJ_ksL1Gk", - "kid": "key-ed02", - "alg": "EdDSA" - }"#, - ) - .await; - let h = build_header("key-ed02", Algorithm::EdDSA); + mock_jwks_response_once(&mock_server, JWK_ED02).await; + let h = build_header("ed02", Algorithm::EdDSA); assert!(ksm.get_key(&h).await.is_err()); - ksm.refresh.minimal_refresh_interval = Duration::from_millis(100); - tokio::time::sleep(Duration::from_millis(101)).await; + // ksm.refresh.refresh_interval = Duration::from_millis(10); + tokio::time::sleep(Duration::from_millis(11)).await; assert!(ksm.get_key(&h).await.is_ok()); mock_server.verify().await; // STEP3: new algorithm -> try to reload mock_server.reset().await; - mock_jwks_response_once( - &mock_server, - r#"{ - "kty": "EC", - "crv": "P-256", - "x": "w7JAoU_gJbZJvV-zCOvU9yFJq0FNC_edCMRM78P8eQQ", - "y": "wQg1EytcsEmGrM70Gb53oluoDbVhCZ3Uq3hHMslHVb4", - "kid": "ec01", - "alg": "ES256", - "use": "sig" - }"#, - ) - .await; + mock_jwks_response_once(&mock_server, JWK_EC01).await; let h = Header::new(Algorithm::ES256); assert!(ksm.get_key(&h).await.is_err()); - tokio::time::sleep(Duration::from_millis(101)).await; + tokio::time::sleep(Duration::from_millis(11)).await; assert!(ksm.get_key(&h).await.is_ok()); mock_server.verify().await; + + // STEP4: new key, refresh elapsed, FAIL + mock_server.reset().await; + tokio::time::sleep(Duration::from_millis(11)).await; + mock_jwks_response_fail_once(&mock_server).await; + let h = build_header("ec02", Algorithm::EdDSA); + assert!(ksm.get_key(&h).await.is_err()); + mock_server.verify().await; + + // STEP5: retry elapsed -> reload + mock_server.reset().await; + tokio::time::sleep(Duration::from_millis(6)).await; + mock_jwks_response_once(&mock_server, JWK_EC02).await; + let h = build_header("ec02", Algorithm::EdDSA); + assert!(ksm.get_key(&h).await.is_ok()); + mock_server.verify().await; } #[tokio::test] - async fn keystore_manager_find_key_with_no_refresh() { + async fn strategy_no_refresh() { let mock_server = MockServer::start().await; - mock_jwks_response_once( - &mock_server, - r#"{ - "kty": "OKP", - "use": "sig", - "crv": "Ed25519", - "x": "uWtSkE-I9aTMYTTvuTE1rtu0rNdxp3DU33cJ_ksL1Gk", - "kid": "key-ed", - "alg": "EdDSA" - }"#, - ) - .await; + mock_jwks_response_once(&mock_server, JWK_ED01).await; let ksm = KeyStoreManager::new( Url::parse(&mock_server.uri()).unwrap(), @@ -445,12 +463,12 @@ mod tests { ); // STEP 1: initial (lazy) reloading - let r = ksm.get_key(&build_header("key-ed", Algorithm::EdDSA)).await; + let r = ksm.get_key(&build_header("ed01", Algorithm::EdDSA)).await; assert!(r.is_ok()); mock_server.verify().await; // STEP2: new kid -> reloading ksm - let h = build_header("key-ed02", Algorithm::EdDSA); + let h = build_header("ed02", Algorithm::EdDSA); assert!(ksm.get_key(&h).await.is_err()); mock_server.verify().await; diff --git a/jwt-authorizer/tests/integration_tests.rs b/jwt-authorizer/tests/integration_tests.rs index c9c50e7..3445506 100644 --- a/jwt-authorizer/tests/integration_tests.rs +++ b/jwt-authorizer/tests/integration_tests.rs @@ -224,7 +224,6 @@ async fn scenario2() { init_test(); let url = run_jwks_server(); let refresh = Refresh { - minimal_refresh_interval: Duration::from_millis(20), refresh_interval: Duration::from_millis(40), retry_interval: Duration::from_millis(0), strategy: RefreshStrategy::Interval, @@ -255,9 +254,8 @@ async fn scenario3() { let url = run_jwks_server(); let refresh = Refresh { strategy: RefreshStrategy::KeyNotFound, - minimal_refresh_interval: Duration::from_millis(20), + refresh_interval: Duration::from_millis(40), retry_interval: Duration::from_millis(0), - ..Default::default() }; let auth: JwtAuthorizer = JwtAuthorizer::from_oidc(&url).refresh(refresh); let mut app = app(auth).await; @@ -287,7 +285,7 @@ async fn scenario4() { let url = run_jwks_server(); let refresh = Refresh { strategy: RefreshStrategy::NoRefresh, - minimal_refresh_interval: Duration::from_millis(0), + refresh_interval: Duration::from_millis(0), retry_interval: Duration::from_millis(0), ..Default::default() };