From abaac5456767ef8a8fe35a315f21d58aa36cc337 Mon Sep 17 00:00:00 2001 From: mlevkov Date: Sun, 2 Aug 2026 14:28:52 -0700 Subject: [PATCH 1/3] feat(common): add redacting serde_secret serializers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `serialize_secret` and `serialize_optional_secret` write the plaintext, which leaves an author who needs `Serialize` on a struct holding a credential with no redacting option — so they reach for the exposing one and believe it protects them. That is the trap #3801 documents. `serialize_redacted` and `serialize_optional_redacted` write a placeholder instead. The optional form keeps `Some` distinguishable from `None`: whether a credential is configured is not itself secret, and collapsing it to null would report a configured field as unset. The module doc now leads with what these helpers actually do, since the absent `Serialize` impl on `SecretString` is the protection and any helper here is a decision to give it up. --- core/common/src/utils/serde_secret.rs | 88 ++++++++++++++++++++++++--- 1 file changed, 81 insertions(+), 7 deletions(-) diff --git a/core/common/src/utils/serde_secret.rs b/core/common/src/utils/serde_secret.rs index 7f8bd2feb3..28b9d9255d 100644 --- a/core/common/src/utils/serde_secret.rs +++ b/core/common/src/utils/serde_secret.rs @@ -17,22 +17,35 @@ //! 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 does not round-trip: deserializing it yields the literal +//! placeholder, so never feed it back into a config loader. +//! +//! 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 +63,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 +136,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" + ); + } } From 7ff68a82c6c31cfc5042370fb18696008beec7a6 Mon Sep 17 00:00:00 2001 From: mlevkov Date: Sun, 2 Aug 2026 14:28:53 -0700 Subject: [PATCH 2/3] docs(connectors): fix the inverted serde_secret redaction claim The Secrets guidance told plugin authors that annotating a `SecretString` with `serialize_secret` made serialization redact. It does the opposite: the helper calls `expose_secret()`. `SecretString` has no `Serialize` impl precisely so a struct holding one cannot be serialized, so adding the attribute is what unblocks the derive and gives up the guarantee. Nine plugins followed that guidance. The claim appeared twice: in the Secrets prose and again as an "Auto-redact on Debug/Display + serialization" row in the patterns table, where it read as a recommended pairing. Both now say which half was true - `Debug` does redact - and the section gives the default for a plugin config struct: do not derive `Serialize` at all, since nothing needs it and leaving it off makes the property compiler-enforced. It also names the redacting helpers for structs that genuinely need serialization, and notes that none of this protects against the runtime control API returning plugin config verbatim, which is #3802 and not fixable from the plugin side. Corrects the in-tree list too: it named delta_sink, which does not use these helpers, and omitted s3_sink and surrealdb_sink, which do. --- .claude/skills/connectors-overview/SKILL.md | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/.claude/skills/connectors-overview/SKILL.md b/.claude/skills/connectors-overview/SKILL.md index 91ff86ec7c..3dd5290129 100644 --- a/.claude/skills/connectors-overview/SKILL.md +++ b/.claude/skills/connectors-overview/SKILL.md @@ -96,14 +96,21 @@ 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 **do not derive `Serialize` at all**. The runtime keeps plugin configuration as the `serde_json::Value` it parsed from TOML and never deserializes into a plugin's config struct, so nothing needs the impl. Leaving it off makes the property 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)] +// No `Serialize`: nothing needs it, and leaving it 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 +120,11 @@ 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. + +In-tree uses of the exposing helpers: `sinks/{postgres,mongodb,elasticsearch,influxdb,s3,surrealdb}_sink`, `sources/{postgres,elasticsearch,influxdb}_source`. ### Errors @@ -193,7 +204,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 From 0ddc886e7bb0226be26f4f5a5fb1cbdb6c98b6ea Mon Sep 17 00:00:00 2001 From: mlevkov Date: Sat, 8 Aug 2026 14:07:50 -0700 Subject: [PATCH 3/3] docs(connectors): correct the plugin config serialization claims Review caught two wrong statements in the guidance this PR was fixing. "The runtime never deserializes into a plugin's config struct" is false: the SDK glue does exactly that, `serde_json::from_str::` under a `DeserializeOwned` bound in `sdk/src/{sink,source}.rs`, so `Deserialize` stays required and the advice now says which half to drop. The property that actually carries the argument is that nothing ever re-serializes the struct. Plugin configuration also does not only come from TOML; the control API accepts it as JSON and env vars can inject it. The in-tree list read as an exhaustive inventory while covering only plugins, which framed two deliberate uses as oversights: the runtime's own `HttpConfig::api_key`, and the `core/common` wire payloads for login, create-user, change-password and PAT, where the credential is the payload. Scoped the sentence to plugin-side callers. Also answers the round-trip question raised in review. The doc now names the failure directly: deserializing redacted output hands back the placeholder as the secret rather than failing. Left mechanical rather than adding a paired `deserialize_with`, because that guard is itself opt-in and a caller who forgets it is exactly the case it claims to cover. A newtype owning both directions is the shape that cannot be half-applied, and it should be designed against a real consumer. --- .claude/skills/connectors-overview/SKILL.md | 13 +++++++++---- core/common/src/utils/serde_secret.rs | 11 +++++++++-- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/.claude/skills/connectors-overview/SKILL.md b/.claude/skills/connectors-overview/SKILL.md index 3dd5290129..230ad5354b 100644 --- a/.claude/skills/connectors-overview/SKILL.md +++ b/.claude/skills/connectors-overview/SKILL.md @@ -100,15 +100,18 @@ Any credential-bearing field (connection strings, API keys, bearer tokens, AWS k **`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 **do not derive `Serialize` at all**. The runtime keeps plugin configuration as the `serde_json::Value` it parsed from TOML and never deserializes into a plugin's config struct, so nothing needs the impl. Leaving it off makes the property compiler-enforced instead of convention-enforced (`sources/http_source/src/lib.rs::HttpSourceConfig` does this, and comments the omission so nobody adds it back). +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}; -// No `Serialize`: nothing needs it, and leaving it off is what makes the -// credential unserializable rather than merely un-serialized. +// `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 { pub connection_string: SecretString, @@ -124,7 +127,9 @@ If a config struct genuinely needs `Serialize`, `serde_secret::serialize_redacte 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. -In-tree uses of the exposing helpers: `sinks/{postgres,mongodb,elasticsearch,influxdb,s3,surrealdb}_sink`, `sources/{postgres,elasticsearch,influxdb}_source`. +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 diff --git a/core/common/src/utils/serde_secret.rs b/core/common/src/utils/serde_secret.rs index 28b9d9255d..b10492e438 100644 --- a/core/common/src/utils/serde_secret.rs +++ b/core/common/src/utils/serde_secret.rs @@ -35,8 +35,15 @@ //! [`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 does not round-trip: deserializing it yields the literal -//! placeholder, so never feed it back into a config loader. +//! +//! **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.