mlevkov opened a new issue, #3801:
URL: https://github.com/apache/iggy/issues/3801
## Summary
The connectors guidance tells plugin authors that annotating a
`SecretString` field with `iggy_common::serde_secret::serialize_secret` gives
them redaction. It does the opposite: the helper calls `expose_secret()` and
writes the value in plaintext. Nine connector plugins follow that guidance
today.
## The claim
`.claude/skills/connectors-overview/SKILL.md`, "Secrets":
> `secrecy::SecretString` + `iggy_common::serde_secret::serialize_secret` |
`sinks/postgres_sink::PostgresSinkConfig::connection_string` | **Auto-redact on
Debug/Display + serialization**
## The code
`core/common/src/utils/serde_secret.rs`:
```rust
pub fn serialize_secret<S: serde::Serializer>(
secret: &SecretString,
serializer: S,
) -> Result<S::Ok, S::Error> {
serializer.serialize_str(secret.expose_secret())
}
```
Its own module doc says the opposite of the skill:
> 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).
>
> Do **not** add `serialize_with` to fields that should remain redacted in
serialized output — rely on `SecretString`'s default behavior instead.
And its test `serialize_optional_secret_with_some_value` asserts the output
is `{"token":"tok_123"}`.
The `Debug` half of the claim is true — `SecretBox`'s `Debug` prints
`[REDACTED]`. Only the serialization half is inverted.
## Why it matters
`SecretString` deliberately has no `Serialize` impl, so a config struct
holding one cannot derive `Serialize` at all. Adding `serialize_with` is what
*unblocks* the derive — it converts a compile-time guarantee into plaintext
output, while the documentation says it does the reverse.
Nine plugins carry the annotation on credential fields:
`sinks/{postgres,s3,influxdb,elasticsearch,mongodb,surrealdb}_sink`,
`sources/{postgres,elasticsearch,influxdb}_source`.
In practice the attribute appears inert: the runtime keeps plugin
configuration as `serde_json::Value` and never deserializes into a plugin's
config struct, so nothing calls these serializers today. The problem is that
the protection authors believe they have does not exist, and the first caller
that serializes a plugin config leaks every credential in it.
## Suggested fix
1. Correct the SKILL.md row — the helper exposes; `SecretString`'s default
(no `Serialize` impl) is what redacts.
2. Consider dropping `Serialize` from plugin config structs that do not need
it. `iggy_connector_http_source` (#3798) does this, which makes the property
compiler-enforced rather than convention-enforced.
3. If a config struct genuinely needs `Serialize`, a `serialize_redacted`
helper alongside the existing one would give authors the thing the docs
currently promise.
Found while preparing #3798.
--
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]
