hubcio commented on code in PR #3803:
URL: https://github.com/apache/iggy/pull/3803#discussion_r3701945066
##########
.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).
Review Comment:
two wording nits in this sentence. the plugin itself does deserialize into
its config struct - the sdk glue calls `serde_json::from_str::<C>` with a
`DeserializeOwned` bound, so `Deserialize` stays required and "never
deserializes" reads wrong; the load-bearing fact for dropping the impl is that
nothing ever re-serializes the struct. and `plugin_config` doesn't only come
from TOML - the control API accepts it as JSON and env vars can inject it too.
##########
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
Review Comment:
worth a paired `deserialize_with` that rejects the `[REDACTED]` literal, so
redacted output fed back into a config loader fails loudly instead of becoming
the actual secret? nothing in-tree can hit this today - the redacting helpers
have no consumers yet and the only persist/reload path round-trips the raw
`serde_json::Value` - so a doc warning may be enough for now. question is
whether to make it mechanical now or when the first consumer shows up.
##########
.claude/skills/connectors-overview/SKILL.md:
##########
@@ -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`.
Review Comment:
this reads as an exhaustive inventory but only covers plugins.
`runtime/src/api/config.rs` also puts `serialize_secret` on
`HttpConfig::api_key` (equally inert - nothing serializes the runtime config
today), and a handful of core/common wire-payload types (login, create-user,
change-password, PAT) use the exposing helpers by design. scoping the sentence
to plugin-side uses is probably the cleanest fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
