Address review comments

Author: Jakub Konka

Cargo.lock

@@ -138,7 +138,7 @@ pub struct NotaryClient {
     /// in notary server.
     #[builder(setter(into, strip_option), default)]
     api_key: Option<String>,
-    /// JWT token used to callnotary server endpoints if JWT authorization is
+    /// JWT token used to call notary server endpoints if JWT authorization is
     /// enabled in notary server.
     #[builder(setter(into, strip_option), default)]
     jwt: Option<String>,

crates/notary/client/src/client.rs

@@ -48,6 +48,7 @@ serde_json = { workspace = true }
 serde_yaml = { version = "0.9" }
 sha1 = { version = "0.10" }
 structopt = { version = "0.3" }
+strum = { version = "0.27", features = ["derive"] }
 thiserror = { workspace = true }
 tokio = { workspace = true, features = ["full"] }
 tokio-rustls = { workspace = true }

crates/notary/server/Cargo.toml

@@ -213,7 +213,7 @@ An optional authorization module is available to only allow requests with a vali
 Please note that only *one* mode can be active at any one time.
 
 ##### Whitelist mode
-In whitelist mode, an API key is attached in the custom HTTP header `X-API-Key`. The API key whitelist path (as well as the flag to enable/disable this module) can be changed in the config (`authorization` field).
+In whitelist mode, a valid API key needs to be attached in the custom HTTP header `X-API-Key`. The path of the API key whitelist, path (as well as the flag to enable/disable this module), can be changed in the config (`authorization` field).
 
 Hot reloading of the whitelist is supported, i.e. modification of the whitelist file will be automatically applied without needing to restart the server. Please take note of the following
 - Avoid using auto save mode when editing the whitelist to prevent spamming hot reloads
@@ -223,6 +223,10 @@ Hot reloading of the whitelist is supported, i.e. modification of the whitelist
 In JWT mode, JSON Web Token is attached in the standard `Authorization` HTTP header as a bearer token. The path to decoding key as well as custom user claims can be changed in the 
 config (`authorization` field).
 
+Care should be taken when defining custom user claims as the middleware will:
+- accept any claim if no custom claim is defined,
+- as long as user defined claims are found, other unknown claims will be ignored.
+
 An example JWT config may look something like this:
 
 ```yaml

crates/notary/server/README.md

@@ -4,31 +4,31 @@ pub(crate) mod whitelist;
 use eyre::{eyre, Result};
 use jwt::load_jwt_key;
 use std::{
-    collections::HashMap,
+    str::FromStr,
     sync::{Arc, Mutex},
 };
 use tracing::debug;
 use whitelist::load_authorization_whitelist;
 
-pub use jwt::{Jwt, JwtError};
-pub use whitelist::{watch_and_reload_authorization_whitelist, AuthorizationWhitelistRecord};
+pub use jwt::{Algorithm, Jwt, JwtValidationError};
+pub use whitelist::{
+    watch_and_reload_authorization_whitelist, AuthorizationWhitelistRecord, Whitelist,
+};
 
 use crate::{AuthorizationModeProperties, NotaryServerProperties};
 
 /// Supported authorization modes.
 #[derive(Clone)]
 pub enum AuthorizationMode {
     Jwt(Jwt),
-    Whitelist(Arc<Mutex<HashMap<String, AuthorizationWhitelistRecord>>>),
+    Whitelist(Whitelist),
 }
 
 impl AuthorizationMode {
-    pub fn as_whitelist(
-        &self,
-    ) -> Option<Arc<Mutex<HashMap<String, AuthorizationWhitelistRecord>>>> {
+    pub fn as_whitelist(&self) -> Option<&Whitelist> {
         match self {
             Self::Jwt(..) => None,
-            Self::Whitelist(whitelist) => Some(whitelist.clone()),
+            Self::Whitelist(whitelist) => Some(whitelist),
         }
     }
 }
@@ -44,11 +44,16 @@ pub async fn load_authorization_mode(
 
     let auth_mode = match config.auth.mode.as_ref().ok_or_else(|| {
         eyre!(
-            "Authorization enabled but neither whitelist nor jwt properties provided in the config"
+            "Authorization enabled but failed to load either whitelist or jwt properties. They are either absent or malformed."
         )
     })? {
         AuthorizationModeProperties::Jwt(jwt_opts) => {
-            let algorithm = jwt_opts.algorithm;
+            let algorithm = Algorithm::from_str(&jwt_opts.algorithm).map_err(|_| {
+                eyre!(
+                    "Unexpected JWT signing algorithm specified: '{}'",
+                    jwt_opts.algorithm
+                )
+            })?;
             let claims = jwt_opts.claims.clone();
             let key = load_jwt_key(&jwt_opts.public_key_path, algorithm).await?;
             AuthorizationMode::Jwt(Jwt {
@@ -58,8 +63,11 @@ pub async fn load_authorization_mode(
             })
         }
         AuthorizationModeProperties::Whitelist(whitelist_csv_path) => {
-            let whitelist = load_authorization_whitelist(whitelist_csv_path)?;
-            AuthorizationMode::Whitelist(Arc::new(Mutex::new(whitelist)))
+            let entries = load_authorization_whitelist(whitelist_csv_path)?;
+            AuthorizationMode::Whitelist(Whitelist {
+                entries: Arc::new(Mutex::new(entries)),
+                csv_path: whitelist_csv_path.clone(),
+            })
         }
     };
 

crates/notary/server/src/auth.rs

@@ -1,21 +1,17 @@
 use eyre::Result;
-use jsonwebtoken::{Algorithm, DecodingKey};
+use jsonwebtoken::{Algorithm as JwtAlgorithm, DecodingKey};
 use serde_json::Value;
-use std::io::Read;
+use strum::EnumString;
 use tracing::error;
 
-use crate::{read_pem_file, JwtClaim, JwtClaimValueType};
+use crate::{JwtClaim, JwtClaimValueType};
 
 /// Custom error for JWT handling
 #[derive(Debug, thiserror::Error, PartialEq)]
-pub enum JwtError {
-    #[error("unsupported algorithm: {0:?}")]
-    UnsupportedAlgorithm(Algorithm),
-    #[error("JWT validation error: {0}")]
-    Validation(String),
-}
+#[error("JWT validation error: {0}")]
+pub struct JwtValidationError(String);
 
-type JwtResult<T> = std::result::Result<T, JwtError>;
+type JwtResult<T> = std::result::Result<T, JwtValidationError>;
 
 /// JWT config which also encapsulates claims validation logic.
 #[derive(Clone)]
@@ -26,32 +22,33 @@ pub struct Jwt {
 }
 
 impl Jwt {
-    pub fn validate(&self, claims: Value) -> JwtResult<()> {
+    pub fn validate(&self, claims: &Value) -> JwtResult<()> {
         Jwt::validate_claims(&self.claims, claims)
     }
 
-    fn validate_claims(expected: &[JwtClaim], claims: Value) -> JwtResult<()> {
+    fn validate_claims(expected: &[JwtClaim], claims: &Value) -> JwtResult<()> {
         expected
             .iter()
-            .try_for_each(|expected| Self::validate_claim(expected, claims.clone()))
+            .try_for_each(|expected| Self::validate_claim(expected, claims))
     }
 
-    fn validate_claim(expected: &JwtClaim, given: Value) -> JwtResult<()> {
-        let field = Jwt::get_field(&expected.name, &given).ok_or(JwtError::Validation(format!(
+    fn validate_claim(expected: &JwtClaim, given: &Value) -> JwtResult<()> {
+        let pointer = format!("/{}", expected.name.replace(".", "/"));
+        let field = given.pointer(&pointer).ok_or(JwtValidationError(format!(
             "missing claim '{}'",
             expected.name
         )))?;
 
         match expected.value_type {
             JwtClaimValueType::String => {
-                let field_typed = field.as_str().ok_or(JwtError::Validation(format!(
+                let field_typed = field.as_str().ok_or(JwtValidationError(format!(
                     "unexpected type for claim '{}': expected '{:?}'",
                     expected.name, expected.value_type
                 )))?;
                 if !expected.values.is_empty() {
                     expected.values.iter().any(|exp| exp == field_typed).then_some(()).ok_or_else(|| {
                         let expected_values = expected.values.iter().map(|x| format!("'{x}'")).collect::<Vec<String>>().join(", ");
-                        JwtError::Validation(format!(
+                        JwtValidationError(format!(
                             "unexpected value for claim '{}': expected one of [ {expected_values} ], received '{field_typed}'", expected.name
                         ))
                     })?;
@@ -61,19 +58,44 @@ impl Jwt {
 
         Ok(())
     }
+}
 
-    fn get_field<'a>(path: &'a str, value: &'a Value) -> Option<&'a Value> {
-        let (field, path) = match path.split_once('.') {
-            Some((field, path)) => (field, Some(path)),
-            None => (path, None),
-        };
-        if let Some(value) = value.get(field) {
-            match path {
-                Some(path) => Jwt::get_field(path, value),
-                None => Some(value),
-            }
-        } else {
-            None
+#[derive(EnumString, Debug, Clone, Copy, PartialEq, Eq)]
+#[strum(ascii_case_insensitive)]
+/// Supported JWT signing algorithms
+pub enum Algorithm {
+    /// RSASSA-PSS using SHA-512
+    RS256,
+    /// RSASSA-PKCS1-v1_5 using SHA-384
+    RS384,
+    /// RSASSA-PKCS1-v1_5 using SHA-384
+    RS512,
+    /// RSASSA-PSS using SHA-256
+    PS256,
+    /// RSASSA-PSS using SHA-384
+    PS384,
+    /// RSASSA-PSS using SHA-512
+    PS512,
+    /// ECDSA using SHA-256
+    ES256,
+    /// ECDSA using SHA-384
+    ES384,
+    /// Edwards-curve Digital Signature Algorithm (EdDSA)
+    EdDSA,
+}
+
+impl From<Algorithm> for JwtAlgorithm {
+    fn from(value: Algorithm) -> Self {
+        match value {
+            Algorithm::RS256 => Self::RS256,
+            Algorithm::RS384 => Self::RS384,
+            Algorithm::RS512 => Self::RS512,
+            Algorithm::PS256 => Self::PS256,
+            Algorithm::PS384 => Self::PS384,
+            Algorithm::PS512 => Self::PS512,
+            Algorithm::ES256 => Self::ES256,
+            Algorithm::ES384 => Self::ES384,
+            Algorithm::EdDSA => Self::EdDSA,
         }
     }
 }
@@ -83,9 +105,7 @@ pub(super) async fn load_jwt_key(
     public_key_pem_path: &str,
     algorithm: Algorithm,
 ) -> Result<DecodingKey> {
-    let mut reader = read_pem_file(public_key_pem_path).await?;
-    let mut key_pem_bytes: Vec<u8> = Vec::new();
-    reader.read_to_end(&mut key_pem_bytes)?;
+    let key_pem_bytes = tokio::fs::read(public_key_pem_path).await?;
     let key = match algorithm {
         Algorithm::RS256
         | Algorithm::RS384
@@ -95,7 +115,6 @@ pub(super) async fn load_jwt_key(
         | Algorithm::PS512 => DecodingKey::from_rsa_pem(&key_pem_bytes)?,
         Algorithm::ES256 | Algorithm::ES384 => DecodingKey::from_ec_pem(&key_pem_bytes)?,
         Algorithm::EdDSA => DecodingKey::from_ed_pem(&key_pem_bytes)?,
-        _ => return Err(JwtError::UnsupportedAlgorithm(algorithm).into()),
     };
     Ok(key)
 }
@@ -116,7 +135,7 @@ mod test {
             "exp": 12345,
             "sub": "test",
         });
-        assert!(Jwt::validate_claim(&expected, given).is_ok());
+        assert!(Jwt::validate_claim(&expected, &given).is_ok());
     }
 
     #[test]
@@ -132,7 +151,7 @@ mod test {
                 "host": "api.tlsn.com",
             },
         });
-        assert!(Jwt::validate_claim(&expected, given).is_ok())
+        assert!(Jwt::validate_claim(&expected, &given).is_ok())
     }
 
     #[test]
@@ -142,7 +161,7 @@ mod test {
             "sub": "test",
             "what": "is_this",
         });
-        assert!(Jwt::validate_claims(&[], given).is_ok())
+        assert!(Jwt::validate_claims(&[], &given).is_ok())
     }
 
     #[test]
@@ -156,8 +175,8 @@ mod test {
             "host": "localhost",
         });
         assert_eq!(
-            Jwt::validate_claim(&expected, given),
-            Err(JwtError::Validation("missing claim 'sub'".to_string()))
+            Jwt::validate_claim(&expected, &given),
+            Err(JwtValidationError("missing claim 'sub'".to_string()))
         )
     }
 
@@ -172,8 +191,8 @@ mod test {
             "sub": "tlsn",
         });
         assert_eq!(
-                Jwt::validate_claim(&expected, given),
-                Err(JwtError::Validation("unexpected value for claim 'sub': expected one of [ 'tlsn_prod', 'tlsn_test' ], received 'tlsn'".to_string()))
+                Jwt::validate_claim(&expected, &given),
+                Err(JwtValidationError("unexpected value for claim 'sub': expected one of [ 'tlsn_prod', 'tlsn_test' ], received 'tlsn'".to_string()))
             )
     }
 }