Fix weak and inconsistent secret default validation#467
Fix weak and inconsistent secret default validation#467ChristianPavilonis wants to merge 3 commits intomainfrom
Conversation
Reject all known placeholder values for synthetic.secret_key and publisher.proxy_secret at runtime startup so deployments using default secrets fail fast instead of running with predictable cryptographic keys.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Review Summary
| # | Severity | File | Finding |
|---|---|---|---|
| 1 | HIGH | settings.rs | proxy_secret has no validation — empty string passes and produces a deterministic crypto key |
| 2 | MEDIUM | settings_data.rs | Test is environment-dependent and can flap when CI overrides secrets |
| 3 | MEDIUM | settings.rs | Placeholder policy is scattered across two types and will drift |
| 4 | LOW | settings_data.rs | Only the first insecure field is reported; operator must fix-and-redeploy to find the second |
| 5 | NIT | settings.rs | Spurious #[allow(dead_code)] on public items that are actually used |
| impl Publisher { | ||
| /// Known placeholder values that must not be used in production. | ||
| #[allow(dead_code)] | ||
| pub const PROXY_SECRET_PLACEHOLDERS: &[&str] = &["change-me-proxy-secret"]; |
There was a problem hiding this comment.
HIGH — proxy_secret has no validation, empty string passes
synthetic.secret_key has #[validate(length(min = 1))] (line 194) but proxy_secret (line 26) has no validator at all. An empty proxy_secret passes both validate() and the placeholder check, then feeds into Sha256::digest at http_util.rs:225 to derive an XChaCha20-Poly1305 key — a deterministic key from an empty string means anyone can decrypt proxied URLs.
Add at minimum #[validate(length(min = 1))] to the proxy_secret field, and consider enforcing a minimum length policy (e.g., 16 chars) for secrets used in cryptographic operations.
| assert!(!settings.synthetic.opid_store.is_empty()); | ||
| assert!(!settings.synthetic.secret_key.is_empty()); | ||
| assert!(!settings.synthetic.template.is_empty()); | ||
| fn rejects_default_placeholder_secrets() { |
There was a problem hiding this comment.
MEDIUM — Test is environment-dependent and can flap
get_settings() reads from include_bytes!("../../../target/trusted-server-out.toml") which is the build-time merged TOML. If CI sets TRUSTED_SERVER__SYNTHETIC__SECRET_KEY and TRUSTED_SERVER__PUBLISHER__PROXY_SECRET to real values, the embedded config has no placeholders and this test unexpectedly passes instead of failing.
Consider adding a pure helper that validates from explicit TOML input so both reject and accept paths can be tested deterministically without depending on build-time environment state.
| } | ||
| /// Known placeholder values that must not be used in production. | ||
| #[allow(dead_code)] | ||
| pub const SECRET_KEY_PLACEHOLDERS: &[&str] = &["secret-key", "secret_key", "trusted-server"]; |
There was a problem hiding this comment.
MEDIUM — Placeholder policy is scattered and will drift
Placeholder constants are hardcoded in two separate locations (Publisher::PROXY_SECRET_PLACEHOLDERS at line 32 and Synthetic::SECRET_KEY_PLACEHOLDERS here), and each type owns its own is_placeholder_* method. As more secrets are added, this pattern will drift — especially since docs (e.g., docs/guide/configuration.md) still only describe older synthetic placeholders.
Consider centralizing placeholder/security policy into a single validator module that serves as the source of truth for both runtime checks and documentation.
| if settings.synthetic.secret_key == "secret-key" { | ||
| return Err(Report::new(TrustedServerError::InsecureSecretKey)); | ||
| // Reject known placeholder values for secrets that feed into cryptographic operations. | ||
| if Synthetic::is_placeholder_secret_key(&settings.synthetic.secret_key) { |
There was a problem hiding this comment.
LOW — Only the first insecure field is reported
The two placeholder checks are sequential early returns. If both secret_key and proxy_secret are placeholders (the default state from trusted-server.toml), the operator only sees the synthetic.secret_key error and must fix-and-redeploy to discover the proxy_secret error.
Collect all violations and report them in a single error to reduce fix/redeploy cycles.
|
|
||
| impl Publisher { | ||
| /// Known placeholder values that must not be used in production. | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
NIT — Spurious #[allow(dead_code)] on public items
Both the constants and the is_placeholder_* methods are pub and called from settings_data.rs. They are not dead code. The #[allow(dead_code)] suppresses nothing and misleads readers into thinking these items might be unused.
2 participants
Summary
synthetic.secret_key("secret-key","secret_key","trusted-server") andpublisher.proxy_secret("change-me-proxy-secret") at runtime startup so misconfigured deployments fail fast.is_placeholder_secret_key,is_placeholder_proxy_secret) with explicit placeholder lists, replacing the inconsistent checks that missed the actual TOML defaults.InsecureSecretKeytoInsecureDefault { field }so the error message identifies which secret triggered rejection.Changes
crates/common/src/error.rsInsecureSecretKey→InsecureDefault { field: String }with a descriptive display messagecrates/common/src/settings.rsPROXY_SECRET_PLACEHOLDERSconst +is_placeholder_proxy_secret()onPublisher; replacedvalidate_secret_key()withSECRET_KEY_PLACEHOLDERSconst +is_placeholder_secret_key()onSynthetic; added 4 unit testscrates/common/src/settings_data.rs== "secret-key"check with calls to both predicates; returns field-specificInsecureDefaulterrors; updated test to assert placeholder rejectioncrates/common/build.rsCloses
Closes #406
Test plan
cargo test --workspacecargo clippy --all-targets --all-features -- -D warningscargo fmt --all -- --checkChecklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!)