mlevkov commented on code in PR #3803:
URL: https://github.com/apache/iggy/pull/3803#discussion_r3742358414
##########
.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:
Fixed in `3cd136fe3`, carried through the rebase. Verified the SDK glue you
described: `sdk/src/{sink,source}.rs` call `serde_json::from_str::<C>` under a
`DeserializeOwned` bound, so the guidance now says derive `Deserialize` but not
`Serialize`, and names the property that actually carries the argument (nothing
re-serializes the struct). Also corrected that `plugin_config` arrives as TOML,
as JSON posted to the control API, or from env.
##########
.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:
Fixed in `3cd136fe3`. Scoped to plugin-side callers, and the sentence now
says the other uses are not mistakes, naming `HttpConfig::api_key` and the
`core/common` login / create-user / change-password / PAT payloads where the
credential is the payload by design.
##########
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:
Left as a doc warning for now, strengthened to name the failure rather than
imply it. My reasoning is in the PR comment: a paired `deserialize_with` is
itself opt-in, so an author who forgets it hits exactly the case it claims to
cover. A newtype owning both directions is the only shape that cannot be
half-applied, and I would rather design that against a real consumer than
guess. Happy to add either form if you disagree.
--
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]
