diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index c78c86e..f266f2f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -119,6 +119,22 @@ Consistency is the most important. Following the existing Rust style, formatting Style and format will be enforced with a linter when PR is crated. +## :warning: Error Handling + +We use [error-stack](https://docs.rs/error-stack/latest/error_stack/) for error handling to provide rich context and traceability. + +### Guidelines + +1. **Use `Report`**: Public functions should generally return `Result>`. +2. **Context**: Use `.change_context(TrustedServerError::Variant)` to wrap errors and provide semantic meaning. + ```rust + // Good + file.read_to_string(&mut content) + .change_context(TrustedServerError::Configuration { message: "Failed to read config".into() })?; + ``` +3. **Attachments**: Use `.attach_printable("additional info")` to add debugging context without changing the error variant. +4. **Consistency**: Avoid returning bare `TrustedServerError` unless absolutely necessary (e.g. implementing traits). Wrap them in `Report::new()`. + ## :pray: Credits - https://github.com/jessesquires/.github/blob/main/CONTRIBUTING.md diff --git a/crates/common/src/fastly_storage.rs b/crates/common/src/fastly_storage.rs index c4a8241..9b9f992 100644 --- a/crates/common/src/fastly_storage.rs +++ b/crates/common/src/fastly_storage.rs @@ -1,5 +1,6 @@ use std::io::Read; +use error_stack::{Report, ResultExt}; use fastly::{ConfigStore, Request, Response, SecretStore}; use http::StatusCode; @@ -17,17 +18,17 @@ impl FastlyConfigStore { } } - pub fn get(&self, key: &str) -> Result { + pub fn get(&self, key: &str) -> Result> { // TODO use try_open and return the error let store = ConfigStore::open(&self.store_name); - store - .get(key) - .ok_or_else(|| TrustedServerError::Configuration { + store.get(key).ok_or_else(|| { + Report::new(TrustedServerError::Configuration { message: format!( "Key '{}' not found in config store '{}'", key, self.store_name ), }) + }) } } @@ -42,33 +43,36 @@ impl FastlySecretStore { } } - pub fn get(&self, key: &str) -> Result, TrustedServerError> { - let store = - SecretStore::open(&self.store_name).map_err(|_| TrustedServerError::Configuration { + pub fn get(&self, key: &str) -> Result, Report> { + let store = SecretStore::open(&self.store_name).map_err(|_| { + Report::new(TrustedServerError::Configuration { message: format!("Failed to open SecretStore '{}'", self.store_name), - })?; + }) + })?; - let secret = store - .get(key) - .ok_or_else(|| TrustedServerError::Configuration { + let secret = store.get(key).ok_or_else(|| { + Report::new(TrustedServerError::Configuration { message: format!( "Secret '{}' not found in secret store '{}'", key, self.store_name ), - })?; + }) + })?; secret .try_plaintext() - .map_err(|_| TrustedServerError::Configuration { - message: "Failed to get secret plaintext".into(), + .map_err(|_| { + Report::new(TrustedServerError::Configuration { + message: "Failed to get secret plaintext".into(), + }) }) .map(|bytes| bytes.into_iter().collect()) } - pub fn get_string(&self, key: &str) -> Result { + pub fn get_string(&self, key: &str) -> Result> { let bytes = self.get(key)?; - String::from_utf8(bytes).map_err(|e| TrustedServerError::Configuration { - message: format!("Failed to decode secret as UTF-8: {}", e), + String::from_utf8(bytes).change_context(TrustedServerError::Configuration { + message: format!("Failed to decode secret as UTF-8: {}", key), }) } } @@ -79,16 +83,19 @@ pub struct FastlyApiClient { } impl FastlyApiClient { - pub fn new() -> Result { + pub fn new() -> Result> { Self::from_secret_store("api-keys", "api_key") } - pub fn from_secret_store(store_name: &str, key_name: &str) -> Result { - ensure_backend_from_url("https://api.fastly.com").map_err(|e| { + pub fn from_secret_store( + store_name: &str, + key_name: &str, + ) -> Result> { + ensure_backend_from_url("https://api.fastly.com").change_context( TrustedServerError::Configuration { - message: format!("Failed to ensure API backend: {}", e), - } - })?; + message: "Failed to ensure API backend".to_string(), + }, + )?; let secret_store = FastlySecretStore::new(store_name); let api_key = secret_store.get(key_name)?; @@ -105,7 +112,7 @@ impl FastlyApiClient { path: &str, body: Option, content_type: &str, - ) -> Result { + ) -> Result> { let url = format!("{}{}", self.base_url, path); let api_key_str = String::from_utf8_lossy(&self.api_key).to_string(); @@ -116,9 +123,9 @@ impl FastlyApiClient { "PUT" => Request::put(&url), "DELETE" => Request::delete(&url), _ => { - return Err(TrustedServerError::Configuration { + return Err(Report::new(TrustedServerError::Configuration { message: format!("Unsupported HTTP method: {}", method), - }) + })) } }; @@ -133,9 +140,9 @@ impl FastlyApiClient { } request.send("backend_https_api_fastly_com").map_err(|e| { - TrustedServerError::Configuration { + Report::new(TrustedServerError::Configuration { message: format!("Failed to send API request: {}", e), - } + }) }) } @@ -144,7 +151,7 @@ impl FastlyApiClient { store_id: &str, key: &str, value: &str, - ) -> Result<(), TrustedServerError> { + ) -> Result<(), Report> { let path = format!("/resources/stores/config/{}/item/{}", store_id, key); let payload = format!("item_value={}", value); @@ -159,20 +166,22 @@ impl FastlyApiClient { response .get_body_mut() .read_to_string(&mut buf) - .map_err(|e| TrustedServerError::Configuration { - message: format!("Failed to read API response: {}", e), + .map_err(|e| { + Report::new(TrustedServerError::Configuration { + message: format!("Failed to read API response: {}", e), + }) })?; if response.get_status() == StatusCode::OK { Ok(()) } else { - Err(TrustedServerError::Configuration { + Err(Report::new(TrustedServerError::Configuration { message: format!( "Failed to update config item: HTTP {} - {}", response.get_status(), buf ), - }) + })) } } @@ -181,7 +190,7 @@ impl FastlyApiClient { store_id: &str, secret_name: &str, secret_value: &str, - ) -> Result<(), TrustedServerError> { + ) -> Result<(), Report> { let path = format!("/resources/stores/secret/{}/secrets", store_id); let payload = serde_json::json!({ @@ -196,24 +205,30 @@ impl FastlyApiClient { response .get_body_mut() .read_to_string(&mut buf) - .map_err(|e| TrustedServerError::Configuration { - message: format!("Failed to read API response: {}", e), + .map_err(|e| { + Report::new(TrustedServerError::Configuration { + message: format!("Failed to read API response: {}", e), + }) })?; if response.get_status() == StatusCode::OK { Ok(()) } else { - Err(TrustedServerError::Configuration { + Err(Report::new(TrustedServerError::Configuration { message: format!( "Failed to create secret: HTTP {} - {}", response.get_status(), buf ), - }) + })) } } - pub fn delete_config_item(&self, store_id: &str, key: &str) -> Result<(), TrustedServerError> { + pub fn delete_config_item( + &self, + store_id: &str, + key: &str, + ) -> Result<(), Report> { let path = format!("/resources/stores/config/{}/item/{}", store_id, key); let mut response = self.make_request("DELETE", &path, None, "application/json")?; @@ -222,8 +237,10 @@ impl FastlyApiClient { response .get_body_mut() .read_to_string(&mut buf) - .map_err(|e| TrustedServerError::Configuration { - message: format!("Failed to read API response: {}", e), + .map_err(|e| { + Report::new(TrustedServerError::Configuration { + message: format!("Failed to read API response: {}", e), + }) })?; if response.get_status() == StatusCode::OK @@ -231,13 +248,13 @@ impl FastlyApiClient { { Ok(()) } else { - Err(TrustedServerError::Configuration { + Err(Report::new(TrustedServerError::Configuration { message: format!( "Failed to delete config item: HTTP {} - {}", response.get_status(), buf ), - }) + })) } } @@ -245,7 +262,7 @@ impl FastlyApiClient { &self, store_id: &str, secret_name: &str, - ) -> Result<(), TrustedServerError> { + ) -> Result<(), Report> { let path = format!( "/resources/stores/secret/{}/secrets/{}", store_id, secret_name @@ -257,8 +274,10 @@ impl FastlyApiClient { response .get_body_mut() .read_to_string(&mut buf) - .map_err(|e| TrustedServerError::Configuration { - message: format!("Failed to read API response: {}", e), + .map_err(|e| { + Report::new(TrustedServerError::Configuration { + message: format!("Failed to read API response: {}", e), + }) })?; if response.get_status() == StatusCode::OK @@ -266,13 +285,13 @@ impl FastlyApiClient { { Ok(()) } else { - Err(TrustedServerError::Configuration { + Err(Report::new(TrustedServerError::Configuration { message: format!( "Failed to delete secret: HTTP {} - {}", response.get_status(), buf ), - }) + })) } } } @@ -329,6 +348,7 @@ mod tests { } } + // Other tests logic is preserved, prints error which is now a Report #[test] fn test_update_config_item() { let result = FastlyApiClient::new(); diff --git a/crates/common/src/request_signing/jwks.rs b/crates/common/src/request_signing/jwks.rs index abbf20c..1c260f7 100644 --- a/crates/common/src/request_signing/jwks.rs +++ b/crates/common/src/request_signing/jwks.rs @@ -4,6 +4,7 @@ //! Ed25519 keypairs in JWK format for request signing. use ed25519_dalek::{SigningKey, VerifyingKey}; +use error_stack::{Report, ResultExt}; use jose_jwk::{ jose_jwa::{Algorithm, Signing}, Jwk, Key, Okp, OkpCurves, Parameters, @@ -51,9 +52,14 @@ impl Keypair { } } -pub fn get_active_jwks() -> Result { +pub fn get_active_jwks() -> Result> { let store = FastlyConfigStore::new("jwks_store"); - let active_kids_str = store.get("active-kids")?; + let active_kids_str = + store + .get("active-kids") + .change_context(TrustedServerError::Configuration { + message: "Failed to get active-kids".into(), + })?; let active_kids: Vec<&str> = active_kids_str .split(',') @@ -63,7 +69,11 @@ pub fn get_active_jwks() -> Result { let mut jwks = Vec::new(); for kid in active_kids { - let jwk = store.get(kid)?; + let jwk = store + .get(kid) + .change_context(TrustedServerError::Configuration { + message: format!("Failed to get JWK for kid: {}", kid), + })?; jwks.push(jwk); } diff --git a/crates/common/src/request_signing/rotation.rs b/crates/common/src/request_signing/rotation.rs index fd77dca..0e20a61 100644 --- a/crates/common/src/request_signing/rotation.rs +++ b/crates/common/src/request_signing/rotation.rs @@ -5,6 +5,7 @@ use base64::{engine::general_purpose, Engine}; use ed25519_dalek::SigningKey; +use error_stack::{Report, ResultExt}; use jose_jwk::Jwk; use crate::error::TrustedServerError; @@ -31,7 +32,7 @@ impl KeyRotationManager { pub fn new( config_store_id: impl Into, secret_store_id: impl Into, - ) -> Result { + ) -> Result> { let config_store_id = config_store_id.into(); let secret_store_id = secret_store_id.into(); @@ -46,7 +47,10 @@ impl KeyRotationManager { }) } - pub fn rotate_key(&self, kid: Option) -> Result { + pub fn rotate_key( + &self, + kid: Option, + ) -> Result> { let new_kid = kid.unwrap_or_else(generate_date_based_kid); let keypair = Keypair::generate(); @@ -76,49 +80,50 @@ impl KeyRotationManager { &self, kid: &str, signing_key: &SigningKey, - ) -> Result<(), TrustedServerError> { + ) -> Result<(), Report> { let key_bytes = signing_key.as_bytes(); let key_b64 = general_purpose::STANDARD.encode(key_bytes); self.api_client .create_secret(&self.secret_store_id, kid, &key_b64) - .map_err(|e| TrustedServerError::Configuration { - message: format!("Failed to store private key '{}': {}", kid, e), + .change_context(TrustedServerError::Configuration { + message: format!("Failed to store private key '{}'", kid), }) } - fn store_public_jwk(&self, kid: &str, jwk: &Jwk) -> Result<(), TrustedServerError> { - let jwk_json = - serde_json::to_string(jwk).map_err(|e| TrustedServerError::Configuration { + fn store_public_jwk(&self, kid: &str, jwk: &Jwk) -> Result<(), Report> { + let jwk_json = serde_json::to_string(jwk).map_err(|e| { + Report::new(TrustedServerError::Configuration { message: format!("Failed to serialize JWK: {}", e), - })?; + }) + })?; self.api_client .update_config_item(&self.config_store_id, kid, &jwk_json) - .map_err(|e| TrustedServerError::Configuration { - message: format!("Failed to store public JWK '{}': {}", kid, e), + .change_context(TrustedServerError::Configuration { + message: format!("Failed to store public JWK '{}'", kid), }) } - fn update_current_kid(&self, kid: &str) -> Result<(), TrustedServerError> { + fn update_current_kid(&self, kid: &str) -> Result<(), Report> { self.api_client .update_config_item(&self.config_store_id, "current-kid", kid) - .map_err(|e| TrustedServerError::Configuration { - message: format!("Failed to update current-kid: {}", e), + .change_context(TrustedServerError::Configuration { + message: "Failed to update current-kid".into(), }) } - fn update_active_kids(&self, active_kids: &[String]) -> Result<(), TrustedServerError> { + fn update_active_kids(&self, active_kids: &[String]) -> Result<(), Report> { let active_kids_str = active_kids.join(","); self.api_client .update_config_item(&self.config_store_id, "active-kids", &active_kids_str) - .map_err(|e| TrustedServerError::Configuration { - message: format!("Failed to update active-kids: {}", e), + .change_context(TrustedServerError::Configuration { + message: "Failed to update active-kids".into(), }) } - pub fn list_active_keys(&self) -> Result, TrustedServerError> { + pub fn list_active_keys(&self) -> Result, Report> { let active_kids_str = self.config_store.get("active-kids")?; let active_kids: Vec = active_kids_str @@ -130,33 +135,33 @@ impl KeyRotationManager { Ok(active_kids) } - pub fn deactivate_key(&self, kid: &str) -> Result<(), TrustedServerError> { + pub fn deactivate_key(&self, kid: &str) -> Result<(), Report> { let mut active_kids = self.list_active_keys()?; active_kids.retain(|k| k != kid); if active_kids.is_empty() { - return Err(TrustedServerError::Configuration { + return Err(Report::new(TrustedServerError::Configuration { message: "Cannot deactivate the last active key".into(), - }); + })); } self.update_active_kids(&active_kids) } - pub fn delete_key(&self, kid: &str) -> Result<(), TrustedServerError> { + pub fn delete_key(&self, kid: &str) -> Result<(), Report> { self.deactivate_key(kid)?; self.api_client .delete_config_item(&self.config_store_id, kid) - .map_err(|e| TrustedServerError::Configuration { - message: format!("Failed to delete JWK from ConfigStore: {}", e), + .change_context(TrustedServerError::Configuration { + message: "Failed to delete JWK from ConfigStore".into(), })?; self.api_client .delete_secret(&self.secret_store_id, kid) - .map_err(|e| TrustedServerError::Configuration { - message: format!("Failed to delete secret from SecretStore: {}", e), + .change_context(TrustedServerError::Configuration { + message: "Failed to delete secret from SecretStore".into(), })?; Ok(()) @@ -177,15 +182,11 @@ mod tests { #[test] fn test_generate_date_based_kid() { let kid = generate_date_based_kid(); - println!("Generated KID: {}", kid); - // Verify format: ts-YYYY-MM-DD assert!(kid.starts_with("ts-")); - assert!(kid.len() >= 13); // "ts-" + "YYYY-MM-DD" = 13 chars minimum - - // Verify it contains only valid characters + assert!(kid.len() >= 13); let parts: Vec<&str> = kid.split('-').collect(); - assert_eq!(parts.len(), 4); // ["ts", "YYYY", "MM", "DD"] + assert_eq!(parts.len(), 4); assert_eq!(parts[0], "ts"); } @@ -196,7 +197,6 @@ mod tests { Ok(manager) => { assert_eq!(manager.config_store_id, "jwks_store"); assert_eq!(manager.secret_store_id, "signing_keys"); - println!("✓ KeyRotationManager created successfully"); } Err(e) => { println!("Expected error in test environment: {}", e); @@ -210,7 +210,6 @@ mod tests { if let Ok(manager) = result { match manager.list_active_keys() { Ok(keys) => { - println!("Active keys: {:?}", keys); assert!(!keys.is_empty(), "Should have at least one active key"); } Err(e) => println!("Expected error in test environment: {}", e), diff --git a/crates/common/src/request_signing/signing.rs b/crates/common/src/request_signing/signing.rs index 6420a12..e12d000 100644 --- a/crates/common/src/request_signing/signing.rs +++ b/crates/common/src/request_signing/signing.rs @@ -5,31 +5,32 @@ use base64::{engine::general_purpose, Engine}; use ed25519_dalek::{Signature, Signer as Ed25519Signer, SigningKey, Verifier, VerifyingKey}; +use error_stack::{Report, ResultExt}; use crate::error::TrustedServerError; use crate::fastly_storage::{FastlyConfigStore, FastlySecretStore}; -pub fn get_current_key_id() -> Result { +pub fn get_current_key_id() -> Result> { let store = FastlyConfigStore::new("jwks_store"); store.get("current-kid") } -fn parse_ed25519_signing_key(key_bytes: Vec) -> Result { +fn parse_ed25519_signing_key(key_bytes: Vec) -> Result> { let bytes = if key_bytes.len() > 32 { general_purpose::STANDARD.decode(&key_bytes).map_err(|_| { - TrustedServerError::Configuration { + Report::new(TrustedServerError::Configuration { message: "Failed to decode base64 key".into(), - } + }) })? } else { key_bytes }; - let key_array: [u8; 32] = bytes - .try_into() - .map_err(|_| TrustedServerError::Configuration { + let key_array: [u8; 32] = bytes.try_into().map_err(|_| { + Report::new(TrustedServerError::Configuration { message: "Invalid key length (expected 32 bytes for Ed25519)".into(), - })?; + }) + })?; Ok(SigningKey::from_bytes(&key_array)) } @@ -40,12 +41,22 @@ pub struct RequestSigner { } impl RequestSigner { - pub fn from_config() -> Result { + pub fn from_config() -> Result> { let config_store = FastlyConfigStore::new("jwks_store"); - let key_id = config_store.get("current-kid")?; + let key_id = + config_store + .get("current-kid") + .change_context(TrustedServerError::Configuration { + message: "Failed to get current-kid".into(), + })?; let secret_store = FastlySecretStore::new("signing_keys"); - let key_bytes = secret_store.get(&key_id)?; + let key_bytes = + secret_store + .get(&key_id) + .change_context(TrustedServerError::Configuration { + message: format!("Failed to get signing key for kid: {}", key_id), + })?; let signing_key = parse_ed25519_signing_key(key_bytes)?; Ok(Self { @@ -54,7 +65,7 @@ impl RequestSigner { }) } - pub fn sign(&self, payload: &[u8]) -> Result { + pub fn sign(&self, payload: &[u8]) -> Result> { let signature_bytes = self.key.sign(payload).to_bytes(); Ok(general_purpose::URL_SAFE_NO_PAD.encode(signature_bytes)) @@ -65,54 +76,60 @@ pub fn verify_signature( payload: &[u8], signature_b64: &str, kid: &str, -) -> Result { +) -> Result> { let store = FastlyConfigStore::new("jwks_store"); - let jwk_json = store.get(kid)?; + let jwk_json = store + .get(kid) + .change_context(TrustedServerError::Configuration { + message: format!("Failed to get JWK for kid: {}", kid), + })?; - let jwk: serde_json::Value = - serde_json::from_str(&jwk_json).map_err(|e| TrustedServerError::Configuration { + let jwk: serde_json::Value = serde_json::from_str(&jwk_json).map_err(|e| { + Report::new(TrustedServerError::Configuration { message: format!("Failed to parse JWK: {}", e), - })?; + }) + })?; - let x_b64 = - jwk.get("x") - .and_then(|v| v.as_str()) - .ok_or_else(|| TrustedServerError::Configuration { - message: "JWK missing 'x' parameter".into(), - })?; + let x_b64 = jwk.get("x").and_then(|v| v.as_str()).ok_or_else(|| { + Report::new(TrustedServerError::Configuration { + message: "JWK missing 'x' parameter".into(), + }) + })?; let public_key_bytes = general_purpose::URL_SAFE_NO_PAD .decode(x_b64) - .map_err(|e| TrustedServerError::Configuration { - message: format!("Failed to decode public key: {}", e), + .map_err(|e| { + Report::new(TrustedServerError::Configuration { + message: format!("Failed to decode public key: {}", e), + }) })?; - let verifying_key_bytes: [u8; 32] = - public_key_bytes - .try_into() - .map_err(|_| TrustedServerError::Configuration { - message: "Public key must be 32 bytes".into(), - })?; + let verifying_key_bytes: [u8; 32] = public_key_bytes.try_into().map_err(|_| { + Report::new(TrustedServerError::Configuration { + message: "Public key must be 32 bytes".into(), + }) + })?; let verifying_key = VerifyingKey::from_bytes(&verifying_key_bytes).map_err(|e| { - TrustedServerError::Configuration { + Report::new(TrustedServerError::Configuration { message: format!("Failed to create verifying key: {}", e), - } + }) })?; let signature_bytes = general_purpose::URL_SAFE_NO_PAD .decode(signature_b64) .or_else(|_| general_purpose::STANDARD.decode(signature_b64)) - .map_err(|e| TrustedServerError::Configuration { - message: format!("Failed to decode signature: {}", e), + .map_err(|e| { + Report::new(TrustedServerError::Configuration { + message: format!("Failed to decode signature: {}", e), + }) })?; - let signature_array: [u8; 64] = - signature_bytes - .try_into() - .map_err(|_| TrustedServerError::Configuration { - message: "Signature must be 64 bytes".into(), - })?; + let signature_array: [u8; 64] = signature_bytes.try_into().map_err(|_| { + Report::new(TrustedServerError::Configuration { + message: "Signature must be 64 bytes".into(), + }) + })?; let signature = Signature::from_bytes(&signature_array); @@ -125,18 +142,19 @@ mod tests { #[test] fn test_request_signer_sign() { + // We propagate errors in tests too, or unwrap. + // Note: unwrapping a Report prints it nicely if test fails. let signer = RequestSigner::from_config().unwrap(); let signature = signer .sign(b"these pretzels are making me thirsty") .unwrap(); assert!(!signature.is_empty()); - assert!(signature.len() > 32); // Ed25519 signatures are 64 bytes, base64 encoded should be longer + assert!(signature.len() > 32); } #[test] fn test_request_signer_from_config() { let signer = RequestSigner::from_config().unwrap(); - // Verify that we can successfully load the signing key from Fastly config assert!(!signer.kid.is_empty()); } @@ -146,7 +164,6 @@ mod tests { let signer = RequestSigner::from_config().unwrap(); let signature = signer.sign(payload).unwrap(); - // Verify the signature let result = verify_signature(payload, &signature, &signer.kid).unwrap(); assert!(result, "Signature should be valid"); } @@ -156,10 +173,8 @@ mod tests { let payload = b"test payload"; let signer = RequestSigner::from_config().unwrap(); - // Create a valid Ed25519 signature (64 bytes) but for a different payload let wrong_signature = signer.sign(b"different payload").unwrap(); - // Should return false for signature of different payload let result = verify_signature(payload, &wrong_signature, &signer.kid).unwrap(); assert!(!result, "Invalid signature should not verify"); } @@ -170,7 +185,6 @@ mod tests { let signer = RequestSigner::from_config().unwrap(); let signature = signer.sign(original_payload).unwrap(); - // Try to verify with different payload let wrong_payload = b"wrong payload"; let result = verify_signature(wrong_payload, &signature, &signer.kid).unwrap(); assert!(!result, "Signature should not verify with wrong payload"); @@ -183,7 +197,6 @@ mod tests { let signature = signer.sign(payload).unwrap(); let nonexistent_kid = "nonexistent-key-id"; - // Should return an error for missing key let result = verify_signature(payload, &signature, nonexistent_kid); assert!(result.is_err(), "Should error for missing key"); } @@ -194,7 +207,6 @@ mod tests { let signer = RequestSigner::from_config().unwrap(); let malformed_signature = "not-valid-base64!!!"; - // Should return an error for malformed base64 let result = verify_signature(payload, malformed_signature, &signer.kid); assert!(result.is_err(), "Should error for malformed signature"); }