diff --git a/Cargo.lock b/Cargo.lock index d81c9a5d1..d36b346a6 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/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 975f1e004..5b3784d3a 100644 --- a/crates/api-types/src/federation/identity_provider.rs +++ b/crates/api-types/src/federation/identity_provider.rs @@ -12,6 +12,7 @@ // // SPDX-License-Identifier: Apache-2.0 //! Federated identity provider types. +use secrecy::SecretString; use serde::{Deserialize, Serialize}; use serde_json::Value; #[cfg(feature = "validate")] @@ -122,7 +123,7 @@ pub struct IdentityProviderResponse { } /// Identity provider data. -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +#[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr( feature = "builder", derive(derive_builder::Builder), @@ -133,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. @@ -171,10 +176,13 @@ pub struct IdentityProviderCreate { /// 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, + #[cfg_attr(feature = "openapi", schema(value_type = String, nullable = false))] + #[serde( + default, + skip_serializing_if = "Option::is_none", + serialize_with = "crate::common::serialize_optional_secret" + )] + pub oidc_client_secret: Option, /// The oidc response mode. #[cfg_attr(feature = "builder", builder(default))] @@ -239,8 +247,73 @@ 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, PartialEq, Serialize)] +#[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr( feature = "builder", derive(derive_builder::Builder), @@ -251,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)))] @@ -273,8 +350,12 @@ pub struct IdentityProviderUpdate { /// The new oidc `client_secret` to use for the private client. #[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 = "crate::common::serialize_nested_optional_secret" + )] + pub oidc_client_secret: Option>, /// The new oidc response mode. #[cfg_attr(feature = "builder", builder(default))] @@ -323,8 +404,72 @@ 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, PartialEq, Serialize)] +#[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] #[cfg_attr(feature = "validate", derive(validator::Validate))] pub struct IdentityProviderCreateRequest { @@ -334,7 +479,7 @@ pub struct IdentityProviderCreateRequest { } /// Identity provider update request. -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +#[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] #[cfg_attr(feature = "validate", derive(validator::Validate))] pub struct IdentityProviderUpdateRequest { @@ -383,3 +528,84 @@ pub struct IdentityProviderListParameters { fn default_list_limit() -> Option { Some(20) } + +#[cfg(test)] +mod tests { + use super::*; + + /// 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_debug_does_not_leak_client_secret() { + use secrecy::ExposeSecret; + let create: IdentityProviderCreate = + serde_json::from_str(r#"{"name":"idp","oidc_client_secret":"CSLEAK"}"#).unwrap(); + assert!( + !format!("{create:?}").contains("CSLEAK"), + "Debug leaked client secret: {create:?}" + ); + assert_eq!( + create + .oidc_client_secret + .as_ref() + .map(|s| s.expose_secret()), + Some("CSLEAK") + ); + } + + /// Same for the update DTO (nested `Option>`). + #[test] + fn idp_update_debug_does_not_leak_client_secret() { + let update: IdentityProviderUpdate = + serde_json::from_str(r#"{"oidc_client_secret":"CSLEAK2"}"#).unwrap(); + assert!( + !format!("{update:?}").contains("CSLEAK2"), + "Debug leaked client secret: {update:?}" + ); + } + + #[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] + 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/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 747dcf185..f11234306 100644 --- a/crates/api-types/src/v3/auth/token.rs +++ b/crates/api-types/src/v3/auth/token.rs @@ -13,6 +13,7 @@ // SPDX-License-Identifier: Apache-2.0 use chrono::{DateTime, Utc}; +use secrecy::SecretString; use serde::{Deserialize, Serialize}; #[cfg(feature = "validate")] use validator::Validate; @@ -131,7 +132,7 @@ pub struct TokenResponse { } /// An authentication request. -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +#[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] #[cfg_attr(feature = "validate", derive(validator::Validate))] pub struct AuthRequest { @@ -141,7 +142,7 @@ pub struct AuthRequest { } /// An authentication request. -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +#[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] #[cfg_attr(feature = "validate", derive(validator::Validate))] pub struct AuthRequestInner { @@ -164,7 +165,7 @@ pub struct AuthRequestInner { } /// An identity object. -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +#[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr( feature = "builder", derive(derive_builder::Builder), @@ -198,7 +199,7 @@ pub struct Identity { } /// The password object, contains the authentication information. -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +#[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr( feature = "builder", derive(derive_builder::Builder), @@ -216,7 +217,7 @@ pub struct PasswordAuth { } /// User password information. -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +#[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr( feature = "builder", derive(derive_builder::Builder), @@ -227,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))] @@ -241,12 +246,18 @@ pub struct UserPassword { #[cfg_attr(feature = "validate", validate(nested))] pub domain: Option, /// User password. - #[cfg_attr(feature = "validate", validate(length(max = 255)))] - pub password: String, + #[cfg_attr(feature = "openapi", schema(value_type = 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, PartialEq, Serialize)] +#[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr( feature = "builder", derive(derive_builder::Builder), @@ -264,7 +275,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), @@ -275,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))] @@ -289,8 +304,14 @@ 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 = "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. @@ -322,7 +343,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), @@ -333,10 +354,20 @@ 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 = "validate", validate(length(max = 1024)))] - pub id: String, + #[cfg_attr(feature = "openapi", schema(value_type = 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)] @@ -376,3 +407,68 @@ 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_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 (the #[instrument] / log vector) must not leak. + assert!( + !format!("{up:?}").contains(PWD), + "Debug leaked the password" + ); + + // 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), + "password not carried for transport: {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_carries_password_for_transport() { + // The password sits 4 levels deep (auth -> identity -> password -> user). + // 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), + "password not carried for transport at depth: {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/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 b515628fb..da4fa5a64 100644 --- a/crates/api-types/src/v3/user.rs +++ b/crates/api-types/src/v3/user.rs @@ -14,6 +14,7 @@ use std::collections::HashMap; use chrono::{DateTime, Utc}; +use secrecy::SecretString; use serde::{Deserialize, Serialize}; use serde_json::Value; #[cfg(feature = "validate")] @@ -102,7 +103,7 @@ pub struct UserResponse { } /// Create user data. -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +#[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr( feature = "builder", derive(derive_builder::Builder), @@ -113,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 @@ -154,14 +159,48 @@ pub struct UserCreate { #[cfg_attr(feature = "validate", validate(nested))] pub options: Option, - /// The password for the user. + /// 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 = "validate", validate(length(min = 1, max = 72)))] - pub password: Option, + #[cfg_attr(feature = "openapi", schema(value_type = Option))] + #[serde( + default, + skip_serializing_if = "Option::is_none", + 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, PartialEq, Serialize)] +#[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] #[cfg_attr(feature = "validate", derive(validator::Validate))] pub struct UserCreateRequest { @@ -171,7 +210,7 @@ pub struct UserCreateRequest { } /// Update user data. -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +#[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr( feature = "builder", derive(derive_builder::Builder), @@ -182,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 @@ -222,11 +265,44 @@ pub struct UserUpdate { /// The password for the user. #[cfg_attr(feature = "builder", builder(default))] - pub password: Option, + #[cfg_attr(feature = "openapi", schema(value_type = Option))] + #[serde( + default, + skip_serializing_if = "Option::is_none", + 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, PartialEq, Serialize)] +#[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] #[cfg_attr(feature = "validate", derive(validator::Validate))] pub struct UserUpdateRequest { @@ -313,8 +389,117 @@ pub struct UserListParameters { #[cfg(test)] mod tests { + use secrecy::ExposeSecret; + use super::*; + /// 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. + #[test] + 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"}"#, + ) + .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") + ); + + // 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 shape. + #[test] + fn userupdate_flatten_keeps_password_and_extra() { + let uu: UserUpdate = + serde_json::from_str(r#"{"password":"UPWLEAK","z_extra":"zz"}"#).unwrap(); + 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}"); + } + + #[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] + 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/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/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..4b73dcb1d 100644 --- a/crates/core-types/src/federation/identity_provider.rs +++ b/crates/core-types/src/federation/identity_provider.rs @@ -14,13 +14,14 @@ //! # Federated identity provider types use derive_builder::Builder; +use secrecy::SecretString; use serde::Serialize; use serde_json::Value; use crate::error::BuilderError; /// Identity provider resource. -#[derive(Builder, Clone, Debug, Default, Serialize, PartialEq)] +#[derive(Builder, Clone, Debug, Default, Serialize)] #[builder(build_fn(error = "BuilderError"))] #[builder(setter(strip_option, into))] pub struct IdentityProvider { @@ -45,8 +46,11 @@ pub struct IdentityProvider { #[builder(default)] pub oidc_client_id: Option, + /// The OIDC client secret. It is never returned back, so it is skipped on + /// serialization; `SecretString` additionally keeps it out of `Debug`. #[builder(default)] - pub oidc_client_secret: Option, + #[serde(skip_serializing)] + pub oidc_client_secret: Option, #[builder(default)] pub oidc_response_mode: Option, @@ -82,7 +86,7 @@ pub struct IdentityProvider { } /// New Identity provider data. -#[derive(Builder, Clone, Debug, Default, PartialEq)] +#[derive(Builder, Clone, Debug, Default)] #[builder(build_fn(error = "BuilderError"))] #[builder(setter(strip_option, into))] pub struct IdentityProviderCreate { @@ -107,8 +111,9 @@ pub struct IdentityProviderCreate { #[builder(default)] pub oidc_client_id: Option, + /// The OIDC client secret. #[builder(default)] - pub oidc_client_secret: Option, + pub oidc_client_secret: Option, #[builder(default)] pub oidc_response_mode: Option, @@ -141,7 +146,7 @@ pub struct IdentityProviderCreate { } /// Identity provider update data. -#[derive(Builder, Clone, Debug, Default, PartialEq)] +#[derive(Builder, Clone, Debug, Default)] #[builder(build_fn(error = "BuilderError"))] #[builder(setter(into))] pub struct IdentityProviderUpdate { @@ -157,8 +162,10 @@ pub struct IdentityProviderUpdate { #[builder(default)] pub oidc_client_id: Option>, + /// The OIDC client secret. 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 +218,41 @@ 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_debug_does_not_leak_client_secret() { + let idp = IdentityProviderBuilder::default() + .id("1") + .name("idp") + .oidc_client_secret(SecretString::from(SECRET)) + .build() + .unwrap(); + + // Debug (the #[instrument] / log vector) must not leak the secret. + assert!( + !format!("{idp:?}").contains(SECRET), + "Debug leaked client secret" + ); + } + + #[test] + fn identity_provider_does_not_serialize_client_secret() { + let idp = IdentityProviderBuilder::default() + .id("1") + .name("idp") + .oidc_client_secret(SecretString::from(SECRET)) + .build() + .unwrap(); + + // 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 95f9fadb1..435b855a1 100644 --- a/crates/core-types/src/identity/user.rs +++ b/crates/core-types/src/identity/user.rs @@ -15,12 +15,37 @@ use std::collections::HashMap; use chrono::{DateTime, Utc}; use derive_builder::Builder; +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`). +// 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 +100,11 @@ 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)] +#[validate(schema(function = "validate_user_create_secret"))] #[builder(build_fn(error = "BuilderError"))] #[builder(setter(strip_option, into))] pub struct UserCreate { @@ -119,10 +148,10 @@ pub struct UserCreate { #[validate(nested)] pub options: Option, - /// User password. + /// User password. Non-emptiness and regex policy are enforced 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 +162,16 @@ pub struct UserCreate { pub user_type: UserType, } -#[derive(Builder, Clone, Debug, Default, PartialEq, Validate)] +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 { @@ -169,10 +207,14 @@ pub struct UserUpdate { #[validate(nested)] pub options: Option, - /// New user password. + /// New user password. 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, +} + +fn validate_user_update_secret(value: &UserUpdate) -> Result<(), ValidationError> { + validate_optional_secret_length(&value.password, 72) } /// User options. @@ -279,7 +321,11 @@ 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)] +#[validate(schema(function = "validate_user_password_auth_secret"))] #[builder(build_fn(error = "BuilderError"))] #[builder(setter(strip_option, into))] pub struct UserPasswordAuthRequest { @@ -298,14 +344,38 @@ pub struct UserPasswordAuthRequest { #[validate(nested)] pub domain: Option, - /// User password expiry date. - #[builder(default)] - #[validate(length(max = 72))] - pub password: String, + /// User password. Required (no builder default: `SecretString` does not + /// implement `Default`). + 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 +/// explicit password. +impl Default for UserPasswordAuthRequest { + fn default() -> Self { + Self { + id: None, + name: None, + domain: None, + password: SecretString::from(""), + } + } } /// 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)] +#[validate(schema(function = "validate_user_totp_auth_secret"))] #[builder(build_fn(error = "BuilderError"))] #[builder(setter(strip_option, into))] pub struct UserTotpAuthRequest { @@ -325,9 +395,11 @@ 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, +} + +fn validate_user_totp_auth_secret(value: &UserTotpAuthRequest) -> Result<(), ValidationError> { + validate_secret_length(&value.passcode, 32) } /// Domain information. 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 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/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/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 687e9275b..1f8f5da39 100644 --- a/crates/identity-driver-sql/src/user/update.rs +++ b/crates/identity-driver-sql/src/user/update.rs @@ -109,7 +109,7 @@ pub async fn update( &txn, conf, local_user.id, - secrecy::SecretString::from(new_password.as_str()), + new_password.clone(), passwords_vec, ) .await?; @@ -256,7 +256,7 @@ mod tests { .into_connection(); let req = UserUpdate { - password: Some("new_password".to_string()), + password: Some("new_password".into()), ..Default::default() }; @@ -330,7 +330,7 @@ mod tests { .into_connection(); let req = UserUpdate { - password: Some("new_password".to_string()), + password: Some("new_password".into()), ..Default::default() }; @@ -376,7 +376,7 @@ mod tests { .into_connection(); let req = UserUpdate { - password: Some("new_password".to_string()), + password: Some("new_password".into()), ..Default::default() }; @@ -416,7 +416,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() }; @@ -463,7 +463,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..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, ) @@ -87,6 +88,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 +118,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())); @@ -174,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())); @@ -276,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, }, 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/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?; 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 ────────────────────────────────────────────────────────