From 86a9f4bfa4b705c54d80b462b7ce6662e89f6ff9 Mon Sep 17 00:00:00 2001 From: Yousef Hussein Date: Fri, 3 Jul 2026 20:00:21 +0300 Subject: [PATCH 1/7] feat(security): Wrap secrets with secrecy crate 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 #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 Signed-off-by: Yousef Hussein --- Cargo.lock | 1 + .../src/federation/identity_provider.rs | 107 ++++++++++-- crates/api-types/src/lib.rs | 1 + crates/api-types/src/secret_serde.rs | 157 ++++++++++++++++++ crates/api-types/src/v3/auth/token.rs | 100 ++++++++++- crates/api-types/src/v3/user.rs | 131 ++++++++++++++- crates/config/src/security_compliance.rs | 57 ++++++- .../src/federation/identity_provider.rs | 132 ++++++++++++++- crates/core-types/src/identity/user.rs | 61 +++++-- crates/core/src/identity/service.rs | 6 +- crates/federation-driver-sql/Cargo.toml | 1 + .../src/identity_provider.rs | 3 +- .../src/identity_provider/create.rs | 7 +- .../src/identity_provider/update.rs | 4 +- .../identity-driver-sql/src/authenticate.rs | 18 +- crates/identity-driver-sql/src/user/create.rs | 10 +- crates/identity-driver-sql/src/user/update.rs | 12 +- .../keystone/src/api/v3/auth/token/common.rs | 3 +- .../keystone/src/api/v3/auth/token/create.rs | 5 +- crates/keystone/src/federation/api/oidc.rs | 7 +- .../keystone/src/federation/api/oidc_utils.rs | 51 +++++- 21 files changed, 777 insertions(+), 97 deletions(-) create mode 100644 crates/api-types/src/secret_serde.rs diff --git a/Cargo.lock b/Cargo.lock index 75ed4f4b6..00c9ba8ee 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4858,6 +4858,7 @@ dependencies = [ "openstack-keystone-core-types", "sea-orm", "sea-orm-migration", + "secrecy", "serde_json", "tokio", "tracing", diff --git a/crates/api-types/src/federation/identity_provider.rs b/crates/api-types/src/federation/identity_provider.rs index 975f1e004..266b419d6 100644 --- a/crates/api-types/src/federation/identity_provider.rs +++ b/crates/api-types/src/federation/identity_provider.rs @@ -12,12 +12,14 @@ // // SPDX-License-Identifier: Apache-2.0 //! Federated identity provider types. +use secrecy::SecretString; use serde::{Deserialize, Serialize}; use serde_json::Value; #[cfg(feature = "validate")] use validator::Validate; use crate::Link; +use crate::secret_serde::{serialize_secret_redacted, serialize_secret_redacted_nested}; /// Identity provider data. #[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] @@ -122,7 +124,10 @@ pub struct IdentityProviderResponse { } /// Identity provider data. -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +/// +/// `PartialEq` is intentionally not derived: `oidc_client_secret` is wrapped in +/// [`SecretString`], which does not implement `PartialEq` by design. +#[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr( feature = "builder", derive(derive_builder::Builder), @@ -169,12 +174,21 @@ pub struct IdentityProviderCreate { pub oidc_client_id: Option, /// The oidc `client_secret` to use for the private client. It is never - /// returned back. - #[cfg_attr(feature = "builder", builder(default))] - #[cfg_attr(feature = "openapi", schema(nullable = false))] - #[serde(skip_serializing_if = "Option::is_none")] - #[cfg_attr(feature = "validate", validate(length(max = 255)))] - pub oidc_client_secret: Option, + /// returned back. Wrapped in [`SecretString`] to prevent accidental + /// exposure via Debug/tracing; redacted (never exposed) when serialized into + /// policy/audit payloads. + /// + /// Field-level `length` validation is intentionally omitted: `SecretString` + /// cannot use validator's built-in `length`/`custom` (the latter requires + /// the field to be `Serialize`, which secrets are not). + #[cfg_attr(feature = "builder", builder(default))] + #[cfg_attr(feature = "openapi", schema(value_type = String, nullable = false))] + #[serde( + default, + serialize_with = "serialize_secret_redacted", + skip_serializing_if = "Option::is_none" + )] + pub oidc_client_secret: Option, /// The oidc response mode. #[cfg_attr(feature = "builder", builder(default))] @@ -240,7 +254,10 @@ pub struct IdentityProviderCreate { } /// New identity provider data. -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +/// +/// `PartialEq` is intentionally not derived: `oidc_client_secret` is wrapped in +/// [`SecretString`], which does not implement `PartialEq` by design. +#[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr( feature = "builder", derive(derive_builder::Builder), @@ -271,10 +288,15 @@ pub struct IdentityProviderUpdate { #[cfg_attr(feature = "validate", validate(length(max = 255)))] pub oidc_client_id: Option>, - /// The new oidc `client_secret` to use for the private client. + /// The new oidc `client_secret` to use for the private client. Wrapped in + /// [`SecretString`] to prevent accidental exposure via Debug/tracing; + /// redacted (never exposed) when serialized into policy/audit payloads. + /// Field-level `length` validation is intentionally omitted (see + /// `IdentityProviderCreate`). #[cfg_attr(feature = "builder", builder(default))] - #[cfg_attr(feature = "validate", validate(length(max = 255)))] - pub oidc_client_secret: Option>, + #[cfg_attr(feature = "openapi", schema(value_type = Option))] + #[serde(default, serialize_with = "serialize_secret_redacted_nested")] + pub oidc_client_secret: Option>, /// The new oidc response mode. #[cfg_attr(feature = "builder", builder(default))] @@ -324,7 +346,10 @@ pub struct IdentityProviderUpdate { } /// Identity provider create request. -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +/// +/// `PartialEq` is not derived because the embedded `IdentityProviderCreate` +/// carries a `SecretString`. +#[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] #[cfg_attr(feature = "validate", derive(validator::Validate))] pub struct IdentityProviderCreateRequest { @@ -334,7 +359,10 @@ pub struct IdentityProviderCreateRequest { } /// Identity provider update request. -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +/// +/// `PartialEq` is not derived because the embedded `IdentityProviderUpdate` +/// carries a `SecretString`. +#[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] #[cfg_attr(feature = "validate", derive(validator::Validate))] pub struct IdentityProviderUpdateRequest { @@ -383,3 +411,56 @@ pub struct IdentityProviderListParameters { fn default_list_limit() -> Option { Some(20) } + +#[cfg(test)] +mod tests { + use super::*; + + /// The create DTO is serialized into OPA policy input + /// (`json!({"identity_provider": req.identity_provider})`). The client secret + /// must be redacted there. + #[test] + fn idp_create_redacts_client_secret_for_policy() { + let create: IdentityProviderCreate = + serde_json::from_str(r#"{"name":"idp","oidc_client_secret":"CSLEAK"}"#).unwrap(); + let rendered = serde_json::json!({ "identity_provider": &create }).to_string(); + assert!( + !rendered.contains("CSLEAK"), + "leaked client secret: {rendered}" + ); + assert!(rendered.contains("[REDACTED]"), "not redacted: {rendered}"); + } + + /// Same for the update DTO (nested `Option>`). + #[test] + fn idp_update_redacts_client_secret_for_policy() { + let update: IdentityProviderUpdate = + serde_json::from_str(r#"{"oidc_client_secret":"CSLEAK2"}"#).unwrap(); + let rendered = serde_json::json!({ "identity_provider": &update }).to_string(); + assert!( + !rendered.contains("CSLEAK2"), + "leaked client secret: {rendered}" + ); + assert!(rendered.contains("[REDACTED]"), "not redacted: {rendered}"); + } + + /// Regression guard: the read/response DTO has no client-secret field, so a + /// client secret can never be returned — even if one is (wrongly) supplied. + #[test] + fn identity_provider_response_never_exposes_client_secret() { + let idp: IdentityProvider = serde_json::from_str( + r#"{"id":"1","name":"idp","enabled":true, + "oidc_client_secret":"SHOULD_BE_IGNORED"}"#, + ) + .unwrap(); + let rendered = serde_json::to_string(&idp).unwrap(); + assert!( + !rendered.contains("client_secret"), + "response exposed a client_secret field: {rendered}" + ); + assert!( + !rendered.contains("SHOULD_BE_IGNORED"), + "response leaked the secret value: {rendered}" + ); + } +} diff --git a/crates/api-types/src/lib.rs b/crates/api-types/src/lib.rs index e9a72a79f..659b0246b 100644 --- a/crates/api-types/src/lib.rs +++ b/crates/api-types/src/lib.rs @@ -30,6 +30,7 @@ pub mod k8s_auth; pub mod scope; #[cfg(feature = "conv")] mod scope_conv; +pub(crate) mod secret_serde; pub mod trust; pub mod v3; pub mod v4; diff --git a/crates/api-types/src/secret_serde.rs b/crates/api-types/src/secret_serde.rs new file mode 100644 index 000000000..5ba2cc87a --- /dev/null +++ b/crates/api-types/src/secret_serde.rs @@ -0,0 +1,157 @@ +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// SPDX-License-Identifier: Apache-2.0 +//! Serialization helpers for [`secrecy::SecretString`] fields. +//! +//! `SecretString` deliberately does not implement `Serialize`. These helpers +//! let DTOs that must remain serializable (e.g. for policy/audit payloads that +//! embed the request or resource) do so **without ever exposing the secret** — +//! a present secret is rendered as a fixed `"[REDACTED]"` marker so field +//! presence is preserved while the value never leaks. +//! +//! This is intentionally different from the "expose once" helper used for +//! secrets that are part of an API contract (e.g. a one-time API-key token): +//! those live next to their own DTO and call `expose_secret()` on purpose. + +use secrecy::SecretString; +use serde::Serializer; + +// NOTE: length/non-empty validation for wrapped secrets is deliberately NOT done +// with validator's field-level `custom` here: validator 0.20 unconditionally +// calls `ValidationError::add_param(&field)`, which requires the field to be +// `Serialize` — and `SecretString` intentionally is not. Password non-emptiness +// is instead enforced centrally in +// `openstack_keystone_config::SecurityComplianceProvider::validate_password`, +// which runs on the wrapped value at the service layer for every write path. + +/// Redact an `Option` on serialize. +pub(crate) fn serialize_secret_redacted( + secret: &Option, + serializer: S, +) -> Result +where + S: Serializer, +{ + match secret { + Some(_) => serializer.serialize_str("[REDACTED]"), + None => serializer.serialize_none(), + } +} + +/// Redact a required `SecretString` on serialize. +pub(crate) fn serialize_secret_redacted_required( + _secret: &SecretString, + serializer: S, +) -> Result +where + S: Serializer, +{ + serializer.serialize_str("[REDACTED]") +} + +/// Redact the nested `Option>` used by update DTOs, where +/// the outer `Option` is "present-in-request" and the inner is "set-or-clear". +pub(crate) fn serialize_secret_redacted_nested( + secret: &Option>, + serializer: S, +) -> Result +where + S: Serializer, +{ + match secret { + Some(Some(_)) => serializer.serialize_str("[REDACTED]"), + _ => serializer.serialize_none(), + } +} + +#[cfg(test)] +mod tests { + use secrecy::SecretString; + use serde::Serialize; + + use super::*; + + const SECRET: &str = "super-secret-value"; + + #[derive(Serialize)] + struct OptHolder { + #[serde(serialize_with = "serialize_secret_redacted")] + secret: Option, + } + + #[derive(Serialize)] + struct RequiredHolder { + #[serde(serialize_with = "serialize_secret_redacted_required")] + secret: SecretString, + } + + #[derive(Serialize)] + struct NestedHolder { + #[serde(serialize_with = "serialize_secret_redacted_nested")] + secret: Option>, + } + + #[test] + fn opt_secret_is_redacted_when_present() { + let json = serde_json::to_string(&OptHolder { + secret: Some(SecretString::from(SECRET)), + }) + .unwrap(); + assert!(!json.contains(SECRET), "secret leaked: {json}"); + assert!(json.contains("[REDACTED]"), "not redacted: {json}"); + } + + #[test] + fn opt_secret_is_null_when_absent() { + let json = serde_json::to_string(&OptHolder { secret: None }).unwrap(); + assert_eq!(json, r#"{"secret":null}"#); + } + + #[test] + fn required_secret_is_redacted() { + let json = serde_json::to_string(&RequiredHolder { + secret: SecretString::from(SECRET), + }) + .unwrap(); + assert!(!json.contains(SECRET), "secret leaked: {json}"); + assert!(json.contains("[REDACTED]"), "not redacted: {json}"); + } + + #[test] + fn nested_secret_is_redacted_only_when_set() { + // Present + set -> redacted. + let set = serde_json::to_string(&NestedHolder { + secret: Some(Some(SecretString::from(SECRET))), + }) + .unwrap(); + assert!(!set.contains(SECRET), "secret leaked: {set}"); + assert!(set.contains("[REDACTED]"), "not redacted: {set}"); + + // Explicit clear and absent both serialize to null (no value to leak). + assert_eq!( + serde_json::to_string(&NestedHolder { secret: Some(None) }).unwrap(), + r#"{"secret":null}"# + ); + assert_eq!( + serde_json::to_string(&NestedHolder { secret: None }).unwrap(), + r#"{"secret":null}"# + ); + } + + #[test] + fn debug_of_secret_does_not_leak() { + // secrecy's own Debug guarantee, pinned here as the core requirement. + let secret = SecretString::from(SECRET); + assert!(!format!("{secret:?}").contains(SECRET)); + } +} diff --git a/crates/api-types/src/v3/auth/token.rs b/crates/api-types/src/v3/auth/token.rs index 747dcf185..216eea009 100644 --- a/crates/api-types/src/v3/auth/token.rs +++ b/crates/api-types/src/v3/auth/token.rs @@ -13,12 +13,14 @@ // SPDX-License-Identifier: Apache-2.0 use chrono::{DateTime, Utc}; +use secrecy::SecretString; use serde::{Deserialize, Serialize}; #[cfg(feature = "validate")] use validator::Validate; use crate::catalog::*; use crate::scope::*; +use crate::secret_serde::serialize_secret_redacted_required; use crate::trust::TokenTrustRepr; use crate::v3::role::RoleRef; @@ -131,7 +133,10 @@ pub struct TokenResponse { } /// An authentication request. -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +/// +/// `PartialEq` is not derived: the embedded identity carries a `SecretString` +/// password. +#[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] #[cfg_attr(feature = "validate", derive(validator::Validate))] pub struct AuthRequest { @@ -141,7 +146,10 @@ pub struct AuthRequest { } /// An authentication request. -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +/// +/// `PartialEq` is not derived: the embedded identity carries a `SecretString` +/// password. +#[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] #[cfg_attr(feature = "validate", derive(validator::Validate))] pub struct AuthRequestInner { @@ -164,7 +172,9 @@ pub struct AuthRequestInner { } /// An identity object. -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +/// +/// `PartialEq` is not derived: the embedded password carries a `SecretString`. +#[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr( feature = "builder", derive(derive_builder::Builder), @@ -198,7 +208,10 @@ pub struct Identity { } /// The password object, contains the authentication information. -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +/// +/// `PartialEq` is not derived: the embedded user carries a `SecretString` +/// password. +#[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr( feature = "builder", derive(derive_builder::Builder), @@ -216,7 +229,10 @@ pub struct PasswordAuth { } /// User password information. -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +/// +/// `PartialEq` is intentionally not derived: `password` is wrapped in +/// [`SecretString`], which does not implement `PartialEq` by design. +#[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr( feature = "builder", derive(derive_builder::Builder), @@ -240,9 +256,11 @@ pub struct UserPassword { #[cfg_attr(feature = "builder", builder(default))] #[cfg_attr(feature = "validate", validate(nested))] pub domain: Option, - /// User password. - #[cfg_attr(feature = "validate", validate(length(max = 255)))] - pub password: String, + /// User password. Wrapped in [`SecretString`] to prevent accidental + /// exposure via Debug/tracing; redacted (never exposed) when serialized. + #[cfg_attr(feature = "openapi", schema(value_type = String))] + #[serde(serialize_with = "serialize_secret_redacted_required")] + pub password: SecretString, } /// The TOTP object, contains the authentication information. @@ -376,3 +394,69 @@ pub struct System { /// All. pub all: bool, } + +#[cfg(test)] +mod tests { + use secrecy::ExposeSecret; + + use super::*; + + const PWD: &str = "hunter2-plaintext"; + + #[test] + fn user_password_deserializes_plaintext_but_never_leaks() { + // Plaintext arrives on the wire and is accepted into the SecretString. + let up: UserPassword = + serde_json::from_str(&format!(r#"{{"name":"alice","password":"{PWD}"}}"#)).unwrap(); + assert_eq!(up.password.expose_secret(), PWD); + + // Debug must not leak (the #[instrument] / log vector). + assert!( + !format!("{up:?}").contains(PWD), + "Debug leaked the password" + ); + + // Serialization (e.g. into a policy/audit payload) must redact. + let json = serde_json::to_string(&up).unwrap(); + assert!(!json.contains(PWD), "serialize leaked the password: {json}"); + assert!(json.contains("[REDACTED]"), "password not redacted: {json}"); + } + + #[test] + fn full_auth_request_debug_does_not_leak_password() { + let req = nested_auth_request(); + // Debug of the whole nested request tree must not leak the password. + assert!( + !format!("{req:?}").contains(PWD), + "Debug leaked via nested request" + ); + } + + #[test] + fn full_auth_request_serialize_redacts_password_at_depth() { + // The password sits 4 levels deep (auth -> identity -> password -> user). + // Prove redaction survives the whole nested Serialize tree, via both + // `to_string` and `to_value`. + let req = nested_auth_request(); + let as_string = serde_json::to_string(&req).unwrap(); + let as_value = serde_json::to_value(&req).unwrap().to_string(); + for rendered in [as_string, as_value] { + assert!( + !rendered.contains(PWD), + "serialize leaked password at depth: {rendered}" + ); + assert!( + rendered.contains("[REDACTED]"), + "nested password not redacted: {rendered}" + ); + } + } + + fn nested_auth_request() -> AuthRequest { + serde_json::from_str(&format!( + r#"{{"auth":{{"identity":{{"methods":["password"], + "password":{{"user":{{"name":"alice","password":"{PWD}"}}}}}}}}}}"# + )) + .unwrap() + } +} diff --git a/crates/api-types/src/v3/user.rs b/crates/api-types/src/v3/user.rs index b515628fb..1ea4a6ca9 100644 --- a/crates/api-types/src/v3/user.rs +++ b/crates/api-types/src/v3/user.rs @@ -14,11 +14,14 @@ use std::collections::HashMap; use chrono::{DateTime, Utc}; +use secrecy::SecretString; use serde::{Deserialize, Serialize}; use serde_json::Value; #[cfg(feature = "validate")] use validator::Validate; +use crate::secret_serde::serialize_secret_redacted; + /// User response object. #[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] #[cfg_attr( @@ -102,7 +105,10 @@ pub struct UserResponse { } /// Create user data. -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +/// +/// `PartialEq` is intentionally not derived: `password` is wrapped in +/// [`SecretString`], which does not implement `PartialEq` by design. +#[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr( feature = "builder", derive(derive_builder::Builder), @@ -154,14 +160,26 @@ pub struct UserCreate { #[cfg_attr(feature = "validate", validate(nested))] pub options: Option, - /// The password for the user. + /// The password for the user. Wrapped in [`SecretString`] to prevent + /// accidental exposure via Debug/tracing; redacted (never exposed) when + /// serialized into policy/audit payloads. Non-emptiness and regex policy are + /// enforced on the wrapped value at the service layer via + /// `security_compliance.validate_password`. #[cfg_attr(feature = "builder", builder(default))] - #[cfg_attr(feature = "validate", validate(length(min = 1, max = 72)))] - pub password: Option, + #[cfg_attr(feature = "openapi", schema(value_type = Option))] + #[serde( + default, + serialize_with = "serialize_secret_redacted", + skip_serializing_if = "Option::is_none" + )] + pub password: Option, } /// Complete create user request. -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +/// +/// `PartialEq` is not derived because the embedded `UserCreate` carries a +/// `SecretString`. +#[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] #[cfg_attr(feature = "validate", derive(validator::Validate))] pub struct UserCreateRequest { @@ -171,7 +189,10 @@ pub struct UserCreateRequest { } /// Update user data. -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +/// +/// `PartialEq` is intentionally not derived: `password` is wrapped in +/// [`SecretString`], which does not implement `PartialEq` by design. +#[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr( feature = "builder", derive(derive_builder::Builder), @@ -220,13 +241,24 @@ pub struct UserUpdate { #[cfg_attr(feature = "validate", validate(nested))] pub options: Option, - /// The password for the user. + /// The password for the user. Wrapped in [`SecretString`] to prevent + /// accidental exposure via Debug/tracing; redacted (never exposed) when + /// serialized into policy/audit payloads. #[cfg_attr(feature = "builder", builder(default))] - pub password: Option, + #[cfg_attr(feature = "openapi", schema(value_type = Option))] + #[serde( + default, + serialize_with = "serialize_secret_redacted", + skip_serializing_if = "Option::is_none" + )] + pub password: Option, } /// Complete update user request. -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +/// +/// `PartialEq` is not derived because the embedded `UserUpdate` carries a +/// `SecretString`. +#[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] #[cfg_attr(feature = "validate", derive(validator::Validate))] pub struct UserUpdateRequest { @@ -313,8 +345,89 @@ pub struct UserListParameters { #[cfg(test)] mod tests { + use secrecy::ExposeSecret; + use super::*; + /// Critical: `UserCreate` carries BOTH `#[serde(flatten)] extra` and a + /// `password` with a custom `serialize_with` + `skip_serializing_if`. This is + /// the exact struct the handler serializes into OPA policy input + /// (`json!({"user": req.user})`). Prove the flatten interaction does not + /// leak the password and does not drop `extra`. + #[test] + fn usercreate_flatten_redacts_password_and_keeps_extra() { + let uc: UserCreate = serde_json::from_str( + r#"{"domain_id":"d","name":"alice","enabled":true, + "password":"PWLEAK","x_custom":"xval","y_custom":"yval"}"#, + ) + .unwrap(); + // sanity: password wrapped, extra captured via flatten + assert_eq!( + uc.password.as_ref().map(|s| s.expose_secret()), + Some("PWLEAK") + ); + assert_eq!( + uc.extra.get("x_custom").and_then(|v| v.as_str()), + Some("xval") + ); + + // Both the raw serialize and the OPA-shaped `json!({"user": uc})` path. + let direct = serde_json::to_string(&uc).unwrap(); + let opa = serde_json::json!({ "user": &uc }).to_string(); + for rendered in [direct, opa] { + assert!( + !rendered.contains("PWLEAK"), + "flatten leaked password: {rendered}" + ); + assert!( + rendered.contains("[REDACTED]"), + "password not redacted: {rendered}" + ); + assert!( + rendered.contains("x_custom") && rendered.contains("xval"), + "extra dropped by flatten+redact: {rendered}" + ); + assert!( + rendered.contains("y_custom") && rendered.contains("yval"), + "extra dropped by flatten+redact: {rendered}" + ); + } + } + + /// `UserUpdate` has the same flatten+redact shape. + #[test] + fn userupdate_flatten_redacts_password_and_keeps_extra() { + let uu: UserUpdate = + serde_json::from_str(r#"{"password":"UPWLEAK","z_extra":"zz"}"#).unwrap(); + let rendered = serde_json::json!({ "user": &uu }).to_string(); + assert!(!rendered.contains("UPWLEAK"), "leaked: {rendered}"); + assert!(rendered.contains("[REDACTED]"), "not redacted: {rendered}"); + assert!(rendered.contains("z_extra"), "extra dropped: {rendered}"); + } + + /// Explicit `null` and absent password both deserialize to `None` (no panic, + /// no plaintext resurrected). + #[test] + fn usercreate_password_null_and_absent_are_none() { + let with_null: UserCreate = + serde_json::from_str(r#"{"domain_id":"d","name":"a","enabled":true,"password":null}"#) + .unwrap(); + assert!(with_null.password.is_none()); + let absent: UserCreate = + serde_json::from_str(r#"{"domain_id":"d","name":"a","enabled":true}"#).unwrap(); + assert!(absent.password.is_none()); + } + + /// Debug of a populated `UserCreate` never renders the password. + #[test] + fn usercreate_debug_does_not_leak_password() { + let uc: UserCreate = serde_json::from_str( + r#"{"domain_id":"d","name":"a","enabled":true,"password":"DBGLEAK"}"#, + ) + .unwrap(); + assert!(!format!("{uc:?}").contains("DBGLEAK")); + } + #[cfg(feature = "builder")] #[test] fn test_user_create() { diff --git a/crates/config/src/security_compliance.rs b/crates/config/src/security_compliance.rs index f0b87a7fa..df99b998e 100644 --- a/crates/config/src/security_compliance.rs +++ b/crates/config/src/security_compliance.rs @@ -67,8 +67,11 @@ pub struct SecurityComplianceProvider { /// used in distributed installations working with the same backend, to make /// them all generate equal hashes for equal invalid passwords). 16 bytes /// (128 bits) or more is recommended. + /// + /// Wrapped in [`SecretString`] to prevent accidental exposure via + /// Debug/tracing (mirrors `database.connection`). #[serde(default)] - pub invalid_password_hash_key: Option, + pub invalid_password_hash_key: Option, /// This option has a sample default set, which means that its actual /// default value may vary from the one documented above. /// @@ -225,16 +228,27 @@ impl SecurityComplianceProvider { Ok(()) } - /// Validate a password against the configured regex pattern. + /// Validate a password against the security-compliance policy. + /// + /// An empty password is **always** rejected, regardless of configuration — + /// this is the central guard for every write path (create/update/change), + /// and replaces the per-DTO `length(min = 1)` check that could not be kept on + /// the wrapped [`SecretString`] fields (validator's `custom`/`length` require + /// the field to be `Serialize`, which secrets deliberately are not). /// - /// Returns `Ok(())` when the password matches the configured pattern, or - /// when no pattern is configured. Returns - /// `Err(SecurityComplianceError::PasswordInvalid)` - /// with the human-readable policy description on mismatch. + /// Beyond that, returns `Ok(())` when the password matches the configured + /// regex pattern (or when no pattern is configured), otherwise + /// `Err(SecurityComplianceError::PasswordInvalid)` with the human-readable + /// policy description. pub fn validate_password( &self, password: &SecretString, ) -> Result<(), SecurityComplianceError> { + if password.expose_secret().is_empty() { + return Err(SecurityComplianceError::PasswordInvalid( + "password must not be empty".to_string(), + )); + } if let Some(ref re) = self.password_regex_re && !re.is_match(password.expose_secret()) { @@ -405,4 +419,35 @@ mod tests { assert!(cfg.compile_regex().is_ok()); assert!(cfg.password_regex_re.is_none()); } + + /// Regression guard for #369: wrapping the password in `SecretString` meant + /// the per-DTO `length(min = 1)` guard could not be kept (validator's + /// `custom` needs the field to be `Serialize`). The non-empty check moved + /// here and MUST reject an empty password even when no regex is configured. + #[test] + fn validate_password_rejects_empty_without_regex() { + let sc = SecurityComplianceProvider::default(); + assert!(sc.password_regex_re.is_none(), "precondition: no regex"); + let result = sc.validate_password(&SecretString::from("")); + assert!( + matches!(result, Err(SecurityComplianceError::PasswordInvalid(_))), + "empty password must be rejected without a regex, got {result:?}" + ); + // A non-empty password still passes when no policy is configured. + assert!(sc.validate_password(&SecretString::from("x")).is_ok()); + } + + /// The `invalid_password_hash_key` secret must never render under `Debug` + /// (Sea-ORM/tracing could otherwise log the whole config section). + #[test] + fn invalid_password_hash_key_redacts_under_debug() { + let sc = SecurityComplianceProvider { + invalid_password_hash_key: Some(SecretString::from("HASHKEYLEAK")), + ..Default::default() + }; + assert!( + !format!("{sc:?}").contains("HASHKEYLEAK"), + "invalid_password_hash_key leaked via Debug" + ); + } } diff --git a/crates/core-types/src/federation/identity_provider.rs b/crates/core-types/src/federation/identity_provider.rs index 2405f3521..88886d0ee 100644 --- a/crates/core-types/src/federation/identity_provider.rs +++ b/crates/core-types/src/federation/identity_provider.rs @@ -14,13 +14,32 @@ //! # Federated identity provider types use derive_builder::Builder; -use serde::Serialize; +use secrecy::{ExposeSecret, SecretString}; +use serde::{Serialize, Serializer}; use serde_json::Value; use crate::error::BuilderError; +/// Serialize an optional secret as a fixed redaction marker so that it never +/// leaks in Debug/policy/audit payloads while still signalling presence. +fn serialize_secret_redacted( + secret: &Option, + serializer: S, +) -> Result +where + S: Serializer, +{ + match secret { + Some(_) => serializer.serialize_str("[REDACTED]"), + None => serializer.serialize_none(), + } +} + /// Identity provider resource. -#[derive(Builder, Clone, Debug, Default, Serialize, PartialEq)] +/// +/// `PartialEq` is intentionally not derived: `oidc_client_secret` is wrapped in +/// [`SecretString`], which does not implement `PartialEq` by design. +#[derive(Builder, Clone, Debug, Default, Serialize)] #[builder(build_fn(error = "BuilderError"))] #[builder(setter(strip_option, into))] pub struct IdentityProvider { @@ -45,8 +64,12 @@ pub struct IdentityProvider { #[builder(default)] pub oidc_client_id: Option, + /// The OIDC client secret. Wrapped in [`SecretString`] to prevent accidental + /// exposure via Debug/tracing; redacted (never exposed) when serialized into + /// policy/audit payloads. #[builder(default)] - pub oidc_client_secret: Option, + #[serde(serialize_with = "serialize_secret_redacted")] + pub oidc_client_secret: Option, #[builder(default)] pub oidc_response_mode: Option, @@ -81,8 +104,42 @@ pub struct IdentityProvider { pub provider_config: Option, } +/// Manual `PartialEq` (the derive cannot be used because `oidc_client_secret` +/// is a [`SecretString`], which does not implement `PartialEq`). Preserves the +/// pre-wrapping equality contract by comparing the exposed secret values. +impl PartialEq for IdentityProvider { + fn eq(&self, other: &Self) -> bool { + self.id == other.id + && self.name == other.name + && self.domain_id == other.domain_id + && self.enabled == other.enabled + && self.oidc_discovery_url == other.oidc_discovery_url + && self.oidc_client_id == other.oidc_client_id + && self + .oidc_client_secret + .as_ref() + .map(ExposeSecret::expose_secret) + == other + .oidc_client_secret + .as_ref() + .map(ExposeSecret::expose_secret) + && self.oidc_response_mode == other.oidc_response_mode + && self.oidc_response_types == other.oidc_response_types + && self.jwks_url == other.jwks_url + && self.jwt_validation_pubkeys == other.jwt_validation_pubkeys + && self.bound_issuer == other.bound_issuer + && self.default_mapping_name == other.default_mapping_name + && self.oidc_scopes == other.oidc_scopes + && self.allowed_redirect_uris == other.allowed_redirect_uris + && self.provider_config == other.provider_config + } +} + /// New Identity provider data. -#[derive(Builder, Clone, Debug, Default, PartialEq)] +/// +/// `PartialEq` is intentionally not derived: `oidc_client_secret` is wrapped in +/// [`SecretString`], which does not implement `PartialEq` by design. +#[derive(Builder, Clone, Debug, Default)] #[builder(build_fn(error = "BuilderError"))] #[builder(setter(strip_option, into))] pub struct IdentityProviderCreate { @@ -107,8 +164,10 @@ pub struct IdentityProviderCreate { #[builder(default)] pub oidc_client_id: Option, + /// The OIDC client secret. Wrapped in [`SecretString`] to prevent accidental + /// exposure via Debug/tracing. #[builder(default)] - pub oidc_client_secret: Option, + pub oidc_client_secret: Option, #[builder(default)] pub oidc_response_mode: Option, @@ -141,7 +200,10 @@ pub struct IdentityProviderCreate { } /// Identity provider update data. -#[derive(Builder, Clone, Debug, Default, PartialEq)] +/// +/// `PartialEq` is intentionally not derived: `oidc_client_secret` is wrapped in +/// [`SecretString`], which does not implement `PartialEq` by design. +#[derive(Builder, Clone, Debug, Default)] #[builder(build_fn(error = "BuilderError"))] #[builder(setter(into))] pub struct IdentityProviderUpdate { @@ -157,8 +219,11 @@ pub struct IdentityProviderUpdate { #[builder(default)] pub oidc_client_id: Option>, + /// The OIDC client secret. Wrapped in [`SecretString`] to prevent accidental + /// exposure via Debug/tracing. Outer `Option` = present-in-request, + /// inner `Option` = set-or-clear. #[builder(default)] - pub oidc_client_secret: Option>, + pub oidc_client_secret: Option>, #[builder(default)] pub oidc_response_mode: Option>, @@ -211,3 +276,56 @@ pub struct IdentityProviderListParameters { /// Filters the response by IDP name. pub name: Option, } + +#[cfg(test)] +mod tests { + use super::*; + + const SECRET: &str = "oidc-top-secret"; + + #[test] + fn identity_provider_never_leaks_client_secret() { + let idp = IdentityProviderBuilder::default() + .id("1") + .name("idp") + .oidc_client_secret(SECRET) + .build() + .unwrap(); + + // Debug (the #[instrument] / log vector) must not leak. + assert!( + !format!("{idp:?}").contains(SECRET), + "Debug leaked client secret" + ); + + // Serialization into the OPA policy / audit payload must redact. + let json = serde_json::to_string(&idp).unwrap(); + assert!( + !json.contains(SECRET), + "serialize leaked client secret: {json}" + ); + assert!( + json.contains("[REDACTED]"), + "client secret not redacted: {json}" + ); + } + + #[test] + fn partial_eq_still_compares_client_secret() { + let base = IdentityProviderBuilder::default() + .id("1") + .name("idp") + .oidc_client_secret(SECRET) + .build() + .unwrap(); + let same = base.clone(); + let different = IdentityProviderBuilder::default() + .id("1") + .name("idp") + .oidc_client_secret("other-secret") + .build() + .unwrap(); + assert_eq!(base, same); + assert_ne!(base, different); + } +} diff --git a/crates/core-types/src/identity/user.rs b/crates/core-types/src/identity/user.rs index 95f9fadb1..168762eb6 100644 --- a/crates/core-types/src/identity/user.rs +++ b/crates/core-types/src/identity/user.rs @@ -15,12 +15,19 @@ use std::collections::HashMap; use chrono::{DateTime, Utc}; use derive_builder::Builder; +use secrecy::SecretString; use serde::Serialize; use serde_json::Value; use validator::Validate; use crate::error::BuilderError; +// NOTE: password length/non-emptiness is not validated with a field-level +// validator here — `SecretString` cannot use validator's `length` (no +// `ValidateLength`) nor `custom` (which requires the field to be `Serialize`). +// Non-emptiness is enforced centrally on the wrapped value at the service layer +// via `security_compliance.validate_password`. + #[derive(Builder, Clone, Debug, PartialEq, Serialize, Validate)] #[builder(build_fn(error = "BuilderError"))] #[builder(setter(strip_option, into))] @@ -75,7 +82,10 @@ pub struct UserResponse { } /// User creation data. -#[derive(Builder, Clone, Debug, PartialEq, Validate)] +/// +/// `PartialEq` is intentionally not derived: `password` is wrapped in +/// [`SecretString`], which does not implement `PartialEq` by design. +#[derive(Builder, Clone, Debug, Validate)] #[builder(build_fn(error = "BuilderError"))] #[builder(setter(strip_option, into))] pub struct UserCreate { @@ -119,10 +129,12 @@ pub struct UserCreate { #[validate(nested)] pub options: Option, - /// User password. + /// User password. Wrapped in [`SecretString`] to prevent accidental + /// exposure via Debug/tracing. Non-emptiness and regex policy are enforced on + /// the wrapped value at the service layer via + /// `security_compliance.validate_password`. #[builder(default)] - #[validate(length(max = 72))] - pub password: Option, + pub password: Option, /// The kind of local-authentication row to create for the user: /// `Local` creates a `local_user` row (password allowed), `NonLocal` @@ -133,7 +145,11 @@ pub struct UserCreate { pub user_type: UserType, } -#[derive(Builder, Clone, Debug, Default, PartialEq, Validate)] +/// User update data. +/// +/// `PartialEq` is intentionally not derived: `password` is wrapped in +/// [`SecretString`], which does not implement `PartialEq` by design. +#[derive(Builder, Clone, Debug, Default, Validate)] #[builder(build_fn(error = "BuilderError"))] #[builder(setter(strip_option, into))] pub struct UserUpdate { @@ -169,10 +185,11 @@ pub struct UserUpdate { #[validate(nested)] pub options: Option, - /// New user password. + /// New user password. Wrapped in [`SecretString`] to prevent accidental + /// exposure via Debug/tracing. Non-emptiness/policy enforced at the service + /// layer via `security_compliance.validate_password`. #[builder(default)] - #[validate(length(max = 72))] - pub password: Option, + pub password: Option, } /// User options. @@ -279,7 +296,10 @@ pub enum UserType { } /// User password information. -#[derive(Builder, Clone, Debug, Default, PartialEq, Validate)] +/// +/// `Default` and `PartialEq` are intentionally not derived: `password` is a +/// required [`SecretString`], which implements neither by design. +#[derive(Builder, Clone, Debug, Validate)] #[builder(build_fn(error = "BuilderError"))] #[builder(setter(strip_option, into))] pub struct UserPasswordAuthRequest { @@ -298,10 +318,25 @@ pub struct UserPasswordAuthRequest { #[validate(nested)] pub domain: Option, - /// User password expiry date. - #[builder(default)] - #[validate(length(max = 72))] - pub password: String, + /// User password. Wrapped in [`SecretString`] to prevent accidental + /// exposure via Debug/tracing. Required (no builder default: `SecretString` + /// does not implement `Default`). + pub password: SecretString, +} + +/// Manual `Default` (the derive cannot be used because `SecretString` does not +/// implement `Default`). Preserves the pre-wrapping default of an empty +/// password. Production code constructs this via the builder, which requires an +/// explicit password. +impl Default for UserPasswordAuthRequest { + fn default() -> Self { + Self { + id: None, + name: None, + domain: None, + password: SecretString::from(""), + } + } } /// User TOTP authentication request. diff --git a/crates/core/src/identity/service.rs b/crates/core/src/identity/service.rs index e2cec1698..58d8920ff 100644 --- a/crates/core/src/identity/service.rs +++ b/crates/core/src/identity/service.rs @@ -589,8 +589,7 @@ impl IdentityApi for IdentityService { // Validate password against configured regex pattern. if let Some(ref password) = mod_user.password { let cfg = ctx.state().config_manager.config.read().await; - cfg.security_compliance - .validate_password(&SecretString::from(password.as_str()))?; + cfg.security_compliance.validate_password(password)?; } let user = if let Some(vsc) = ctx.ctx() { let backend_driver = &self.backend_driver; @@ -1257,8 +1256,7 @@ impl IdentityApi for IdentityService { // Validate password against configured regex pattern. if let Some(ref password) = user.password { let cfg = ctx.state().config_manager.config.read().await; - cfg.security_compliance - .validate_password(&SecretString::from(password.as_str()))?; + cfg.security_compliance.validate_password(password)?; } let user = if let Some(vsc) = ctx.ctx() { let backend_driver = &self.backend_driver; diff --git a/crates/federation-driver-sql/Cargo.toml b/crates/federation-driver-sql/Cargo.toml index b338bae68..d1e52764e 100644 --- a/crates/federation-driver-sql/Cargo.toml +++ b/crates/federation-driver-sql/Cargo.toml @@ -17,6 +17,7 @@ inventory.workspace = true openstack-keystone-core.workspace = true openstack-keystone-core-types.workspace = true sea-orm = { workspace = true, features = ["default"] } +secrecy.workspace = true sea-orm-migration.workspace = true serde_json.workspace = true tracing.workspace = true diff --git a/crates/federation-driver-sql/src/identity_provider.rs b/crates/federation-driver-sql/src/identity_provider.rs index 2757b6572..8197a9055 100644 --- a/crates/federation-driver-sql/src/identity_provider.rs +++ b/crates/federation-driver-sql/src/identity_provider.rs @@ -47,7 +47,8 @@ impl TryFrom for IdentityProvider { builder.oidc_client_id(val); } if let Some(val) = &value.oidc_client_secret { - builder.oidc_client_secret(val); + // Wrap the plaintext DB value back into a SecretString on read. + builder.oidc_client_secret(val.as_str()); } if let Some(val) = &value.oidc_response_mode { builder.oidc_response_mode(val); diff --git a/crates/federation-driver-sql/src/identity_provider/create.rs b/crates/federation-driver-sql/src/identity_provider/create.rs index b59a80945..477c0ed45 100644 --- a/crates/federation-driver-sql/src/identity_provider/create.rs +++ b/crates/federation-driver-sql/src/identity_provider/create.rs @@ -16,6 +16,7 @@ use sea_orm::DatabaseConnection; use sea_orm::TransactionTrait; use sea_orm::entity::*; use sea_orm::sea_query::OnConflict; +use secrecy::ExposeSecret; use uuid::Uuid; use openstack_keystone_core::error::DbContextExt; @@ -61,10 +62,12 @@ pub async fn create( .unwrap_or(NotSet) .into(), oidc_client_id: idp.oidc_client_id.clone().map(Set).unwrap_or(NotSet).into(), + // Exposure boundary: the secret is persisted as plaintext in the DB + // column, so it is unwrapped only here at the final storage write. oidc_client_secret: idp .oidc_client_secret - .clone() - .map(Set) + .as_ref() + .map(|s| Set(s.expose_secret().to_string())) .unwrap_or(NotSet) .into(), oidc_response_mode: idp diff --git a/crates/federation-driver-sql/src/identity_provider/update.rs b/crates/federation-driver-sql/src/identity_provider/update.rs index 62ed1902f..8926e62fe 100644 --- a/crates/federation-driver-sql/src/identity_provider/update.rs +++ b/crates/federation-driver-sql/src/identity_provider/update.rs @@ -15,6 +15,7 @@ use sea_orm::DatabaseConnection; use sea_orm::TransactionTrait; use sea_orm::entity::*; +use secrecy::ExposeSecret; use openstack_keystone_core::error::DbContextExt; use openstack_keystone_core::federation::FederationProviderError; @@ -74,7 +75,8 @@ pub async fn update>( entry.oidc_client_id = Set(val.to_owned()); } if let Some(val) = idp.oidc_client_secret { - entry.oidc_client_secret = Set(val.to_owned()); + // Exposure boundary: unwrapped only here at the final storage write. + entry.oidc_client_secret = Set(val.map(|s| s.expose_secret().to_string())); } if let Some(val) = idp.oidc_response_mode { entry.oidc_response_mode = Set(val.to_owned()); diff --git a/crates/identity-driver-sql/src/authenticate.rs b/crates/identity-driver-sql/src/authenticate.rs index b1291f903..b0309c07d 100644 --- a/crates/identity-driver-sql/src/authenticate.rs +++ b/crates/identity-driver-sql/src/authenticate.rs @@ -14,6 +14,7 @@ //! User account authentication implementation. use chrono::Utc; use sea_orm::DatabaseConnection; +use secrecy::ExposeSecret; use tracing::info; use openstack_keystone_config::Config; @@ -79,9 +80,11 @@ pub async fn authenticate_by_password( .await .map_err(IdentityProviderError::password_hash)?; - let _ = password_hashing::verify_password(config, &auth.password, dummy_hash) - .await - .map_err(IdentityProviderError::password_hash)?; + // Exposure boundary: unwrapped only to feed the constant-time verify. + let _ = + password_hashing::verify_password(config, auth.password.expose_secret(), dummy_hash) + .await + .map_err(IdentityProviderError::password_hash)?; return Err(AuthenticationError::UserNameOrPasswordWrong.into()); } @@ -128,7 +131,8 @@ pub async fn authenticate_by_password( // Verify the password let now = Utc::now(); - if !password_hashing::verify_password(config, &auth.password, expected_hash) + // Exposure boundary: unwrapped only to feed the constant-time verify. + if !password_hashing::verify_password(config, auth.password.expose_secret(), expected_hash) .await .map_err(IdentityProviderError::password_hash)? { @@ -473,7 +477,7 @@ mod tests { &db, &UserPasswordAuthRequest { id: Some("user_id".into()), - password, + password: password.clone().into(), ..Default::default() }, ) @@ -877,7 +881,7 @@ mod tests { &db, &UserPasswordAuthRequest { id: Some("user_id".into()), - password: password.clone(), + password: password.clone().into(), ..Default::default() }, ) @@ -927,7 +931,7 @@ mod tests { &db, &UserPasswordAuthRequest { id: Some("user_id".into()), - password: password.clone(), + password: password.clone().into(), ..Default::default() }, ) diff --git a/crates/identity-driver-sql/src/user/create.rs b/crates/identity-driver-sql/src/user/create.rs index c980bdfac..4ddc373ca 100644 --- a/crates/identity-driver-sql/src/user/create.rs +++ b/crates/identity-driver-sql/src/user/create.rs @@ -196,14 +196,8 @@ pub async fn create( response_builder.merge_local_user_data(&local_user); if let Some(password) = &user.password { - password::set_new_password( - &txn, - conf, - local_user.id, - secrecy::SecretString::from(password.as_str()), - Vec::new(), - ) - .await?; + password::set_new_password(&txn, conf, local_user.id, password.clone(), Vec::new()) + .await?; response_builder.merge_passwords_data(password::list(&txn, local_user.id).await?); } } diff --git a/crates/identity-driver-sql/src/user/update.rs b/crates/identity-driver-sql/src/user/update.rs index 6c48d94db..e8a0259c6 100644 --- a/crates/identity-driver-sql/src/user/update.rs +++ b/crates/identity-driver-sql/src/user/update.rs @@ -107,7 +107,7 @@ pub async fn update( &txn, conf, local_user.id, - secrecy::SecretString::from(new_password.as_str()), + new_password.clone(), passwords_vec, ) .await?; @@ -334,7 +334,7 @@ mod tests { .into_connection(); let req = UserUpdate { - password: Some("new_password".to_string()), + password: Some("new_password".into()), ..Default::default() }; @@ -408,7 +408,7 @@ mod tests { .into_connection(); let req = UserUpdate { - password: Some("new_password".to_string()), + password: Some("new_password".into()), ..Default::default() }; @@ -454,7 +454,7 @@ mod tests { .into_connection(); let req = UserUpdate { - password: Some("new_password".to_string()), + password: Some("new_password".into()), ..Default::default() }; @@ -494,7 +494,7 @@ mod tests { let req = UserUpdate { name: Some("new_name".to_string()), - password: Some("new_password".to_string()), + password: Some("new_password".into()), ..Default::default() }; @@ -541,7 +541,7 @@ mod tests { .into_connection(); let req = UserUpdate { - password: Some(new_password.to_string()), + password: Some(new_password.into()), ..Default::default() }; diff --git a/crates/keystone/src/api/v3/auth/token/common.rs b/crates/keystone/src/api/v3/auth/token/common.rs index dfee06d37..82338bf40 100644 --- a/crates/keystone/src/api/v3/auth/token/common.rs +++ b/crates/keystone/src/api/v3/auth/token/common.rs @@ -87,6 +87,7 @@ mod tests { use openstack_keystone_core_types::auth::*; use openstack_keystone_core_types::identity::{UserPasswordAuthRequest, UserResponseBuilder}; use openstack_keystone_core_types::resource::Domain; + use secrecy::ExposeSecret; use super::super::types::*; use super::*; @@ -116,7 +117,7 @@ mod tests { .expect_authenticate_by_password() .withf(|_, req: &UserPasswordAuthRequest| { req.id == Some("uid".to_string()) - && req.password == "pwd" + && req.password.expose_secret() == "pwd" && req.name == Some("uname".to_string()) }) .returning(move |_, _| Ok(auth_clone.clone())); diff --git a/crates/keystone/src/api/v3/auth/token/create.rs b/crates/keystone/src/api/v3/auth/token/create.rs index ccf118209..0c4726a32 100644 --- a/crates/keystone/src/api/v3/auth/token/create.rs +++ b/crates/keystone/src/api/v3/auth/token/create.rs @@ -156,6 +156,7 @@ mod tests { use openstack_keystone_core_types::identity::UserPasswordAuthRequest; use openstack_keystone_core_types::resource::{Domain, DomainBuilder, Project}; use openstack_keystone_core_types::token::{ProjectScopePayload, TokenProviderError}; + use secrecy::ExposeSecret; use crate::api::v3::auth::token::types::*; use crate::assignment::MockAssignmentProvider; @@ -213,7 +214,7 @@ mod tests { .expect_authenticate_by_password() .withf(|_, req: &UserPasswordAuthRequest| { req.id == Some("uid".to_string()) - && req.password == "pass" + && req.password.expose_secret() == "pass" && req.name == Some("uname".to_string()) }) .returning(move |_, _| Ok(auth.clone())); @@ -442,7 +443,7 @@ mod tests { .expect_authenticate_by_password() .withf(|_, req: &UserPasswordAuthRequest| { req.id == Some("uid".to_string()) - && req.password == "pass" + && req.password.expose_secret() == "pass" && req.name == Some("uname".to_string()) }) .returning(move |_, _| Ok(auth.clone())); diff --git a/crates/keystone/src/federation/api/oidc.rs b/crates/keystone/src/federation/api/oidc.rs index 331729a4b..ecdbef48d 100644 --- a/crates/keystone/src/federation/api/oidc.rs +++ b/crates/keystone/src/federation/api/oidc.rs @@ -15,6 +15,7 @@ use axum::{Json, debug_handler, extract::State, http::StatusCode, response::IntoResponse}; use chrono::Utc; +use secrecy::ExposeSecret; use url::Url; use utoipa_axum::{router::OpenApiRouter, routes}; @@ -157,7 +158,8 @@ async fn callback_inner( let token_response = exchange_code( &metadata.token_endpoint, client_id, - idp.oidc_client_secret.as_deref(), + // Exposure boundary: unwrapped only to build the token-endpoint request. + idp.oidc_client_secret.as_ref().map(|s| s.expose_secret()), &query.code, &auth_state.redirect_uri, &auth_state.pkce_verifier, @@ -171,7 +173,8 @@ async fn callback_inner( // claim MUST contain the RP's client ID. Validate issuer, nonce, and // audience in a single verification pass. let claims_value: serde_json::Value = verify_jwt( - &id_token, + // Exposure boundary: the ID token is unwrapped only to verify it. + id_token.expose_secret(), &jwks, Some(metadata.issuer.as_str()), Some(&auth_state.nonce), diff --git a/crates/keystone/src/federation/api/oidc_utils.rs b/crates/keystone/src/federation/api/oidc_utils.rs index 4738d910c..768edbeb1 100644 --- a/crates/keystone/src/federation/api/oidc_utils.rs +++ b/crates/keystone/src/federation/api/oidc_utils.rs @@ -21,6 +21,7 @@ use base64::{Engine as _, engine::general_purpose::URL_SAFE_NO_PAD}; use jsonwebtoken::{ Algorithm, DecodingKey, Header, TokenData, Validation, decode, decode_header, jwk::JwkSet, }; +use secrecy::SecretString; use serde::Deserialize; use sha2::{Digest, Sha256}; use url::Url; @@ -129,11 +130,15 @@ pub(super) fn build_auth_url( } /// Response body from the token endpoint. +/// +/// `id_token` and `access_token` are OAuth/OIDC bearer tokens: wrapped in +/// [`SecretString`] so this `Debug`-deriving struct can never leak them through +/// tracing/logs. They are consumed only at the JWT-verification boundary. #[derive(Debug, Deserialize)] pub(super) struct TokenExchangeResponse { - pub id_token: Option, + pub id_token: Option, #[allow(dead_code)] - pub access_token: Option, + pub access_token: Option, #[allow(dead_code)] pub token_type: String, #[allow(dead_code)] @@ -335,6 +340,8 @@ pub(super) fn build_http_client() -> Result { #[cfg(test)] mod tests { + use secrecy::ExposeSecret; + use super::*; use chrono::{TimeDelta, Utc}; use httpmock::prelude::*; @@ -701,8 +708,35 @@ nCCsPCcZ_m39ehWRD5EuL3yrQGE7HJo2a7E9J2bb0xBQEzXd_UzBI-lOOw2nvwIm\ .await .unwrap(); - assert_eq!(resp.access_token, Some("at-value".to_string())); - assert_eq!(resp.id_token, Some("id.token.value".to_string())); + assert_eq!( + resp.access_token.as_ref().map(|s| s.expose_secret()), + Some("at-value") + ); + assert_eq!( + resp.id_token.as_ref().map(|s| s.expose_secret()), + Some("id.token.value") + ); + } + + #[test] + fn token_exchange_response_debug_never_leaks_tokens() { + let resp: TokenExchangeResponse = serde_json::from_value(json!({ + "id_token": "id.token.LEAKME", + "access_token": "access.token.LEAKME", + "token_type": "Bearer", + })) + .unwrap(); + // The Debug-derived struct must never render the bearer tokens. + let dbg = format!("{resp:?}"); + assert!( + !dbg.contains("LEAKME"), + "Debug leaked OIDC bearer tokens: {dbg}" + ); + // ...but the values remain retrievable at the exposure boundary. + assert_eq!( + resp.id_token.as_ref().map(|s| s.expose_secret()), + Some("id.token.LEAKME") + ); } #[tokio::test] @@ -1123,11 +1157,14 @@ nCCsPCcZ_m39ehWRD5EuL3yrQGE7HJo2a7E9J2bb0xBQEzXd_UzBI-lOOw2nvwIm\ .await .unwrap(); - assert_eq!( - resp.access_token, None, + assert!( + resp.access_token.is_none(), "access_token must be absent in id-token-only response" ); - assert_eq!(resp.id_token, Some("id.token.value".to_string())); + assert_eq!( + resp.id_token.as_ref().map(|s| s.expose_secret()), + Some("id.token.value") + ); } // ── iat validation ──────────────────────────────────────────────────────── From 2c979d577406b9dc52245e5e9b763d7d693cd4c7 Mon Sep 17 00:00:00 2001 From: Yousef Hussein Date: Sat, 4 Jul 2026 00:52:21 +0300 Subject: [PATCH 2/7] refactor(security): Use SecretString directly 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 Signed-off-by: Yousef Hussein --- .../src/federation/identity_provider.rs | 99 ++++++----- crates/api-types/src/lib.rs | 1 - crates/api-types/src/secret_serde.rs | 157 ------------------ crates/api-types/src/v3/auth/token.rs | 79 +++++---- crates/api-types/src/v3/auth_conv.rs | 3 +- crates/api-types/src/v3/user.rs | 113 ++++++------- .../src/federation/identity_provider.rs | 111 +++---------- crates/core-types/src/identity/user.rs | 25 ++- crates/core/src/identity/service.rs | 10 +- .../src/identity_provider/get.rs | 16 +- .../src/identity_provider/list.rs | 54 +++--- .../keystone/src/api/v3/auth/token/common.rs | 7 +- 12 files changed, 223 insertions(+), 452 deletions(-) delete mode 100644 crates/api-types/src/secret_serde.rs diff --git a/crates/api-types/src/federation/identity_provider.rs b/crates/api-types/src/federation/identity_provider.rs index 266b419d6..0c25b03bc 100644 --- a/crates/api-types/src/federation/identity_provider.rs +++ b/crates/api-types/src/federation/identity_provider.rs @@ -12,14 +12,45 @@ // // SPDX-License-Identifier: Apache-2.0 //! Federated identity provider types. -use secrecy::SecretString; -use serde::{Deserialize, Serialize}; +use secrecy::{ExposeSecret, SecretString}; +use serde::{Deserialize, Serialize, Serializer}; use serde_json::Value; #[cfg(feature = "validate")] use validator::Validate; use crate::Link; -use crate::secret_serde::{serialize_secret_redacted, serialize_secret_redacted_nested}; + +/// Serialize an optional client secret transparently for transport (the client +/// must send the real value). `Debug` redaction from `SecretString` is what +/// keeps it out of logs; the value is never serialized back to a client because +/// the response type has no secret field. +fn serialize_optional_secret( + secret: &Option, + serializer: S, +) -> Result +where + S: Serializer, +{ + match secret { + Some(secret) => serializer.serialize_some(secret.expose_secret()), + None => serializer.serialize_none(), + } +} + +/// Same as [`serialize_optional_secret`] for the update DTO's nested option +/// (outer = present-in-request, inner = set-or-clear). +fn serialize_nested_optional_secret( + secret: &Option>, + serializer: S, +) -> Result +where + S: Serializer, +{ + match secret { + Some(Some(secret)) => serializer.serialize_some(secret.expose_secret()), + _ => serializer.serialize_none(), + } +} /// Identity provider data. #[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] @@ -124,9 +155,6 @@ pub struct IdentityProviderResponse { } /// Identity provider data. -/// -/// `PartialEq` is intentionally not derived: `oidc_client_secret` is wrapped in -/// [`SecretString`], which does not implement `PartialEq` by design. #[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr( feature = "builder", @@ -174,19 +202,13 @@ pub struct IdentityProviderCreate { pub oidc_client_id: Option, /// The oidc `client_secret` to use for the private client. It is never - /// returned back. Wrapped in [`SecretString`] to prevent accidental - /// exposure via Debug/tracing; redacted (never exposed) when serialized into - /// policy/audit payloads. - /// - /// Field-level `length` validation is intentionally omitted: `SecretString` - /// cannot use validator's built-in `length`/`custom` (the latter requires - /// the field to be `Serialize`, which secrets are not). + /// returned back. #[cfg_attr(feature = "builder", builder(default))] #[cfg_attr(feature = "openapi", schema(value_type = String, nullable = false))] #[serde( default, - serialize_with = "serialize_secret_redacted", - skip_serializing_if = "Option::is_none" + skip_serializing_if = "Option::is_none", + serialize_with = "serialize_optional_secret" )] pub oidc_client_secret: Option, @@ -254,9 +276,6 @@ pub struct IdentityProviderCreate { } /// New identity provider data. -/// -/// `PartialEq` is intentionally not derived: `oidc_client_secret` is wrapped in -/// [`SecretString`], which does not implement `PartialEq` by design. #[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr( feature = "builder", @@ -288,14 +307,10 @@ pub struct IdentityProviderUpdate { #[cfg_attr(feature = "validate", validate(length(max = 255)))] pub oidc_client_id: Option>, - /// The new oidc `client_secret` to use for the private client. Wrapped in - /// [`SecretString`] to prevent accidental exposure via Debug/tracing; - /// redacted (never exposed) when serialized into policy/audit payloads. - /// Field-level `length` validation is intentionally omitted (see - /// `IdentityProviderCreate`). + /// The new oidc `client_secret` to use for the private client. #[cfg_attr(feature = "builder", builder(default))] #[cfg_attr(feature = "openapi", schema(value_type = Option))] - #[serde(default, serialize_with = "serialize_secret_redacted_nested")] + #[serde(default, serialize_with = "serialize_nested_optional_secret")] pub oidc_client_secret: Option>, /// The new oidc response mode. @@ -346,9 +361,6 @@ pub struct IdentityProviderUpdate { } /// Identity provider create request. -/// -/// `PartialEq` is not derived because the embedded `IdentityProviderCreate` -/// carries a `SecretString`. #[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] #[cfg_attr(feature = "validate", derive(validator::Validate))] @@ -359,9 +371,6 @@ pub struct IdentityProviderCreateRequest { } /// Identity provider update request. -/// -/// `PartialEq` is not derived because the embedded `IdentityProviderUpdate` -/// carries a `SecretString`. #[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] #[cfg_attr(feature = "validate", derive(validator::Validate))] @@ -416,32 +425,36 @@ fn default_list_limit() -> Option { mod tests { use super::*; - /// The create DTO is serialized into OPA policy input - /// (`json!({"identity_provider": req.identity_provider})`). The client secret - /// must be redacted there. + /// The client secret must never leak through the DTO's `Debug` (the tracing + /// / logging vector), while remaining available via `expose_secret` for the + /// storage write. #[test] - fn idp_create_redacts_client_secret_for_policy() { + fn idp_create_debug_does_not_leak_client_secret() { + use secrecy::ExposeSecret; let create: IdentityProviderCreate = serde_json::from_str(r#"{"name":"idp","oidc_client_secret":"CSLEAK"}"#).unwrap(); - let rendered = serde_json::json!({ "identity_provider": &create }).to_string(); assert!( - !rendered.contains("CSLEAK"), - "leaked client secret: {rendered}" + !format!("{create:?}").contains("CSLEAK"), + "Debug leaked client secret: {create:?}" + ); + assert_eq!( + create + .oidc_client_secret + .as_ref() + .map(|s| s.expose_secret()), + Some("CSLEAK") ); - assert!(rendered.contains("[REDACTED]"), "not redacted: {rendered}"); } /// Same for the update DTO (nested `Option>`). #[test] - fn idp_update_redacts_client_secret_for_policy() { + fn idp_update_debug_does_not_leak_client_secret() { let update: IdentityProviderUpdate = serde_json::from_str(r#"{"oidc_client_secret":"CSLEAK2"}"#).unwrap(); - let rendered = serde_json::json!({ "identity_provider": &update }).to_string(); assert!( - !rendered.contains("CSLEAK2"), - "leaked client secret: {rendered}" + !format!("{update:?}").contains("CSLEAK2"), + "Debug leaked client secret: {update:?}" ); - assert!(rendered.contains("[REDACTED]"), "not redacted: {rendered}"); } /// Regression guard: the read/response DTO has no client-secret field, so a diff --git a/crates/api-types/src/lib.rs b/crates/api-types/src/lib.rs index 659b0246b..e9a72a79f 100644 --- a/crates/api-types/src/lib.rs +++ b/crates/api-types/src/lib.rs @@ -30,7 +30,6 @@ pub mod k8s_auth; pub mod scope; #[cfg(feature = "conv")] mod scope_conv; -pub(crate) mod secret_serde; pub mod trust; pub mod v3; pub mod v4; diff --git a/crates/api-types/src/secret_serde.rs b/crates/api-types/src/secret_serde.rs deleted file mode 100644 index 5ba2cc87a..000000000 --- a/crates/api-types/src/secret_serde.rs +++ /dev/null @@ -1,157 +0,0 @@ -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -// -// SPDX-License-Identifier: Apache-2.0 -//! Serialization helpers for [`secrecy::SecretString`] fields. -//! -//! `SecretString` deliberately does not implement `Serialize`. These helpers -//! let DTOs that must remain serializable (e.g. for policy/audit payloads that -//! embed the request or resource) do so **without ever exposing the secret** — -//! a present secret is rendered as a fixed `"[REDACTED]"` marker so field -//! presence is preserved while the value never leaks. -//! -//! This is intentionally different from the "expose once" helper used for -//! secrets that are part of an API contract (e.g. a one-time API-key token): -//! those live next to their own DTO and call `expose_secret()` on purpose. - -use secrecy::SecretString; -use serde::Serializer; - -// NOTE: length/non-empty validation for wrapped secrets is deliberately NOT done -// with validator's field-level `custom` here: validator 0.20 unconditionally -// calls `ValidationError::add_param(&field)`, which requires the field to be -// `Serialize` — and `SecretString` intentionally is not. Password non-emptiness -// is instead enforced centrally in -// `openstack_keystone_config::SecurityComplianceProvider::validate_password`, -// which runs on the wrapped value at the service layer for every write path. - -/// Redact an `Option` on serialize. -pub(crate) fn serialize_secret_redacted( - secret: &Option, - serializer: S, -) -> Result -where - S: Serializer, -{ - match secret { - Some(_) => serializer.serialize_str("[REDACTED]"), - None => serializer.serialize_none(), - } -} - -/// Redact a required `SecretString` on serialize. -pub(crate) fn serialize_secret_redacted_required( - _secret: &SecretString, - serializer: S, -) -> Result -where - S: Serializer, -{ - serializer.serialize_str("[REDACTED]") -} - -/// Redact the nested `Option>` used by update DTOs, where -/// the outer `Option` is "present-in-request" and the inner is "set-or-clear". -pub(crate) fn serialize_secret_redacted_nested( - secret: &Option>, - serializer: S, -) -> Result -where - S: Serializer, -{ - match secret { - Some(Some(_)) => serializer.serialize_str("[REDACTED]"), - _ => serializer.serialize_none(), - } -} - -#[cfg(test)] -mod tests { - use secrecy::SecretString; - use serde::Serialize; - - use super::*; - - const SECRET: &str = "super-secret-value"; - - #[derive(Serialize)] - struct OptHolder { - #[serde(serialize_with = "serialize_secret_redacted")] - secret: Option, - } - - #[derive(Serialize)] - struct RequiredHolder { - #[serde(serialize_with = "serialize_secret_redacted_required")] - secret: SecretString, - } - - #[derive(Serialize)] - struct NestedHolder { - #[serde(serialize_with = "serialize_secret_redacted_nested")] - secret: Option>, - } - - #[test] - fn opt_secret_is_redacted_when_present() { - let json = serde_json::to_string(&OptHolder { - secret: Some(SecretString::from(SECRET)), - }) - .unwrap(); - assert!(!json.contains(SECRET), "secret leaked: {json}"); - assert!(json.contains("[REDACTED]"), "not redacted: {json}"); - } - - #[test] - fn opt_secret_is_null_when_absent() { - let json = serde_json::to_string(&OptHolder { secret: None }).unwrap(); - assert_eq!(json, r#"{"secret":null}"#); - } - - #[test] - fn required_secret_is_redacted() { - let json = serde_json::to_string(&RequiredHolder { - secret: SecretString::from(SECRET), - }) - .unwrap(); - assert!(!json.contains(SECRET), "secret leaked: {json}"); - assert!(json.contains("[REDACTED]"), "not redacted: {json}"); - } - - #[test] - fn nested_secret_is_redacted_only_when_set() { - // Present + set -> redacted. - let set = serde_json::to_string(&NestedHolder { - secret: Some(Some(SecretString::from(SECRET))), - }) - .unwrap(); - assert!(!set.contains(SECRET), "secret leaked: {set}"); - assert!(set.contains("[REDACTED]"), "not redacted: {set}"); - - // Explicit clear and absent both serialize to null (no value to leak). - assert_eq!( - serde_json::to_string(&NestedHolder { secret: Some(None) }).unwrap(), - r#"{"secret":null}"# - ); - assert_eq!( - serde_json::to_string(&NestedHolder { secret: None }).unwrap(), - r#"{"secret":null}"# - ); - } - - #[test] - fn debug_of_secret_does_not_leak() { - // secrecy's own Debug guarantee, pinned here as the core requirement. - let secret = SecretString::from(SECRET); - assert!(!format!("{secret:?}").contains(SECRET)); - } -} diff --git a/crates/api-types/src/v3/auth/token.rs b/crates/api-types/src/v3/auth/token.rs index 216eea009..c076ebba4 100644 --- a/crates/api-types/src/v3/auth/token.rs +++ b/crates/api-types/src/v3/auth/token.rs @@ -13,17 +13,26 @@ // SPDX-License-Identifier: Apache-2.0 use chrono::{DateTime, Utc}; -use secrecy::SecretString; -use serde::{Deserialize, Serialize}; +use secrecy::{ExposeSecret, SecretString}; +use serde::{Deserialize, Serialize, Serializer}; #[cfg(feature = "validate")] use validator::Validate; use crate::catalog::*; use crate::scope::*; -use crate::secret_serde::serialize_secret_redacted_required; use crate::trust::TokenTrustRepr; use crate::v3::role::RoleRef; +/// Serialize a secret transparently for transport. These fields arrive in the +/// auth request body and must round-trip to the server; `SecretString` keeps +/// them out of `Debug`/logs, which is the exposure vector that matters. +fn serialize_secret_string(secret: &SecretString, serializer: S) -> Result +where + S: Serializer, +{ + serializer.serialize_str(secret.expose_secret()) +} + /// Authorization token. #[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] #[cfg_attr( @@ -133,9 +142,6 @@ pub struct TokenResponse { } /// An authentication request. -/// -/// `PartialEq` is not derived: the embedded identity carries a `SecretString` -/// password. #[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] #[cfg_attr(feature = "validate", derive(validator::Validate))] @@ -146,9 +152,6 @@ pub struct AuthRequest { } /// An authentication request. -/// -/// `PartialEq` is not derived: the embedded identity carries a `SecretString` -/// password. #[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] #[cfg_attr(feature = "validate", derive(validator::Validate))] @@ -172,8 +175,6 @@ pub struct AuthRequestInner { } /// An identity object. -/// -/// `PartialEq` is not derived: the embedded password carries a `SecretString`. #[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr( feature = "builder", @@ -208,9 +209,6 @@ pub struct Identity { } /// The password object, contains the authentication information. -/// -/// `PartialEq` is not derived: the embedded user carries a `SecretString` -/// password. #[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr( feature = "builder", @@ -229,9 +227,6 @@ pub struct PasswordAuth { } /// User password information. -/// -/// `PartialEq` is intentionally not derived: `password` is wrapped in -/// [`SecretString`], which does not implement `PartialEq` by design. #[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr( feature = "builder", @@ -256,15 +251,14 @@ pub struct UserPassword { #[cfg_attr(feature = "builder", builder(default))] #[cfg_attr(feature = "validate", validate(nested))] pub domain: Option, - /// User password. Wrapped in [`SecretString`] to prevent accidental - /// exposure via Debug/tracing; redacted (never exposed) when serialized. + /// User password. #[cfg_attr(feature = "openapi", schema(value_type = String))] - #[serde(serialize_with = "serialize_secret_redacted_required")] + #[serde(serialize_with = "serialize_secret_string")] pub password: SecretString, } /// The TOTP object, contains the authentication information. -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +#[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr( feature = "builder", derive(derive_builder::Builder), @@ -282,7 +276,7 @@ pub struct TotpAuth { } /// User TOTP information. -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +#[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr( feature = "builder", derive(derive_builder::Builder), @@ -307,8 +301,9 @@ pub struct TotpUser { #[cfg_attr(feature = "validate", validate(nested))] pub domain: Option, /// The passcode generated by the user's TOTP device/app. - #[cfg_attr(feature = "validate", validate(length(max = 32)))] - pub passcode: String, + #[cfg_attr(feature = "openapi", schema(value_type = String))] + #[serde(serialize_with = "serialize_secret_string")] + pub passcode: SecretString, } /// User information. @@ -340,7 +335,7 @@ pub struct User { } /// The token object, contains the authentication information. -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +#[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr( feature = "builder", derive(derive_builder::Builder), @@ -353,8 +348,9 @@ pub struct User { #[cfg_attr(feature = "validate", derive(validator::Validate))] pub struct TokenAuth { /// An authentication token. - #[cfg_attr(feature = "validate", validate(length(max = 1024)))] - pub id: String, + #[cfg_attr(feature = "openapi", schema(value_type = String))] + #[serde(serialize_with = "serialize_secret_string")] + pub id: SecretString, } #[derive(Clone, Debug, Deserialize, Serialize)] @@ -404,22 +400,25 @@ mod tests { const PWD: &str = "hunter2-plaintext"; #[test] - fn user_password_deserializes_plaintext_but_never_leaks() { - // Plaintext arrives on the wire and is accepted into the SecretString. + fn user_password_deserializes_plaintext_and_debug_never_leaks() { + // Plaintext arrives on the wire and is accepted into the secret field. let up: UserPassword = serde_json::from_str(&format!(r#"{{"name":"alice","password":"{PWD}"}}"#)).unwrap(); assert_eq!(up.password.expose_secret(), PWD); - // Debug must not leak (the #[instrument] / log vector). + // Debug (the #[instrument] / log vector) must not leak. assert!( !format!("{up:?}").contains(PWD), "Debug leaked the password" ); - // Serialization (e.g. into a policy/audit payload) must redact. + // Serialization is transparent: the auth body must carry the real + // password so the request round-trips to the server. let json = serde_json::to_string(&up).unwrap(); - assert!(!json.contains(PWD), "serialize leaked the password: {json}"); - assert!(json.contains("[REDACTED]"), "password not redacted: {json}"); + assert!( + json.contains(PWD), + "password not carried for transport: {json}" + ); } #[test] @@ -433,21 +432,17 @@ mod tests { } #[test] - fn full_auth_request_serialize_redacts_password_at_depth() { + fn full_auth_request_serialize_carries_password_for_transport() { // The password sits 4 levels deep (auth -> identity -> password -> user). - // Prove redaction survives the whole nested Serialize tree, via both - // `to_string` and `to_value`. + // Serialization must carry it through the whole nested tree so a client + // can send the request, via both `to_string` and `to_value`. let req = nested_auth_request(); let as_string = serde_json::to_string(&req).unwrap(); let as_value = serde_json::to_value(&req).unwrap().to_string(); for rendered in [as_string, as_value] { assert!( - !rendered.contains(PWD), - "serialize leaked password at depth: {rendered}" - ); - assert!( - rendered.contains("[REDACTED]"), - "nested password not redacted: {rendered}" + rendered.contains(PWD), + "password not carried for transport at depth: {rendered}" ); } } diff --git a/crates/api-types/src/v3/auth_conv.rs b/crates/api-types/src/v3/auth_conv.rs index bc8e9cdfb..b66cc1110 100644 --- a/crates/api-types/src/v3/auth_conv.rs +++ b/crates/api-types/src/v3/auth_conv.rs @@ -13,6 +13,7 @@ // SPDX-License-Identifier: Apache-2.0 use openstack_keystone_core_types::error::BuilderError; +use secrecy::ExposeSecret; use crate::v3::auth::token as api_types; @@ -41,7 +42,7 @@ impl TryFrom } upa.domain(domain_builder.build()?); } - upa.password(value.password.clone()); + upa.password(value.password.expose_secret()); upa.build() } } diff --git a/crates/api-types/src/v3/user.rs b/crates/api-types/src/v3/user.rs index 1ea4a6ca9..1bb5336e1 100644 --- a/crates/api-types/src/v3/user.rs +++ b/crates/api-types/src/v3/user.rs @@ -14,13 +14,27 @@ use std::collections::HashMap; use chrono::{DateTime, Utc}; -use secrecy::SecretString; -use serde::{Deserialize, Serialize}; +use secrecy::{ExposeSecret, SecretString}; +use serde::{Deserialize, Serialize, Serializer}; use serde_json::Value; #[cfg(feature = "validate")] use validator::Validate; -use crate::secret_serde::serialize_secret_redacted; +/// Serialize an optional password transparently for transport (the client must +/// send the real value). `SecretString` keeps it out of `Debug`/logs; the +/// response type carries no password field. +fn serialize_optional_secret( + secret: &Option, + serializer: S, +) -> Result +where + S: Serializer, +{ + match secret { + Some(secret) => serializer.serialize_some(secret.expose_secret()), + None => serializer.serialize_none(), + } +} /// User response object. #[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] @@ -105,9 +119,6 @@ pub struct UserResponse { } /// Create user data. -/// -/// `PartialEq` is intentionally not derived: `password` is wrapped in -/// [`SecretString`], which does not implement `PartialEq` by design. #[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr( feature = "builder", @@ -160,25 +171,19 @@ pub struct UserCreate { #[cfg_attr(feature = "validate", validate(nested))] pub options: Option, - /// The password for the user. Wrapped in [`SecretString`] to prevent - /// accidental exposure via Debug/tracing; redacted (never exposed) when - /// serialized into policy/audit payloads. Non-emptiness and regex policy are - /// enforced on the wrapped value at the service layer via - /// `security_compliance.validate_password`. + /// The password for the user. Non-emptiness and regex policy are enforced at + /// the service layer via `security_compliance.validate_password`. #[cfg_attr(feature = "builder", builder(default))] #[cfg_attr(feature = "openapi", schema(value_type = Option))] #[serde( default, - serialize_with = "serialize_secret_redacted", - skip_serializing_if = "Option::is_none" + skip_serializing_if = "Option::is_none", + serialize_with = "serialize_optional_secret" )] pub password: Option, } /// Complete create user request. -/// -/// `PartialEq` is not derived because the embedded `UserCreate` carries a -/// `SecretString`. #[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] #[cfg_attr(feature = "validate", derive(validator::Validate))] @@ -189,9 +194,6 @@ pub struct UserCreateRequest { } /// Update user data. -/// -/// `PartialEq` is intentionally not derived: `password` is wrapped in -/// [`SecretString`], which does not implement `PartialEq` by design. #[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr( feature = "builder", @@ -241,23 +243,18 @@ pub struct UserUpdate { #[cfg_attr(feature = "validate", validate(nested))] pub options: Option, - /// The password for the user. Wrapped in [`SecretString`] to prevent - /// accidental exposure via Debug/tracing; redacted (never exposed) when - /// serialized into policy/audit payloads. + /// The password for the user. #[cfg_attr(feature = "builder", builder(default))] #[cfg_attr(feature = "openapi", schema(value_type = Option))] #[serde( default, - serialize_with = "serialize_secret_redacted", - skip_serializing_if = "Option::is_none" + skip_serializing_if = "Option::is_none", + serialize_with = "serialize_optional_secret" )] pub password: Option, } /// Complete update user request. -/// -/// `PartialEq` is not derived because the embedded `UserUpdate` carries a -/// `SecretString`. #[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] #[cfg_attr(feature = "validate", derive(validator::Validate))] @@ -350,12 +347,10 @@ mod tests { use super::*; /// Critical: `UserCreate` carries BOTH `#[serde(flatten)] extra` and a - /// `password` with a custom `serialize_with` + `skip_serializing_if`. This is - /// the exact struct the handler serializes into OPA policy input - /// (`json!({"user": req.user})`). Prove the flatten interaction does not - /// leak the password and does not drop `extra`. + /// `password`. Prove the flatten interaction round-trips the password for + /// transport and does not drop `extra`, while `Debug` never leaks the value. #[test] - fn usercreate_flatten_redacts_password_and_keeps_extra() { + fn usercreate_flatten_keeps_password_and_extra() { let uc: UserCreate = serde_json::from_str( r#"{"domain_id":"d","name":"alice","enabled":true, "password":"PWLEAK","x_custom":"xval","y_custom":"yval"}"#, @@ -371,37 +366,39 @@ mod tests { Some("xval") ); - // Both the raw serialize and the OPA-shaped `json!({"user": uc})` path. - let direct = serde_json::to_string(&uc).unwrap(); - let opa = serde_json::json!({ "user": &uc }).to_string(); - for rendered in [direct, opa] { - assert!( - !rendered.contains("PWLEAK"), - "flatten leaked password: {rendered}" - ); - assert!( - rendered.contains("[REDACTED]"), - "password not redacted: {rendered}" - ); - assert!( - rendered.contains("x_custom") && rendered.contains("xval"), - "extra dropped by flatten+redact: {rendered}" - ); - assert!( - rendered.contains("y_custom") && rendered.contains("yval"), - "extra dropped by flatten+redact: {rendered}" - ); - } + // Debug (the logging vector) must never reveal the password. + assert!( + !format!("{uc:?}").contains("PWLEAK"), + "Debug leaked password: {uc:?}" + ); + + // Serialization is transparent (the body must round-trip on the wire), + // and the flattened `extra` keys are preserved alongside `password`. + let rendered = serde_json::to_string(&uc).unwrap(); + assert!( + rendered.contains("PWLEAK"), + "password not carried for transport: {rendered}" + ); + assert!( + rendered.contains("x_custom") && rendered.contains("xval"), + "extra dropped by flatten: {rendered}" + ); + assert!( + rendered.contains("y_custom") && rendered.contains("yval"), + "extra dropped by flatten: {rendered}" + ); } - /// `UserUpdate` has the same flatten+redact shape. + /// `UserUpdate` has the same flatten shape. #[test] - fn userupdate_flatten_redacts_password_and_keeps_extra() { + fn userupdate_flatten_keeps_password_and_extra() { let uu: UserUpdate = serde_json::from_str(r#"{"password":"UPWLEAK","z_extra":"zz"}"#).unwrap(); - let rendered = serde_json::json!({ "user": &uu }).to_string(); - assert!(!rendered.contains("UPWLEAK"), "leaked: {rendered}"); - assert!(rendered.contains("[REDACTED]"), "not redacted: {rendered}"); + assert!( + !format!("{uu:?}").contains("UPWLEAK"), + "Debug leaked password: {uu:?}" + ); + let rendered = serde_json::to_string(&uu).unwrap(); assert!(rendered.contains("z_extra"), "extra dropped: {rendered}"); } diff --git a/crates/core-types/src/federation/identity_provider.rs b/crates/core-types/src/federation/identity_provider.rs index 88886d0ee..4b73dcb1d 100644 --- a/crates/core-types/src/federation/identity_provider.rs +++ b/crates/core-types/src/federation/identity_provider.rs @@ -14,31 +14,13 @@ //! # Federated identity provider types use derive_builder::Builder; -use secrecy::{ExposeSecret, SecretString}; -use serde::{Serialize, Serializer}; +use secrecy::SecretString; +use serde::Serialize; use serde_json::Value; use crate::error::BuilderError; -/// Serialize an optional secret as a fixed redaction marker so that it never -/// leaks in Debug/policy/audit payloads while still signalling presence. -fn serialize_secret_redacted( - secret: &Option, - serializer: S, -) -> Result -where - S: Serializer, -{ - match secret { - Some(_) => serializer.serialize_str("[REDACTED]"), - None => serializer.serialize_none(), - } -} - /// Identity provider resource. -/// -/// `PartialEq` is intentionally not derived: `oidc_client_secret` is wrapped in -/// [`SecretString`], which does not implement `PartialEq` by design. #[derive(Builder, Clone, Debug, Default, Serialize)] #[builder(build_fn(error = "BuilderError"))] #[builder(setter(strip_option, into))] @@ -64,11 +46,10 @@ pub struct IdentityProvider { #[builder(default)] pub oidc_client_id: Option, - /// The OIDC client secret. Wrapped in [`SecretString`] to prevent accidental - /// exposure via Debug/tracing; redacted (never exposed) when serialized into - /// policy/audit payloads. + /// The OIDC client secret. It is never returned back, so it is skipped on + /// serialization; `SecretString` additionally keeps it out of `Debug`. #[builder(default)] - #[serde(serialize_with = "serialize_secret_redacted")] + #[serde(skip_serializing)] pub oidc_client_secret: Option, #[builder(default)] @@ -104,41 +85,7 @@ pub struct IdentityProvider { pub provider_config: Option, } -/// Manual `PartialEq` (the derive cannot be used because `oidc_client_secret` -/// is a [`SecretString`], which does not implement `PartialEq`). Preserves the -/// pre-wrapping equality contract by comparing the exposed secret values. -impl PartialEq for IdentityProvider { - fn eq(&self, other: &Self) -> bool { - self.id == other.id - && self.name == other.name - && self.domain_id == other.domain_id - && self.enabled == other.enabled - && self.oidc_discovery_url == other.oidc_discovery_url - && self.oidc_client_id == other.oidc_client_id - && self - .oidc_client_secret - .as_ref() - .map(ExposeSecret::expose_secret) - == other - .oidc_client_secret - .as_ref() - .map(ExposeSecret::expose_secret) - && self.oidc_response_mode == other.oidc_response_mode - && self.oidc_response_types == other.oidc_response_types - && self.jwks_url == other.jwks_url - && self.jwt_validation_pubkeys == other.jwt_validation_pubkeys - && self.bound_issuer == other.bound_issuer - && self.default_mapping_name == other.default_mapping_name - && self.oidc_scopes == other.oidc_scopes - && self.allowed_redirect_uris == other.allowed_redirect_uris - && self.provider_config == other.provider_config - } -} - /// New Identity provider data. -/// -/// `PartialEq` is intentionally not derived: `oidc_client_secret` is wrapped in -/// [`SecretString`], which does not implement `PartialEq` by design. #[derive(Builder, Clone, Debug, Default)] #[builder(build_fn(error = "BuilderError"))] #[builder(setter(strip_option, into))] @@ -164,8 +111,7 @@ pub struct IdentityProviderCreate { #[builder(default)] pub oidc_client_id: Option, - /// The OIDC client secret. Wrapped in [`SecretString`] to prevent accidental - /// exposure via Debug/tracing. + /// The OIDC client secret. #[builder(default)] pub oidc_client_secret: Option, @@ -200,9 +146,6 @@ pub struct IdentityProviderCreate { } /// Identity provider update data. -/// -/// `PartialEq` is intentionally not derived: `oidc_client_secret` is wrapped in -/// [`SecretString`], which does not implement `PartialEq` by design. #[derive(Builder, Clone, Debug, Default)] #[builder(build_fn(error = "BuilderError"))] #[builder(setter(into))] @@ -219,9 +162,8 @@ pub struct IdentityProviderUpdate { #[builder(default)] pub oidc_client_id: Option>, - /// The OIDC client secret. Wrapped in [`SecretString`] to prevent accidental - /// exposure via Debug/tracing. Outer `Option` = present-in-request, - /// inner `Option` = set-or-clear. + /// The OIDC client secret. Outer `Option` = present-in-request, inner + /// `Option` = set-or-clear. #[builder(default)] pub oidc_client_secret: Option>, @@ -284,48 +226,33 @@ mod tests { const SECRET: &str = "oidc-top-secret"; #[test] - fn identity_provider_never_leaks_client_secret() { + fn identity_provider_debug_does_not_leak_client_secret() { let idp = IdentityProviderBuilder::default() .id("1") .name("idp") - .oidc_client_secret(SECRET) + .oidc_client_secret(SecretString::from(SECRET)) .build() .unwrap(); - // Debug (the #[instrument] / log vector) must not leak. + // Debug (the #[instrument] / log vector) must not leak the secret. assert!( !format!("{idp:?}").contains(SECRET), "Debug leaked client secret" ); - - // Serialization into the OPA policy / audit payload must redact. - let json = serde_json::to_string(&idp).unwrap(); - assert!( - !json.contains(SECRET), - "serialize leaked client secret: {json}" - ); - assert!( - json.contains("[REDACTED]"), - "client secret not redacted: {json}" - ); } #[test] - fn partial_eq_still_compares_client_secret() { - let base = IdentityProviderBuilder::default() - .id("1") - .name("idp") - .oidc_client_secret(SECRET) - .build() - .unwrap(); - let same = base.clone(); - let different = IdentityProviderBuilder::default() + fn identity_provider_does_not_serialize_client_secret() { + let idp = IdentityProviderBuilder::default() .id("1") .name("idp") - .oidc_client_secret("other-secret") + .oidc_client_secret(SecretString::from(SECRET)) .build() .unwrap(); - assert_eq!(base, same); - assert_ne!(base, different); + + // The secret is never returned back, so it must not appear on the wire. + let json = serde_json::to_string(&idp).unwrap(); + assert!(!json.contains(SECRET), "serialize leaked client secret"); + assert!(!json.contains("oidc_client_secret")); } } diff --git a/crates/core-types/src/identity/user.rs b/crates/core-types/src/identity/user.rs index 168762eb6..8431ac45d 100644 --- a/crates/core-types/src/identity/user.rs +++ b/crates/core-types/src/identity/user.rs @@ -129,10 +129,8 @@ pub struct UserCreate { #[validate(nested)] pub options: Option, - /// User password. Wrapped in [`SecretString`] to prevent accidental - /// exposure via Debug/tracing. Non-emptiness and regex policy are enforced on - /// the wrapped value at the service layer via - /// `security_compliance.validate_password`. + /// User password. Non-emptiness and regex policy are enforced at the service + /// layer via `security_compliance.validate_password`. #[builder(default)] pub password: Option, @@ -185,9 +183,8 @@ pub struct UserUpdate { #[validate(nested)] pub options: Option, - /// New user password. Wrapped in [`SecretString`] to prevent accidental - /// exposure via Debug/tracing. Non-emptiness/policy enforced at the service - /// layer via `security_compliance.validate_password`. + /// New user password. Non-emptiness/policy enforced at the service layer via + /// `security_compliance.validate_password`. #[builder(default)] pub password: Option, } @@ -318,9 +315,8 @@ pub struct UserPasswordAuthRequest { #[validate(nested)] pub domain: Option, - /// User password. Wrapped in [`SecretString`] to prevent accidental - /// exposure via Debug/tracing. Required (no builder default: `SecretString` - /// does not implement `Default`). + /// User password. Required (no builder default: `SecretString` does not + /// implement `Default`). pub password: SecretString, } @@ -340,7 +336,10 @@ impl Default for UserPasswordAuthRequest { } /// User TOTP authentication request. -#[derive(Builder, Clone, Debug, Default, PartialEq, Validate)] +/// +/// `PartialEq`/`Default` are intentionally not derived: `passcode` is a required +/// [`SecretString`], which implements neither by design. +#[derive(Builder, Clone, Debug, Validate)] #[builder(build_fn(error = "BuilderError"))] #[builder(setter(strip_option, into))] pub struct UserTotpAuthRequest { @@ -360,9 +359,7 @@ pub struct UserTotpAuthRequest { pub domain: Option, /// The passcode generated by the user's TOTP device/app. - #[builder(default)] - #[validate(length(max = 32))] - pub passcode: String, + pub passcode: SecretString, } /// Domain information. diff --git a/crates/core/src/identity/service.rs b/crates/core/src/identity/service.rs index 58d8920ff..4c91280f0 100644 --- a/crates/core/src/identity/service.rs +++ b/crates/core/src/identity/service.rs @@ -16,7 +16,7 @@ use async_trait::async_trait; use chrono::{DateTime, Utc}; -use secrecy::SecretString; +use secrecy::{ExposeSecret, SecretString}; use std::collections::{HashMap, HashSet}; use std::sync::Arc; use tokio::sync::RwLock; @@ -438,7 +438,13 @@ impl IdentityApi for IdentityService { .and_then(serde_json::Value::as_u64) .map(|d| d as u32) .unwrap_or(30); - crate::credential::totp::verify_totp(seed, &auth.passcode, digits, period, now) + crate::credential::totp::verify_totp( + seed, + auth.passcode.expose_secret(), + digits, + period, + now, + ) }); if !matched { diff --git a/crates/federation-driver-sql/src/identity_provider/get.rs b/crates/federation-driver-sql/src/identity_provider/get.rs index 7a4a9f1f8..fd6ee66a9 100644 --- a/crates/federation-driver-sql/src/identity_provider/get.rs +++ b/crates/federation-driver-sql/src/identity_provider/get.rs @@ -59,15 +59,13 @@ mod tests { .append_query_results([vec![get_idp_mock("1")]]) .into_connection(); - assert_eq!( - get(&db, "1").await.unwrap().unwrap(), - IdentityProvider { - id: "1".into(), - name: "name".into(), - domain_id: Some("did".into()), - ..Default::default() - } - ); + // `IdentityProvider` holds an `oidc_client_secret: SecretString`, which + // is not `PartialEq`, so the returned value is asserted field by field. + let idp = get(&db, "1").await.unwrap().unwrap(); + assert_eq!(idp.id, "1"); + assert_eq!(idp.name, "name"); + assert_eq!(idp.domain_id, Some("did".into())); + assert!(idp.oidc_client_secret.is_none()); // Checking transaction log: single SELECT from the right table let txns = db.into_transaction_log(); diff --git a/crates/federation-driver-sql/src/identity_provider/list.rs b/crates/federation-driver-sql/src/identity_provider/list.rs index 06b307b19..192a730fc 100644 --- a/crates/federation-driver-sql/src/identity_provider/list.rs +++ b/crates/federation-driver-sql/src/identity_provider/list.rs @@ -145,17 +145,15 @@ mod tests { let db = MockDatabase::new(DatabaseBackend::Postgres) .append_query_results([vec![get_idp_mock("1")]]) .into_connection(); - assert_eq!( - list(&db, &IdentityProviderListParameters::default()) - .await - .unwrap(), - vec![IdentityProvider { - id: "1".into(), - name: "name".into(), - domain_id: Some("did".into()), - ..Default::default() - }] - ); + // `IdentityProvider` is not `PartialEq` (secret field), so assert per + // field on the single returned entry. + let idps = list(&db, &IdentityProviderListParameters::default()) + .await + .unwrap(); + assert_eq!(idps.len(), 1); + assert_eq!(idps[0].id, "1"); + assert_eq!(idps[0].name, "name"); + assert_eq!(idps[0].domain_id, Some("did".into())); // Checking transaction log: single SELECT from the right table let txns = db.into_transaction_log(); @@ -172,25 +170,21 @@ mod tests { .append_query_results([vec![get_idp_mock("1")]]) .into_connection(); - assert_eq!( - list( - &db, - &IdentityProviderListParameters { - name: Some("idp_name".into()), - domain_ids: Some(HashSet::from([Some("did".into())])), - limit: Some(1), - marker: Some("marker".into()), - } - ) - .await - .unwrap(), - vec![IdentityProvider { - id: "1".into(), - name: "name".into(), - domain_id: Some("did".into()), - ..Default::default() - }] - ); + let idps = list( + &db, + &IdentityProviderListParameters { + name: Some("idp_name".into()), + domain_ids: Some(HashSet::from([Some("did".into())])), + limit: Some(1), + marker: Some("marker".into()), + }, + ) + .await + .unwrap(); + assert_eq!(idps.len(), 1); + assert_eq!(idps[0].id, "1"); + assert_eq!(idps[0].name, "name"); + assert_eq!(idps[0].domain_id, Some("did".into())); // Checking transaction log: single SELECT with correct filters let txns = db.into_transaction_log(); diff --git a/crates/keystone/src/api/v3/auth/token/common.rs b/crates/keystone/src/api/v3/auth/token/common.rs index 82338bf40..6df68257d 100644 --- a/crates/keystone/src/api/v3/auth/token/common.rs +++ b/crates/keystone/src/api/v3/auth/token/common.rs @@ -13,6 +13,7 @@ // SPDX-License-Identifier: Apache-2.0 use openstack_keystone_core::auth::ExecutionContext; +use secrecy::ExposeSecret; use crate::api::error::KeystoneApiError; use crate::api::v3::auth::token::types::AuthRequest; @@ -60,7 +61,7 @@ pub(super) async fn authenticate_request( .get_token_provider() .authorize_by_token( &ExecutionContext::internal(state), - &token.id, + token.id.expose_secret(), Some(false), None, ) @@ -175,7 +176,7 @@ mod tests { identity_mock .expect_authenticate_by_totp() .withf(|_, req: &UserTotpAuthRequest| { - req.id == Some("uid".to_string()) && req.passcode == "123456" + req.id == Some("uid".to_string()) && req.passcode.expose_secret() == "123456" }) .returning(move |_, _| Ok(auth_clone.clone())); @@ -277,7 +278,7 @@ mod tests { methods: vec!["token".to_string()], password: None, token: Some(TokenAuth { - id: "fake_token".to_string() + id: "fake_token".into() }), totp: None, }, From 1bc291748eb08a1367e1c58927f44cf43b41cf09 Mon Sep 17 00:00:00 2001 From: Yousef Hussein Date: Tue, 7 Jul 2026 00:48:30 +0300 Subject: [PATCH 3/7] fix(security): Redact secret policy input Signed-off-by: Yousef Hussein --- crates/api-types/src/common.rs | 86 ++++++++ .../src/federation/identity_provider.rs | 204 ++++++++++++++---- crates/api-types/src/k8s_auth/auth.rs | 13 +- crates/api-types/src/lib.rs | 1 + crates/api-types/src/v3/auth/token.rs | 47 ++-- crates/api-types/src/v3/user.rs | 115 ++++++++-- crates/api-types/src/v4/api_key.rs | 13 +- crates/core-types/src/identity/user.rs | 44 +++- crates/core/src/credential/totp.rs | 52 ++++- crates/core/src/identity/service.rs | 10 +- crates/keystone/src/api/v3/user/create.rs | 2 +- crates/keystone/src/api/v3/user/update.rs | 2 +- .../api/identity_provider/create.rs | 2 +- .../api/identity_provider/update.rs | 4 +- 14 files changed, 482 insertions(+), 113 deletions(-) create mode 100644 crates/api-types/src/common.rs diff --git a/crates/api-types/src/common.rs b/crates/api-types/src/common.rs new file mode 100644 index 000000000..fdc8be9c5 --- /dev/null +++ b/crates/api-types/src/common.rs @@ -0,0 +1,86 @@ +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// SPDX-License-Identifier: Apache-2.0 + +use secrecy::{ExposeSecret, SecretString}; +use serde::Serializer; + +pub(crate) fn serialize_secret_string( + secret: &SecretString, + serializer: S, +) -> Result +where + S: Serializer, +{ + serializer.serialize_str(secret.expose_secret()) +} + +pub(crate) fn serialize_optional_secret( + secret: &Option, + serializer: S, +) -> Result +where + S: Serializer, +{ + match secret { + Some(secret) => serializer.serialize_some(secret.expose_secret()), + None => serializer.serialize_none(), + } +} + +pub(crate) fn serialize_nested_optional_secret( + secret: &Option>, + serializer: S, +) -> Result +where + S: Serializer, +{ + match secret { + Some(Some(secret)) => serializer.serialize_some(secret.expose_secret()), + _ => serializer.serialize_none(), + } +} + +#[cfg(feature = "validate")] +pub(crate) fn validate_secret_length( + secret: &SecretString, + max: usize, +) -> Result<(), validator::ValidationError> { + if secret.expose_secret().chars().count() <= max { + Ok(()) + } else { + Err(validator::ValidationError::new("length")) + } +} + +#[cfg(feature = "validate")] +pub(crate) fn validate_optional_secret_length( + secret: &Option, + max: usize, +) -> Result<(), validator::ValidationError> { + match secret { + Some(secret) => validate_secret_length(secret, max), + None => Ok(()), + } +} + +#[cfg(feature = "validate")] +pub(crate) fn validate_nested_optional_secret_length( + secret: &Option>, + max: usize, +) -> Result<(), validator::ValidationError> { + match secret { + Some(Some(secret)) => validate_secret_length(secret, max), + _ => Ok(()), + } +} diff --git a/crates/api-types/src/federation/identity_provider.rs b/crates/api-types/src/federation/identity_provider.rs index 0c25b03bc..5b3784d3a 100644 --- a/crates/api-types/src/federation/identity_provider.rs +++ b/crates/api-types/src/federation/identity_provider.rs @@ -12,46 +12,14 @@ // // SPDX-License-Identifier: Apache-2.0 //! Federated identity provider types. -use secrecy::{ExposeSecret, SecretString}; -use serde::{Deserialize, Serialize, Serializer}; +use secrecy::SecretString; +use serde::{Deserialize, Serialize}; use serde_json::Value; #[cfg(feature = "validate")] use validator::Validate; use crate::Link; -/// Serialize an optional client secret transparently for transport (the client -/// must send the real value). `Debug` redaction from `SecretString` is what -/// keeps it out of logs; the value is never serialized back to a client because -/// the response type has no secret field. -fn serialize_optional_secret( - secret: &Option, - serializer: S, -) -> Result -where - S: Serializer, -{ - match secret { - Some(secret) => serializer.serialize_some(secret.expose_secret()), - None => serializer.serialize_none(), - } -} - -/// Same as [`serialize_optional_secret`] for the update DTO's nested option -/// (outer = present-in-request, inner = set-or-clear). -fn serialize_nested_optional_secret( - secret: &Option>, - serializer: S, -) -> Result -where - S: Serializer, -{ - match secret { - Some(Some(secret)) => serializer.serialize_some(secret.expose_secret()), - _ => serializer.serialize_none(), - } -} - /// Identity provider data. #[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] #[cfg_attr( @@ -166,6 +134,10 @@ pub struct IdentityProviderResponse { )] #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] #[cfg_attr(feature = "validate", derive(validator::Validate))] +#[cfg_attr( + feature = "validate", + validate(schema(function = "validate_identity_provider_create_secret")) +)] pub struct IdentityProviderCreate { // TODO: add ID /// Identity provider name. @@ -208,7 +180,7 @@ pub struct IdentityProviderCreate { #[serde( default, skip_serializing_if = "Option::is_none", - serialize_with = "serialize_optional_secret" + serialize_with = "crate::common::serialize_optional_secret" )] pub oidc_client_secret: Option, @@ -275,6 +247,71 @@ pub struct IdentityProviderCreate { pub provider_config: Option, } +impl IdentityProviderCreate { + #[must_use] + pub fn to_policy_input(&self) -> serde_json::Value { + let mut input = serde_json::Map::new(); + input.insert("name".to_string(), serde_json::json!(self.name)); + input.insert("enabled".to_string(), serde_json::json!(self.enabled)); + if let Some(value) = &self.domain_id { + input.insert("domain_id".to_string(), serde_json::json!(value)); + } + if let Some(value) = &self.oidc_discovery_url { + input.insert("oidc_discovery_url".to_string(), serde_json::json!(value)); + } + 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() { + input.insert( + "oidc_client_secret".to_string(), + serde_json::json!("[REDACTED]"), + ); + } + if let Some(value) = &self.oidc_response_mode { + input.insert("oidc_response_mode".to_string(), serde_json::json!(value)); + } + if let Some(value) = &self.oidc_response_types { + input.insert("oidc_response_types".to_string(), serde_json::json!(value)); + } + if let Some(value) = &self.jwks_url { + input.insert("jwks_url".to_string(), serde_json::json!(value)); + } + if let Some(value) = &self.jwt_validation_pubkeys { + input.insert( + "jwt_validation_pubkeys".to_string(), + serde_json::json!(value), + ); + } + if let Some(value) = &self.bound_issuer { + input.insert("bound_issuer".to_string(), serde_json::json!(value)); + } + if let Some(value) = &self.default_mapping_name { + input.insert("default_mapping_name".to_string(), serde_json::json!(value)); + } + if let Some(value) = &self.oidc_scopes { + input.insert("oidc_scopes".to_string(), serde_json::json!(value)); + } + if let Some(value) = &self.allowed_redirect_uris { + input.insert( + "allowed_redirect_uris".to_string(), + serde_json::json!(value), + ); + } + if let Some(value) = &self.provider_config { + input.insert("provider_config".to_string(), value.clone()); + } + serde_json::Value::Object(input) + } +} + +#[cfg(feature = "validate")] +fn validate_identity_provider_create_secret( + value: &IdentityProviderCreate, +) -> Result<(), validator::ValidationError> { + crate::common::validate_optional_secret_length(&value.oidc_client_secret, 255) +} + /// New identity provider data. #[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr( @@ -287,6 +324,10 @@ pub struct IdentityProviderCreate { )] #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] #[cfg_attr(feature = "validate", derive(validator::Validate))] +#[cfg_attr( + feature = "validate", + validate(schema(function = "validate_identity_provider_update_secret")) +)] pub struct IdentityProviderUpdate { /// The new name of the federated identity provider. #[cfg_attr(feature = "validate", validate(length(max = 255)))] @@ -310,7 +351,10 @@ pub struct IdentityProviderUpdate { /// The new oidc `client_secret` to use for the private client. #[cfg_attr(feature = "builder", builder(default))] #[cfg_attr(feature = "openapi", schema(value_type = Option))] - #[serde(default, serialize_with = "serialize_nested_optional_secret")] + #[serde( + default, + serialize_with = "crate::common::serialize_nested_optional_secret" + )] pub oidc_client_secret: Option>, /// The new oidc response mode. @@ -360,6 +404,70 @@ pub struct IdentityProviderUpdate { pub provider_config: Option>, } +impl IdentityProviderUpdate { + #[must_use] + pub fn to_policy_input(&self) -> serde_json::Value { + let mut input = serde_json::Map::new(); + input.insert("name".to_string(), serde_json::json!(self.name)); + input.insert("enabled".to_string(), serde_json::json!(self.enabled)); + input.insert( + "oidc_discovery_url".to_string(), + serde_json::json!(self.oidc_discovery_url), + ); + input.insert( + "oidc_client_id".to_string(), + serde_json::json!(self.oidc_client_id), + ); + if self.oidc_client_secret.is_some() { + input.insert( + "oidc_client_secret".to_string(), + serde_json::json!("[REDACTED]"), + ); + } + input.insert( + "oidc_response_mode".to_string(), + serde_json::json!(self.oidc_response_mode), + ); + input.insert( + "oidc_response_types".to_string(), + serde_json::json!(self.oidc_response_types), + ); + input.insert("jwks_url".to_string(), serde_json::json!(self.jwks_url)); + input.insert( + "jwt_validation_pubkeys".to_string(), + serde_json::json!(self.jwt_validation_pubkeys), + ); + input.insert( + "bound_issuer".to_string(), + serde_json::json!(self.bound_issuer), + ); + input.insert( + "default_mapping_name".to_string(), + serde_json::json!(self.default_mapping_name), + ); + input.insert( + "oidc_scopes".to_string(), + serde_json::json!(self.oidc_scopes), + ); + input.insert( + "allowed_redirect_uris".to_string(), + serde_json::json!(self.allowed_redirect_uris), + ); + input.insert( + "provider_config".to_string(), + serde_json::json!(self.provider_config), + ); + serde_json::Value::Object(input) + } +} + +#[cfg(feature = "validate")] +fn validate_identity_provider_update_secret( + value: &IdentityProviderUpdate, +) -> Result<(), validator::ValidationError> { + crate::common::validate_nested_optional_secret_length(&value.oidc_client_secret, 255) +} + /// Identity provider create request. #[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] @@ -457,6 +565,30 @@ mod tests { ); } + #[test] + fn idp_policy_input_redacts_client_secret() { + let create: IdentityProviderCreate = + serde_json::from_str(r#"{"name":"idp","oidc_client_secret":"CSLEAK"}"#).unwrap(); + let rendered = create.to_policy_input().to_string(); + assert!( + !rendered.contains("CSLEAK"), + "policy input leaked client secret: {rendered}" + ); + + let update: IdentityProviderUpdate = + serde_json::from_str(r#"{"oidc_client_secret":"CSLEAK2"}"#).unwrap(); + let input = update.to_policy_input(); + let rendered = input.to_string(); + assert!( + !rendered.contains("CSLEAK2"), + "policy input leaked client secret: {rendered}" + ); + assert_eq!( + input.get("oidc_client_secret").and_then(|v| v.as_str()), + Some("[REDACTED]") + ); + } + /// Regression guard: the read/response DTO has no client-secret field, so a /// client secret can never be returned — even if one is (wrongly) supplied. #[test] diff --git a/crates/api-types/src/k8s_auth/auth.rs b/crates/api-types/src/k8s_auth/auth.rs index 64b25620f..8c840a92f 100644 --- a/crates/api-types/src/k8s_auth/auth.rs +++ b/crates/api-types/src/k8s_auth/auth.rs @@ -13,8 +13,8 @@ // SPDX-License-Identifier: Apache-2.0 //! # K8s Auth configuration types. -use secrecy::{ExposeSecret, SecretString}; -use serde::{Deserialize, Serialize, Serializer}; +use secrecy::SecretString; +use serde::{Deserialize, Serialize}; /// K8s authentication request. /// @@ -24,7 +24,7 @@ use serde::{Deserialize, Serialize, Serializer}; #[cfg_attr(feature = "validate", derive(validator::Validate))] pub struct K8sAuthRequest { #[cfg_attr(feature = "openapi", schema(value_type = String))] - #[serde(serialize_with = "serialize_secret_string")] + #[serde(serialize_with = "crate::common::serialize_secret_string")] /// JWT service account token. pub jwt: SecretString, @@ -33,10 +33,3 @@ pub struct K8sAuthRequest { #[cfg_attr(feature = "validate", validate(length(max = 255)))] pub rule_name: Option, } - -fn serialize_secret_string(secret: &SecretString, serializer: S) -> Result -where - S: Serializer, -{ - serializer.serialize_str(secret.expose_secret()) -} diff --git a/crates/api-types/src/lib.rs b/crates/api-types/src/lib.rs index e9a72a79f..d7e99f178 100644 --- a/crates/api-types/src/lib.rs +++ b/crates/api-types/src/lib.rs @@ -22,6 +22,7 @@ use serde::{Deserialize, Serialize}; pub mod catalog; #[cfg(feature = "conv")] mod catalog_conv; +mod common; pub mod error; #[cfg(feature = "conv")] mod error_conv; diff --git a/crates/api-types/src/v3/auth/token.rs b/crates/api-types/src/v3/auth/token.rs index c076ebba4..f11234306 100644 --- a/crates/api-types/src/v3/auth/token.rs +++ b/crates/api-types/src/v3/auth/token.rs @@ -13,8 +13,8 @@ // SPDX-License-Identifier: Apache-2.0 use chrono::{DateTime, Utc}; -use secrecy::{ExposeSecret, SecretString}; -use serde::{Deserialize, Serialize, Serializer}; +use secrecy::SecretString; +use serde::{Deserialize, Serialize}; #[cfg(feature = "validate")] use validator::Validate; @@ -23,16 +23,6 @@ use crate::scope::*; use crate::trust::TokenTrustRepr; use crate::v3::role::RoleRef; -/// Serialize a secret transparently for transport. These fields arrive in the -/// auth request body and must round-trip to the server; `SecretString` keeps -/// them out of `Debug`/logs, which is the exposure vector that matters. -fn serialize_secret_string(secret: &SecretString, serializer: S) -> Result -where - S: Serializer, -{ - serializer.serialize_str(secret.expose_secret()) -} - /// Authorization token. #[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] #[cfg_attr( @@ -238,6 +228,10 @@ pub struct PasswordAuth { )] #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] #[cfg_attr(feature = "validate", derive(validator::Validate))] +#[cfg_attr( + feature = "validate", + validate(schema(function = "validate_user_password_secret")) +)] pub struct UserPassword { /// User ID. #[cfg_attr(feature = "builder", builder(default))] @@ -253,10 +247,15 @@ pub struct UserPassword { pub domain: Option, /// User password. #[cfg_attr(feature = "openapi", schema(value_type = String))] - #[serde(serialize_with = "serialize_secret_string")] + #[serde(serialize_with = "crate::common::serialize_secret_string")] pub password: SecretString, } +#[cfg(feature = "validate")] +fn validate_user_password_secret(value: &UserPassword) -> Result<(), validator::ValidationError> { + crate::common::validate_secret_length(&value.password, 72) +} + /// The TOTP object, contains the authentication information. #[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr( @@ -287,6 +286,10 @@ pub struct TotpAuth { )] #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] #[cfg_attr(feature = "validate", derive(validator::Validate))] +#[cfg_attr( + feature = "validate", + validate(schema(function = "validate_totp_user_secret")) +)] pub struct TotpUser { /// User ID. #[cfg_attr(feature = "builder", builder(default))] @@ -302,10 +305,15 @@ pub struct TotpUser { pub domain: Option, /// The passcode generated by the user's TOTP device/app. #[cfg_attr(feature = "openapi", schema(value_type = String))] - #[serde(serialize_with = "serialize_secret_string")] + #[serde(serialize_with = "crate::common::serialize_secret_string")] pub passcode: SecretString, } +#[cfg(feature = "validate")] +fn validate_totp_user_secret(value: &TotpUser) -> Result<(), validator::ValidationError> { + crate::common::validate_secret_length(&value.passcode, 32) +} + /// User information. #[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] #[cfg_attr( @@ -346,13 +354,22 @@ pub struct User { )] #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] #[cfg_attr(feature = "validate", derive(validator::Validate))] +#[cfg_attr( + feature = "validate", + validate(schema(function = "validate_token_auth_secret")) +)] pub struct TokenAuth { /// An authentication token. #[cfg_attr(feature = "openapi", schema(value_type = String))] - #[serde(serialize_with = "serialize_secret_string")] + #[serde(serialize_with = "crate::common::serialize_secret_string")] pub id: SecretString, } +#[cfg(feature = "validate")] +fn validate_token_auth_secret(value: &TokenAuth) -> Result<(), validator::ValidationError> { + crate::common::validate_secret_length(&value.id, 1024) +} + #[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr(feature = "openapi", derive(utoipa::IntoParams))] #[cfg_attr(feature = "validate", derive(validator::Validate))] diff --git a/crates/api-types/src/v3/user.rs b/crates/api-types/src/v3/user.rs index 1bb5336e1..da4fa5a64 100644 --- a/crates/api-types/src/v3/user.rs +++ b/crates/api-types/src/v3/user.rs @@ -14,28 +14,12 @@ use std::collections::HashMap; use chrono::{DateTime, Utc}; -use secrecy::{ExposeSecret, SecretString}; -use serde::{Deserialize, Serialize, Serializer}; +use secrecy::SecretString; +use serde::{Deserialize, Serialize}; use serde_json::Value; #[cfg(feature = "validate")] use validator::Validate; -/// Serialize an optional password transparently for transport (the client must -/// send the real value). `SecretString` keeps it out of `Debug`/logs; the -/// response type carries no password field. -fn serialize_optional_secret( - secret: &Option, - serializer: S, -) -> Result -where - S: Serializer, -{ - match secret { - Some(secret) => serializer.serialize_some(secret.expose_secret()), - None => serializer.serialize_none(), - } -} - /// User response object. #[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] #[cfg_attr( @@ -130,6 +114,10 @@ pub struct UserResponse { )] #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] #[cfg_attr(feature = "validate", derive(validator::Validate))] +#[cfg_attr( + feature = "validate", + validate(schema(function = "validate_user_create_secret")) +)] pub struct UserCreate { /// The ID of the default project for the user. A user's default project /// must not be a domain. Setting this attribute does not grant any actual @@ -178,11 +166,39 @@ pub struct UserCreate { #[serde( default, skip_serializing_if = "Option::is_none", - serialize_with = "serialize_optional_secret" + serialize_with = "crate::common::serialize_optional_secret" )] pub password: Option, } +impl UserCreate { + #[must_use] + pub fn to_policy_input(&self) -> serde_json::Value { + let mut input = self + .extra + .clone() + .into_iter() + .collect::>(); + input.insert( + "default_project_id".to_string(), + serde_json::json!(self.default_project_id), + ); + input.insert("domain_id".to_string(), serde_json::json!(self.domain_id)); + input.insert("enabled".to_string(), serde_json::json!(self.enabled)); + input.insert("name".to_string(), serde_json::json!(self.name)); + input.insert("options".to_string(), serde_json::json!(self.options)); + if self.password.is_some() { + input.insert("password".to_string(), serde_json::json!("[REDACTED]")); + } + serde_json::Value::Object(input) + } +} + +#[cfg(feature = "validate")] +fn validate_user_create_secret(value: &UserCreate) -> Result<(), validator::ValidationError> { + crate::common::validate_optional_secret_length(&value.password, 72) +} + /// Complete create user request. #[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] @@ -205,6 +221,10 @@ pub struct UserCreateRequest { )] #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] #[cfg_attr(feature = "validate", derive(validator::Validate))] +#[cfg_attr( + feature = "validate", + validate(schema(function = "validate_user_update_secret")) +)] pub struct UserUpdate { /// The ID of the default project for the user. A user's default project /// must not be a domain. Setting this attribute does not grant any actual @@ -249,11 +269,38 @@ pub struct UserUpdate { #[serde( default, skip_serializing_if = "Option::is_none", - serialize_with = "serialize_optional_secret" + serialize_with = "crate::common::serialize_optional_secret" )] pub password: Option, } +impl UserUpdate { + #[must_use] + pub fn to_policy_input(&self) -> serde_json::Value { + let mut input = self + .extra + .clone() + .into_iter() + .collect::>(); + input.insert( + "default_project_id".to_string(), + serde_json::json!(self.default_project_id), + ); + input.insert("enabled".to_string(), serde_json::json!(self.enabled)); + input.insert("name".to_string(), serde_json::json!(self.name)); + input.insert("options".to_string(), serde_json::json!(self.options)); + if self.password.is_some() { + input.insert("password".to_string(), serde_json::json!("[REDACTED]")); + } + serde_json::Value::Object(input) + } +} + +#[cfg(feature = "validate")] +fn validate_user_update_secret(value: &UserUpdate) -> Result<(), validator::ValidationError> { + crate::common::validate_optional_secret_length(&value.password, 72) +} + /// Complete update user request. #[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] @@ -402,6 +449,34 @@ mod tests { assert!(rendered.contains("z_extra"), "extra dropped: {rendered}"); } + #[test] + fn user_policy_input_redacts_password_and_keeps_extra() { + let create: UserCreate = serde_json::from_str( + r#"{"domain_id":"d","name":"alice","enabled":true, + "password":"PWLEAK","x_custom":"xval"}"#, + ) + .unwrap(); + let input = create.to_policy_input(); + let rendered = input.to_string(); + assert!( + !rendered.contains("PWLEAK"), + "policy input leaked password: {rendered}" + ); + assert_eq!( + input.get("password").and_then(|v| v.as_str()), + Some("[REDACTED]") + ); + assert_eq!(input.get("x_custom").and_then(|v| v.as_str()), Some("xval")); + + let update: UserUpdate = + serde_json::from_str(r#"{"password":"UPWLEAK","z_extra":"zz"}"#).unwrap(); + let rendered = update.to_policy_input().to_string(); + assert!( + !rendered.contains("UPWLEAK"), + "policy input leaked password: {rendered}" + ); + } + /// Explicit `null` and absent password both deserialize to `None` (no panic, /// no plaintext resurrected). #[test] diff --git a/crates/api-types/src/v4/api_key.rs b/crates/api-types/src/v4/api_key.rs index ef0c16cef..500dac4ed 100644 --- a/crates/api-types/src/v4/api_key.rs +++ b/crates/api-types/src/v4/api_key.rs @@ -14,8 +14,8 @@ //! API Key (SCIM ingress machine identity) API types (ADR 0021). use chrono::{DateTime, Utc}; -use secrecy::{ExposeSecret, SecretString}; -use serde::{Deserialize, Serialize, Serializer}; +use secrecy::SecretString; +use serde::{Deserialize, Serialize}; #[cfg(feature = "validate")] use validator::Validate; @@ -116,17 +116,10 @@ pub struct ApiKeyCreateResponse { /// The full `kscim_...` bearer token. Shown once; store it now. #[cfg_attr(feature = "openapi", schema(value_type = String))] - #[serde(serialize_with = "serialize_secret_string")] + #[serde(serialize_with = "crate::common::serialize_secret_string")] pub token: SecretString, } -fn serialize_secret_string(secret: &SecretString, serializer: S) -> Result -where - S: Serializer, -{ - serializer.serialize_str(secret.expose_secret()) -} - /// API Key update request payload (`PUT /v4/api-keys/{client_id}`). /// /// `allowed_ips` and `description` use nested `Option`s: the field being diff --git a/crates/core-types/src/identity/user.rs b/crates/core-types/src/identity/user.rs index 8431ac45d..435b855a1 100644 --- a/crates/core-types/src/identity/user.rs +++ b/crates/core-types/src/identity/user.rs @@ -15,13 +15,31 @@ use std::collections::HashMap; use chrono::{DateTime, Utc}; use derive_builder::Builder; -use secrecy::SecretString; +use secrecy::{ExposeSecret, SecretString}; use serde::Serialize; use serde_json::Value; -use validator::Validate; +use validator::{Validate, ValidationError}; use crate::error::BuilderError; +fn validate_secret_length(secret: &SecretString, max: usize) -> Result<(), ValidationError> { + if secret.expose_secret().chars().count() <= max { + Ok(()) + } else { + Err(ValidationError::new("length")) + } +} + +fn validate_optional_secret_length( + secret: &Option, + max: usize, +) -> Result<(), ValidationError> { + match secret { + Some(secret) => validate_secret_length(secret, max), + None => Ok(()), + } +} + // NOTE: password length/non-emptiness is not validated with a field-level // validator here — `SecretString` cannot use validator's `length` (no // `ValidateLength`) nor `custom` (which requires the field to be `Serialize`). @@ -86,6 +104,7 @@ pub struct UserResponse { /// `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"))] #[builder(build_fn(error = "BuilderError"))] #[builder(setter(strip_option, into))] pub struct UserCreate { @@ -143,11 +162,16 @@ pub struct UserCreate { pub user_type: UserType, } +fn validate_user_create_secret(value: &UserCreate) -> Result<(), ValidationError> { + validate_optional_secret_length(&value.password, 72) +} + /// User update data. /// /// `PartialEq` is intentionally not derived: `password` is wrapped in /// [`SecretString`], which does not implement `PartialEq` by design. #[derive(Builder, Clone, Debug, Default, Validate)] +#[validate(schema(function = "validate_user_update_secret"))] #[builder(build_fn(error = "BuilderError"))] #[builder(setter(strip_option, into))] pub struct UserUpdate { @@ -189,6 +213,10 @@ pub struct UserUpdate { pub password: Option, } +fn validate_user_update_secret(value: &UserUpdate) -> Result<(), ValidationError> { + validate_optional_secret_length(&value.password, 72) +} + /// User options. #[derive(Builder, Clone, Debug, Default, PartialEq, Serialize, Validate)] #[builder(build_fn(error = "BuilderError"))] @@ -297,6 +325,7 @@ pub enum UserType { /// `Default` and `PartialEq` are intentionally not derived: `password` is a /// required [`SecretString`], which implements neither by design. #[derive(Builder, Clone, Debug, Validate)] +#[validate(schema(function = "validate_user_password_auth_secret"))] #[builder(build_fn(error = "BuilderError"))] #[builder(setter(strip_option, into))] pub struct UserPasswordAuthRequest { @@ -320,6 +349,12 @@ pub struct UserPasswordAuthRequest { pub password: SecretString, } +fn validate_user_password_auth_secret( + value: &UserPasswordAuthRequest, +) -> Result<(), ValidationError> { + validate_secret_length(&value.password, 72) +} + /// Manual `Default` (the derive cannot be used because `SecretString` does not /// implement `Default`). Preserves the pre-wrapping default of an empty /// password. Production code constructs this via the builder, which requires an @@ -340,6 +375,7 @@ impl Default for UserPasswordAuthRequest { /// `PartialEq`/`Default` are intentionally not derived: `passcode` is a required /// [`SecretString`], which implements neither by design. #[derive(Builder, Clone, Debug, Validate)] +#[validate(schema(function = "validate_user_totp_auth_secret"))] #[builder(build_fn(error = "BuilderError"))] #[builder(setter(strip_option, into))] pub struct UserTotpAuthRequest { @@ -362,6 +398,10 @@ pub struct UserTotpAuthRequest { pub passcode: SecretString, } +fn validate_user_totp_auth_secret(value: &UserTotpAuthRequest) -> Result<(), ValidationError> { + validate_secret_length(&value.passcode, 32) +} + /// Domain information. #[derive(Builder, Clone, Debug, Default, PartialEq, Validate)] #[builder(build_fn(error = "BuilderError"))] diff --git a/crates/core/src/credential/totp.rs b/crates/core/src/credential/totp.rs index 7896bf346..92e3d4a08 100644 --- a/crates/core/src/credential/totp.rs +++ b/crates/core/src/credential/totp.rs @@ -21,6 +21,7 @@ //! Fernet decryption that already produces the shared plaintext seed. use data_encoding::BASE32_NOPAD; +use secrecy::{ExposeSecret, SecretString}; use super::ec2_signature::hmac_sha1_raw; @@ -67,7 +68,14 @@ fn generate_hotp(secret: &[u8], counter: u64, digits: u32) -> String { /// `true` if `passcode` matches the current or immediately preceding /// time-step's HOTP value; `false` on any mismatch or malformed seed. #[must_use] -pub fn verify_totp(seed: &str, passcode: &str, digits: u32, period: u32, now_unix: i64) -> bool { +pub fn verify_totp( + seed: &str, + passcode: &SecretString, + digits: u32, + period: u32, + now_unix: i64, +) -> bool { + let passcode = passcode.expose_secret(); if digits == 0 || digits > 10 || period == 0 || passcode.len() != digits as usize { return false; } @@ -112,7 +120,7 @@ mod tests { fn test_verify_totp_current_window() { assert!(verify_totp( RFC6238_SEED_BASE32, - "94287082", + &SecretString::from("94287082"), 8, 30, 59, // counter = 59/30 = 1 @@ -124,22 +132,46 @@ mod tests { // counter for now_unix=90 is 3; passcode for counter=1 must still be // rejected since it is two windows back (only current & previous are // accepted). - assert!(!verify_totp(RFC6238_SEED_BASE32, "94287082", 8, 30, 90)); + assert!(!verify_totp( + RFC6238_SEED_BASE32, + &SecretString::from("94287082"), + 8, + 30, + 90 + )); // But the passcode for counter=2 (previous window relative to now=90, // counter=3) must be accepted. let secret = decode_base32_seed(RFC6238_SEED_BASE32).unwrap(); let previous_code = generate_hotp(&secret, 2, 8); - assert!(verify_totp(RFC6238_SEED_BASE32, &previous_code, 8, 30, 90)); + assert!(verify_totp( + RFC6238_SEED_BASE32, + &SecretString::from(previous_code), + 8, + 30, + 90 + )); } #[test] fn test_verify_totp_wrong_passcode() { - assert!(!verify_totp(RFC6238_SEED_BASE32, "00000000", 8, 30, 59)); + assert!(!verify_totp( + RFC6238_SEED_BASE32, + &SecretString::from("00000000"), + 8, + 30, + 59 + )); } #[test] fn test_verify_totp_malformed_seed() { - assert!(!verify_totp("not-valid-base32!!!", "123456", 6, 30, 59)); + assert!(!verify_totp( + "not-valid-base32!!!", + &SecretString::from("123456"), + 6, + 30, + 59 + )); } #[test] @@ -147,7 +179,13 @@ mod tests { // A 4-digit passcode against a 6-digit credential must be rejected // outright rather than falling through to a (mismatched-length) // constant-time comparison. - assert!(!verify_totp("JBSWY3DPEHPK3PXP", "1234", 6, 30, 59)); + assert!(!verify_totp( + "JBSWY3DPEHPK3PXP", + &SecretString::from("1234"), + 6, + 30, + 59 + )); } #[test] diff --git a/crates/core/src/identity/service.rs b/crates/core/src/identity/service.rs index 4c91280f0..58d8920ff 100644 --- a/crates/core/src/identity/service.rs +++ b/crates/core/src/identity/service.rs @@ -16,7 +16,7 @@ use async_trait::async_trait; use chrono::{DateTime, Utc}; -use secrecy::{ExposeSecret, SecretString}; +use secrecy::SecretString; use std::collections::{HashMap, HashSet}; use std::sync::Arc; use tokio::sync::RwLock; @@ -438,13 +438,7 @@ impl IdentityApi for IdentityService { .and_then(serde_json::Value::as_u64) .map(|d| d as u32) .unwrap_or(30); - crate::credential::totp::verify_totp( - seed, - auth.passcode.expose_secret(), - digits, - period, - now, - ) + crate::credential::totp::verify_totp(seed, &auth.passcode, digits, period, now) }); if !matched { diff --git a/crates/keystone/src/api/v3/user/create.rs b/crates/keystone/src/api/v3/user/create.rs index 09211d2c7..b45091c03 100644 --- a/crates/keystone/src/api/v3/user/create.rs +++ b/crates/keystone/src/api/v3/user/create.rs @@ -45,7 +45,7 @@ pub(super) async fn create( .enforce( "identity/user/create", &user_auth, - json!({"user": req.user}), + json!({"user": req.user.to_policy_input()}), None, ) .await?; diff --git a/crates/keystone/src/api/v3/user/update.rs b/crates/keystone/src/api/v3/user/update.rs index 02b56f5c5..d3cfd9018 100644 --- a/crates/keystone/src/api/v3/user/update.rs +++ b/crates/keystone/src/api/v3/user/update.rs @@ -64,7 +64,7 @@ pub(super) async fn update( .enforce( "identity/user/update", &user_auth, - json!({"user": req.user}), + json!({"user": req.user.to_policy_input()}), existing_user, ) .await?; diff --git a/crates/keystone/src/federation/api/identity_provider/create.rs b/crates/keystone/src/federation/api/identity_provider/create.rs index 494765c51..79da99d08 100644 --- a/crates/keystone/src/federation/api/identity_provider/create.rs +++ b/crates/keystone/src/federation/api/identity_provider/create.rs @@ -57,7 +57,7 @@ pub(super) async fn create( .enforce( "identity/federation/identity_provider/create", &user_auth, - json!({"identity_provider": req.identity_provider}), + json!({"identity_provider": req.identity_provider.to_policy_input()}), None, ) .await?; diff --git a/crates/keystone/src/federation/api/identity_provider/update.rs b/crates/keystone/src/federation/api/identity_provider/update.rs index ade6d072d..9ee7e94c5 100644 --- a/crates/keystone/src/federation/api/identity_provider/update.rs +++ b/crates/keystone/src/federation/api/identity_provider/update.rs @@ -70,8 +70,8 @@ pub(super) async fn update( .enforce( "identity/federation/identity_provider/update", &user_auth, - json!({"identity_provider": current}), - Some(json!({"identity_provider": req.identity_provider})), + json!({"identity_provider": req.identity_provider.to_policy_input()}), + Some(json!({"identity_provider": current})), ) .await?; From cf3bc42a75f8186fcf02c4c5cf810d442d56c233 Mon Sep 17 00:00:00 2001 From: Yousef Hussein Date: Wed, 8 Jul 2026 00:25:01 +0300 Subject: [PATCH 4/7] fix(security): Address secret-wrapping review feedback Resolve gtema's review round on PR #912 (#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 Claude-Session: https://claude.ai/code/session_01DPZxPWH9yYH36jgfwGqZdQ Signed-off-by: Yousef Hussein --- .../src/federation/identity_provider.rs | 45 ++++--- crates/api-types/src/v3/auth/token.rs | 12 ++ crates/api-types/src/v3/auth_conv.rs | 5 +- crates/api-types/src/v3/user.rs | 61 ++++++--- crates/core-types/src/identity/user.rs | 28 +++- crates/keystone/src/api/v4/user/create.rs | 84 +++++++++++- crates/keystone/src/api/v4/user/update.rs | 124 +++++++++++++++++- 7 files changed, 313 insertions(+), 46 deletions(-) diff --git a/crates/api-types/src/federation/identity_provider.rs b/crates/api-types/src/federation/identity_provider.rs index 5b3784d3a..e94dcaaa1 100644 --- a/crates/api-types/src/federation/identity_provider.rs +++ b/crates/api-types/src/federation/identity_provider.rs @@ -262,12 +262,6 @@ impl IdentityProviderCreate { 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() { - input.insert( - "oidc_client_secret".to_string(), - serde_json::json!("[REDACTED]"), - ); - } if let Some(value) = &self.oidc_response_mode { input.insert("oidc_response_mode".to_string(), serde_json::json!(value)); } @@ -306,6 +300,10 @@ impl IdentityProviderCreate { } #[cfg(feature = "validate")] +// NOTE: Struct-level (not field-level #[validate(custom)]) because validator +// 0.20 serializes the failing field into ValidationError, which does not +// compile for SecretString and would leak the secret; the derive still +// validates all other fields. fn validate_identity_provider_create_secret( value: &IdentityProviderCreate, ) -> Result<(), validator::ValidationError> { @@ -418,12 +416,6 @@ impl IdentityProviderUpdate { "oidc_client_id".to_string(), serde_json::json!(self.oidc_client_id), ); - if self.oidc_client_secret.is_some() { - input.insert( - "oidc_client_secret".to_string(), - serde_json::json!("[REDACTED]"), - ); - } input.insert( "oidc_response_mode".to_string(), serde_json::json!(self.oidc_response_mode), @@ -462,6 +454,10 @@ impl IdentityProviderUpdate { } #[cfg(feature = "validate")] +// NOTE: Struct-level (not field-level #[validate(custom)]) because validator +// 0.20 serializes the failing field into ValidationError, which does not +// compile for SecretString and would leak the secret; the derive still +// validates all other fields. fn validate_identity_provider_update_secret( value: &IdentityProviderUpdate, ) -> Result<(), validator::ValidationError> { @@ -566,14 +562,17 @@ mod tests { } #[test] - fn idp_policy_input_redacts_client_secret() { + fn idp_policy_input_omits_client_secret() { let create: IdentityProviderCreate = serde_json::from_str(r#"{"name":"idp","oidc_client_secret":"CSLEAK"}"#).unwrap(); - let rendered = create.to_policy_input().to_string(); + let input = create.to_policy_input(); + let rendered = input.to_string(); assert!( !rendered.contains("CSLEAK"), "policy input leaked client secret: {rendered}" ); + assert!(input.get("oidc_client_secret").is_none()); + assert_eq!(input.get("name").and_then(|v| v.as_str()), Some("idp")); let update: IdentityProviderUpdate = serde_json::from_str(r#"{"oidc_client_secret":"CSLEAK2"}"#).unwrap(); @@ -583,10 +582,8 @@ mod tests { !rendered.contains("CSLEAK2"), "policy input leaked client secret: {rendered}" ); - assert_eq!( - input.get("oidc_client_secret").and_then(|v| v.as_str()), - Some("[REDACTED]") - ); + assert!(input.get("oidc_client_secret").is_none()); + assert!(input.get("oidc_client_id").is_some()); } /// Regression guard: the read/response DTO has no client-secret field, so a @@ -608,4 +605,16 @@ mod tests { "response leaked the secret value: {rendered}" ); } + + #[cfg(feature = "validate")] + #[test] + fn idp_update_validates_overlong_client_secret() { + let update: IdentityProviderUpdate = serde_json::from_str(&format!( + r#"{{"oidc_client_secret":"{}"}}"#, + "x".repeat(256) + )) + .unwrap(); + + assert!(update.validate().is_err()); + } } diff --git a/crates/api-types/src/v3/auth/token.rs b/crates/api-types/src/v3/auth/token.rs index f11234306..2daa813f1 100644 --- a/crates/api-types/src/v3/auth/token.rs +++ b/crates/api-types/src/v3/auth/token.rs @@ -252,6 +252,10 @@ pub struct UserPassword { } #[cfg(feature = "validate")] +// NOTE: Struct-level (not field-level #[validate(custom)]) because validator +// 0.20 serializes the failing field into ValidationError, which does not +// compile for SecretString and would leak the secret; the derive still +// validates all other fields. fn validate_user_password_secret(value: &UserPassword) -> Result<(), validator::ValidationError> { crate::common::validate_secret_length(&value.password, 72) } @@ -310,6 +314,10 @@ pub struct TotpUser { } #[cfg(feature = "validate")] +// NOTE: Struct-level (not field-level #[validate(custom)]) because validator +// 0.20 serializes the failing field into ValidationError, which does not +// compile for SecretString and would leak the secret; the derive still +// validates all other fields. fn validate_totp_user_secret(value: &TotpUser) -> Result<(), validator::ValidationError> { crate::common::validate_secret_length(&value.passcode, 32) } @@ -366,6 +374,10 @@ pub struct TokenAuth { } #[cfg(feature = "validate")] +// NOTE: Struct-level (not field-level #[validate(custom)]) because validator +// 0.20 serializes the failing field into ValidationError, which does not +// compile for SecretString and would leak the secret; the derive still +// validates all other fields. fn validate_token_auth_secret(value: &TokenAuth) -> Result<(), validator::ValidationError> { crate::common::validate_secret_length(&value.id, 1024) } diff --git a/crates/api-types/src/v3/auth_conv.rs b/crates/api-types/src/v3/auth_conv.rs index b66cc1110..eb84cbf2d 100644 --- a/crates/api-types/src/v3/auth_conv.rs +++ b/crates/api-types/src/v3/auth_conv.rs @@ -13,7 +13,6 @@ // SPDX-License-Identifier: Apache-2.0 use openstack_keystone_core_types::error::BuilderError; -use secrecy::ExposeSecret; use crate::v3::auth::token as api_types; @@ -42,7 +41,7 @@ impl TryFrom } upa.domain(domain_builder.build()?); } - upa.password(value.password.expose_secret()); + upa.password(value.password); upa.build() } } @@ -70,7 +69,7 @@ impl TryFrom for openstack_keystone_core_types::identity::U } uta.domain(domain_builder.build()?); } - uta.passcode(value.passcode.clone()); + uta.passcode(value.passcode); uta.build() } } diff --git a/crates/api-types/src/v3/user.rs b/crates/api-types/src/v3/user.rs index da4fa5a64..102a6d66d 100644 --- a/crates/api-types/src/v3/user.rs +++ b/crates/api-types/src/v3/user.rs @@ -159,8 +159,8 @@ pub struct UserCreate { #[cfg_attr(feature = "validate", validate(nested))] pub options: Option, - /// The password for the user. Non-emptiness and regex policy are enforced at - /// the service layer via `security_compliance.validate_password`. + /// The password for the user. Non-emptiness and regex policy are enforced + /// at the service layer via `security_compliance.validate_password`. #[cfg_attr(feature = "builder", builder(default))] #[cfg_attr(feature = "openapi", schema(value_type = Option))] #[serde( @@ -187,14 +187,15 @@ impl UserCreate { input.insert("enabled".to_string(), serde_json::json!(self.enabled)); input.insert("name".to_string(), serde_json::json!(self.name)); input.insert("options".to_string(), serde_json::json!(self.options)); - if self.password.is_some() { - input.insert("password".to_string(), serde_json::json!("[REDACTED]")); - } serde_json::Value::Object(input) } } #[cfg(feature = "validate")] +// NOTE: Struct-level (not field-level #[validate(custom)]) because validator +// 0.20 serializes the failing field into ValidationError, which does not +// compile for SecretString and would leak the secret; the derive still +// validates all other fields. fn validate_user_create_secret(value: &UserCreate) -> Result<(), validator::ValidationError> { crate::common::validate_optional_secret_length(&value.password, 72) } @@ -289,14 +290,15 @@ impl UserUpdate { input.insert("enabled".to_string(), serde_json::json!(self.enabled)); input.insert("name".to_string(), serde_json::json!(self.name)); input.insert("options".to_string(), serde_json::json!(self.options)); - if self.password.is_some() { - input.insert("password".to_string(), serde_json::json!("[REDACTED]")); - } serde_json::Value::Object(input) } } #[cfg(feature = "validate")] +// NOTE: Struct-level (not field-level #[validate(custom)]) because validator +// 0.20 serializes the failing field into ValidationError, which does not +// compile for SecretString and would leak the secret; the derive still +// validates all other fields. fn validate_user_update_secret(value: &UserUpdate) -> Result<(), validator::ValidationError> { crate::common::validate_optional_secret_length(&value.password, 72) } @@ -395,7 +397,8 @@ mod tests { /// Critical: `UserCreate` carries BOTH `#[serde(flatten)] extra` and a /// `password`. Prove the flatten interaction round-trips the password for - /// transport and does not drop `extra`, while `Debug` never leaks the value. + /// transport and does not drop `extra`, while `Debug` never leaks the + /// value. #[test] fn usercreate_flatten_keeps_password_and_extra() { let uc: UserCreate = serde_json::from_str( @@ -450,7 +453,7 @@ mod tests { } #[test] - fn user_policy_input_redacts_password_and_keeps_extra() { + fn user_policy_input_omits_password_and_keeps_extra() { let create: UserCreate = serde_json::from_str( r#"{"domain_id":"d","name":"alice","enabled":true, "password":"PWLEAK","x_custom":"xval"}"#, @@ -462,23 +465,23 @@ mod tests { !rendered.contains("PWLEAK"), "policy input leaked password: {rendered}" ); - assert_eq!( - input.get("password").and_then(|v| v.as_str()), - Some("[REDACTED]") - ); + assert!(input.get("password").is_none()); assert_eq!(input.get("x_custom").and_then(|v| v.as_str()), Some("xval")); let update: UserUpdate = serde_json::from_str(r#"{"password":"UPWLEAK","z_extra":"zz"}"#).unwrap(); - let rendered = update.to_policy_input().to_string(); + let input = update.to_policy_input(); + let rendered = input.to_string(); assert!( !rendered.contains("UPWLEAK"), "policy input leaked password: {rendered}" ); + assert!(input.get("password").is_none()); + assert_eq!(input.get("z_extra").and_then(|v| v.as_str()), Some("zz")); } - /// Explicit `null` and absent password both deserialize to `None` (no panic, - /// no plaintext resurrected). + /// Explicit `null` and absent password both deserialize to `None` (no + /// panic, no plaintext resurrected). #[test] fn usercreate_password_null_and_absent_are_none() { let with_null: UserCreate = @@ -500,6 +503,30 @@ mod tests { assert!(!format!("{uc:?}").contains("DBGLEAK")); } + #[cfg(feature = "validate")] + #[test] + fn usercreate_validates_password_and_other_fields() { + let valid: UserCreate = serde_json::from_str( + r#"{"domain_id":"d","name":"alice","enabled":true,"password":"secret"}"#, + ) + .unwrap(); + assert!(valid.validate().is_ok()); + + let overlong_password: UserCreate = serde_json::from_str(&format!( + r#"{{"domain_id":"d","name":"alice","enabled":true,"password":"{}"}}"#, + "x".repeat(73) + )) + .unwrap(); + assert!(overlong_password.validate().is_err()); + + let overlong_name: UserCreate = serde_json::from_str(&format!( + r#"{{"domain_id":"d","name":"{}","enabled":true,"password":"secret"}}"#, + "x".repeat(256) + )) + .unwrap(); + assert!(overlong_name.validate().is_err()); + } + #[cfg(feature = "builder")] #[test] fn test_user_create() { diff --git a/crates/core-types/src/identity/user.rs b/crates/core-types/src/identity/user.rs index 435b855a1..1648906ec 100644 --- a/crates/core-types/src/identity/user.rs +++ b/crates/core-types/src/identity/user.rs @@ -148,8 +148,8 @@ pub struct UserCreate { #[validate(nested)] pub options: Option, - /// User password. Non-emptiness and regex policy are enforced at the service - /// layer via `security_compliance.validate_password`. + /// User password. Non-emptiness and regex policy are enforced at the + /// service layer via `security_compliance.validate_password`. #[builder(default)] pub password: Option, @@ -162,6 +162,10 @@ pub struct UserCreate { pub user_type: UserType, } +// NOTE: Struct-level (not field-level #[validate(custom)]) because validator +// 0.20 serializes the failing field into ValidationError, which does not +// compile for SecretString and would leak the secret; the derive still +// validates all other fields. fn validate_user_create_secret(value: &UserCreate) -> Result<(), ValidationError> { validate_optional_secret_length(&value.password, 72) } @@ -207,12 +211,16 @@ pub struct UserUpdate { #[validate(nested)] pub options: Option, - /// New user password. Non-emptiness/policy enforced at the service layer via - /// `security_compliance.validate_password`. + /// New user password. Non-emptiness/policy enforced at the service layer + /// via `security_compliance.validate_password`. #[builder(default)] pub password: Option, } +// NOTE: Struct-level (not field-level #[validate(custom)]) because validator +// 0.20 serializes the failing field into ValidationError, which does not +// compile for SecretString and would leak the secret; the derive still +// validates all other fields. fn validate_user_update_secret(value: &UserUpdate) -> Result<(), ValidationError> { validate_optional_secret_length(&value.password, 72) } @@ -349,6 +357,10 @@ pub struct UserPasswordAuthRequest { pub password: SecretString, } +// NOTE: Struct-level (not field-level #[validate(custom)]) because validator +// 0.20 serializes the failing field into ValidationError, which does not +// compile for SecretString and would leak the secret; the derive still +// validates all other fields. fn validate_user_password_auth_secret( value: &UserPasswordAuthRequest, ) -> Result<(), ValidationError> { @@ -372,8 +384,8 @@ impl Default for UserPasswordAuthRequest { /// User TOTP authentication request. /// -/// `PartialEq`/`Default` are intentionally not derived: `passcode` is a required -/// [`SecretString`], which implements neither by design. +/// `PartialEq`/`Default` are intentionally not derived: `passcode` is a +/// required [`SecretString`], which implements neither by design. #[derive(Builder, Clone, Debug, Validate)] #[validate(schema(function = "validate_user_totp_auth_secret"))] #[builder(build_fn(error = "BuilderError"))] @@ -398,6 +410,10 @@ pub struct UserTotpAuthRequest { pub passcode: SecretString, } +// NOTE: Struct-level (not field-level #[validate(custom)]) because validator +// 0.20 serializes the failing field into ValidationError, which does not +// compile for SecretString and would leak the secret; the derive still +// validates all other fields. fn validate_user_totp_auth_secret(value: &UserTotpAuthRequest) -> Result<(), ValidationError> { validate_secret_length(&value.passcode, 32) } diff --git a/crates/keystone/src/api/v4/user/create.rs b/crates/keystone/src/api/v4/user/create.rs index 87f9555d8..d78ea2127 100644 --- a/crates/keystone/src/api/v4/user/create.rs +++ b/crates/keystone/src/api/v4/user/create.rs @@ -46,7 +46,7 @@ pub(super) async fn create( .enforce( "identity/user/create", &user_auth, - json!({"user": req.user}), + json!({"user": req.user.to_policy_input()}), None, ) .await?; @@ -68,14 +68,21 @@ pub(super) async fn create( #[cfg(test)] mod tests { + use std::sync::Arc; + use axum::{ body::Body, http::{self, Request, StatusCode}, }; use http_body_util::BodyExt; + use openstack_keystone_audit::AuditDispatcher; + use openstack_keystone_config::{Config, ConfigManager}; + use sea_orm::DatabaseConnection; + use serde_json::{Value, json}; use tower::ServiceExt; use tower_http::trace::TraceLayer; + use openstack_keystone_core::policy::{PolicyError, PolicyEvaluationResult}; use openstack_keystone_core_types::identity::*; use super::super::openapi_router; @@ -84,6 +91,8 @@ mod tests { UserCreateBuilder as ApiUserCreate, UserCreateRequest, UserResponse as ApiUserResponse, }; use crate::identity::MockIdentityProvider; + use crate::keystone::{Service, ServiceState}; + use crate::policy::MockPolicy; use crate::provider::Provider; #[tokio::test] @@ -143,6 +152,79 @@ mod tests { assert_eq!(created_user.user.name, user.user.name); } + async fn get_state_with_policy( + identity_mock: MockIdentityProvider, + policy_enforcer_mock: MockPolicy, + ) -> Result> { + let provider = Provider::mocked_builder() + .mock_identity(identity_mock) + .build()?; + let service = Service::new( + ConfigManager::not_watched(Config::default()), + DatabaseConnection::Disconnected, + provider, + Arc::new(policy_enforcer_mock), + AuditDispatcher::noop(), + None, + ) + .await?; + Ok(Arc::new(service)) + } + + #[tokio::test] + async fn test_create_policy_input_omits_password() + -> Result<(), Box> { + const POLICY_PASSWORD: &str = "CreatePolicyLeak1!"; + + let vsc = test_fixture_scoped(); + let identity_mock = MockIdentityProvider::default(); + + let mut policy_enforcer_mock = MockPolicy::default(); + policy_enforcer_mock + .expect_enforce() + .returning(|_, _, target, existing| { + assert!(existing.is_none()); + assert!(!target.to_string().contains(POLICY_PASSWORD)); + let user = target.get("user").and_then(Value::as_object); + assert!(user.is_some_and(|user| !user.contains_key("password"))); + Err(PolicyError::Forbidden(PolicyEvaluationResult::forbidden())) + }); + policy_enforcer_mock + .expect_health_check() + .returning(|| Ok(())); + + let state = get_state_with_policy(identity_mock, policy_enforcer_mock).await?; + + let mut api = openapi_router() + .layer(TraceLayer::new_for_http()) + .with_state(state.clone()); + + let user = json!({ + "user": { + "domain_id": "domain", + "enabled": true, + "name": "name", + "password": POLICY_PASSWORD + } + }); + + let response = api + .as_service() + .oneshot( + Request::builder() + .method("POST") + .header(http::header::CONTENT_TYPE, "application/json") + .uri("/") + .extension(vsc) + .body(Body::from(user.to_string()))?, + ) + .await?; + + assert_eq!(response.status(), StatusCode::FORBIDDEN); + + Ok(()) + } + #[tokio::test] async fn test_create_policy_denied() { let vsc = test_fixture_scoped(); diff --git a/crates/keystone/src/api/v4/user/update.rs b/crates/keystone/src/api/v4/user/update.rs index 871e496f5..8102e0f94 100644 --- a/crates/keystone/src/api/v4/user/update.rs +++ b/crates/keystone/src/api/v4/user/update.rs @@ -62,7 +62,7 @@ pub(super) async fn update( .enforce( "identity/user/update", &user_auth, - json!({"user": req.user}), + json!({"user": req.user.to_policy_input()}), existing_user, ) .await?; @@ -94,14 +94,21 @@ pub(super) async fn update( #[cfg(test)] mod tests { + use std::{collections::HashMap, sync::Arc}; + use axum::{ body::Body, http::{self, Request, StatusCode}, }; use http_body_util::BodyExt; + use openstack_keystone_audit::AuditDispatcher; + use openstack_keystone_config::{Config, ConfigManager}; + use sea_orm::DatabaseConnection; + use serde_json::{Value, json}; use tower::ServiceExt; use tower_http::trace::TraceLayer; + use openstack_keystone_core::policy::PolicyEvaluationResult; use openstack_keystone_core_types::identity::*; use super::super::openapi_router; @@ -110,6 +117,8 @@ mod tests { UserResponse as ApiUserResponse, UserUpdateBuilder as ApiUserUpdate, UserUpdateRequest, }; use crate::identity::MockIdentityProvider; + use crate::keystone::{Service, ServiceState}; + use crate::policy::MockPolicy; use crate::provider::Provider; #[tokio::test] @@ -186,6 +195,119 @@ mod tests { assert_eq!(updated_user.user.id, "bar"); } + async fn get_state_with_policy( + identity_mock: MockIdentityProvider, + policy_enforcer_mock: MockPolicy, + ) -> Result> { + let provider = Provider::mocked_builder() + .mock_identity(identity_mock) + .build()?; + let service = Service::new( + ConfigManager::not_watched(Config::default()), + DatabaseConnection::Disconnected, + provider, + Arc::new(policy_enforcer_mock), + AuditDispatcher::noop(), + None, + ) + .await?; + Ok(Arc::new(service)) + } + + #[tokio::test] + async fn test_update_policy_input_omits_password() + -> Result<(), Box> { + const POLICY_PASSWORD: &str = "UPDATE_POLICY_LEAK"; + + let vsc = test_fixture_scoped(); + let mut identity_mock = MockIdentityProvider::default(); + + identity_mock + .expect_get_user() + .withf(|_, id: &'_ str| id == "bar") + .returning(|_, _| { + Ok(Some(UserResponse { + default_project_id: None, + domain_id: "did".to_string(), + enabled: true, + extra: HashMap::new(), + federated: None, + id: "bar".to_string(), + name: "old_name".to_string(), + options: UserOptions::default(), + password_expires_at: None, + })) + }); + + identity_mock + .expect_update_user() + .withf( + |_, id: &'_ str, _: &openstack_keystone_core_types::identity::UserUpdate| { + id == "bar" + }, + ) + .returning(|_, _, _| { + Ok(UserResponse { + default_project_id: None, + domain_id: "did".to_string(), + enabled: true, + extra: HashMap::new(), + federated: None, + id: "bar".to_string(), + name: "new_name".to_string(), + options: UserOptions::default(), + password_expires_at: None, + }) + }); + + let mut policy_enforcer_mock = MockPolicy::default(); + policy_enforcer_mock + .expect_enforce() + .returning(|_, _, target, existing| { + assert!(!target.to_string().contains(POLICY_PASSWORD)); + let user = target.get("user").and_then(Value::as_object); + assert!(user.is_some_and(|user| !user.contains_key("password"))); + let current = existing + .as_ref() + .and_then(|existing| existing.get("user")) + .and_then(Value::as_object); + assert!(current.is_some_and(|user| !user.contains_key("password"))); + Ok(PolicyEvaluationResult::allowed()) + }); + policy_enforcer_mock + .expect_health_check() + .returning(|| Ok(())); + + let state = get_state_with_policy(identity_mock, policy_enforcer_mock).await?; + + let mut api = openapi_router() + .layer(TraceLayer::new_for_http()) + .with_state(state.clone()); + + let update_req = json!({ + "user": { + "name": "new_name", + "password": POLICY_PASSWORD + } + }); + + let response = api + .as_service() + .oneshot( + Request::builder() + .method("PUT") + .header(http::header::CONTENT_TYPE, "application/json") + .uri("/bar") + .extension(vsc) + .body(Body::from(update_req.to_string()))?, + ) + .await?; + + assert_eq!(response.status(), StatusCode::OK); + + Ok(()) + } + #[tokio::test] async fn test_update_not_found() { let vsc = test_fixture_scoped(); From 21b23e2af8630887ac66844bb5365df2524764cf Mon Sep 17 00:00:00 2001 From: Yousef Hussein Date: Wed, 8 Jul 2026 17:51:49 +0300 Subject: [PATCH 5/7] fix(security): Strip policy-input secrets via Value Address gtema's review on PR #912 (#369): build to_policy_input by serializing the whole struct to a Value and removing the secret key(s) instead of hand-building the map. This is shorter and avoids silently dropping newly added non-secret fields from policy input. - UserCreate::to_policy_input removes "password". - IdentityProviderCreate/Update::to_policy_input remove "oidc_client_secret". The transient exposed value lives only in the in-process Value and is removed before return; it never reaches OPA. Tests assert the secret key is absent while a non-secret key remains. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01DPZxPWH9yYH36jgfwGqZdQ Signed-off-by: Yousef Hussein --- .../src/federation/identity_provider.rs | 99 ++----------------- crates/api-types/src/v3/user.rs | 19 +--- 2 files changed, 14 insertions(+), 104 deletions(-) diff --git a/crates/api-types/src/federation/identity_provider.rs b/crates/api-types/src/federation/identity_provider.rs index e94dcaaa1..c1f4d436a 100644 --- a/crates/api-types/src/federation/identity_provider.rs +++ b/crates/api-types/src/federation/identity_provider.rs @@ -250,52 +250,11 @@ pub struct IdentityProviderCreate { impl IdentityProviderCreate { #[must_use] pub fn to_policy_input(&self) -> serde_json::Value { - let mut input = serde_json::Map::new(); - input.insert("name".to_string(), serde_json::json!(self.name)); - input.insert("enabled".to_string(), serde_json::json!(self.enabled)); - if let Some(value) = &self.domain_id { - input.insert("domain_id".to_string(), serde_json::json!(value)); + let mut value = serde_json::to_value(self).unwrap_or_default(); + if let Some(input) = value.as_object_mut() { + input.remove("oidc_client_secret"); } - if let Some(value) = &self.oidc_discovery_url { - input.insert("oidc_discovery_url".to_string(), serde_json::json!(value)); - } - if let Some(value) = &self.oidc_client_id { - input.insert("oidc_client_id".to_string(), serde_json::json!(value)); - } - if let Some(value) = &self.oidc_response_mode { - input.insert("oidc_response_mode".to_string(), serde_json::json!(value)); - } - if let Some(value) = &self.oidc_response_types { - input.insert("oidc_response_types".to_string(), serde_json::json!(value)); - } - if let Some(value) = &self.jwks_url { - input.insert("jwks_url".to_string(), serde_json::json!(value)); - } - if let Some(value) = &self.jwt_validation_pubkeys { - input.insert( - "jwt_validation_pubkeys".to_string(), - serde_json::json!(value), - ); - } - if let Some(value) = &self.bound_issuer { - input.insert("bound_issuer".to_string(), serde_json::json!(value)); - } - if let Some(value) = &self.default_mapping_name { - input.insert("default_mapping_name".to_string(), serde_json::json!(value)); - } - if let Some(value) = &self.oidc_scopes { - input.insert("oidc_scopes".to_string(), serde_json::json!(value)); - } - if let Some(value) = &self.allowed_redirect_uris { - input.insert( - "allowed_redirect_uris".to_string(), - serde_json::json!(value), - ); - } - if let Some(value) = &self.provider_config { - input.insert("provider_config".to_string(), value.clone()); - } - serde_json::Value::Object(input) + value } } @@ -405,51 +364,11 @@ pub struct IdentityProviderUpdate { impl IdentityProviderUpdate { #[must_use] pub fn to_policy_input(&self) -> serde_json::Value { - let mut input = serde_json::Map::new(); - input.insert("name".to_string(), serde_json::json!(self.name)); - input.insert("enabled".to_string(), serde_json::json!(self.enabled)); - input.insert( - "oidc_discovery_url".to_string(), - serde_json::json!(self.oidc_discovery_url), - ); - input.insert( - "oidc_client_id".to_string(), - serde_json::json!(self.oidc_client_id), - ); - input.insert( - "oidc_response_mode".to_string(), - serde_json::json!(self.oidc_response_mode), - ); - input.insert( - "oidc_response_types".to_string(), - serde_json::json!(self.oidc_response_types), - ); - input.insert("jwks_url".to_string(), serde_json::json!(self.jwks_url)); - input.insert( - "jwt_validation_pubkeys".to_string(), - serde_json::json!(self.jwt_validation_pubkeys), - ); - input.insert( - "bound_issuer".to_string(), - serde_json::json!(self.bound_issuer), - ); - input.insert( - "default_mapping_name".to_string(), - serde_json::json!(self.default_mapping_name), - ); - input.insert( - "oidc_scopes".to_string(), - serde_json::json!(self.oidc_scopes), - ); - input.insert( - "allowed_redirect_uris".to_string(), - serde_json::json!(self.allowed_redirect_uris), - ); - input.insert( - "provider_config".to_string(), - serde_json::json!(self.provider_config), - ); - serde_json::Value::Object(input) + let mut value = serde_json::to_value(self).unwrap_or_default(); + if let Some(input) = value.as_object_mut() { + input.remove("oidc_client_secret"); + } + value } } diff --git a/crates/api-types/src/v3/user.rs b/crates/api-types/src/v3/user.rs index 102a6d66d..6668c0824 100644 --- a/crates/api-types/src/v3/user.rs +++ b/crates/api-types/src/v3/user.rs @@ -174,20 +174,11 @@ pub struct UserCreate { impl UserCreate { #[must_use] pub fn to_policy_input(&self) -> serde_json::Value { - let mut input = self - .extra - .clone() - .into_iter() - .collect::>(); - input.insert( - "default_project_id".to_string(), - serde_json::json!(self.default_project_id), - ); - input.insert("domain_id".to_string(), serde_json::json!(self.domain_id)); - input.insert("enabled".to_string(), serde_json::json!(self.enabled)); - input.insert("name".to_string(), serde_json::json!(self.name)); - input.insert("options".to_string(), serde_json::json!(self.options)); - serde_json::Value::Object(input) + let mut value = serde_json::to_value(self).unwrap_or_default(); + if let Some(input) = value.as_object_mut() { + input.remove("password"); + } + value } } From ffe3e603f9e993413abcce4bb0884cf4f7b0d6d3 Mon Sep 17 00:00:00 2001 From: Yousef Hussein Date: Wed, 8 Jul 2026 17:52:02 +0300 Subject: [PATCH 6/7] fix(security): Expose password inside the hashers Per gtema's review on PR #912 (#369), hash_password and verify_password now take a &SecretString and call expose_secret internally, right before dispatching to the concrete hasher. The identity and application-credential SQL drivers pass the wrapped secret down instead of exposing it at the call site, shrinking the plaintext surface. Hasher unit-test call sites wrap their inputs in SecretString. The invalid-UTF-8 rejection test is dropped: a SecretString is built from a String, so non-UTF-8 input can no longer reach the API. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01DPZxPWH9yYH36jgfwGqZdQ Signed-off-by: Yousef Hussein --- .../src/application_credential/create.rs | 9 +-- .../src/common/password_hashing/bcrypt.rs | 16 +++-- .../common/password_hashing/bcrypt_sha256.rs | 30 ++++++--- .../core/src/common/password_hashing/mod.rs | 67 +++++++++++-------- .../src/common/password_hashing/pbkdf2.rs | 13 ++-- .../src/common/password_hashing/plaintext.rs | 22 ++---- .../src/common/password_hashing/scrypt.rs | 14 ++-- .../identity-driver-sql/src/authenticate.rs | 24 +++---- crates/identity-driver-sql/src/local_user.rs | 3 +- crates/identity-driver-sql/src/password.rs | 3 +- .../src/password/check_history.rs | 3 +- 11 files changed, 109 insertions(+), 95 deletions(-) diff --git a/crates/appcred-driver-sql/src/application_credential/create.rs b/crates/appcred-driver-sql/src/application_credential/create.rs index 87fa0d2b6..77794a461 100644 --- a/crates/appcred-driver-sql/src/application_credential/create.rs +++ b/crates/appcred-driver-sql/src/application_credential/create.rs @@ -15,7 +15,6 @@ use sea_orm::DatabaseConnection; use sea_orm::entity::*; use sea_orm::query::*; -use secrecy::ExposeSecret; use uuid::Uuid; use openstack_keystone_config::Config; @@ -84,11 +83,9 @@ pub async fn create( .context("starting transaction for persisting application credential")?; let mut model = db_application_credential::ActiveModel::try_from(rec.clone())?; model.secret_hash = if let Some(secret) = &rec.secret { - Set( - password_hashing::hash_password(conf, secret.expose_secret()) - .await - .map_err(ApplicationCredentialProviderError::password_hash)?, - ) + Set(password_hashing::hash_password(conf, secret) + .await + .map_err(ApplicationCredentialProviderError::password_hash)?) } else { return Err(ApplicationCredentialProviderError::SecretMissing); }; diff --git a/crates/core/src/common/password_hashing/bcrypt.rs b/crates/core/src/common/password_hashing/bcrypt.rs index b03892135..2cd8c9171 100644 --- a/crates/core/src/common/password_hashing/bcrypt.rs +++ b/crates/core/src/common/password_hashing/bcrypt.rs @@ -58,6 +58,7 @@ mod tests { use super::super::{hash_password, verify_password}; use openstack_keystone_config::PasswordHashingAlgo; use rand::distr::{Alphanumeric, SampleString}; + use secrecy::SecretString; use tracing_test::traced_test; #[tokio::test] @@ -66,7 +67,7 @@ mod tests { let python_hash = "$2b$12$0DJQbRXGHzPsBrwGt/DebuerSmDAslUjtPYtph84hMimE3XiK9K4e"; assert!( - verify_password(&conf, "password123", python_hash) + verify_password(&conf, &SecretString::from("password123"), python_hash) .await .unwrap(), "Rust Bcrypt verification rejected a real Keystone Python Bcrypt hash" @@ -83,9 +84,10 @@ mod tests { let conf = mock_config(PasswordHashingAlgo::Bcrypt, 255); let python_hash = "$2b$12$WzlPV/xopC8EI12Uz6kak.Edrg/n6QqM71MXoxegUUPxr.F52Hpsi"; let untruncated_73_byte_password = "A".repeat(73); + let password = SecretString::from(untruncated_73_byte_password); assert!( - verify_password(&conf, &untruncated_73_byte_password, python_hash) + verify_password(&conf, &password, python_hash) .await .unwrap(), "Rust Bcrypt did not truncate at the same 72-byte boundary as Keystone Python" @@ -97,11 +99,12 @@ mod tests { async fn test_roundtrip_bcrypt() { let conf = mock_config(PasswordHashingAlgo::Bcrypt, 255); let pass = "abcdefg"; - let hashed = hash_password(&conf, &pass).await.unwrap(); + let secret = SecretString::from(pass); + let hashed = hash_password(&conf, &secret).await.unwrap(); - assert!(verify_password(&conf, &pass, &hashed).await.unwrap()); + assert!(verify_password(&conf, &secret, &hashed).await.unwrap()); assert!( - !verify_password(&conf, "wrong_password", &hashed) + !verify_password(&conf, &SecretString::from("wrong_password"), &hashed) .await .unwrap() ); @@ -114,6 +117,7 @@ mod tests { async fn test_roundtrip_bcrypt_bad_hash() { let conf = mock_config(PasswordHashingAlgo::Bcrypt, 255); let pass = Alphanumeric.sample_string(&mut rand::rng(), 80); - assert!(!verify_password(&conf, &pass, "foobar").await.unwrap()); + let secret = SecretString::from(pass); + assert!(!verify_password(&conf, &secret, "foobar").await.unwrap()); } } diff --git a/crates/core/src/common/password_hashing/bcrypt_sha256.rs b/crates/core/src/common/password_hashing/bcrypt_sha256.rs index 160bb44ec..e64d88224 100644 --- a/crates/core/src/common/password_hashing/bcrypt_sha256.rs +++ b/crates/core/src/common/password_hashing/bcrypt_sha256.rs @@ -224,6 +224,7 @@ mod tests { use super::super::tests::mock_config; use super::super::{hash_password, verify_password}; use openstack_keystone_config::PasswordHashingAlgo; + use secrecy::SecretString; #[tokio::test] async fn test_bcrypt_sha256_matches_keystone_python_ascii_password() { @@ -232,7 +233,7 @@ mod tests { "$bcrypt-sha256$v=2,t=2b,r=12$dWWyn1sALNWeny6KjQhSUu$iOmfSpzioo6ThZbSXwWYSZAQcGlba/q"; assert!( - verify_password(&conf, "password123", python_hash) + verify_password(&conf, &SecretString::from("password123"), python_hash) .await .unwrap(), "Rust BcryptSha256 verification rejected a real Keystone Python BcryptSha256 hash" @@ -249,9 +250,10 @@ mod tests { let python_hash = "$bcrypt-sha256$v=2,t=2b,r=12$BL2N.GYa/h9LYksj.qtsb.$oUw7Xg.rbmPt8aXB2z544HOIQMrQbZ6"; let full_73_byte_password = "A".repeat(73); + let password = SecretString::from(full_73_byte_password); assert!( - verify_password(&conf, &full_73_byte_password, python_hash) + verify_password(&conf, &password, python_hash) .await .unwrap(), "Rust BcryptSha256 must not truncate at 72 bytes - that would diverge from Keystone Python" @@ -265,9 +267,13 @@ mod tests { "$bcrypt-sha256$v=2,t=2b,r=12$eP5KRHawhX/K86TK3IOLoO$fpoVOwh9QvOLy1Y9GKxMkf.RnkfO60."; assert!( - verify_password(&conf, "🚀-rocket-password", python_hash) - .await - .unwrap(), + verify_password( + &conf, + &SecretString::from("🚀-rocket-password"), + python_hash + ) + .await + .unwrap(), "Rust BcryptSha256 verification rejected a real Keystone Python hash of a UTF-8 password" ); } @@ -275,9 +281,11 @@ mod tests { #[tokio::test] async fn test_bcrypt_sha256_rejects_wrong_password() { let conf = mock_config(PasswordHashingAlgo::BcryptSha256, 255); - let hash = hash_password(&conf, "correct_password").await.unwrap(); + let hash = hash_password(&conf, &SecretString::from("correct_password")) + .await + .unwrap(); - let result = verify_password(&conf, "wrong_password", &hash) + let result = verify_password(&conf, &SecretString::from("wrong_password"), &hash) .await .unwrap(); assert!( @@ -295,7 +303,7 @@ mod tests { "$bcrypt-sha256$", // nearly empty "not-even-a-hash-string", // no '$' at all ] { - let result = verify_password(&conf, "anything", malformed).await; + let result = verify_password(&conf, &SecretString::from("anything"), malformed).await; assert!( matches!(result, Ok(false)) || result.is_err(), "Malformed hash `{malformed}` should fail safely, not panic" @@ -306,10 +314,10 @@ mod tests { #[tokio::test] async fn test_verify_bcrypt_sha256_roundtrip() { let conf = mock_config(PasswordHashingAlgo::BcryptSha256, 255); - let password = b"password123"; - let generated_hash = hash_password(&conf, password).await.unwrap(); + let password = SecretString::from("password123"); + let generated_hash = hash_password(&conf, &password).await.unwrap(); - let is_valid = verify_password(&conf, password, &generated_hash) + let is_valid = verify_password(&conf, &password, &generated_hash) .await .expect("Verification function failed"); assert!( diff --git a/crates/core/src/common/password_hashing/mod.rs b/crates/core/src/common/password_hashing/mod.rs index 210e736c6..eb03d2a74 100644 --- a/crates/core/src/common/password_hashing/mod.rs +++ b/crates/core/src/common/password_hashing/mod.rs @@ -34,6 +34,7 @@ use thiserror::Error; use tracing::debug; use rand::distr::{Alphanumeric, SampleString}; +use secrecy::{ExposeSecret, SecretString}; use openstack_keystone_config::{Config, PasswordHashingAlgo}; @@ -234,21 +235,22 @@ pub async fn generate_dummy_hash(conf: &Config) -> Result>( +pub async fn hash_password( conf: &Config, - password: S, + password: &SecretString, ) -> Result { // Truncation uses the *configured* algorithm, not any algorithm detected // from an existing hash string. This is the correct behaviour during // algorithm migrations: a user whose hash is in the old format and whose // password is longer than the new algorithm's limit must be truncated // consistently with what Python Keystone would do. + let exposed = password.expose_secret(); let truncated = verify_length_and_trunc_password( - password.as_ref(), + exposed.as_bytes(), &conf.identity.password_hashing_algorithm, conf.identity.max_password_length, ); @@ -263,9 +265,9 @@ pub async fn hash_password>( } /// Verify the password matches the hashed value. -pub async fn verify_password, H: AsRef>( +pub async fn verify_password>( conf: &Config, - password: P, + password: &SecretString, hash: H, ) -> Result { let hash_str = hash.as_ref(); @@ -279,8 +281,9 @@ pub async fn verify_password, H: AsRef>( // verification once the config switches to a non-truncating algorithm. let detected = detect_algo(hash_str, &conf.identity.password_hashing_algorithm); + let exposed = password.expose_secret(); let truncated = verify_length_and_trunc_password( - password.as_ref(), + exposed.as_bytes(), &conf.identity.password_hashing_algorithm, conf.identity.max_password_length, ); @@ -375,11 +378,13 @@ mod tests { async fn test_truncation_behavior_differences() { let long_password = "A".repeat(100); let truncated_72_password = "A".repeat(72); + let long_secret = SecretString::from(long_password); + let truncated_secret = SecretString::from(truncated_72_password); // 1. Standard Bcrypt Truncation Check let conf_bcrypt = mock_config(PasswordHashingAlgo::Bcrypt, 255); - let hash_bcrypt = hash_password(&conf_bcrypt, &long_password).await.unwrap(); - let is_valid_bcrypt = verify_password(&conf_bcrypt, &truncated_72_password, &hash_bcrypt) + let hash_bcrypt = hash_password(&conf_bcrypt, &long_secret).await.unwrap(); + let is_valid_bcrypt = verify_password(&conf_bcrypt, &truncated_secret, &hash_bcrypt) .await .unwrap(); assert!( @@ -389,25 +394,21 @@ mod tests { // 2. BcryptSha256 Arbitrary Length Check let conf_bcrypt_sha256 = mock_config(PasswordHashingAlgo::BcryptSha256, 255); - let hash_bcrypt_sha256 = hash_password(&conf_bcrypt_sha256, &long_password) + let hash_bcrypt_sha256 = hash_password(&conf_bcrypt_sha256, &long_secret) .await .unwrap(); - let is_valid_substring = verify_password( - &conf_bcrypt_sha256, - &truncated_72_password, - &hash_bcrypt_sha256, - ) - .await - .unwrap(); + let is_valid_substring = + verify_password(&conf_bcrypt_sha256, &truncated_secret, &hash_bcrypt_sha256) + .await + .unwrap(); assert!( !is_valid_substring, "BcryptSha256 incorrectly truncated the password to 72 bytes!" ); - let is_valid_full = - verify_password(&conf_bcrypt_sha256, &long_password, &hash_bcrypt_sha256) - .await - .unwrap(); + let is_valid_full = verify_password(&conf_bcrypt_sha256, &long_secret, &hash_bcrypt_sha256) + .await + .unwrap(); assert!( is_valid_full, "BcryptSha256 failed to verify the full 100-byte password sequence." @@ -428,7 +429,9 @@ mod tests { ); let pass = Alphanumeric.sample_string(&mut rand::rng(), 32); - let result = verify_password(&conf, &pass, &dummy_hash).await.unwrap(); + let result = verify_password(&conf, &SecretString::from(pass), &dummy_hash) + .await + .unwrap(); assert!( !result, @@ -445,7 +448,9 @@ mod tests { assert!(!dummy_hash.is_empty(), "Dummy hash should not be empty"); let pass = Alphanumeric.sample_string(&mut rand::rng(), 32); - let result = verify_password(&conf, &pass, &dummy_hash).await.unwrap(); + let result = verify_password(&conf, &SecretString::from(pass), &dummy_hash) + .await + .unwrap(); assert!(!result); } @@ -565,7 +570,9 @@ mod tests { async fn test_cross_verify_bcrypt() { let conf = mock_config(PasswordHashingAlgo::Bcrypt, 255); let password = "openstack123"; - let hash = hash_password(&conf, password).await.unwrap(); + let hash = hash_password(&conf, &SecretString::from(password)) + .await + .unwrap(); match python_cross_verify("bcrypt", password, &hash).await { None => return, // no Python checkout configured - skip @@ -578,7 +585,9 @@ mod tests { async fn test_cross_verify_bcrypt_sha256() { let conf = mock_config(PasswordHashingAlgo::BcryptSha256, 255); let password = "openstack123"; - let hash = hash_password(&conf, password).await.unwrap(); + let hash = hash_password(&conf, &SecretString::from(password)) + .await + .unwrap(); match python_cross_verify("bcrypt_sha256", password, &hash).await { None => return, @@ -591,7 +600,9 @@ mod tests { async fn test_cross_verify_scrypt() { let conf = mock_config(PasswordHashingAlgo::Scrypt, 255); let password = "openstack123"; - let hash = hash_password(&conf, password).await.unwrap(); + let hash = hash_password(&conf, &SecretString::from(password)) + .await + .unwrap(); match python_cross_verify("scrypt", password, &hash).await { None => return, @@ -604,7 +615,9 @@ mod tests { async fn test_cross_verify_pbkdf2_sha512() { let conf = mock_config(PasswordHashingAlgo::Pbkdf2Sha512, 255); let password = "openstack123"; - let hash = hash_password(&conf, password).await.unwrap(); + let hash = hash_password(&conf, &SecretString::from(password)) + .await + .unwrap(); match python_cross_verify("pbkdf2_sha512", password, &hash).await { None => return, diff --git a/crates/core/src/common/password_hashing/pbkdf2.rs b/crates/core/src/common/password_hashing/pbkdf2.rs index 16018a03a..adc8283a6 100644 --- a/crates/core/src/common/password_hashing/pbkdf2.rs +++ b/crates/core/src/common/password_hashing/pbkdf2.rs @@ -163,6 +163,7 @@ mod tests { use super::super::tests::{TEST_PASSWORD, mock_config}; use super::super::{hash_password, verify_password}; use openstack_keystone_config::PasswordHashingAlgo; + use secrecy::SecretString; #[tokio::test] async fn test_pbkdf2_sha512_matches_keystone_python_hash() { @@ -170,7 +171,7 @@ mod tests { let python_hash = "$pbkdf2-sha512$25000$z1PryJDTkFQEQN/E5K0nLQ$CzQ9XdgqUzdTOTjUSRGMN9r9O7WQmiyUl4fVA2jwJpB6zSXEonqw9Jfg4WImljlZ7fRPPFXmZZVdVhnCTJZymg"; assert!( - verify_password(&conf, TEST_PASSWORD, python_hash) + verify_password(&conf, &SecretString::from(TEST_PASSWORD), python_hash) .await .unwrap(), "Rust PBKDF2-SHA512 verification rejected a real Keystone Python PBKDF2-SHA512 hash" @@ -182,14 +183,15 @@ mod tests { let mut conf = mock_config(PasswordHashingAlgo::Pbkdf2Sha512, 255); conf.identity.password_hash_rounds = None; // exercise the default (25000) let password = "pbkdf2_roundtrip_password"; + let secret = SecretString::from(password); - let hashed = hash_password(&conf, password).await.unwrap(); + let hashed = hash_password(&conf, &secret).await.unwrap(); assert!( hashed.starts_with("$pbkdf2-sha512$25000$"), "PBKDF2 hash should embed the default round count" ); assert!( - verify_password(&conf, password, &hashed).await.unwrap(), + verify_password(&conf, &secret, &hashed).await.unwrap(), "PBKDF2 roundtrip failed with default rounds" ); } @@ -201,14 +203,15 @@ mod tests { let mut conf = mock_config(PasswordHashingAlgo::Pbkdf2Sha512, 255); conf.identity.password_hash_rounds = Some(10000); let password = "pbkdf2_custom_rounds"; + let secret = SecretString::from(password); - let hashed = hash_password(&conf, password).await.unwrap(); + let hashed = hash_password(&conf, &secret).await.unwrap(); assert!( hashed.starts_with("$pbkdf2-sha512$10000$"), "PBKDF2 hash must embed the configured round count, not the default" ); assert!( - verify_password(&conf, password, &hashed).await.unwrap(), + verify_password(&conf, &secret, &hashed).await.unwrap(), "PBKDF2 roundtrip failed with non-default rounds" ); } diff --git a/crates/core/src/common/password_hashing/plaintext.rs b/crates/core/src/common/password_hashing/plaintext.rs index 79b90a848..048260508 100644 --- a/crates/core/src/common/password_hashing/plaintext.rs +++ b/crates/core/src/common/password_hashing/plaintext.rs @@ -49,37 +49,25 @@ mod tests { use super::super::tests::mock_config; use super::super::{hash_password, verify_password}; use openstack_keystone_config::PasswordHashingAlgo; + use secrecy::SecretString; #[tokio::test] async fn test_none_algorithm_hash_then_verify_roundtrip() { let conf = mock_config(PasswordHashingAlgo::None, 255); let password = "plaintext_password"; + let secret = SecretString::from(password); - let hashed = hash_password(&conf, password).await.unwrap(); + let hashed = hash_password(&conf, &secret).await.unwrap(); assert_eq!( hashed, password, "None algorithm must store the password unchanged" ); - assert!(verify_password(&conf, password, &hashed).await.unwrap()); + assert!(verify_password(&conf, &secret, &hashed).await.unwrap()); assert!( - !verify_password(&conf, "wrong_password", &hashed) + !verify_password(&conf, &SecretString::from("wrong_password"), &hashed) .await .unwrap() ); } - - #[tokio::test] - async fn test_reject_invalid_utf8() { - let conf = mock_config(PasswordHashingAlgo::None, 72); - let invalid_utf8_password = b"bad\xFFpassword"; - - // Ensure our strict UTF-8 validation safely rejects invalid sequence - // vulnerabilities - let hash_result = hash_password(&conf, invalid_utf8_password).await; - assert!( - hash_result.is_err(), - "None algorithm should reject invalid UTF-8 strings during hash generation" - ); - } } diff --git a/crates/core/src/common/password_hashing/scrypt.rs b/crates/core/src/common/password_hashing/scrypt.rs index f830be46d..3909dad2e 100644 --- a/crates/core/src/common/password_hashing/scrypt.rs +++ b/crates/core/src/common/password_hashing/scrypt.rs @@ -117,6 +117,7 @@ mod tests { use super::super::tests::{TEST_PASSWORD, mock_config}; use super::super::{hash_password, verify_password}; use openstack_keystone_config::PasswordHashingAlgo; + use secrecy::SecretString; #[tokio::test] async fn test_scrypt_matches_keystone_python_hash() { @@ -126,7 +127,7 @@ mod tests { let python_hash = "$scrypt$ln=16,r=8,p=1$Gx7wZNue5sPNsfTOmI4YNg$umTMUw1tH3HhQBqHUG9tEr7x6RxfyVgNty/COb+m1IM"; assert!( - verify_password(&conf, TEST_PASSWORD, python_hash) + verify_password(&conf, &SecretString::from(TEST_PASSWORD), python_hash) .await .unwrap(), "Rust Scrypt verification rejected a real Keystone Python Scrypt hash" @@ -137,14 +138,15 @@ mod tests { async fn test_scrypt_hash_then_verify_roundtrip() { let conf = mock_config(PasswordHashingAlgo::Scrypt, 255); let password = "scrypt_roundtrip_password"; + let secret = SecretString::from(password); - let hashed = hash_password(&conf, password).await.unwrap(); + let hashed = hash_password(&conf, &secret).await.unwrap(); assert!( - verify_password(&conf, password, &hashed).await.unwrap(), + verify_password(&conf, &secret, &hashed).await.unwrap(), "Scrypt hash_password output failed to verify against the same password" ); assert!( - !verify_password(&conf, "wrong_password", &hashed) + !verify_password(&conf, &SecretString::from("wrong_password"), &hashed) .await .unwrap(), "Scrypt verification incorrectly accepted a wrong password" @@ -156,7 +158,9 @@ mod tests { // Verify the wire format prefix matches what Python emits: // $scrypt$ln=16,r=8,p=1$$ let conf = mock_config(PasswordHashingAlgo::Scrypt, 255); - let hashed = hash_password(&conf, "any_password").await.unwrap(); + let hashed = hash_password(&conf, &SecretString::from("any_password")) + .await + .unwrap(); assert!( hashed.starts_with("$scrypt$ln=16,r=8,p=1$"), "Scrypt hash format must match Python Keystone's prefix; got: {hashed}" diff --git a/crates/identity-driver-sql/src/authenticate.rs b/crates/identity-driver-sql/src/authenticate.rs index b0309c07d..c671c401f 100644 --- a/crates/identity-driver-sql/src/authenticate.rs +++ b/crates/identity-driver-sql/src/authenticate.rs @@ -14,7 +14,6 @@ //! User account authentication implementation. use chrono::Utc; use sea_orm::DatabaseConnection; -use secrecy::ExposeSecret; use tracing::info; use openstack_keystone_config::Config; @@ -80,11 +79,9 @@ pub async fn authenticate_by_password( .await .map_err(IdentityProviderError::password_hash)?; - // Exposure boundary: unwrapped only to feed the constant-time verify. - let _ = - password_hashing::verify_password(config, auth.password.expose_secret(), dummy_hash) - .await - .map_err(IdentityProviderError::password_hash)?; + let _ = password_hashing::verify_password(config, &auth.password, dummy_hash) + .await + .map_err(IdentityProviderError::password_hash)?; return Err(AuthenticationError::UserNameOrPasswordWrong.into()); } @@ -131,8 +128,7 @@ pub async fn authenticate_by_password( // Verify the password let now = Utc::now(); - // Exposure boundary: unwrapped only to feed the constant-time verify. - if !password_hashing::verify_password(config, auth.password.expose_secret(), expected_hash) + if !password_hashing::verify_password(config, &auth.password, expected_hash) .await .map_err(IdentityProviderError::password_hash)? { @@ -234,6 +230,7 @@ async fn should_lock( mod tests { use chrono::{DateTime, NaiveDateTime, TimeDelta, Utc}; use sea_orm::{DatabaseBackend, MockDatabase, MockExecResult, Transaction}; + use secrecy::SecretString; use tracing_test::traced_test; use openstack_keystone_core_types::identity::UserOptions; @@ -456,9 +453,10 @@ mod tests { async fn test_authenticate() { let config = Config::default(); let password = String::from("pass"); + let password_secret = SecretString::from(password.clone()); let db = MockDatabase::new(DatabaseBackend::Postgres) .append_query_results([vec![get_local_user_with_password_mock( - password_hashing::hash_password(&config, &password) + password_hashing::hash_password(&config, &password_secret) .await .unwrap(), )]]) @@ -592,7 +590,7 @@ mod tests { db_password::ModelBuilder::default() .local_user_id(1) .password_hash( - password_hashing::hash_password(&config, &password) + password_hashing::hash_password(&config, &SecretString::from(password)) .await .unwrap(), ) @@ -857,12 +855,13 @@ mod tests { async fn test_authenticate_expired_password() { let config = Config::default(); let password = String::from("foo_pass"); + let password_secret = SecretString::from(password.clone()); let db = MockDatabase::new(DatabaseBackend::Postgres) .append_query_results([vec![( get_local_user_mock("user_id"), db_password::ModelBuilder::default() .password_hash( - password_hashing::hash_password(&config, &password) + password_hashing::hash_password(&config, &password_secret) .await .unwrap(), ) @@ -903,12 +902,13 @@ mod tests { async fn test_authenticate_exempt_expired_password() { let config = Config::default(); let password = String::from("foo_pass"); + let password_secret = SecretString::from(password.clone()); let db = MockDatabase::new(DatabaseBackend::Postgres) .append_query_results([vec![( get_local_user_mock("user_id"), db_password::ModelBuilder::expired() .password_hash( - password_hashing::hash_password(&config, &password) + password_hashing::hash_password(&config, &password_secret) .await .unwrap(), ) diff --git a/crates/identity-driver-sql/src/local_user.rs b/crates/identity-driver-sql/src/local_user.rs index 321d084e8..30e95f794 100644 --- a/crates/identity-driver-sql/src/local_user.rs +++ b/crates/identity-driver-sql/src/local_user.rs @@ -12,7 +12,6 @@ // // SPDX-License-Identifier: Apache-2.0 -use secrecy::ExposeSecret; use secrecy::SecretString; use openstack_keystone_config::Config; @@ -78,7 +77,7 @@ pub async fn update_password( ))?; // Verify original password - if !password_hashing::verify_password(conf, original_password.expose_secret(), expected_hash) + if !password_hashing::verify_password(conf, &original_password, expected_hash) .await .map_err(IdentityProviderError::password_hash)? { diff --git a/crates/identity-driver-sql/src/password.rs b/crates/identity-driver-sql/src/password.rs index 5c6413282..89f26b661 100644 --- a/crates/identity-driver-sql/src/password.rs +++ b/crates/identity-driver-sql/src/password.rs @@ -12,7 +12,6 @@ // // SPDX-License-Identifier: Apache-2.0 use chrono::{DateTime, Utc}; -use secrecy::ExposeSecret; use secrecy::SecretString; use openstack_keystone_config::Config; @@ -67,7 +66,7 @@ pub async fn set_new_password( .security_compliance .unique_last_password_count .unwrap_or(0); - let hashed_password = password_hashing::hash_password(conf, password.expose_secret()) + let hashed_password = password_hashing::hash_password(conf, &password) .await .map_err(IdentityProviderError::password_hash)?; diff --git a/crates/identity-driver-sql/src/password/check_history.rs b/crates/identity-driver-sql/src/password/check_history.rs index 3ea3fe049..bf28299ed 100644 --- a/crates/identity-driver-sql/src/password/check_history.rs +++ b/crates/identity-driver-sql/src/password/check_history.rs @@ -12,7 +12,6 @@ // // SPDX-License-Identifier: Apache-2.0 -use secrecy::ExposeSecret; use secrecy::SecretString; use openstack_keystone_config::Config; @@ -56,7 +55,7 @@ where if let Some(ref check_hash) = check_password.password_hash && openstack_keystone_core::common::password_hashing::verify_password( conf, - new_password.expose_secret(), + new_password, check_hash, ) .await From fae12f4c97e13bf365755bfbf196650cb6540079 Mon Sep 17 00:00:00 2001 From: Yousef Hussein Date: Wed, 8 Jul 2026 17:52:14 +0300 Subject: [PATCH 7/7] fix(security): Wrap authorize_by_token credential Per gtema's review on PR #912 (#369), authorize_by_token now takes the token credential as a &SecretString so the plaintext never crosses the trait boundary where logging or decorators could capture it. The token is exposed only inside the TokenService implementation, just before validate_to_context_impl. Callers pass the wrapped SecretString (the v3 auth handler passes &token.id; the X-Auth-Token extractor wraps the header). The mock and its matcher are updated for the new signature. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01DPZxPWH9yYH36jgfwGqZdQ Signed-off-by: Yousef Hussein --- crates/core/src/api/auth.rs | 4 +++- crates/core/src/mocks.rs | 2 +- crates/core/src/token/provider_api.rs | 5 +++-- crates/core/src/token/service.rs | 12 +++++++++--- crates/keystone/src/api/v3/auth/token/common.rs | 10 ++++++---- 5 files changed, 22 insertions(+), 11 deletions(-) diff --git a/crates/core/src/api/auth.rs b/crates/core/src/api/auth.rs index feb17b361..93d48e503 100644 --- a/crates/core/src/api/auth.rs +++ b/crates/core/src/api/auth.rs @@ -20,6 +20,7 @@ use axum::{ extract::{FromRef, FromRequestParts}, http::request::Parts, }; +use secrecy::SecretString; use tracing::{debug, error}; use openstack_keystone_config::Interface; @@ -153,12 +154,13 @@ where .and_then(|h| h.to_str().ok()) { tracing::debug!("authenticating request with the x-auth-token"); + let auth_token = SecretString::from(auth_header.to_owned()); let vsc = state .provider .get_token_provider() .authorize_by_token( &ExecutionContext::internal(&state), - auth_header, + &auth_token, Some(false), None, ) diff --git a/crates/core/src/mocks.rs b/crates/core/src/mocks.rs index a64bdae1a..cc1e15d6b 100644 --- a/crates/core/src/mocks.rs +++ b/crates/core/src/mocks.rs @@ -1088,7 +1088,7 @@ mod token { async fn authorize_by_token<'a>( &self, ctx: &ExecutionContext<'a>, - credential: &'a str, + credential: &SecretString, allow_expired: Option, window_seconds: Option, ) -> Result; diff --git a/crates/core/src/token/provider_api.rs b/crates/core/src/token/provider_api.rs index 63a9ee75a..3a7a0af3b 100644 --- a/crates/core/src/token/provider_api.rs +++ b/crates/core/src/token/provider_api.rs @@ -14,6 +14,7 @@ //! Token provider types. use async_trait::async_trait; +use secrecy::SecretString; use openstack_keystone_core_types::token::*; @@ -28,7 +29,7 @@ pub trait TokenApi: Send + Sync { /// /// # Parameters /// - `state`: The current service state. - /// - `credential`: The token credential string. + /// - `credential`: The token credential. /// - `allow_expired`: Whether to allow expired tokens. /// - `window_seconds`: Expiration buffer in seconds. /// @@ -38,7 +39,7 @@ pub trait TokenApi: Send + Sync { async fn authorize_by_token<'a>( &self, ctx: &ExecutionContext<'a>, - credential: &'a str, + credential: &SecretString, allow_expired: Option, window_seconds: Option, ) -> Result; diff --git a/crates/core/src/token/service.rs b/crates/core/src/token/service.rs index 6d32c6767..25fb09292 100644 --- a/crates/core/src/token/service.rs +++ b/crates/core/src/token/service.rs @@ -24,6 +24,7 @@ use std::sync::Arc; use async_trait::async_trait; use chrono::{DateTime, TimeDelta, Utc}; +use secrecy::{ExposeSecret, SecretString}; use tracing::trace; use uuid::Uuid; @@ -398,7 +399,7 @@ impl TokenApi for TokenService { /// /// # Parameters /// - `state`: The current service state. - /// - `credential`: The token credential string. + /// - `credential`: The token credential. /// - `allow_expired`: Whether to allow expired tokens. /// - `window_seconds`: Expiration buffer in seconds. /// @@ -408,12 +409,17 @@ impl TokenApi for TokenService { async fn authorize_by_token<'a>( &self, ctx: &ExecutionContext<'a>, - credential: &'a str, + credential: &SecretString, allow_expired: Option, window_seconds: Option, ) -> Result { let vsc = self - .validate_to_context_impl(ctx, credential, allow_expired, window_seconds) + .validate_to_context_impl( + ctx, + credential.expose_secret(), + allow_expired, + window_seconds, + ) .await?; if let FernetToken::Restricted(restriction) = vsc.token()? diff --git a/crates/keystone/src/api/v3/auth/token/common.rs b/crates/keystone/src/api/v3/auth/token/common.rs index 6df68257d..d0a2a4fa6 100644 --- a/crates/keystone/src/api/v3/auth/token/common.rs +++ b/crates/keystone/src/api/v3/auth/token/common.rs @@ -13,7 +13,6 @@ // SPDX-License-Identifier: Apache-2.0 use openstack_keystone_core::auth::ExecutionContext; -use secrecy::ExposeSecret; use crate::api::error::KeystoneApiError; use crate::api::v3::auth::token::types::AuthRequest; @@ -61,7 +60,7 @@ pub(super) async fn authenticate_request( .get_token_provider() .authorize_by_token( &ExecutionContext::internal(state), - token.id.expose_secret(), + &token.id, Some(false), None, ) @@ -251,8 +250,11 @@ mod tests { token_mock .expect_authorize_by_token() .withf( - |_, id: &'_ str, allow_expired: &Option, window: &Option| { - id == "fake_token" && *allow_expired == Some(false) && window.is_none() + |_, + _id: &secrecy::SecretString, + allow_expired: &Option, + window: &Option| { + *allow_expired == Some(false) && window.is_none() }, ) .returning(move |_state, _, _, _| Ok(vsc_clone.clone()));