mlevkov opened a new pull request, #3803:
URL: https://github.com/apache/iggy/pull/3803
Closes #3801.
## What was wrong
`.claude/skills/connectors-overview/SKILL.md` told plugin authors that
annotating
a `SecretString` field with `iggy_common::serde_secret::serialize_secret`
made
`Debug` **and serialization** redact. Only the `Debug` half is true. The
helper
calls `expose_secret()` and writes the plaintext:
```rust
pub fn serialize_secret<S: serde::Serializer>(
secret: &SecretString,
serializer: S,
) -> Result<S::Ok, S::Error> {
serializer.serialize_str(secret.expose_secret())
}
```
`serde_secret.rs`'s own module doc already said the opposite of the skill
("Do **not** add `serialize_with` to fields that should remain redacted"),
so the
two documents contradicted each other and the skill is the one plugin authors
read.
The inversion matters more than a typo would, because `SecretString` has no
`Serialize` impl *by design* — a struct holding one cannot derive
`Serialize` at
all. Adding the attribute is what **unblocks** the derive. The guidance
therefore
recommended the exact step that converts a compile-time guarantee into
plaintext
output, while describing it as protection.
Nine plugins carry the annotation on credential fields. It is inert today
(the
runtime keeps plugin config as `serde_json::Value` and never deserializes
into a
plugin's config struct, so nothing calls these serializers), so this is a
correctness-of-documentation fix rather than a live leak — but the protection
those authors believe they have does not exist.
## What this changes
**`docs(connectors)`** — the Secrets section now says which half of the
claim was
true, and gives the default: *do not derive `Serialize` on a plugin config
struct at all*, since nothing needs it and leaving it off makes the property
compiler-enforced rather than convention-enforced.
`iggy_connector_http_source` (#3798) does exactly that.
It also notes that none of this protects the credential from the runtime's
own
control API, which returns plugin configuration verbatim — that is #3802,
and it
is not addressable from the plugin side.
While in there: the "In-tree uses" list named `delta_sink`, which does not
use
these helpers, and omitted `s3_sink` and `surrealdb_sink`, which do.
**`feat(common)`** — adds `serialize_redacted` and
`serialize_optional_redacted`
so the corrected guidance has something to point at. Suggestion 3 in the
issue.
Without them, an author who genuinely needs `Serialize` has no redacting
option
and reaches for the exposing one, which is the trap. 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. Documented as not round-tripping, so nobody feeds redacted output back
into a config loader.
## Deliberately not in scope
Suggestion 2 in the issue — dropping `Serialize` from the nine plugin config
structs that do not need it. It is the right follow-up and it is safe, but it
touches nine crates and each needs checking for a real serializing caller,
so it
does not belong in the same review as the documentation fix. Happy to do it
as a
separate PR; say the word and I will.
## Verification
`cargo fmt --all --check`, `cargo sort --check --no-format --workspace`,
`cargo clippy -p iggy_common --all-features --all-targets -- -D warnings`,
`cargo test -p iggy_common serde_secret` (6 pass), `taplo fmt --check`,
`hawkeye check`, `typos`, `markdownlint`, trailing whitespace/newline — all
exit
0.
--
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]
