chore(security): add guardrails against secret values in error strings (#5244)

* fix(kms): do not include secret value in static KMS format error

The malformed-format error message in build_static_kms_config embedded the
raw env var value. The most likely misconfiguration is setting
RUSTFS_KMS_STATIC_SECRET_KEY to the bare base64 key without the <key-name>:
prefix, in which case the full secret key material would be written to
startup logs. Drop the value from the message, matching the equivalent
parsing error in KmsConfig::from_env().

* chore(security): add guardrails against secret values in error strings

Follow-up to the PR #5222 review finding fixed in PR #5243: a config value
that fails secret-format parsing is typically the raw secret itself, and
error messages are log content — they propagate via ? and are printed by
startup/error logging far from the construction site.

Three layers so this class of leak is caught earlier next time:

- security-advisory-lessons: state explicitly that error/panic messages are
  log content; parse-failure hints must name the env var and expected
  format, never echo the input. Add a matching review prompt.
- adversarial-validation: add a security-reviewer probe that greps
  error-construction sites on secret-bearing config paths for interpolation
  of the raw value, including the duplicated-parse trap where the leak hid.
- check_logging_guardrails.sh: mechanical backstop — flag inline format
  interpolation of secret-named identifiers (secret/password/passwd/
  private_key) in checked files; add crates/kms/src/config.rs (the twin
  parse site) to the checked list. Verified the check passes on the fixed
  tree and catches the original leak at rustfs/src/init.rs:399.

.agents/skills/adversarial-validation/SKILL.md

@@ -84,6 +84,9 @@ Null report example: "Rewrote the diff as an in-place edit (no smaller equivalen
 - For any secret/token/signature/password comparison in the diff, check it uses a constant-time compare (e.g. subtle/constant_time_eq), not == or early-return byte loops. Then check the failure-response paths: construct an invalid-user request and an invalid-secret request and confirm they are indistinguishable (same error, no early length short-circuit) so an attacker cannot enumerate valid users or time-side-channel the secret.
   - Where: crates/protocols/ (FTPS/WebDAV/FormPost auth), crates/credentials/, rustfs/src/auth.rs, RPC signature verification
   - Evidence: GHSA-3p3x-734c-h5vx (FTPS/WebDAV early-return string equality + distinguishable invalid-user vs invalid-password). Fix commits 3c3113619 (constant-time FTPS/WebDAV) and c41062f27 (constant-time FormPost signature). 3p3x was fixed by PR #4403.
+- If the diff parses or transports secret-bearing config (env vars, key files, connection strings), grep every error-construction and format site on that value's path (`format!` feeding `Error::other`/`configuration_error`/`panic!`/`expect`) for interpolation of the raw value or of variables named like secret material. Construct the likeliest misconfiguration: the operator supplies the bare secret without the expected `<name>:` prefix (or with a stray newline) — if the parse-failure hint echoes the input, the secret lands in startup logs. Error strings are log content; the hint may name the env var and expected format, never the value. If the diff re-implements an existing parse helper, diff the two error paths — the duplicate is where the leak hides.
+  - Where: rustfs/src/init.rs (env plumbing), crates/kms/src/config.rs, crates/credentials/, any from_env/parse on secret values; mechanical backstop in scripts/check_logging_guardrails.sh (secret-interpolation check)
+  - Evidence: PR #5222 introduced `got: {secret_str}` in build_static_kms_config's format-hint error — a bare base64 key (the secret itself) would have been echoed into startup logs; fixed by PR #5243. The parallel parse in KmsConfig::from_env already omitted the value: the leak lived only in the duplicated copy (AGENTS.md 'Reuse Before You Write').
 - If the diff touches internode/RPC auth secret handling, trace whether the RPC HMAC secret can fall back to a public default (e.g. 'rustfsadmin', 'rustfs rpc') or be derived deterministically from the S3 root credentials. Construct the case where RUSTFS_RPC_SECRET is unset and confirm the code fails closed rather than silently using a default or a root-derived key. Verify RPC signing keys are independent random secrets, not reused across S3-root/RPC-HMAC/STS-JWT roles.
   - Where: crates/credentials/, crates/ecstore/src/rpc/, internode auth setup
   - Evidence: GHSA-r5qv-rc46-hv8q (fell back to 'rustfsadmin'), GHSA-75fx/68cw (RPC secret derivable from root creds → forgeable signatures), GHSA-h956 (hard-coded 'rustfs rpc'), GHSA-m77q (STS JWT reused root secret). Fix commit 7b2055405 (fail closed when deriving RPC secret from default credentials, PR#4402).

.agents/skills/security-advisory-lessons/SKILL.md

@@ -99,6 +99,8 @@ For the full pattern map, read [advisory-patterns.md](references/advisory-patter
 ### Logging and debug output
 - Logs must never include access keys beyond safe identifiers, secret keys, session tokens, JWT claims, HMAC secrets, expected signatures, license secrets, or raw response bodies containing credentials.
 - Treat `Debug` implementations, `?value` tracing, merged config dumps, and dependency-level HTTP body logging as leak surfaces.
+- Error and panic messages are log content: they propagate through `?` and get printed by `error!`/startup logging far from where they were constructed. Never interpolate a raw config or credential value into an error string.
+- A value that fails secret-format parsing is usually the secret itself (e.g. a bare base64 key missing its `<name>:` prefix), so a parse-failure hint must name the env var or file and the expected format, never echo the input. Redacting `Debug` impls does not cover this channel.
 - Add log-capture tests or targeted unit tests for redaction wrappers when changing credential structs or response bodies.
 
 ### RPC, parsing, and panic safety
@@ -139,6 +141,7 @@ Use these prompts while reviewing a diff:
 - Does a public/default/empty config change security behavior from fail-closed to fail-open?
 - Is any attacker-controlled value later used as a path, policy condition, credential identity, log field, URL, Origin, or response body?
 - Does this response contain stored replication, remote target, or service credentials that need redaction or stricter authorization?
+- Does any error constructor or `format!` interpolate a variable that can hold secret material, including a config parse error that echoes the raw input?
 - Does an IAM export/import path expose or trust plaintext credential secrets beyond the caller's intended authority?
 - Can this STS/OIDC path issue credentials without SigV4, trusted issuer validation, allowlisted redirects, or trusted-proxy host/scheme handling?
 - Can a service-account or STS token omit `exp`, forge `sessionPolicy`, or use a principal-controlled key as signing authority?

scripts/check_logging_guardrails.sh

@@ -26,6 +26,7 @@ checked_files=(
   "rustfs/src/admin/handlers/system.rs"
   "rustfs/src/storage/rpc/http_service.rs"
   "rustfs/src/storage/rpc/node_service.rs"
+  "crates/kms/src/config.rs"
   "crates/audit/src/pipeline.rs"
   "crates/audit/src/system.rs"
   "crates/audit/src/global.rs"
@@ -655,6 +656,23 @@ for pattern in "${forbidden_patterns[@]}"; do
   fi
 done
 
+# Secret material must never be interpolated into log or error strings.
+# Error messages are log content: they propagate via `?` and are printed by
+# startup/error logging far from the construction site, and a value that fails
+# secret-format parsing is typically the raw secret itself (PR #5222/#5243:
+# `got: {secret_str}` would have echoed a bare base64 KMS key into startup
+# logs). Parse-failure hints must name the env var and expected format only.
+# Heuristic: flag inline format interpolation of secret-named identifiers.
+# Uppercase const names (env-var names like ENV_KMS_STATIC_SECRET_KEY) do not
+# match by design; test assertion messages only print on local test failure.
+secret_interpolation='\{[a-z_]*(secret|password|passwd|private_key)[a-z_]*(:[^}]*)?\}'
+secret_hits="$(rg -n -- "$secret_interpolation" "${checked_files[@]}" | rg -v 'assert' || true)"
+if [[ -n "$secret_hits" ]]; then
+  echo "❌ logging guardrail violation: secret-named variable interpolated into a format/log/error string" >&2
+  echo "$secret_hits" >&2
+  exit 1
+fi
+
 systemd_unit="deploy/build/rustfs.service"
 if rg -n '^Standard(Output|Error)=append:.*rustfs.*\.log$' "$systemd_unit" >/dev/null; then
   echo "❌ logging guardrail violation: systemd must not append stdout/stderr to a RustFS-managed rolling log" >&2