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.

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::<Vec<_>>();
 
         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<HashSet<Algorithm>>) -> Option<JwkSet> {
+pub(super) async fn get_jwks(url: Url) -> Option<JwkSet> {
     let data = if url.scheme() == "file" {
         let path = url
             .to_file_path()
@@ -189,39 +184,39 @@ pub(super) async fn get_jwks(url: Url, algorithms: Option<HashSet<Algorithm>>) -
     // 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| {
             tracing::error!(%e, "could not create JSON Value from url content");
             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");

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,