diff --git a/.claude/skills/connectors-overview/SKILL.md b/.claude/skills/connectors-overview/SKILL.md index 91ff86ec7c..230ad5354b 100644 --- a/.claude/skills/connectors-overview/SKILL.md +++ b/.claude/skills/connectors-overview/SKILL.md @@ -96,14 +96,24 @@ The connectors codebase is intentionally repetitive across plugins. Cross-plugin ### Secrets -Any credential-bearing field (connection strings, API keys, bearer tokens, AWS keys) must be `SecretString` from the `secrecy` crate, with the workspace serde wrapper applied so `Debug` and serialization both redact. Runtime exposes plugin configs over the `/stats` HTTP surface via serialization - plain `String` leaks the secret to anyone who can hit the endpoint. Plain `String` for a credential is a review-blocker. Pattern (from `sinks/postgres_sink/src/lib.rs::PostgresSinkConfig`): +Any credential-bearing field (connection strings, API keys, bearer tokens, AWS keys) must be `SecretString` from the `secrecy` crate. Plain `String` for a credential is a review-blocker: `SecretString` redacts on `Debug`, so it is what keeps a credential out of a log line that formats the whole config. + +**`serde_secret::serialize_secret` EXPOSES the secret. It does not redact.** It calls `expose_secret()` and writes the plaintext. `SecretString` deliberately has no `Serialize` impl, and that absence is the protection - so adding `serialize_with` is what *unblocks* the derive and turns a compile-time guarantee into plaintext output. Use it only where the plaintext is the point: a wire payload, a persisted config, an API response that exposes credentials by design. + +So the default for a plugin config struct is **derive `Deserialize`, but not `Serialize`**. `Deserialize` is required: the SDK glue deserializes the config into the plugin's own struct (`sdk/src/{sink,source}.rs` call `serde_json::from_str::` under a `DeserializeOwned` bound). + +What never happens is the return trip. The runtime holds plugin configuration as a `serde_json::Value` - parsed from TOML, posted as JSON to the control API, or injected by env var - and hands that across the FFI, so nothing re-serializes the plugin's struct. Leaving `Serialize` off makes that compiler-enforced instead of convention-enforced (`sources/http_source/src/lib.rs::HttpSourceConfig` does this, and comments the omission so nobody adds it back). + +Pattern: ```rust use secrecy::{ExposeSecret, SecretString}; -#[derive(Debug, Clone, Serialize, Deserialize)] +// `Deserialize` only. Nothing re-serializes a plugin config, and leaving +// `Serialize` off is what makes the credential unserializable rather than +// merely un-serialized. +#[derive(Debug, Clone, Deserialize)] pub struct MyConfig { - #[serde(serialize_with = "iggy_common::serde_secret::serialize_secret")] pub connection_string: SecretString, } @@ -113,7 +123,13 @@ let pool = PgPoolOptions::new() .await?; ``` -In-tree uses: `sinks/{postgres,mongodb,elasticsearch,influxdb,delta}_sink`, `sources/{postgres,elasticsearch,influxdb}_source`. +If a config struct genuinely needs `Serialize`, `serde_secret::serialize_redacted` (and `serialize_optional_redacted`) write `[REDACTED]` in place of the value. Reach for `serialize_secret` only when the caller must get the real thing back. The sinks and sources listed below predate that helper and use the exposing one; the annotation is inert today, but it is not the protection it looks like. + +Note that none of this protects the credential from the runtime's own control API, which returns plugin configuration verbatim - see #3802. Plugin-side annotations are inert there because the runtime never routes through them. + +Plugin-side uses of the exposing helpers: `sinks/{postgres,mongodb,elasticsearch,influxdb,s3,surrealdb}_sink`, `sources/{postgres,elasticsearch,influxdb}_source`. + +That list is plugin-side only, not an inventory of every caller in the tree, and the others are not all mistakes: `runtime/src/api/config.rs` puts `serialize_secret` on `HttpConfig::api_key` (inert for the same reason), and several `core/common` wire-payload types (login, create-user, change-password, PAT) use these helpers by design, because there the credential *is* the payload. ### Errors @@ -193,7 +209,7 @@ Each implemented in at least one in-tree plugin or runtime path. | `flume::unbounded()` channel | `runtime/src/source.rs::spawn_source_handler` / `source_forwarding_loop` | MPSC handoff from SDK async task to runtime loop | | `tokio::sync::watch::channel(())` | `sdk/src/{sink,source}.rs`, `runtime/src/sink.rs`, `runtime/src/manager/*` | One-shot shutdown broadcast | | `dashmap::DashMap` | `runtime/src/manager/sink.rs`, `source.rs::SOURCE_SENDERS`, SDK `INSTANCES` | Lock-free concurrent keyed access | -| `secrecy::SecretString` + `iggy_common::serde_secret::serialize_secret` | `sinks/postgres_sink::PostgresSinkConfig::connection_string` | Auto-redact on Debug/Display + serialization | +| `secrecy::SecretString` + `iggy_common::serde_secret::serialize_secret` | `sinks/postgres_sink::PostgresSinkConfig::connection_string` | `Debug` redacts; `serialize_secret` EXPOSES | ## Drop accounting diff --git a/core/common/src/utils/serde_secret.rs b/core/common/src/utils/serde_secret.rs index 7f8bd2feb3..b10492e438 100644 --- a/core/common/src/utils/serde_secret.rs +++ b/core/common/src/utils/serde_secret.rs @@ -17,22 +17,42 @@ //! Serde serialization helpers for `SecretString` fields. //! -//! `SecretString` intentionally does not implement `Serialize` to prevent -//! accidental secret exposure. These helpers are for fields that **must** be -//! serialized (e.g., wire protocol payloads, persisted TOML configs, API -//! responses that already expose credentials by design). +//! `SecretString` intentionally does not implement `Serialize`, and that +//! absence is the protection: a struct holding one cannot derive `Serialize` +//! at all. Adding `serialize_with` is therefore what *unblocks* the derive, so +//! reaching for a helper here is a decision to serialize a credential, never a +//! way to avoid it. +//! +//! [`serialize_secret`] and [`serialize_optional_secret`] write the plaintext. +//! Use them only where the plaintext is the point: wire protocol payloads, +//! persisted configs, API responses that expose credentials by design. //! -//! Usage: //! ```ignore //! #[serde(serialize_with = "crate::utils::serde_secret::serialize_secret")] //! pub password: SecretString, //! ``` //! -//! Do **not** add `serialize_with` to fields that should remain redacted in -//! serialized output — rely on `SecretString`'s default behavior instead. +//! [`serialize_redacted`] and [`serialize_optional_redacted`] write +//! [`REDACTED`] in place of the value, for a struct that must be serializable +//! for unrelated reasons but whose credential no reader is entitled to. +//! +//! **Redacted output is not a config.** Deserializing it hands back the literal +//! [`REDACTED`] as the secret, silently, so a redact-then-reload round trip +//! replaces the credential with the placeholder instead of failing. Nothing +//! in-tree can reach that today: these helpers have no consumers, and the one +//! persist/reload path round-trips a raw `serde_json::Value` rather than a +//! typed struct. If a consumer ever needs the round trip closed mechanically, +//! the shape that cannot be half-applied is a newtype owning both directions, +//! not a paired `deserialize_with` that a caller can forget to add. +//! +//! If neither applies, leave `serialize_with` off and let the missing impl keep +//! the field unserializable. use secrecy::{ExposeSecret, SecretString}; +/// Placeholder written in place of a redacted secret. +pub const REDACTED: &str = "[REDACTED]"; + pub fn serialize_secret( secret: &SecretString, serializer: S, @@ -50,6 +70,28 @@ pub fn serialize_optional_secret( } } +/// Writes [`REDACTED`] instead of the secret. +pub fn serialize_redacted( + _secret: &SecretString, + serializer: S, +) -> Result { + serializer.serialize_str(REDACTED) +} + +/// Writes [`REDACTED`] instead of the secret, keeping `None` distinguishable. +/// +/// Whether a credential is configured at all is not itself a secret, and +/// collapsing `Some` to `null` would tell a reader the field is unset. +pub fn serialize_optional_redacted( + secret: &Option, + serializer: S, +) -> Result { + match secret { + Some(_) => serializer.serialize_some(REDACTED), + None => serializer.serialize_none(), + } +} + #[cfg(test)] mod tests { use super::*; @@ -101,4 +143,43 @@ mod tests { let json = serde_json::to_string(&s).unwrap(); assert_eq!(json, r#"{"token":null}"#); } + + #[derive(Serialize)] + struct WithRedactedSecret { + #[serde(serialize_with = "serialize_redacted")] + password: SecretString, + } + + #[derive(Serialize)] + struct WithOptionalRedactedSecret { + #[serde(serialize_with = "serialize_optional_redacted")] + token: Option, + } + + #[test] + fn serialize_redacted_replaces_value_in_json() { + let s = WithRedactedSecret { + password: SecretString::from("my_password"), + }; + let json = serde_json::to_string(&s).unwrap(); + assert_eq!(json, r#"{"password":"[REDACTED]"}"#); + assert!(!json.contains("my_password")); + } + + #[test] + fn serialize_optional_redacted_keeps_some_distinguishable_from_none() { + let present = WithOptionalRedactedSecret { + token: Some(SecretString::from("tok_123")), + }; + let absent = WithOptionalRedactedSecret { token: None }; + + let present_json = serde_json::to_string(&present).unwrap(); + assert_eq!(present_json, r#"{"token":"[REDACTED]"}"#); + assert!(!present_json.contains("tok_123")); + assert_eq!( + serde_json::to_string(&absent).unwrap(), + r#"{"token":null}"#, + "a configured credential must not read as an unset one" + ); + } }