diff --git a/crates/common/build.rs b/crates/common/build.rs index cce3beab..3dddd230 100644 --- a/crates/common/build.rs +++ b/crates/common/build.rs @@ -29,6 +29,11 @@ fn main() { // Merge base TOML with environment variable overrides and write output. // Panics if admin endpoints are not covered by a handler. + // Note: placeholder secret rejection is intentionally NOT done here. + // The base trusted-server.toml ships with placeholder secrets that + // production deployments override via TRUSTED_SERVER__* env vars at + // build time. Runtime startup (get_settings) rejects any remaining + // placeholders so a misconfigured deployment fails fast. let settings = settings::Settings::from_toml_and_env(&toml_content) .expect("Failed to parse settings at build time"); diff --git a/crates/common/src/error.rs b/crates/common/src/error.rs index c5964cfe..0e4a7456 100644 --- a/crates/common/src/error.rs +++ b/crates/common/src/error.rs @@ -33,10 +33,11 @@ pub enum TrustedServerError { #[display("GDPR consent error: {message}")] GdprConsent { message: String }, - /// The synthetic secret key is using the insecure default value. - - #[display("Synthetic secret key is set to the default value - this is insecure")] - InsecureSecretKey, + /// A configuration secret is still set to a known placeholder value. + #[display( + "Configuration field '{field}' is set to a known placeholder value - this is insecure" + )] + InsecureDefault { field: String }, /// Invalid UTF-8 data encountered. #[display("Invalid UTF-8 data: {message}")] @@ -98,7 +99,7 @@ impl IntoHttpResponse for TrustedServerError { Self::Configuration { .. } | Self::Settings { .. } => StatusCode::INTERNAL_SERVER_ERROR, Self::Gam { .. } => StatusCode::BAD_GATEWAY, Self::GdprConsent { .. } => StatusCode::BAD_REQUEST, - Self::InsecureSecretKey => StatusCode::INTERNAL_SERVER_ERROR, + Self::InsecureDefault { .. } => StatusCode::INTERNAL_SERVER_ERROR, Self::InvalidHeaderValue { .. } => StatusCode::BAD_REQUEST, Self::InvalidUtf8 { .. } => StatusCode::BAD_REQUEST, Self::KvStore { .. } => StatusCode::SERVICE_UNAVAILABLE, diff --git a/crates/common/src/settings.rs b/crates/common/src/settings.rs index 536cfcdc..989d5ca0 100644 --- a/crates/common/src/settings.rs +++ b/crates/common/src/settings.rs @@ -23,10 +23,23 @@ pub struct Publisher { pub origin_url: String, /// Secret used to encrypt/decrypt proxied URLs in `/first-party/proxy`. /// Keep this secret stable to allow existing links to decode. + #[validate(length(min = 1))] pub proxy_secret: String, } impl Publisher { + /// Known placeholder values that must not be used in production. + pub const PROXY_SECRET_PLACEHOLDERS: &[&str] = &["change-me-proxy-secret"]; + + /// Returns `true` if `proxy_secret` matches a known placeholder value + /// (case-insensitive). + #[must_use] + pub fn is_placeholder_proxy_secret(proxy_secret: &str) -> bool { + Self::PROXY_SECRET_PLACEHOLDERS + .iter() + .any(|p| p.eq_ignore_ascii_case(proxy_secret)) + } + /// Extracts the host (including port if present) from the `origin_url`. /// /// # Examples @@ -180,23 +193,23 @@ impl DerefMut for IntegrationSettings { pub struct Synthetic { pub counter_store: String, pub opid_store: String, - #[validate(length(min = 1), custom(function = Synthetic::validate_secret_key))] + #[validate(length(min = 1))] pub secret_key: String, #[validate(length(min = 1))] pub template: String, } impl Synthetic { - /// Validates that the secret key is not the placeholder value. - /// - /// # Errors - /// - /// Returns a validation error if the secret key is `"secret_key"` (the placeholder). - pub fn validate_secret_key(secret_key: &str) -> Result<(), ValidationError> { - match secret_key { - "secret_key" => Err(ValidationError::new("Secret key is not valid")), - _ => Ok(()), - } + /// Known placeholder values that must not be used in production. + pub const SECRET_KEY_PLACEHOLDERS: &[&str] = &["secret-key", "secret_key", "trusted-server"]; + + /// Returns `true` if `secret_key` matches a known placeholder value + /// (case-insensitive). + #[must_use] + pub fn is_placeholder_secret_key(secret_key: &str) -> bool { + Self::SECRET_KEY_PLACEHOLDERS + .iter() + .any(|p| p.eq_ignore_ascii_case(secret_key)) } } @@ -381,6 +394,33 @@ impl Settings { Ok(settings) } + /// Checks all secret fields for known placeholder values and returns an + /// error listing every offending field. This centralises the placeholder + /// policy so callers don't need to know which fields are secrets. + /// + /// # Errors + /// + /// Returns [`TrustedServerError::InsecureDefault`] when one or more secret + /// fields still contain a placeholder value. + pub fn reject_placeholder_secrets(&self) -> Result<(), Report> { + let mut insecure_fields: Vec<&str> = Vec::new(); + + if Synthetic::is_placeholder_secret_key(&self.synthetic.secret_key) { + insecure_fields.push("synthetic.secret_key"); + } + if Publisher::is_placeholder_proxy_secret(&self.publisher.proxy_secret) { + insecure_fields.push("publisher.proxy_secret"); + } + + if insecure_fields.is_empty() { + Ok(()) + } else { + Err(Report::new(TrustedServerError::InsecureDefault { + field: insecure_fields.join(", "), + })) + } + } + #[must_use] pub fn handler_for_path(&self, path: &str) -> Option<&Handler> { self.handlers @@ -661,6 +701,7 @@ mod tests { #[test] fn test_settings_missing_required_fields() { let re = Regex::new(r"origin_url = .*").expect("regex should compile"); + let toml_str = crate_test_settings_str(); let toml_str = re.replace(&toml_str, ""); @@ -671,6 +712,62 @@ mod tests { ); } + #[test] + fn is_placeholder_secret_key_rejects_all_known_placeholders() { + for placeholder in Synthetic::SECRET_KEY_PLACEHOLDERS { + assert!( + Synthetic::is_placeholder_secret_key(placeholder), + "should detect placeholder secret_key '{placeholder}'" + ); + } + } + + #[test] + fn is_placeholder_secret_key_is_case_insensitive() { + assert!( + Synthetic::is_placeholder_secret_key("SECRET-KEY"), + "should detect case-insensitive placeholder secret_key" + ); + assert!( + Synthetic::is_placeholder_secret_key("Trusted-Server"), + "should detect mixed-case placeholder secret_key" + ); + } + + #[test] + fn is_placeholder_secret_key_accepts_non_placeholder() { + assert!( + !Synthetic::is_placeholder_secret_key("test-secret-key"), + "should accept non-placeholder secret_key" + ); + } + + #[test] + fn is_placeholder_proxy_secret_rejects_all_known_placeholders() { + for placeholder in Publisher::PROXY_SECRET_PLACEHOLDERS { + assert!( + Publisher::is_placeholder_proxy_secret(placeholder), + "should detect placeholder proxy_secret '{placeholder}'" + ); + } + } + + #[test] + fn is_placeholder_proxy_secret_is_case_insensitive() { + assert!( + Publisher::is_placeholder_proxy_secret("CHANGE-ME-PROXY-SECRET"), + "should detect case-insensitive placeholder proxy_secret" + ); + } + + #[test] + fn is_placeholder_proxy_secret_accepts_non_placeholder() { + assert!( + !Publisher::is_placeholder_proxy_secret("unit-test-proxy-secret"), + "should accept non-placeholder proxy_secret" + ); + } + #[test] fn test_settings_empty_toml() { let toml_str = ""; diff --git a/crates/common/src/settings_data.rs b/crates/common/src/settings_data.rs index 4f8e36a0..d764f0bd 100644 --- a/crates/common/src/settings_data.rs +++ b/crates/common/src/settings_data.rs @@ -34,9 +34,8 @@ pub fn get_settings() -> Result> { message: "Failed to validate configuration".to_string(), })?; - if settings.synthetic.secret_key == "secret-key" { - return Err(Report::new(TrustedServerError::InsecureSecretKey)); - } + // Reject known placeholder values for secrets that feed into cryptographic operations. + settings.reject_placeholder_secrets()?; if !settings.proxy.certificate_check { log::warn!( @@ -49,22 +48,110 @@ pub fn get_settings() -> Result> { #[cfg(test)] mod tests { - use super::*; + use crate::error::TrustedServerError; + use crate::settings::Settings; + use crate::test_support::tests::crate_test_settings_str; + + /// Builds a TOML string with the given secret values swapped in. + /// + /// # Panics + /// + /// Panics if the replacement patterns no longer match the test TOML, + /// which would cause the substitution to silently no-op. + fn toml_with_secrets(secret_key: &str, proxy_secret: &str) -> String { + let original = crate_test_settings_str(); + let after_secret_key = original.replace( + r#"secret_key = "test-secret-key""#, + &format!(r#"secret_key = "{secret_key}""#), + ); + assert_ne!( + after_secret_key, original, + "should have replaced secret_key value" + ); + let result = after_secret_key.replace( + r#"proxy_secret = "unit-test-proxy-secret""#, + &format!(r#"proxy_secret = "{proxy_secret}""#), + ); + assert_ne!( + result, after_secret_key, + "should have replaced proxy_secret value" + ); + result + } + + #[test] + fn rejects_placeholder_secret_key() { + let toml = toml_with_secrets("secret-key", "real-proxy-secret"); + let settings = Settings::from_toml(&toml).expect("should parse TOML"); + let err = settings + .reject_placeholder_secrets() + .expect_err("should reject placeholder secret_key"); + let root = err.current_context(); + assert!( + matches!(root, TrustedServerError::InsecureDefault { field } if field.contains("synthetic.secret_key")), + "error should mention synthetic.secret_key, got: {root}" + ); + } #[test] - fn test_get_settings() { - // Test that Settings::new() loads successfully - let settings = get_settings(); - assert!(settings.is_ok(), "Settings should load from embedded TOML"); - - let settings = settings.expect("should load settings from embedded TOML"); - // Verify basic structure is loaded - assert!(!settings.publisher.domain.is_empty()); - assert!(!settings.publisher.cookie_domain.is_empty()); - assert!(!settings.publisher.origin_url.is_empty()); - assert!(!settings.synthetic.counter_store.is_empty()); - assert!(!settings.synthetic.opid_store.is_empty()); - assert!(!settings.synthetic.secret_key.is_empty()); - assert!(!settings.synthetic.template.is_empty()); + fn rejects_placeholder_proxy_secret() { + let toml = toml_with_secrets("real-secret-key", "change-me-proxy-secret"); + let settings = Settings::from_toml(&toml).expect("should parse TOML"); + let err = settings + .reject_placeholder_secrets() + .expect_err("should reject placeholder proxy_secret"); + let root = err.current_context(); + assert!( + matches!(root, TrustedServerError::InsecureDefault { field } if field.contains("publisher.proxy_secret")), + "error should mention publisher.proxy_secret, got: {root}" + ); + } + + #[test] + fn rejects_both_placeholders_in_single_error() { + let toml = toml_with_secrets("secret_key", "change-me-proxy-secret"); + let settings = Settings::from_toml(&toml).expect("should parse TOML"); + let err = settings + .reject_placeholder_secrets() + .expect_err("should reject both placeholder secrets"); + let root = err.current_context(); + match root { + TrustedServerError::InsecureDefault { field } => { + assert!( + field.contains("synthetic.secret_key"), + "error should mention synthetic.secret_key, got: {field}" + ); + assert!( + field.contains("publisher.proxy_secret"), + "error should mention publisher.proxy_secret, got: {field}" + ); + } + other => panic!("expected InsecureDefault, got: {other}"), + } + } + + #[test] + fn accepts_non_placeholder_secrets() { + let toml = toml_with_secrets("production-secret-key", "production-proxy-secret"); + let settings = Settings::from_toml(&toml).expect("should parse TOML"); + settings + .reject_placeholder_secrets() + .expect("non-placeholder secrets should pass validation"); + } + + /// Smoke-test the full `get_settings()` pipeline (embedded bytes → UTF-8 → + /// parse → validate → placeholder check). The build-time TOML ships with + /// placeholder secrets, so the expected outcome is an [`InsecureDefault`] + /// error — but reaching that error proves every earlier stage succeeded. + #[test] + fn get_settings_rejects_embedded_placeholder_secrets() { + let err = super::get_settings().expect_err("should reject embedded placeholder secrets"); + assert!( + matches!( + err.current_context(), + TrustedServerError::InsecureDefault { .. } + ), + "should fail with InsecureDefault, got: {err}" + ); } } diff --git a/docs/guide/configuration.md b/docs/guide/configuration.md index 7fa3cfdf..cc83660b 100644 --- a/docs/guide/configuration.md +++ b/docs/guide/configuration.md @@ -244,6 +244,7 @@ TRUSTED_SERVER__PUBLISHER__PROXY_SECRET=your-secret-here **Security**: - Keep confidential and secure +- Cannot be the placeholder `"change-me-proxy-secret"` (case-insensitive) — startup will fail - Rotate periodically (90 days recommended) - Use cryptographically random values (32+ bytes) - Never commit to version control @@ -359,7 +360,7 @@ fastly kv-store create --name=opid_store **Security**: - Must be non-empty -- Cannot be `"secret_key"` or `"secret-key"` (reserved/invalid) +- Cannot be a known placeholder: `"secret-key"`, `"secret_key"`, or `"trusted-server"` (case-insensitive) - Rotate periodically for security - Store securely (environment variable recommended) @@ -373,7 +374,7 @@ openssl rand -hex 32 **Validation**: Application startup fails if: - Empty string -- Exactly `"secret_key"` or `"secret-key"` (default placeholders) +- Exactly `"secret-key"`, `"secret_key"`, or `"trusted-server"` (known placeholders, case-insensitive) #### `template` @@ -922,11 +923,12 @@ Configuration is validated at startup: - All fields non-empty - `origin_url` is valid URL +- `proxy_secret` ≠ known placeholder (`"change-me-proxy-secret"` — case-insensitive) **Synthetic Validation**: - `secret_key` ≥ 1 character -- `secret_key` ≠ `"secret-key"` +- `secret_key` ≠ known placeholders (`"secret-key"`, `"secret_key"`, `"trusted-server"` — case-insensitive) - `template` non-empty **Handler Validation**: @@ -1036,11 +1038,12 @@ trusted-server.dev.toml # Development overrides - Verify all required fields present - Check environment variable format -**"Secret key is not valid"**: +**"Configuration field '...' is set to a known placeholder value"**: -- Cannot use `"secret-key"` (placeholder) +- `synthetic.secret_key` cannot be `"secret-key"`, `"secret_key"`, or `"trusted-server"` (case-insensitive) +- `publisher.proxy_secret` cannot be `"change-me-proxy-secret"` (case-insensitive) - Must be non-empty -- Change to secure random value +- Change to a secure random value (see generation commands above) **"Invalid regex"**: