docs(connectors): fix the inverted serde_secret redaction claim - #3803
docs(connectors): fix the inverted serde_secret redaction claim#3803mlevkov wants to merge 2 commits into
Conversation
`serialize_secret` and `serialize_optional_secret` write the plaintext, which leaves an author who needs `Serialize` on a struct holding a credential with no redacting option — so they reach for the exposing one and believe it protects them. That is the trap apache#3801 documents. `serialize_redacted` and `serialize_optional_redacted` write a placeholder instead. 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. The module doc now leads with what these helpers actually do, since the absent `Serialize` impl on `SecretString` is the protection and any helper here is a decision to give it up.
|
Thanks for the PR. It is labeled Slash commands (own line, regular comment) move it around the queue:
See CONTRIBUTING.md for details. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3803 +/- ##
============================================
- Coverage 75.72% 74.05% -1.68%
Complexity 969 969
============================================
Files 1322 1322
Lines 159363 154110 -5253
Branches 132746 127569 -5177
============================================
- Hits 120684 114125 -6559
- Misses 35041 36047 +1006
- Partials 3638 3938 +300
🚀 New features to boost your workflow:
|
The Secrets guidance told plugin authors that annotating a `SecretString` with `serialize_secret` made serialization redact. It does the opposite: the helper calls `expose_secret()`. `SecretString` has no `Serialize` impl precisely so a struct holding one cannot be serialized, so adding the attribute is what unblocks the derive and gives up the guarantee. Nine plugins followed that guidance. The claim appeared twice: in the Secrets prose and again as an "Auto-redact on Debug/Display + serialization" row in the patterns table, where it read as a recommended pairing. Both now say which half was true - `Debug` does redact - and the section gives the default for a plugin config struct: do not derive `Serialize` at all, since nothing needs it and leaving it off makes the property compiler-enforced. It also names the redacting helpers for structs that genuinely need serialization, and notes that none of this protects against the runtime control API returning plugin config verbatim, which is apache#3802 and not fixable from the plugin side. Corrects the in-tree list too: it named delta_sink, which does not use these helpers, and omitted s3_sink and surrealdb_sink, which do.
76a7612 to
fd1e1cc
Compare
|
/request-review @hubcio |
hubcio
left a comment
hubcio
left a comment
There was a problem hiding this comment.
follow-up outside this diff: runtime/src/api/config.rs Debug impl hardcodes "[REDACTED]", which now duplicates the new REDACTED const. same literal is also hardcoded in core/common auth credentials, server state models and the s3_sink URL redaction - worth a small sweep to the const later.
|
|
||
| **`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). |
There was a problem hiding this 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.
|
|
||
| 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`. |
There was a problem hiding this 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.
| //! [`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 |
There was a problem hiding this 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.
github-actions
Bot
added
S-waiting-on-author
2 participants
Closes #3801.
What was wrong
.claude/skills/connectors-overview/SKILL.mdtold plugin authors that annotatinga
SecretStringfield withiggy_common::serde_secret::serialize_secretmadeDebugand serialization redact. Only theDebughalf is true. The helpercalls
expose_secret()and writes the plaintext:serde_secret.rs's own module doc already said the opposite of the skill("Do not add
serialize_withto fields that should remain redacted"), so thetwo documents contradicted each other and the skill is the one plugin authors
read.
The inversion matters more than a typo would, because
SecretStringhas noSerializeimpl by design — a struct holding one cannot deriveSerializeatall. 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::Valueand never deserializes into aplugin'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 wastrue, and gives the default: do not derive
Serializeon a plugin configstruct 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 usethese helpers, and omitted
s3_sinkandsurrealdb_sink, which do.feat(common)— addsserialize_redactedandserialize_optional_redactedso the corrected guidance has something to point at. Suggestion 3 in the issue.
Without them, an author who genuinely needs
Serializehas no redacting optionand reaches for the exposing one, which is the trap. The optional form keeps
Somedistinguishable fromNone: whether a credential is configured is notitself secret, and collapsing it to
nullwould report a configured field asunset. 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
Serializefrom the nine plugin configstructs 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 exit0.