feat(security): Wrap secrets with secrecy crate (#369)#912
Conversation
a9abce7 to
43b1cb5
Compare
43b1cb5 to
7131791
Compare
|
Thanks for the review. I reworked the secret handling around a single
|
7131791 to
29b7957
Compare
56ac8df to
2e15c47
Compare
2e15c47 to
cdb73dc
Compare
| if let Some(value) = &self.oidc_client_id { | ||
| input.insert("oidc_client_id".to_string(), serde_json::json!(value)); | ||
| } | ||
| if self.oidc_client_secret.is_some() { |
There was a problem hiding this comment.
this is not necessary and makes no sense (passing masked input where it should be skipped completely). Technically I would rather convert the whole struct to Value and remove secrets out of it. It is much shorter. The same would apply to every other case
There was a problem hiding this comment.
Done in 7c360743 — to_policy_input() now omits the secret entirely instead of masking it with [REDACTED], for UserCreate/Update and IdentityProviderCreate/Update.
| #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] | ||
| #[cfg_attr(feature = "validate", derive(validator::Validate))] | ||
| #[cfg_attr( | ||
| feature = "validate", |
There was a problem hiding this comment.
problem is that now you ONLY validate a single field dropping validation of all other fields. Instead you can attach a specific validation function to a field directly.
There was a problem hiding this comment.
A field-level #[validate(custom)] can't be attached to a SecretString: validator 0.20 calls err.add_param("value", &field) on failure, which requires Serialize — it won't compile and would serialize the secret into the error. So I kept #[derive(Validate)] (every non-secret field is still validated) and check only the secret via a struct-level schema fn that never serializes it, documented with a NOTE inline. Added a test that an over-long non-secret field (e.g. name) still fails. 7c360743
| #[cfg_attr(feature = "validate", derive(validator::Validate))] | ||
| #[cfg_attr( | ||
| feature = "validate", | ||
| validate(schema(function = "validate_identity_provider_update_secret")) |
There was a problem hiding this comment.
same issue with the validation as above
| !rendered.contains("CSLEAK2"), | ||
| "policy input leaked client secret: {rendered}" | ||
| ); | ||
| assert_eq!( |
There was a problem hiding this comment.
as mentioned above - the field should simply not appear at all in the policy data
There was a problem hiding this comment.
As above — the secret key no longer appears in the policy data.
| #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] | ||
| #[cfg_attr(feature = "validate", derive(validator::Validate))] | ||
| #[cfg_attr( | ||
| feature = "validate", |
There was a problem hiding this comment.
same issue with broken validation
| upa.domain(domain_builder.build()?); | ||
| } | ||
| upa.password(value.password.clone()); | ||
| upa.password(value.password.expose_secret()); |
There was a problem hiding this comment.
we should not expose secret here, but pass it down. This signals that upa.password must also be converted to SecretString
There was a problem hiding this comment.
Done in 7c360743 — the conversion passes the SecretString down instead of exposing it at conversion time.
| #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] | ||
| #[cfg_attr(feature = "validate", derive(validator::Validate))] | ||
| #[cfg_attr( | ||
| feature = "validate", |
| #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] | ||
| #[cfg_attr(feature = "validate", derive(validator::Validate))] | ||
| #[cfg_attr( | ||
| feature = "validate", |
|
|
||
| use crate::error::BuilderError; | ||
|
|
||
| fn validate_secret_length(secret: &SecretString, max: usize) -> Result<(), ValidationError> { |
There was a problem hiding this comment.
also this validators can be placed to common for reuse
There was a problem hiding this comment.
The api-types and core-types copies are separate because each crate gates validation behind its own validate feature, so a shared module would couple the feature sets. Happy to extract one if you'd prefer.
| /// `PartialEq` is intentionally not derived: `password` is wrapped in | ||
| /// [`SecretString`], which does not implement `PartialEq` by design. | ||
| #[derive(Builder, Clone, Debug, Validate)] | ||
| #[validate(schema(function = "validate_user_create_secret"))] |
Resolve gtema's review round on PR openstack-experimental#912 (openstack-experimental#369): - Policy input now omits secrets entirely instead of masking them with "[REDACTED]"; route the v3/v4 user and identity provider handlers through to_policy_input(). - Fix a plaintext password leak into OPA from the v4 user create/update handlers, which passed the raw DTO. - Keep secret validation via a struct-level schema fn that never serializes the secret. validator 0.20 cannot attach a field-level validator to a SecretString (it requires Serialize and would leak the value); documented inline. - Pass the SecretString down in auth_conv instead of exposing it at conversion time. - Add regression tests asserting secrets never reach policy input and that non-secret field validation still applies. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DPZxPWH9yYH36jgfwGqZdQ Signed-off-by: Yousef Hussein <ymh1874@gmail.com>
Wrap every remaining plaintext secret in `secrecy::SecretString` so it can never be exposed through `Debug`, `#[tracing::instrument]`, or accidental serialization, and redact secrets to `"[REDACTED]"` on the serialize paths that feed OPA policy input and audit payloads. Part of issue openstack-experimental#369 (token / password / application-credential secret), done as one PR since touching api-types pulls through every downstream crate. Wrapped, edge-to-consumer: - OIDC `oidc_client_secret` across the api DTO, the three core-types domain structs, the driver conversions, and the token-exchange consumer. `oidc_client_id` stays plaintext (per maintainer). - User/auth passwords: lifted the existing deep-layer wrapping up to the wire so `UserCreate`/`UserUpdate`/`UserPassword` and the whole auth request tree are `SecretString` from deserialize down; exposure now happens only at hash/verify/`validate_password`. - Config `invalid_password_hash_key` (mirrors `database.connection`). - OIDC token-exchange `id_token`/`access_token` bearer tokens, which were plaintext in a `Debug`-deriving struct. This also fixes three pre-existing plaintext leaks: `oidc_client_secret` and user `password` were serialized in the clear into OPA policy input via `json!({...})`; they now redact. Security hardening: - Restore the empty-password guard centrally in `SecurityComplianceProvider::validate_password` (rejects an empty password on every write path). The per-DTO `length(min = 1)` check could not be kept on the wrapped field: validator's `custom` requires the field to be `Serialize`, which `SecretString` deliberately is not. Notes: - The Sea-ORM `oidc_client_secret` column stays plaintext `String`; the secret is unwrapped only at the final DB-write boundary. - `PartialEq`/`Default` are dropped where unused and re-implemented manually where tests rely on them (`IdentityProvider`, `UserPasswordAuthRequest`). - OpenAPI schema is byte-for-byte unchanged (fields still `string`). - Adds an adversarial redaction/breakage test suite (flatten+redact, deep-nested serialize, OPA-path, Debug matrix, empty-password guard, response-never-leaks, OIDC token redaction). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Yousef Hussein <ymh1874@gmail.com>
Address review feedback on the secret wrapping. Drop the SecretField newtype. `secrecy::SecretString` already keeps the value out of `Debug`, which is the reason the crate is used, so the DTO fields hold a `SecretString` directly and expose it for transport with a small `serialize_with` helper, the same way `K8sAuthRequest` does. - Keep api-types decoupled from core-types: the dependency is optional again and gated behind the builder/conv features as before, so a crate that only wants the API types does not pull it in. api-types uses `secrecy` directly, which it already depended on. - Drop `PartialEq` from the secret-bearing structs. `SecretString` does not implement it and `K8sAuthRequest` does not derive it either, so there is no hand-written impl to forget a field. The two driver tests that compared a whole `IdentityProvider` now compare fields. - Wrap the TOTP `passcode` and the token-auth `id`, and skip the domain identity provider's client secret on serialize since it is never returned. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Yousef Hussein <ymh1874@gmail.com>
Signed-off-by: Yousef Hussein <ymh1874@gmail.com>
Resolve gtema's review round on PR openstack-experimental#912 (openstack-experimental#369): - Policy input now omits secrets entirely instead of masking them with "[REDACTED]"; route the v3/v4 user and identity provider handlers through to_policy_input(). - Fix a plaintext password leak into OPA from the v4 user create/update handlers, which passed the raw DTO. - Keep secret validation via a struct-level schema fn that never serializes the secret. validator 0.20 cannot attach a field-level validator to a SecretString (it requires Serialize and would leak the value); documented inline. - Pass the SecretString down in auth_conv instead of exposing it at conversion time. - Add regression tests asserting secrets never reach policy input and that non-secret field validation still applies. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DPZxPWH9yYH36jgfwGqZdQ Signed-off-by: Yousef Hussein <ymh1874@gmail.com>
7c36074 to
40826ca
Compare
TL;DR
Wraps every remaining plaintext secret in the codebase — the OIDC
client_secret, user/auth passwords (all the way up to the wire), the configinvalid_password_hash_key, and the OIDC token-exchange bearer tokens — insecrecy::SecretString, so they can never leak throughDebug,#[tracing::instrument], or accidental serialization. Secrets that must appear inOPA policy / audit payloads are redacted to
"[REDACTED]"instead of exposed.Along the way this fixes three pre-existing plaintext leaks into OPA policy
input and closes an empty-password acceptance gap. The public OpenAPI schema
is byte-for-byte unchanged, and the change is covered by an adversarial
redaction/breakage test suite. Part of #369; delivered as one PR because touching
api-typespulls through every downstream crate.Background
secrecywas already a workspace dependency with an established pattern(
database.connection: SecretString), and passwords were already wrapped at theprovider/backend layers. This PR closes the remaining gaps the audit found and
lifts the wrapping to a consistent edge-to-consumer boundary: a secret is
SecretStringfrom the moment it is deserialized off the wire (or read from theDB) until the single point where it is genuinely consumed (hash / verify / HTTP
form / DB write), where it is explicitly
.expose_secret()'d.What is wrapped (edge → consumer)
oidc_client_secretapi-typesDTO →core-types(3 structs) → SQL driver conversions → token-exchange consumer.oidc_client_idstays plaintext (per maintainer).UserCreate/UserUpdate/UserPassword+ the full nestedAuthRequesttree, and the mirrorcore-typesdomain structs, are nowSecretStringfrom deserialize down. Exposure happens only athash_password/verify_password/validate_password.invalid_password_hash_keySecurityComplianceProvider(mirrorsdatabase.connection).id_token/access_tokenTokenExchangeResponsebearer tokens, previously plaintextStringin aDebug-deriving struct; exposed only at the JWT-verify boundary.Redaction, not exposure
SecretStringintentionally does not implementSerialize. For the DTOs thatmust stay serializable (they are embedded in OPA policy input / audit payloads),
a
serialize_withhelper renders a present secret as a fixed"[REDACTED]"marker — field presence is preserved, the value never leaves. This is deliberately
different from the existing "expose once" helper used for the one-time API-key
token (that one is part of the API contract).
Pre-existing leaks this fixes
json!({"identity_provider": current}),json!({"user": req.user})(and thecreate/update variants) serialize the domain/DTO structs straight into OPA
policy input. Before this PR those payloads contained the plaintext
oidc_client_secretand userpassword. They now redact.Security hardening — empty-password guard
Wrapping the password meant the per-DTO
length(min = 1)validation could not bekept: validator 0.20's
customunconditionally callsValidationError::add_param(&field), which requires the field to beSerialize—and secrets deliberately are not. Without that guard, on a default deployment
(no
password_regex) an empty password would be accepted and stored (bcrypthashes an empty string fine).
Fixed by moving the non-empty check into the correct central layer —
SecurityComplianceProvider::validate_password— which runs on the wrapped valuefor every write path (create / update / change). This is strictly more secure
than before (previously only create was guarded).
Design decisions / notes
oidc_client_secretcolumn stays plaintextString. Wrapping theDB column would require custom
TryGetable/Into<Value>impls and would notstop SQL-parameter-level logging anyway; the DB write is the final consuming
method, where the secret is unwrapped.
PartialEq/Defaultare dropped from the wrapped structs where nothingrelies on them (matching the existing app-cred / k8s pattern), and
re-implemented manually where tests do rely on them (
IdentityProviderread model;
UserPasswordAuthRequest), preserving the prior public contract.#[schema(value_type = String / Option<String>)]keepsevery wrapped field documented exactly as before; the generated spec has zero
structural diff vs
main.SecretStringare fundamentally incompatible for field-levellength/custom validation (see the empty-password section) — documented inline so
a future pass doesn't reintroduce it.
Testing & verification
cargo check --workspace --all-targets --all-features✅cargo clippy --lib --tests --all-features✅ ·cargo fmt --check✅keystone, and the SQL drivers.
main— 0 structural changes.#[serde(flatten)] extra+ customserialize_with+skiponUserCreate/UserUpdate(the exact struct serialized to OPA) — password redacted,extrapreserved, via both
to_stringandto_value.AuthRequestserialize redaction (password 4 levels deep).Debugredaction matrix across every wrapped type, incl. the config hash key.Debugredaction; deserialize edge cases (null / absent)..expose_secret()call site re-audited — none feeds a log / print /format; each is a genuine consume boundary (hash, verify, DB write, HTTP form,
JWT verify).
Out of scope (follow-up #370)
The credential
blob(EC2 secret key / TOTP seed material) is also a secret but isoutside the maintainer's stated scope for this effort; left as a follow-up so it
isn't silently dropped.
🤖 Generated with Claude Code