Skip to content

Commit 4c29dbf

Browse files
Remove runtime secret placeholder validation that breaks deployments (#527)
* Remove runtime secret placeholder validation that breaks deployments The runtime placeholder checks introduced in #467 reject known placeholder values for synthetic.secret_key and publisher.proxy_secret at startup. The expanded placeholder list and case-insensitive matching now catches deployments that were previously working. Remove the runtime validation, the InsecureDefault error variant, and all associated tests and docs while keeping the existing non-empty validation for secret_key. * Address PR review feedback - Rename test to get_settings_loads_embedded_toml_successfully - Remove redundant assert!(is_ok) pattern, keep only expect() - Add comment explaining why placeholder secrets are expected in tests - Add log::warn! when secret_key or proxy_secret match default placeholders
1 parent cd87d62 commit 4c29dbf

5 files changed

Lines changed: 28 additions & 240 deletions

File tree

crates/trusted-server-core/build.rs

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -38,11 +38,6 @@ fn main() {
3838

3939
// Merge base TOML with environment variable overrides and write output.
4040
// Panics if admin endpoints are not covered by a handler.
41-
// Note: placeholder secret rejection is intentionally NOT done here.
42-
// The base trusted-server.toml ships with placeholder secrets that
43-
// production deployments override via TRUSTED_SERVER__* env vars at
44-
// build time. Runtime startup (get_settings) rejects any remaining
45-
// placeholders so a misconfigured deployment fails fast.
4641
let settings = settings::Settings::from_toml_and_env(&toml_content)
4742
.expect("Failed to parse settings at build time");
4843

crates/trusted-server-core/src/error.rs

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -33,12 +33,6 @@ pub enum TrustedServerError {
3333
#[display("GDPR consent error: {message}")]
3434
GdprConsent { message: String },
3535

36-
/// A configuration secret is still set to a known placeholder value.
37-
#[display(
38-
"Configuration field '{field}' is set to a known placeholder value - this is insecure"
39-
)]
40-
InsecureDefault { field: String },
41-
4236
/// Invalid UTF-8 data encountered.
4337
#[display("Invalid UTF-8 data: {message}")]
4438
InvalidUtf8 { message: String },
@@ -99,7 +93,6 @@ impl IntoHttpResponse for TrustedServerError {
9993
Self::Configuration { .. } | Self::Settings { .. } => StatusCode::INTERNAL_SERVER_ERROR,
10094
Self::Gam { .. } => StatusCode::BAD_GATEWAY,
10195
Self::GdprConsent { .. } => StatusCode::BAD_REQUEST,
102-
Self::InsecureDefault { .. } => StatusCode::INTERNAL_SERVER_ERROR,
10396
Self::InvalidHeaderValue { .. } => StatusCode::BAD_REQUEST,
10497
Self::InvalidUtf8 { .. } => StatusCode::BAD_REQUEST,
10598
Self::KvStore { .. } => StatusCode::SERVICE_UNAVAILABLE,

crates/trusted-server-core/src/settings.rs

Lines changed: 0 additions & 112 deletions
Original file line numberDiff line numberDiff line change
@@ -31,18 +31,6 @@ pub struct Publisher {
3131
}
3232

3333
impl Publisher {
34-
/// Known placeholder values that must not be used in production.
35-
pub const PROXY_SECRET_PLACEHOLDERS: &[&str] = &["change-me-proxy-secret"];
36-
37-
/// Returns `true` if `proxy_secret` matches a known placeholder value
38-
/// (case-insensitive).
39-
#[must_use]
40-
pub fn is_placeholder_proxy_secret(proxy_secret: &str) -> bool {
41-
Self::PROXY_SECRET_PLACEHOLDERS
42-
.iter()
43-
.any(|p| p.eq_ignore_ascii_case(proxy_secret))
44-
}
45-
4634
/// Extracts the host (including port if present) from the `origin_url`.
4735
///
4836
/// # Examples
@@ -215,25 +203,8 @@ pub struct Synthetic {
215203
}
216204

217205
impl Synthetic {
218-
/// Known placeholder values that must not be used in production.
219-
pub const SECRET_KEY_PLACEHOLDERS: &[&str] = &["secret-key", "secret_key", "trusted-server"];
220-
221-
/// Returns `true` if `secret_key` matches a known placeholder value
222-
/// (case-insensitive).
223-
#[must_use]
224-
pub fn is_placeholder_secret_key(secret_key: &str) -> bool {
225-
Self::SECRET_KEY_PLACEHOLDERS
226-
.iter()
227-
.any(|p| p.eq_ignore_ascii_case(secret_key))
228-
}
229-
230206
/// Validates that the secret key is not empty.
231207
///
232-
/// Placeholder detection is intentionally **not** performed here because
233-
/// this validator runs at build time (via `from_toml_and_env`) when the
234-
/// config legitimately contains placeholder values. Placeholder rejection
235-
/// happens at runtime via [`Settings::reject_placeholder_secrets`].
236-
///
237208
/// # Errors
238209
///
239210
/// Returns a validation error if the secret key is empty.
@@ -486,33 +457,6 @@ impl Settings {
486457
Ok(None)
487458
}
488459

489-
/// Checks all secret fields for known placeholder values and returns an
490-
/// error listing every offending field. This centralises the placeholder
491-
/// policy so callers don't need to know which fields are secrets.
492-
///
493-
/// # Errors
494-
///
495-
/// Returns [`TrustedServerError::InsecureDefault`] when one or more secret
496-
/// fields still contain a placeholder value.
497-
pub fn reject_placeholder_secrets(&self) -> Result<(), Report<TrustedServerError>> {
498-
let mut insecure_fields: Vec<&str> = Vec::new();
499-
500-
if Synthetic::is_placeholder_secret_key(self.synthetic.secret_key.expose()) {
501-
insecure_fields.push("synthetic.secret_key");
502-
}
503-
if Publisher::is_placeholder_proxy_secret(self.publisher.proxy_secret.expose()) {
504-
insecure_fields.push("publisher.proxy_secret");
505-
}
506-
507-
if insecure_fields.is_empty() {
508-
Ok(())
509-
} else {
510-
Err(Report::new(TrustedServerError::InsecureDefault {
511-
field: insecure_fields.join(", "),
512-
}))
513-
}
514-
}
515-
516460
/// Known admin endpoint paths that must be covered by a handler.
517461
///
518462
/// [`from_toml_and_env`](Self::from_toml_and_env) rejects configurations
@@ -843,62 +787,6 @@ mod tests {
843787
);
844788
}
845789

846-
#[test]
847-
fn is_placeholder_secret_key_rejects_all_known_placeholders() {
848-
for placeholder in Synthetic::SECRET_KEY_PLACEHOLDERS {
849-
assert!(
850-
Synthetic::is_placeholder_secret_key(placeholder),
851-
"should detect placeholder secret_key '{placeholder}'"
852-
);
853-
}
854-
}
855-
856-
#[test]
857-
fn is_placeholder_secret_key_is_case_insensitive() {
858-
assert!(
859-
Synthetic::is_placeholder_secret_key("SECRET-KEY"),
860-
"should detect case-insensitive placeholder secret_key"
861-
);
862-
assert!(
863-
Synthetic::is_placeholder_secret_key("Trusted-Server"),
864-
"should detect mixed-case placeholder secret_key"
865-
);
866-
}
867-
868-
#[test]
869-
fn is_placeholder_secret_key_accepts_non_placeholder() {
870-
assert!(
871-
!Synthetic::is_placeholder_secret_key("test-secret-key"),
872-
"should accept non-placeholder secret_key"
873-
);
874-
}
875-
876-
#[test]
877-
fn is_placeholder_proxy_secret_rejects_all_known_placeholders() {
878-
for placeholder in Publisher::PROXY_SECRET_PLACEHOLDERS {
879-
assert!(
880-
Publisher::is_placeholder_proxy_secret(placeholder),
881-
"should detect placeholder proxy_secret '{placeholder}'"
882-
);
883-
}
884-
}
885-
886-
#[test]
887-
fn is_placeholder_proxy_secret_is_case_insensitive() {
888-
assert!(
889-
Publisher::is_placeholder_proxy_secret("CHANGE-ME-PROXY-SECRET"),
890-
"should detect case-insensitive placeholder proxy_secret"
891-
);
892-
}
893-
894-
#[test]
895-
fn is_placeholder_proxy_secret_accepts_non_placeholder() {
896-
assert!(
897-
!Publisher::is_placeholder_proxy_secret("unit-test-proxy-secret"),
898-
"should accept non-placeholder proxy_secret"
899-
);
900-
}
901-
902790
#[test]
903791
fn test_settings_empty_toml() {
904792
let toml_str = "";

crates/trusted-server-core/src/settings_data.rs

Lines changed: 28 additions & 104 deletions
Original file line numberDiff line numberDiff line change
@@ -34,124 +34,48 @@ pub fn get_settings() -> Result<Settings, Report<TrustedServerError>> {
3434
message: "Failed to validate configuration".to_string(),
3535
})?;
3636

37-
// Reject known placeholder values for secrets that feed into cryptographic operations.
38-
settings.reject_placeholder_secrets()?;
39-
4037
if !settings.proxy.certificate_check {
4138
log::warn!(
4239
"INSECURE: proxy.certificate_check is disabled — TLS certificates will NOT be verified"
4340
);
4441
}
4542

46-
Ok(settings)
47-
}
48-
49-
#[cfg(test)]
50-
mod tests {
51-
use crate::error::TrustedServerError;
52-
use crate::settings::Settings;
53-
use crate::test_support::tests::crate_test_settings_str;
54-
55-
/// Builds a TOML string with the given secret values swapped in.
56-
///
57-
/// # Panics
58-
///
59-
/// Panics if the replacement patterns no longer match the test TOML,
60-
/// which would cause the substitution to silently no-op.
61-
fn toml_with_secrets(secret_key: &str, proxy_secret: &str) -> String {
62-
let original = crate_test_settings_str();
63-
let after_secret_key = original.replace(
64-
r#"secret_key = "test-secret-key""#,
65-
&format!(r#"secret_key = "{secret_key}""#),
66-
);
67-
assert_ne!(
68-
after_secret_key, original,
69-
"should have replaced secret_key value"
70-
);
71-
let result = after_secret_key.replace(
72-
r#"proxy_secret = "unit-test-proxy-secret""#,
73-
&format!(r#"proxy_secret = "{proxy_secret}""#),
74-
);
75-
assert_ne!(
76-
result, after_secret_key,
77-
"should have replaced proxy_secret value"
78-
);
79-
result
80-
}
81-
82-
#[test]
83-
fn rejects_placeholder_secret_key() {
84-
let toml = toml_with_secrets("secret-key", "real-proxy-secret");
85-
let settings = Settings::from_toml(&toml).expect("should parse TOML");
86-
let err = settings
87-
.reject_placeholder_secrets()
88-
.expect_err("should reject placeholder secret_key");
89-
let root = err.current_context();
90-
assert!(
91-
matches!(root, TrustedServerError::InsecureDefault { field } if field.contains("synthetic.secret_key")),
92-
"error should mention synthetic.secret_key, got: {root}"
43+
if settings.synthetic.secret_key.expose() == "trusted-server" {
44+
log::warn!(
45+
"INSECURE: synthetic.secret_key is set to the default placeholder — \
46+
HMAC-SHA256 signatures can be forged. \
47+
Override via TRUSTED_SERVER__SYNTHETIC__SECRET_KEY at build time"
9348
);
9449
}
9550

96-
#[test]
97-
fn rejects_placeholder_proxy_secret() {
98-
let toml = toml_with_secrets("real-secret-key", "change-me-proxy-secret");
99-
let settings = Settings::from_toml(&toml).expect("should parse TOML");
100-
let err = settings
101-
.reject_placeholder_secrets()
102-
.expect_err("should reject placeholder proxy_secret");
103-
let root = err.current_context();
104-
assert!(
105-
matches!(root, TrustedServerError::InsecureDefault { field } if field.contains("publisher.proxy_secret")),
106-
"error should mention publisher.proxy_secret, got: {root}"
51+
if settings.publisher.proxy_secret.expose() == "change-me-proxy-secret" {
52+
log::warn!(
53+
"INSECURE: publisher.proxy_secret is set to the default placeholder — \
54+
XChaCha20-Poly1305 encrypted URLs can be decrypted by anyone. \
55+
Override via TRUSTED_SERVER__PUBLISHER__PROXY_SECRET at build time"
10756
);
10857
}
10958

110-
#[test]
111-
fn rejects_both_placeholders_in_single_error() {
112-
let toml = toml_with_secrets("secret_key", "change-me-proxy-secret");
113-
let settings = Settings::from_toml(&toml).expect("should parse TOML");
114-
let err = settings
115-
.reject_placeholder_secrets()
116-
.expect_err("should reject both placeholder secrets");
117-
let root = err.current_context();
118-
match root {
119-
TrustedServerError::InsecureDefault { field } => {
120-
assert!(
121-
field.contains("synthetic.secret_key"),
122-
"error should mention synthetic.secret_key, got: {field}"
123-
);
124-
assert!(
125-
field.contains("publisher.proxy_secret"),
126-
"error should mention publisher.proxy_secret, got: {field}"
127-
);
128-
}
129-
other => panic!("expected InsecureDefault, got: {other}"),
130-
}
131-
}
59+
Ok(settings)
60+
}
13261

133-
#[test]
134-
fn accepts_non_placeholder_secrets() {
135-
let toml = toml_with_secrets("production-secret-key", "production-proxy-secret");
136-
let settings = Settings::from_toml(&toml).expect("should parse TOML");
137-
settings
138-
.reject_placeholder_secrets()
139-
.expect("non-placeholder secrets should pass validation");
140-
}
62+
#[cfg(test)]
63+
mod tests {
64+
use super::*;
14165

142-
/// Smoke-test the full `get_settings()` pipeline (embedded bytes → UTF-8 →
143-
/// parse → validate → placeholder check). The build-time TOML ships with
144-
/// placeholder secrets, so the expected outcome is an [`InsecureDefault`]
145-
/// error — but reaching that error proves every earlier stage succeeded.
14666
#[test]
147-
fn get_settings_rejects_embedded_placeholder_secrets() {
148-
let err = super::get_settings().expect_err("should reject embedded placeholder secrets");
149-
assert!(
150-
matches!(
151-
err.current_context(),
152-
TrustedServerError::InsecureDefault { .. }
153-
),
154-
"should fail with InsecureDefault, got: {err}"
155-
);
67+
fn get_settings_loads_embedded_toml_successfully() {
68+
// The embedded TOML contains placeholder secrets (e.g. "trusted-server",
69+
// "change-me-proxy-secret"). This is expected — production builds override
70+
// them via TRUSTED_SERVER__* env vars at build time.
71+
let settings = get_settings().expect("should load settings from embedded TOML");
72+
// Verify basic structure is loaded
73+
assert!(!settings.publisher.domain.is_empty());
74+
assert!(!settings.publisher.cookie_domain.is_empty());
75+
assert!(!settings.publisher.origin_url.is_empty());
76+
assert!(!settings.synthetic.counter_store.is_empty());
77+
assert!(!settings.synthetic.opid_store.is_empty());
78+
assert!(!settings.synthetic.secret_key.expose().is_empty());
79+
assert!(!settings.synthetic.template.is_empty());
15680
}
15781
}

docs/guide/configuration.md

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -244,7 +244,6 @@ TRUSTED_SERVER__PUBLISHER__PROXY_SECRET=your-secret-here
244244
**Security**:
245245

246246
- Keep confidential and secure
247-
- Cannot be the placeholder `"change-me-proxy-secret"` (case-insensitive) — startup will fail
248247
- Rotate periodically (90 days recommended)
249248
- Use cryptographically random values (32+ bytes)
250249
- Never commit to version control
@@ -360,7 +359,6 @@ fastly kv-store create --name=opid_store
360359
**Security**:
361360

362361
- Must be non-empty
363-
- Cannot be a known placeholder: `"secret-key"`, `"secret_key"`, or `"trusted-server"` (case-insensitive)
364362
- Rotate periodically for security
365363
- Store securely (environment variable recommended)
366364

@@ -374,7 +372,6 @@ openssl rand -hex 32
374372
**Validation**: Application startup fails if:
375373

376374
- Empty string
377-
- Exactly `"secret-key"`, `"secret_key"`, or `"trusted-server"` (known placeholders, case-insensitive)
378375

379376
#### `template`
380377

@@ -923,12 +920,10 @@ Configuration is validated at startup:
923920

924921
- All fields non-empty
925922
- `origin_url` is valid URL
926-
- `proxy_secret` ≠ known placeholder (`"change-me-proxy-secret"` — case-insensitive)
927923

928924
**Synthetic Validation**:
929925

930926
- `secret_key` ≥ 1 character
931-
- `secret_key` ≠ known placeholders (`"secret-key"`, `"secret_key"`, `"trusted-server"` — case-insensitive)
932927
- `template` non-empty
933928

934929
**Handler Validation**:
@@ -1038,13 +1033,6 @@ trusted-server.dev.toml # Development overrides
10381033
- Verify all required fields present
10391034
- Check environment variable format
10401035

1041-
**"Configuration field '...' is set to a known placeholder value"**:
1042-
1043-
- `synthetic.secret_key` cannot be `"secret-key"`, `"secret_key"`, or `"trusted-server"` (case-insensitive)
1044-
- `publisher.proxy_secret` cannot be `"change-me-proxy-secret"` (case-insensitive)
1045-
- Must be non-empty
1046-
- Change to a secure random value (see generation commands above)
1047-
10481036
**"Invalid regex"**:
10491037

10501038
- Handler `path` must be valid regex

0 commit comments

Comments
 (0)