fix: Improve fernet keys loading

benches/fernet_token.rs

@@ -1,7 +1,6 @@
 use criterion::{BenchmarkId, Criterion, criterion_group, criterion_main};
 use std::fs::File;
 use std::io::Write;
-use std::path::PathBuf;
 use tempfile::tempdir;
 
 use openstack_keystone::config::Config;
@@ -19,8 +18,14 @@ fn bench_decrypt_token(c: &mut Criterion) {
     let mut tmp_file = File::create(file_path).unwrap();
     write!(tmp_file, "BFTs1CIVIBLTP4GOrQ26VETrJ7Zwz1O4wbEcCQ966eM=").unwrap();
 
+    let builder = config::Config::builder()
+        .set_override("auth.methods", "password,token")
+        .unwrap()
+        .set_override("database.connection", "dummy")
+        .unwrap();
+    let mut config: Config = Config::try_from(builder).expect("can build a valid config");
     let mut backend = FernetTokenProvider::default();
-    let mut config = Config::new(PathBuf::new()).unwrap();
+
     config.fernet_tokens.key_repository = tmp_dir.keep();
     backend.set_config(config);
     backend.load_keys().unwrap();

src/config.rs

@@ -30,8 +30,7 @@ pub struct Config {
     #[serde(default)]
     pub assignment: AssignmentSection,
 
-    /// Auth
-    #[serde(default)]
+    /// Authentication configuration.
     pub auth: AuthSection,
 
     /// Catalog
@@ -46,7 +45,7 @@ pub struct Config {
     pub fernet_tokens: FernetTokenSection,
 
     /// Database configuration
-    #[serde(default)]
+    //#[serde(default)]
     pub database: DatabaseSection,
 
     /// Identity provider related configuration
@@ -82,8 +81,10 @@ pub struct DefaultSection {
     pub public_endpoint: Option<String>,
 }
 
+/// Authentication configuration.
 #[derive(Debug, Default, Deserialize, Clone)]
 pub struct AuthSection {
+    /// Authentication methods to be enabled and used for token validation.
     #[serde(deserialize_with = "csv")]
     pub methods: Vec<String>,
 }
@@ -213,6 +214,20 @@ impl Config {
     pub fn new(path: PathBuf) -> Result<Self, Report> {
         let mut builder = config::Config::builder();
 
+        if std::path::Path::new(&path).is_file() {
+            builder = builder.add_source(File::from(path).format(FileFormat::Ini));
+        }
+
+        builder.try_into()
+    }
+}
+
+impl TryFrom<config::ConfigBuilder<config::builder::DefaultState>> for Config {
+    type Error = Report;
+    fn try_from(
+        builder: config::ConfigBuilder<config::builder::DefaultState>,
+    ) -> Result<Self, Self::Error> {
+        let mut builder = builder;
         builder = builder
             .set_default("api_policy.enable", "true")?
             .set_default("api_policy.opa_base_url", "http://localhost:8181")?
@@ -224,9 +239,6 @@ impl Config {
             .set_default("federation.driver", "sql")?
             .set_default("resource.driver", "sql")?
             .set_default("token.expiration", "3600")?;
-        if std::path::Path::new(&path).is_file() {
-            builder = builder.add_source(File::from(path).format(FileFormat::Ini));
-        }
 
         builder
             .build()

src/identity/password_hashing.rs

@@ -61,7 +61,6 @@ pub fn verify_password<P: AsRef<[u8]>, H: AsRef<str>>(
 #[cfg(test)]
 mod tests {
     use super::*;
-    use std::path::PathBuf;
 
     #[test]
     fn test_verify_length_and_trunc_password() {
@@ -82,7 +81,12 @@ mod tests {
 
     #[test]
     fn test_hash_bcrypt() {
-        let conf = Config::new(PathBuf::new()).unwrap();
+        let builder = config::Config::builder()
+            .set_override("auth.methods", "")
+            .unwrap()
+            .set_override("database.connection", "dummy")
+            .unwrap();
+        let conf: Config = Config::try_from(builder).expect("can build a valid config");
         assert!(hash_password(&conf, "abcdefg").is_ok());
     }
 }

src/tests/token.rs

@@ -14,7 +14,6 @@
 
 use std::fs::File;
 use std::io::Write;
-use std::path::PathBuf;
 use tempfile::tempdir;
 
 use crate::config::Config;
@@ -23,15 +22,18 @@ pub fn setup_config() -> Config {
     let keys_dir = tempdir().unwrap();
     // write fernet key used to generate tokens in python
     let file_path = keys_dir.path().join("0");
-    let mut tmp_file = File::create(file_path).unwrap();
+    let mut tmp_file = File::create(&file_path).unwrap();
     write!(tmp_file, "BFTs1CIVIBLTP4GOrQ26VETrJ7Zwz1O4wbEcCQ966eM=").unwrap();
-    let mut config = Config::new(PathBuf::new()).unwrap();
+
+    let builder = config::Config::builder()
+        .set_override(
+            "auth.methods",
+            "password,token,openid,application_credential",
+        )
+        .unwrap()
+        .set_override("database.connection", "dummy")
+        .unwrap();
+    let mut config: Config = Config::try_from(builder).expect("can build a valid config");
     config.fernet_tokens.key_repository = keys_dir.keep();
-    config.auth.methods = vec![
-        "password".into(),
-        "token".into(),
-        "openid".into(),
-        "application_credential".into(),
-    ];
     config
 }

src/token/error.rs

@@ -46,7 +46,7 @@ pub enum TokenProviderError {
     },
 
     /// Missing fernet keys
-    #[error("missing fernet keys")]
+    #[error("no usable fernet keys has been found")]
     FernetKeysMissing,
 
     /// Invalid token data

src/token/fernet.rs

@@ -13,7 +13,7 @@
 // SPDX-License-Identifier: Apache-2.0
 
 use bytes::Bytes;
-use fernet::{Fernet, MultiFernet};
+use fernet::MultiFernet;
 use rmp::{
     Marker,
     decode::{ValueReadError, read_marker, read_u8},
@@ -206,11 +206,7 @@ impl FernetTokenProvider {
     /// Get MultiFernet initialized with repository keys
     pub fn get_fernet(&self) -> Result<MultiFernet, TokenProviderError> {
         Ok(MultiFernet::new(
-            self.utils
-                .load_keys()?
-                .into_iter()
-                .filter_map(|x| Fernet::new(&x))
-                .collect::<Vec<_>>(),
+            self.utils.load_keys()?.into_iter().collect::<Vec<_>>(),
         ))
     }
 
@@ -280,7 +276,6 @@ pub(super) mod tests {
     use chrono::{Local, SubsecRound};
     use std::fs::File;
     use std::io::Write;
-    use std::path::PathBuf;
     use tempfile::tempdir;
     use uuid::Uuid;
 
@@ -290,14 +285,17 @@ pub(super) mod tests {
         let file_path = keys_dir.path().join("0");
         let mut tmp_file = File::create(file_path).unwrap();
         write!(tmp_file, "BFTs1CIVIBLTP4GOrQ26VETrJ7Zwz1O4wbEcCQ966eM=").unwrap();
-        let mut config = Config::new(PathBuf::new()).unwrap();
+
+        let builder = config::Config::builder()
+            .set_override(
+                "auth.methods",
+                "password,token,openid,application_credential",
+            )
+            .unwrap()
+            .set_override("database.connection", "dummy")
+            .unwrap();
+        let mut config: Config = Config::try_from(builder).expect("can build a valid config");
         config.fernet_tokens.key_repository = keys_dir.keep();
-        config.auth.methods = vec![
-            "password".into(),
-            "token".into(),
-            "openid".into(),
-            "application_credential".into(),
-        ];
         config
     }
 

src/token/fernet_utils.rs

@@ -14,6 +14,7 @@
 
 use base64::{Engine as _, engine::general_purpose::URL_SAFE};
 use chrono::{DateTime, Utc};
+use fernet::Fernet;
 use rmp::{
     Marker,
     decode::{self, *},
@@ -25,7 +26,7 @@ use std::io;
 use std::io::Read;
 use std::path::PathBuf;
 use tokio::fs as fs_async;
-use tracing::trace;
+use tracing::{trace, warn};
 use uuid::Uuid;
 
 use crate::token::error::TokenProviderError;
@@ -41,53 +42,73 @@ impl FernetUtils {
         Ok(self.key_repository.exists())
     }
 
-    pub fn load_keys(
-        &self,
-    ) -> Result<impl IntoIterator<Item = String> + use<>, TokenProviderError> {
-        let mut keys: BTreeMap<i8, String> = BTreeMap::new();
+    pub fn load_keys(&self) -> Result<impl IntoIterator<Item = Fernet>, TokenProviderError> {
+        let mut keys: BTreeMap<i8, Fernet> = BTreeMap::new();
         if self.validate_key_repository()? {
             for entry in fs::read_dir(&self.key_repository)? {
                 let entry = entry?;
                 if let Ok(fname) = entry.file_name().into_string() {
                     if let Ok(key_order) = fname.parse::<i8>() {
                         // We are only interested in files named as integer (0, 1, 2, ...)
                         trace!("Loading key {:?}", entry.file_name());
-                        let key = fs::read_to_string(entry.path()).map_err(|e| {
-                            TokenProviderError::FernetKeyRead {
-                                source: e,
-                                path: entry.path(),
-                            }
-                        })?;
-                        keys.insert(key_order, key);
+                        if let Some(fernet) = Fernet::new(
+                            fs::read_to_string(entry.path())
+                                .map_err(|e| TokenProviderError::FernetKeyRead {
+                                    source: e,
+                                    path: entry.path(),
+                                })?
+                                .trim_end(),
+                        ) {
+                            keys.insert(key_order, fernet);
+                        } else {
+                            warn!(
+                                "The key {:?} is not usable for Fernet library",
+                                entry.file_name()
+                            )
+                        }
                     }
                 }
             }
         }
+        if keys.len() == 0 {
+            return Err(TokenProviderError::FernetKeysMissing);
+        }
         Ok(keys.into_values().rev())
     }
 
     pub async fn load_keys_async(
         &self,
-    ) -> Result<impl IntoIterator<Item = String> + use<>, TokenProviderError> {
-        let mut keys: BTreeMap<i8, String> = BTreeMap::new();
+    ) -> Result<impl IntoIterator<Item = Fernet>, TokenProviderError> {
+        let mut keys: BTreeMap<i8, Fernet> = BTreeMap::new();
         if self.validate_key_repository()? {
             let mut entries = fs_async::read_dir(&self.key_repository).await?;
             while let Some(entry) = entries.next_entry().await? {
                 if let Ok(fname) = entry.file_name().into_string() {
                     if let Ok(key_order) = fname.parse::<i8>() {
                         // We are only interested in files named as integer (0, 1, 2, ...)
                         trace!("Loading key {:?}", entry.file_name());
-                        let key = fs::read_to_string(entry.path()).map_err(|e| {
-                            TokenProviderError::FernetKeyRead {
-                                source: e,
-                                path: entry.path(),
-                            }
-                        })?;
-                        keys.insert(key_order, key);
+                        if let Some(fernet) = Fernet::new(
+                            fs::read_to_string(entry.path())
+                                .map_err(|e| TokenProviderError::FernetKeyRead {
+                                    source: e,
+                                    path: entry.path(),
+                                })?
+                                .trim_end(),
+                        ) {
+                            keys.insert(key_order, fernet);
+                        } else {
+                            warn!(
+                                "The key {:?} is not usable for Fernet library",
+                                entry.file_name()
+                            )
+                        }
                     }
                 }
             }
         }
+        if keys.len() == 0 {
+            return Err(TokenProviderError::FernetKeysMissing);
+        }
         Ok(keys.into_values().rev())
     }
 }
@@ -298,7 +319,23 @@ mod tests {
     use super::*;
 
     #[tokio::test]
-    async fn test_load_keys() {
+    async fn test_load_keys_valid() {
+        let tmp_dir = tempdir().unwrap();
+        for i in 0..5 {
+            let file_path = tmp_dir.path().join(format!("{i}"));
+            let mut tmp_file = File::create(file_path).unwrap();
+            write!(tmp_file, "{}", Fernet::generate_key()).unwrap();
+        }
+        let utils = FernetUtils {
+            key_repository: tmp_dir.keep(),
+            ..Default::default()
+        };
+        let keys: Vec<Fernet> = utils.load_keys().unwrap().into_iter().collect();
+        assert_eq!(5, keys.len());
+    }
+
+    #[tokio::test]
+    async fn test_load_keys_all_invalid() {
         let tmp_dir = tempdir().unwrap();
         for i in 0..5 {
             let file_path = tmp_dir.path().join(format!("{i}"));
@@ -314,18 +351,28 @@ mod tests {
             key_repository: tmp_dir.keep(),
             ..Default::default()
         };
-        let keys: Vec<String> = utils.load_keys_async().await.unwrap().into_iter().collect();
-
-        assert_eq!(
-            vec![
-                "4".to_string(),
-                "3".to_string(),
-                "2".to_string(),
-                "1".to_string(),
-                "0".to_string()
-            ],
-            keys
-        );
+        let res = utils.load_keys();
+
+        if let Err(TokenProviderError::FernetKeysMissing) = res {
+        } else {
+            panic!("Should have raised an exception");
+        }
+    }
+
+    #[tokio::test]
+    async fn test_load_keys_trim() {
+        let tmp_dir = tempdir().unwrap();
+        for i in 0..5 {
+            let file_path = tmp_dir.path().join(format!("{i}"));
+            let mut tmp_file = File::create(file_path).unwrap();
+            write!(tmp_file, "{}\n", Fernet::generate_key()).unwrap();
+        }
+        let utils = FernetUtils {
+            key_repository: tmp_dir.keep(),
+            ..Default::default()
+        };
+        let keys: Vec<Fernet> = utils.load_keys().unwrap().into_iter().collect();
+        assert_eq!(5, keys.len());
     }
 
     #[test]