This is an automated email from the ASF dual-hosted git repository.
hubcio pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/iggy.git
The following commit(s) were added to refs/heads/master by this push:
new f900e7abe docs(connectors): fix the inverted serde_secret redaction
claim (#3803)
f900e7abe is described below
commit f900e7abe55d62115ac9c8675039fc07017267f5
Author: Maxim Levkov <[email protected]>
AuthorDate: Mon Aug 10 03:05:34 2026 -0700
docs(connectors): fix the inverted serde_secret redaction claim (#3803)
---
.claude/skills/connectors-overview/SKILL.md | 26 ++++++--
core/common/src/utils/serde_secret.rs | 95 ++++++++++++++++++++++++++---
2 files changed, 109 insertions(+), 12 deletions(-)
diff --git a/.claude/skills/connectors-overview/SKILL.md
b/.claude/skills/connectors-overview/SKILL.md
index 91ff86ec7..230ad5354 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::<C>` 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 7f8bd2feb..b10492e43 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<S: serde::Serializer>(
secret: &SecretString,
serializer: S,
@@ -50,6 +70,28 @@ pub fn serialize_optional_secret<S: serde::Serializer>(
}
}
+/// Writes [`REDACTED`] instead of the secret.
+pub fn serialize_redacted<S: serde::Serializer>(
+ _secret: &SecretString,
+ serializer: S,
+) -> Result<S::Ok, S::Error> {
+ 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<S: serde::Serializer>(
+ secret: &Option<SecretString>,
+ serializer: S,
+) -> Result<S::Ok, S::Error> {
+ 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<SecretString>,
+ }
+
+ #[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"
+ );
+ }
}
