fix: String() on signers must not leak private keys - #48
Open
bajtos wants to merge 1 commit into
Open
Conversation
`multikey` signers are `[]byte` holding private key material, so passing one to `fmt.*` with `%s` or `%v` printed the raw key. Declare `String() string` on `ucan.Signer` and implement it on both multikey signers to return the key DID instead, which fixes every call site in dependents at once. Signed-off-by: Miroslav Bajto拧 <oss@bajtos.net> Assisted-by: Claude:claude-opus-5[1m]
Contributor
There was a problem hiding this comment.
Pull request overview
This PR prevents accidental private-key leakage when multikey signers (which are []byte holding key material) are passed to fmt.* by adding a String() string method to ucan.Signer and implementing it for the ed25519 and secp256k1 multikey signers to return the key DID instead of raw bytes.
Changes:
- Extend
ucan.Signerwith aString() stringcontract that must not include private key material. - Implement
String()for ed25519 and secp256k1 multikey signers to returnKeyDID().String(). - Add tests asserting
fmt.Sprint(signer)matches the signer鈥檚 key DID for both algorithms.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ucan/ucan.go | Adds String() to the ucan.Signer interface with explicit non-leak guidance. |
| multikey/secp256k1/signer.go | Implements Signer.String() intended to avoid exposing raw private key bytes. |
| multikey/secp256k1/signer_test.go | Adds coverage ensuring formatting uses the DID string. |
| multikey/ed25519/signer.go | Implements Signer.String() intended to avoid exposing raw private key bytes. |
| multikey/ed25519/signer_test.go | Adds coverage ensuring formatting uses the DID string. |
馃挕 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+105
to
+109
| // String returns the signer's key DID. It deliberately never exposes the | ||
| // private key bytes, so that passing a Signer to fmt.* cannot leak them. | ||
| func (s Signer) String() string { | ||
| return s.KeyDID().String() | ||
| } |
Comment on lines
+100
to
+104
| // String returns the signer's key DID. It deliberately never exposes the | ||
| // private key bytes, so that passing a Signer to fmt.* cannot leak them. | ||
| func (s Signer) String() string { | ||
| return s.KeyDID().String() | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
multikeysigners are[]byteholding private key material, so passing one tofmt.*with%sor%vprinted the raw key.Declare
String() stringonucan.Signerand implement it on both multikey signers to return the key DID instead, which fixes every call site in dependents at once.Related: