feat(security): Wrap secrets with secrecy crate (#369)

crates/api-types/src/federation/identity_provider.rs

@@ -12,13 +12,46 @@
 //
 // SPDX-License-Identifier: Apache-2.0
 //! Federated identity provider types.
-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;
 
+/// 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<S>(
+    secret: &Option<SecretString>,
+    serializer: S,
+) -> Result<S::Ok, S::Error>
+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<S>(
+    secret: &Option<Option<SecretString>>,
+    serializer: S,
+) -> Result<S::Ok, S::Error>
+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(
@@ -122,7 +155,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),
@@ -171,10 +204,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<String>,
+    #[cfg_attr(feature = "openapi", schema(value_type = String, nullable = false))]
+    #[serde(
+        default,
+        skip_serializing_if = "Option::is_none",
+        serialize_with = "serialize_optional_secret"
+    )]
+    pub oidc_client_secret: Option<SecretString>,
 
     /// The oidc response mode.
     #[cfg_attr(feature = "builder", builder(default))]
@@ -240,7 +276,7 @@ pub struct IdentityProviderCreate {
 }
 
 /// New identity provider data.
-#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)]
+#[derive(Clone, Debug, Deserialize, Serialize)]
 #[cfg_attr(
     feature = "builder",
     derive(derive_builder::Builder),
@@ -273,8 +309,9 @@ 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<Option<String>>,
+    #[cfg_attr(feature = "openapi", schema(value_type = Option<String>))]
+    #[serde(default, serialize_with = "serialize_nested_optional_secret")]
+    pub oidc_client_secret: Option<Option<SecretString>>,
 
     /// The new oidc response mode.
     #[cfg_attr(feature = "builder", builder(default))]
@@ -324,7 +361,7 @@ pub struct IdentityProviderUpdate {
 }
 
 /// 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 +371,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 +420,60 @@ pub struct IdentityProviderListParameters {
 fn default_list_limit() -> Option<u64> {
     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<Option<SecretString>>`).
+    #[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:?}"
+        );
+    }
+
+    /// 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}"
+        );
+    }
+}

crates/api-types/src/v3/auth/token.rs

@@ -13,7 +13,8 @@
 // SPDX-License-Identifier: Apache-2.0
 
 use chrono::{DateTime, Utc};
-use serde::{Deserialize, Serialize};
+use secrecy::{ExposeSecret, SecretString};
+use serde::{Deserialize, Serialize, Serializer};
 #[cfg(feature = "validate")]
 use validator::Validate;
 
@@ -22,6 +23,16 @@ 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<S>(secret: &SecretString, serializer: S) -> Result<S::Ok, S::Error>
+where
+    S: Serializer,
+{
+    serializer.serialize_str(secret.expose_secret())
+}
+
 /// Authorization token.
 #[derive(Clone, Debug, Deserialize, PartialEq, Serialize)]
 #[cfg_attr(
@@ -131,7 +142,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 +152,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 +175,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 +209,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 +227,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),
@@ -241,12 +252,13 @@ pub struct UserPassword {
     #[cfg_attr(feature = "validate", validate(nested))]
     pub domain: Option<Domain>,
     /// User password.
-    #[cfg_attr(feature = "validate", validate(length(max = 255)))]
-    pub password: String,
+    #[cfg_attr(feature = "openapi", schema(value_type = String))]
+    #[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),
@@ -264,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),
@@ -289,8 +301,9 @@ pub struct TotpUser {
     #[cfg_attr(feature = "validate", validate(nested))]
     pub domain: Option<Domain>,
     /// 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.
@@ -322,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),
@@ -335,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)]
@@ -376,3 +390,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()
+    }
+}

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<api_types::UserPassword>
             }
             upa.domain(domain_builder.build()?);
         }
-        upa.password(value.password.clone());
+        upa.password(value.password.expose_secret());
         upa.build()
     }
 }