From 86d28d4317b36157606e4a0e87ade6af5d0b72b5 Mon Sep 17 00:00:00 2001 From: Artem Goncharov Date: Fri, 10 Oct 2025 16:17:14 +0000 Subject: [PATCH] fix: Improve fernet keys loading - trim trailing line breaks, as those are making keys invalid. - raise explicit exception when no usable keys has been found. - log warning when a key file was not considered usable. Irrelevant: make config.auth section mandatory Since without specifying auth.methods the whole thing is not able to work make the section non-default. --- benches/fernet_token.rs | 9 ++- src/config.rs | 24 +++++-- src/identity/password_hashing.rs | 8 ++- src/tests/token.rs | 20 +++--- src/token/error.rs | 2 +- src/token/fernet.rs | 26 ++++--- src/token/fernet_utils.rs | 115 ++++++++++++++++++++++--------- 7 files changed, 136 insertions(+), 68 deletions(-) diff --git a/benches/fernet_token.rs b/benches/fernet_token.rs index 759df20c..dcb58d1b 100644 --- a/benches/fernet_token.rs +++ b/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(); diff --git a/src/config.rs b/src/config.rs index 14fdd5b9..83bc75de 100644 --- a/src/config.rs +++ b/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, } +/// 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, } @@ -213,6 +214,20 @@ impl Config { pub fn new(path: PathBuf) -> Result { 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> for Config { + type Error = Report; + fn try_from( + builder: config::ConfigBuilder, + ) -> Result { + 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() diff --git a/src/identity/password_hashing.rs b/src/identity/password_hashing.rs index 76925143..3ac67d9b 100644 --- a/src/identity/password_hashing.rs +++ b/src/identity/password_hashing.rs @@ -61,7 +61,6 @@ pub fn verify_password, H: AsRef>( #[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()); } } diff --git a/src/tests/token.rs b/src/tests/token.rs index e8ca1e67..ae9dd425 100644 --- a/src/tests/token.rs +++ b/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 } diff --git a/src/token/error.rs b/src/token/error.rs index f8c19e8b..7f7e6ee6 100644 --- a/src/token/error.rs +++ b/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 diff --git a/src/token/fernet.rs b/src/token/fernet.rs index b300416e..9a29b8b8 100644 --- a/src/token/fernet.rs +++ b/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 { Ok(MultiFernet::new( - self.utils - .load_keys()? - .into_iter() - .filter_map(|x| Fernet::new(&x)) - .collect::>(), + self.utils.load_keys()?.into_iter().collect::>(), )) } @@ -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 } diff --git a/src/token/fernet_utils.rs b/src/token/fernet_utils.rs index 1bf3717a..08c20ede 100644 --- a/src/token/fernet_utils.rs +++ b/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,10 +42,8 @@ impl FernetUtils { Ok(self.key_repository.exists()) } - pub fn load_keys( - &self, - ) -> Result + use<>, TokenProviderError> { - let mut keys: BTreeMap = BTreeMap::new(); + pub fn load_keys(&self) -> Result, TokenProviderError> { + let mut keys: BTreeMap = BTreeMap::new(); if self.validate_key_repository()? { for entry in fs::read_dir(&self.key_repository)? { let entry = entry?; @@ -52,24 +51,35 @@ impl FernetUtils { if let Ok(key_order) = fname.parse::() { // 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 + use<>, TokenProviderError> { - let mut keys: BTreeMap = BTreeMap::new(); + ) -> Result, TokenProviderError> { + let mut keys: BTreeMap = 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? { @@ -77,17 +87,28 @@ impl FernetUtils { if let Ok(key_order) = fname.parse::() { // 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 = 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 = 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 = utils.load_keys().unwrap().into_iter().collect(); + assert_eq!(5, keys.len()); } #[test]