Cleartext Debug on VaultRecord / AdminOpBody; no zeroize convention for payload buffers

Found by the security review of #28 (out of that PR's scope: credentials-core).

VaultRecord derives Debug while carrying payload: Vec<u8> (crates/credentials-core/src/record.rs:158-181); AdminOpBody derives Debug around a boxed VaultRecord (crates/credentials-core/src/admin_ops.rs:28-38). No current log site prints them, but OAuthCredential and the CLI's ServedCredential already follow a redacted-Debug convention, and the contract says material must never be exposed by Debug. Same review noted there is no zeroize convention for payload buffers in the CLI paths (the master key is Zeroizing; the material it protects is a plain Vec<u8>).

Proposed: manual redacted Debug for both (payload: [N bytes redacted]), and Zeroizing<Vec<u8>> for material buffers where the type crosses a function boundary. Happy to PR it after #28 settles if you want it.

opencode: vault custody for static API keys through an in-process aut…

core: redact Debug on VaultRecord and AdminOpBody, and pin it in both…

opencode: vault custody for static API keys through an in-process aut…

The Debug half shipped at 0679dea, including one you did not name — AdminOpBody::RevokeHandle.handle is the raw ckh_ bearer rather than a hash, which is worse than an encrypted payload in a log because it needs no key to use. Details in the commit.

I said the zeroize half needed its own measurement before I touched it. Here it is, and it changes the shape you proposed.

What is protected today, and the pattern in it

MasterKey     Zeroizing<[u8; 32]>
AdminMacKey   Zeroizing<[u8; 32]>
audit_key     Zeroizing<[u8; 32]>

Every one is a fixed-size KEY. Not one is credential material. That is the gap you spotted and it is real.

But the shape is load-bearing rather than incidental: an array never reallocates, so Zeroizing<[u8; N]> clears the only allocation that ever held those bytes. Zeroizing<Vec<u8>> does not have that property, and this is where the proposal runs into the code.

The record holds the same secret twice, in two representations

pub struct VaultRecord {
    pub oauth: Option<OAuthCredential>,   // access_token: String
    pub payload: Vec<u8>,
}

For an OAuth record the payload IS typically the access token — that is stated at the field, and I measured it this morning while fixing the Debug half: rendering one record printed access_token: "<redacted>" next to payload: [115, 107, 45, ...], and those bytes decode to the same secret.

So wrapping payload in Zeroizing<Vec<u8>> clears one of two live copies inside a single struct. The String beside it is untouched, and the login path puts it there through tokens.access_token.clone().into_bytes() — a clone whose source String outlives the conversion.

That is my objection, and it is not that the change is too small. A partial scrub reads as complete. The next reader sees Zeroizing on the payload, concludes material is handled, and stops looking at the field two lines up that holds the same bytes in a type nothing clears. An absent mitigation asks the question every time; a partial one answers it wrongly, and this repo has spent a fortnight on exactly that failure mode in other forms.

What I would take

Either the whole chain or a documented decision not to, and I lean toward the second for now — with the reasoning written at the field rather than left implicit:

The CLI is a short-lived process. The daemon holds decrypted material transiently per get and does not cache it.
Making OAuthCredential's fields Zeroizing<String> touches a serialized type on the record-body schema. That is a real change to the sealed envelope's shape, and it wants its own gate rather than riding a hardening pass.
Vec reallocation means even the full chain is best-effort against a determined memory adversary. Worth having, worth not overstating.

If you want to take it, the version I would review is the whole chain plus a comment stating what it does not cover. What I would push back on is Zeroizing<Vec<u8>> on the payload alone, because it buys less than it appears to and costs a future reader the question.

No rush — #28 is the bigger thread and this is not blocking it.

opencode: vault custody for static API keys through an in-process aut…

Update: the Debug half is resolved — 0679dea redacts VaultRecord and AdminOpBody, including the raw revoke-handle case. The zeroize half stays open on purpose: wrapping only payload would leave the duplicate OAuth String copy unsanitized (09-02 ruling here), so what remains is a whole-lifetime design for secret material, not a wrapper.

opencode: vault custody for static API keys through an in-process aut…

core: a Secret type that scrubs, serialises transparently, and cannot…

The zeroize half is closed, and it landed nine hours after your update here — so you have been tracking a gap that shut the same night.

your update              2026-09-04T13:44
4076452                  2026-09-04T22:28   "record the scrubbing frontier where Secret stops reaching"

On master today:

crates/credentials-core/src/record.rs:228    pub payload: SecretBytes
crates/credentials-core/src/oauth.rs:32      pub access_token: SecretString
crates/credentials-core/src/oauth.rs:36      pub refresh_token: SecretString

All three, which was the condition you set. Your 09-02 objection is the reason it is all three rather than one: wrapping payload alone would have left the duplicate OAuth String live, and a partial scrub that reads as complete protection is worse than none. That objection shaped the change.

It is a wrapper, and your "not a wrapper" framing was right about why

Secret<T> is not Zeroizing<T> renamed. The difference is the third row:

property	`Zeroizing<String>`	`Secret`
scrubbed on drop	yes	yes
serialises byte-identically	yes	yes
cannot be printed in the clear	no — `Debug` delegates	yes

Zeroizing derives Debug from its inner type, so {:?} on a wrapped token prints the token — which would have quietly undone the redaction half of this very issue. A type that is unprintable by construction cannot be un-redacted by someone adding a derive in a hurry.

The other deliberate choice is expose() as a named method rather than Deref. Deref would make every read invisible, and the audit question — where does secret material leave its scrubbed buffer — would have no mechanical answer. With expose, it is one grep: 78 call sites today, each one a place someone decided to take the cleartext out.

The frontier, because this is where an overclaim would live

A Secret scrubs the buffer it owns and nothing else. It cannot reach a copy taken via expose() and stored elsewhere, and it does not reach the HTTP layer at all — a provider's refresh response body arrives in reqwest's buffers holding a fresh token in the clear, and those are outside this type. Wrapping the last two layers while the first two stayed bare would reproduce exactly the partial-scrub illusion you objected to, which is why the boundary is written at the site rather than implied by the field list.

And the threat model is narrower than the word attracts: this is defence in depth against memory DISCLOSURE AFTER THE VALUE IS DEAD — a core dump, a swap page, a reused heap page. It does nothing against an attacker reading live process memory at the moment of use. The vault's real boundaries are encryption at rest, the master-key gate, and capability handles; this narrows the window in which a freed buffer still reads as a credential.

Closing

Both halves are on master: 0679dea for the redacted Debug, 4076452 for the scrubbing. Closing this — reopen if the frontier as stated is narrower than you think it should be, because that is a live design question rather than a settled one.

Navigation Menu

Cleartext Debug on VaultRecord / AdminOpBody; no zeroize convention for payload buffers #29

What is protected today, and the pattern in it

The record holds the same secret twice, in two representations

What I would take

It is a wrapper, and your "not a wrapper" framing was right about why

The frontier, because this is where an overclaim would live

Closing

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Cleartext Debug on VaultRecord / AdminOpBody; no zeroize convention for payload buffers #29

Description

Activity

ckcred-alfonso commented on Sep 2, 2026

What is protected today, and the pattern in it

The record holds the same secret twice, in two representations

What I would take

iceteaSA commented on Sep 4, 2026

ckcred-alfonso commented on Sep 11, 2026

It is a wrapper, and your "not a wrapper" framing was right about why

The frontier, because this is where an overclaim would live

Closing

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions