From ca26305bbed9f7df457810e4c43f39a8f245e4c9 Mon Sep 17 00:00:00 2001 From: Gary Pennington Date: Wed, 27 Sep 2023 15:10:53 +0100 Subject: [PATCH 1/6] Ignore JWKS keys with an algorithm (alg) of ES512 There are JWKS out there which contain keys which have algorithms which aren't supported by the router. We exclude any such known keys from processing to allow the rest of the JWKS to be used. Currently we exclude alg of ES512 fixes: #3853 --- .../src/plugins/authentication/jwks.rs | 31 ++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/apollo-router/src/plugins/authentication/jwks.rs b/apollo-router/src/plugins/authentication/jwks.rs index 854408a8fb..8bb825e7a5 100644 --- a/apollo-router/src/plugins/authentication/jwks.rs +++ b/apollo-router/src/plugins/authentication/jwks.rs @@ -14,6 +14,7 @@ use http::header::CONTENT_TYPE; use jsonwebtoken::jwk::JwkSet; use jsonwebtoken::Algorithm; use mime::APPLICATION_JSON; +use serde_json::Value; use tokio::fs::read_to_string; use tokio::sync::oneshot; use tower::BoxError; @@ -178,7 +179,35 @@ pub(super) async fn get_jwks(url: Url) -> Option { }) .ok()? }; - let jwks: JwkSet = serde_json::from_str(&data) + // Some JWKS contain algorithms which are not supported by the jsonwebtoken library. That means + // we can't just deserialize from the retrieved data and proceed. Any unrecognised + // algorithms will cause deserialization to fail. + // + // The only known failing case right now is "ES512". To accomodate this, we create a Value from + // the retrieved JWKS data. We then process this to discard any entries with an algorithm of "ES512". + // + // We always print a WARN to let people know that if their JWKS contains a key with an alg of + // ES512 we will not be using it. + // + // We may need to continue to update this over time as the jsonwebtoken library evolves or if + // other problematic algorithms are identified. + + tracing::warn!( + "If your JWKS contains keys with an 'alg' of 'ES512', they are ignored by the router." + ); + let mut raw_json: Value = serde_json::from_str(&data) + .map_err(|e| { + tracing::error!(%e, "could not create JSON Value from url content"); + e + }) + .ok()?; + let ignore_alg = serde_json::json!("ES512"); + raw_json.get_mut("keys").and_then(|keys| { + keys.as_array_mut().map(|array| { + array.retain(|row| row.get("alg").unwrap_or_else(|| &Value::Null) != &ignore_alg) + }) + }); + let jwks: JwkSet = serde_json::from_value(raw_json) .map_err(|e| { tracing::error!(%e, "could not create JWKS from url content"); e From cbded912bb42659af203e531f01280cbc2cd6cfa Mon Sep 17 00:00:00 2001 From: Gary Pennington Date: Wed, 27 Sep 2023 15:34:08 +0100 Subject: [PATCH 2/6] add a changeset --- .changesets/fix_garypen_3853_jwks_improve.md | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 .changesets/fix_garypen_3853_jwks_improve.md diff --git a/.changesets/fix_garypen_3853_jwks_improve.md b/.changesets/fix_garypen_3853_jwks_improve.md new file mode 100644 index 0000000000..08759d83a2 --- /dev/null +++ b/.changesets/fix_garypen_3853_jwks_improve.md @@ -0,0 +1,11 @@ +### Ignore JWKS keys with an algorithm (alg) of ES512 ([Issue #3853](https://github.com/apollographql/router/issues/3853)) + +If you have a JWKS which contains a Key which has an algorithm (alg) which the router doesn't recognise, then the entire JWKS is disregarded. This is unsatisfactory, since there are likely to be many other keys in the JWKS which the router could use. + +We have changed the JWKS processing logic so that keys which we know aren't supported by the router are excluded from the set of keys we use. + +Currently we exclude any keys with an algorithm "ES512", this may change in the future. + +We also print a warning message at router startup to let you know we are doing this. + +By [@garypen](https://github.com/garypen) in https://github.com/apollographql/router/pull/3922 \ No newline at end of file From 8f675fd2c76645369d0af047d2f8cb574484d108 Mon Sep 17 00:00:00 2001 From: Gary Pennington Date: Wed, 27 Sep 2023 18:22:31 +0100 Subject: [PATCH 3/6] Better idea, use the existing `algorithms` configuration We can check if there is a specified list of algorithms. If there is, use this to exclude keys which aren't supported. --- .changesets/fix_garypen_3853_jwks_improve.md | 10 +--- .../src/plugins/authentication/jwks.rs | 59 ++++++++++++------- 2 files changed, 40 insertions(+), 29 deletions(-) diff --git a/.changesets/fix_garypen_3853_jwks_improve.md b/.changesets/fix_garypen_3853_jwks_improve.md index 08759d83a2..09cecd64c9 100644 --- a/.changesets/fix_garypen_3853_jwks_improve.md +++ b/.changesets/fix_garypen_3853_jwks_improve.md @@ -1,11 +1,7 @@ -### Ignore JWKS keys with an algorithm (alg) of ES512 ([Issue #3853](https://github.com/apollographql/router/issues/3853)) +### Ignore JWKS keys which aren't in the algorithms configuration ([Issue #3853](https://github.com/apollographql/router/issues/3853)) If you have a JWKS which contains a Key which has an algorithm (alg) which the router doesn't recognise, then the entire JWKS is disregarded. This is unsatisfactory, since there are likely to be many other keys in the JWKS which the router could use. -We have changed the JWKS processing logic so that keys which we know aren't supported by the router are excluded from the set of keys we use. +We have changed the JWKS processing logic so that we use the list of `algorithms` specified in your configuration to exclude keys which are not contained in that list. -Currently we exclude any keys with an algorithm "ES512", this may change in the future. - -We also print a warning message at router startup to let you know we are doing this. - -By [@garypen](https://github.com/garypen) in https://github.com/apollographql/router/pull/3922 \ No newline at end of file +By [@garypen](https://github.com/garypen) in https://github.com/apollographql/router/pull/3922 diff --git a/apollo-router/src/plugins/authentication/jwks.rs b/apollo-router/src/plugins/authentication/jwks.rs index 8bb825e7a5..5f8dc13627 100644 --- a/apollo-router/src/plugins/authentication/jwks.rs +++ b/apollo-router/src/plugins/authentication/jwks.rs @@ -1,5 +1,6 @@ use std::collections::HashMap; use std::collections::HashSet; +use std::str::FromStr; use std::sync::Arc; use std::sync::RwLock; @@ -52,9 +53,14 @@ impl JwksManager { let downloads = list .iter() .cloned() - .map(|JwksConfig { url, .. }| { - get_jwks(url.clone()).map(|opt_jwks| opt_jwks.map(|jwks| (url, jwks))) - }) + .map( + |JwksConfig { + url, algorithms, .. + }| { + get_jwks(url.clone(), algorithms.clone()) + .map(|opt_jwks| opt_jwks.map(|jwks| (url, jwks))) + }, + ) .collect::>(); let jwks_map: HashMap<_, _> = join_all(downloads).await.into_iter().flatten().collect(); @@ -103,7 +109,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()).await { + if let Some(jwks) = get_jwks(config.url.clone(), config.algorithms.clone()).await { if let Ok(mut map) = jwks_map.write() { map.insert(config.url, jwks); } @@ -133,7 +139,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) -> Option { +pub(super) async fn get_jwks(url: Url, algorithms: Option>) -> Option { let data = if url.scheme() == "file" { let path = url .to_file_path() @@ -183,30 +189,39 @@ pub(super) async fn get_jwks(url: Url) -> Option { // we can't just deserialize from the retrieved data and proceed. Any unrecognised // algorithms will cause deserialization to fail. // - // The only known failing case right now is "ES512". To accomodate this, we create a Value from - // the retrieved JWKS data. We then process this to discard any entries with an algorithm of "ES512". - // - // We always print a WARN to let people know that if their JWKS contains a key with an alg of - // ES512 we will not be using it. - // - // We may need to continue to update this over time as the jsonwebtoken library evolves or if - // other problematic algorithms are identified. + // 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. - tracing::warn!( - "If your JWKS contains keys with an 'alg' of 'ES512', they are ignored by the router." - ); let mut raw_json: Value = serde_json::from_str(&data) .map_err(|e| { tracing::error!(%e, "could not create JSON Value from url content"); e }) .ok()?; - let ignore_alg = serde_json::json!("ES512"); - raw_json.get_mut("keys").and_then(|keys| { - keys.as_array_mut().map(|array| { - array.retain(|row| row.get("alg").unwrap_or_else(|| &Value::Null) != &ignore_alg) - }) - }); + 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, + } + } + }) + }) + }); + } let jwks: JwkSet = serde_json::from_value(raw_json) .map_err(|e| { tracing::error!(%e, "could not create JWKS from url content"); From 2adc1ec889253add8fc3cc44f85a78738216561c Mon Sep 17 00:00:00 2001 From: Gary Pennington Date: Thu, 28 Sep 2023 08:35:51 +0100 Subject: [PATCH 4/6] add a test to check that invalid algorithms in jwks will work --- .../src/plugins/authentication/tests.rs | 37 ++++++++++++++ .../tests/fixtures/jwks-unknown-alg.json | 49 +++++++++++++++++++ 2 files changed, 86 insertions(+) create mode 100644 apollo-router/tests/fixtures/jwks-unknown-alg.json diff --git a/apollo-router/src/plugins/authentication/tests.rs b/apollo-router/src/plugins/authentication/tests.rs index 1e257199bd..fff84817e2 100644 --- a/apollo-router/src/plugins/authentication/tests.rs +++ b/apollo-router/src/plugins/authentication/tests.rs @@ -920,3 +920,40 @@ async fn it_rejects_key_with_restricted_algorithm() { assert!(search_jwks(&jwks_manager, &criteria).is_none()); } + +#[tokio::test] +async fn it_rejects_key_with_restricted_algorithm_and_unknown_jwks_algorithm() { + let mut sets = vec![]; + let mut urls = vec![]; + + // Use a jwks which contains an algorithm (ES512) which jsonwebtoken doesn't support + let jwks_url = create_an_url("jwks-unknown-alg.json"); + + sets.push(jwks_url); + + for s_url in &sets { + let url: Url = Url::from_str(s_url).expect("created a valid url"); + urls.push(JwksConfig { + url, + issuer: None, + algorithms: Some(HashSet::from([Algorithm::RS256, Algorithm::HS256])), + }); + } + + let jwks_manager = JwksManager::new(urls).await.unwrap(); + + // the JWT contains a HMAC key but we configured a restriction to RSA signing + let criteria = JWTCriteria { + kid: None, + alg: Algorithm::HS256, + }; + + assert!(search_jwks(&jwks_manager, &criteria).is_some()); + + let criteria = JWTCriteria { + kid: None, + alg: Algorithm::RS256, + }; + + assert!(search_jwks(&jwks_manager, &criteria).is_some()); +} diff --git a/apollo-router/tests/fixtures/jwks-unknown-alg.json b/apollo-router/tests/fixtures/jwks-unknown-alg.json new file mode 100644 index 0000000000..3278b32dcb --- /dev/null +++ b/apollo-router/tests/fixtures/jwks-unknown-alg.json @@ -0,0 +1,49 @@ +{ + "keys": [ + { + "kty": "oct", + "kid": "key1", + "alg": "HS256", + "k": "c2VjcmV0Cg==", + "use": "sig" + }, + { + "kty": "oct", + "kid": "key2", + "k": "c2VjcmV0Cg==", + "use": "sig" + }, + { + "crv": "P-256", + "key_ops": [ + "verify" + ], + "kty": "EC", + "x": "opFUViwCYVZLmsbG2cJTA9uPvOF5Gg8W7uNhrcorGhI", + "y": "bPxvCFKmlqTdEFc34OekvpviUUyelGrbi020dlgIsqo", + "alg": "ES256", + "use": "sig", + "kid": "afda85e09a320cf748177874592de64d" + }, + { + "alg": "RS256", + "e": "AQAB", + "key_ops": [ + "verify" + ], + "kty": "RSA", + "n": "wg1GEw1MJZ7IWORpJegYEkc8WB1ck-uNVlKP9Aqjt8tck1JN5dmM_Dwbp-JLsL49xgaXByVAqOO4mWefgx-_sGzYOTMUfAxOYuj3XwBYEAtrBETjGQB-xfdIq8yVnrrwIhKovo42O9TPjQQZ-vL2YmgKdqqO5tQPvKVI20nQVkT04I1orMsbbcSVT6xZiOCWwZWkre2wJbk8hMR_IXHOotpUgAazN99A413c5R9ZBwSZni12PhXf4vM4hoiLTNSy-PEiFle1o2yKItwVZ55SL_P1LuL4A0Sh4F7CRclpdTIql8mv2hVUfdH5U7oIch3O3HDm6pzaxAzNgENaDLL4iYf7UCd1muTu2R8X-2D-qGdz8mhVGsVBhn57S9-zWU6ShM4D-frHibpmcJhnFPJ8zcVGscbc59qWLZH6a3n176eny_TM9CZIeLu9U1fRL1jiAn9m5wJR6G_rQhXWaiKyP2SMyqKrDrxlacxApGwllcOOCXG11tEDaH4W01MpuvVL", + "use": "sig", + "kid": "022516583d56b68faf40260fda72978a" + }, + { + "kty": "EC", + "kid": "IcDV_1dOLRNRTRCrGtG9h5bXkVE", + "use": "sig", + "alg": "ES512", + "x": "AKwEPYNsPBEZi-PZKfE7igKDZaqY8xMwIu2AkjPOWJ_bJ9lv6QZIr4mVKSyj0d8wHPExlkWJkX7cxqb3UO_KMVF7", + "y": "AVo3Gnbj4uJEBRariAw1q5pQmzjvHGSaUtbZME9ZtFyOIJmDmoy_Y37PqAnyWwL6QB7iSPfzWn7j-3aZ8pnQIC9D", + "crv": "P-521" + } + ] +} From 7bca1709eb4df746b840254f6f8d220ffc73b7d5 Mon Sep 17 00:00:00 2001 From: Gary Pennington Date: Thu, 28 Sep 2023 12:28:31 +0100 Subject: [PATCH 5/6] 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, From 3980f070c1b431fad7d34a72c368da6e28163d88 Mon Sep 17 00:00:00 2001 From: Gary Pennington Date: Thu, 28 Sep 2023 12:32:44 +0100 Subject: [PATCH 6/6] update changeset --- .changesets/fix_garypen_3853_jwks_improve.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.changesets/fix_garypen_3853_jwks_improve.md b/.changesets/fix_garypen_3853_jwks_improve.md index 09cecd64c9..9e04641e5d 100644 --- a/.changesets/fix_garypen_3853_jwks_improve.md +++ b/.changesets/fix_garypen_3853_jwks_improve.md @@ -1,7 +1,7 @@ -### Ignore JWKS keys which aren't in the algorithms configuration ([Issue #3853](https://github.com/apollographql/router/issues/3853)) +### Ignore JWKS keys which aren't supported by the router ([Issue #3853](https://github.com/apollographql/router/issues/3853)) -If you have a JWKS which contains a Key which has an algorithm (alg) which the router doesn't recognise, then the entire JWKS is disregarded. This is unsatisfactory, since there are likely to be many other keys in the JWKS which the router could use. +If you have a JWKS which contains a key which has an algorithm (alg) which the router doesn't recognise, then the entire JWKS is disregarded. This is unsatisfactory, since there are likely to be many other keys in the JWKS which the router could use. -We have changed the JWKS processing logic so that we use the list of `algorithms` specified in your configuration to exclude keys which are not contained in that list. +We have changed the JWKS processing logic so that we remove entries with an unrecognised algorithm from the list of available keys. We print a warning with the name of the algorithm for each removed entry. By [@garypen](https://github.com/garypen) in https://github.com/apollographql/router/pull/3922