From 7bca1709eb4df746b840254f6f8d220ffc73b7d5 Mon Sep 17 00:00:00 2001 From: Gary Pennington Date: Thu, 28 Sep 2023 12:28:31 +0100 Subject: [PATCH] code review feedback Rather than using `algorithms` to restric the list of keys, remove keys which have an `alg` which `jsonwebtoken` doesn't recognise. Print a warning message each time we encounter such a key when the JWKS is processed. --- .../src/plugins/authentication/jwks.rs | 61 +++++++++---------- .../src/plugins/authentication/tests.rs | 7 ++- 2 files changed, 32 insertions(+), 36 deletions(-) diff --git a/apollo-router/src/plugins/authentication/jwks.rs b/apollo-router/src/plugins/authentication/jwks.rs index 5f8dc13627..e99677a7a8 100644 --- a/apollo-router/src/plugins/authentication/jwks.rs +++ b/apollo-router/src/plugins/authentication/jwks.rs @@ -53,14 +53,9 @@ impl JwksManager { let downloads = list .iter() .cloned() - .map( - |JwksConfig { - url, algorithms, .. - }| { - get_jwks(url.clone(), algorithms.clone()) - .map(|opt_jwks| opt_jwks.map(|jwks| (url, jwks))) - }, - ) + .map(|JwksConfig { url, .. }| { + get_jwks(url.clone()).map(|opt_jwks| opt_jwks.map(|jwks| (url, jwks))) + }) .collect::>(); let jwks_map: HashMap<_, _> = join_all(downloads).await.into_iter().flatten().collect(); @@ -109,7 +104,7 @@ async fn poll( repeat((config, jwks_map)).then(|(config, jwks_map)| async move { tokio::time::sleep(DEFAULT_AUTHENTICATION_DOWNLOAD_INTERVAL).await; - if let Some(jwks) = get_jwks(config.url.clone(), config.algorithms.clone()).await { + if let Some(jwks) = get_jwks(config.url.clone()).await { if let Ok(mut map) = jwks_map.write() { map.insert(config.url, jwks); } @@ -139,7 +134,7 @@ async fn poll( // This function is expected to return an Optional value, but we'd like to let // users know the various failure conditions. Hence the various clumsy map_err() // scattered through the processing. -pub(super) async fn get_jwks(url: Url, algorithms: Option>) -> Option { +pub(super) async fn get_jwks(url: Url) -> Option { let data = if url.scheme() == "file" { let path = url .to_file_path() @@ -189,9 +184,8 @@ pub(super) async fn get_jwks(url: Url, algorithms: Option>) - // we can't just deserialize from the retrieved data and proceed. Any unrecognised // algorithms will cause deserialization to fail. // - // We use the list of algorithms specified by the user in configuration to constrain the - // processing and exclude keys which are not in the optional specified list. If there is no - // optional list of algorithms, we accept the full set. + // Try to identify any entries which contain algorithms which are not supported by + // jsonwebtoken. let mut raw_json: Value = serde_json::from_str(&data) .map_err(|e| { @@ -199,29 +193,30 @@ pub(super) async fn get_jwks(url: Url, algorithms: Option>) - e }) .ok()?; - if let Some(retain_list) = algorithms { - raw_json.get_mut("keys").and_then(|keys| { - keys.as_array_mut().map(|array| { - array.retain(|key| { - let alg_json = key.get("alg").unwrap_or_else(|| &Value::Null); - // Retention rules are, retain if: - // - we can't find an alg field - // - alg field isn't a string - // - our list of algorithms contains the identifed algorithm - if alg_json == &Value::Null || !alg_json.is_string() { - true - } else { - match Algorithm::from_str( - alg_json.as_str().expect("we checked it's a string"), - ) { - Ok(alg) => retain_list.contains(&alg), - Err(_) => false, + raw_json.get_mut("keys").and_then(|keys| { + keys.as_array_mut().map(|array| { + array.retain(|key| { + let alg_json = key.get("alg").unwrap_or_else(|| &Value::Null); + // Retention rules are, retain if: + // - we can't find an alg field + // - alg field isn't a string + // - jsonwebtoken supports the algorithm (determined by creating an enum member) + if alg_json == &Value::Null || !alg_json.is_string() { + true + } else { + let alg_name = alg_json.as_str().expect("we checked it's a string"); + match Algorithm::from_str(alg_name) + { + Ok(_) => true, + Err(_) => { + tracing::warn!("ignoring a key with algorithm `{alg_name}` since it is not supported"); + false } } - }) + } }) - }); - } + }) + }); let jwks: JwkSet = serde_json::from_value(raw_json) .map_err(|e| { tracing::error!(%e, "could not create JWKS from url content"); diff --git a/apollo-router/src/plugins/authentication/tests.rs b/apollo-router/src/plugins/authentication/tests.rs index fff84817e2..9a75703cc9 100644 --- a/apollo-router/src/plugins/authentication/tests.rs +++ b/apollo-router/src/plugins/authentication/tests.rs @@ -922,7 +922,7 @@ async fn it_rejects_key_with_restricted_algorithm() { } #[tokio::test] -async fn it_rejects_key_with_restricted_algorithm_and_unknown_jwks_algorithm() { +async fn it_rejects_and_accepts_keys_with_restricted_algorithms_and_unknown_jwks_algorithm() { let mut sets = vec![]; let mut urls = vec![]; @@ -936,7 +936,7 @@ async fn it_rejects_key_with_restricted_algorithm_and_unknown_jwks_algorithm() { urls.push(JwksConfig { url, issuer: None, - algorithms: Some(HashSet::from([Algorithm::RS256, Algorithm::HS256])), + algorithms: Some(HashSet::from([Algorithm::RS256])), }); } @@ -948,8 +948,9 @@ async fn it_rejects_key_with_restricted_algorithm_and_unknown_jwks_algorithm() { alg: Algorithm::HS256, }; - assert!(search_jwks(&jwks_manager, &criteria).is_some()); + assert!(search_jwks(&jwks_manager, &criteria).is_none()); + // the JWT contains a RSA key (configured to allow) let criteria = JWTCriteria { kid: None, alg: Algorithm::RS256,