Skip to content

Use KeyDID() method instead of signer string representation in errors - #61

Closed
bajtos wants to merge 1 commit into
mainfrom
claude/encode-signer-pem-key-exposure-j55ooq
Closed

Use KeyDID() method instead of signer string representation in errors#61
bajtos wants to merge 1 commit into
mainfrom
claude/encode-signer-pem-key-exposure-j55ooq

Conversation

@bajtos

@bajtos bajtos commented Aug 11, 2026

Copy link
Copy Markdown
Member

Summary

Updated error messages in the PEM encoding function to use the KeyDID() method for more consistent and meaningful signer identification instead of relying on the signer's string representation.

Changes

  • Modified error messages in EncodeSignerToPEM() to call signer.KeyDID() instead of passing the signer object directly to fmt.Errorf()
  • Applied to two error cases:
    • Private key marshaling error
    • Private key PEM encoding error

Details

This change improves error messages by using the signer's DID (Decentralized Identifier) as a stable, meaningful identifier rather than the default string representation of the signer object. This makes debugging and error tracking more reliable and user-friendly.

https://claude.ai/code/session_017uKN5QJLWhCX6jTTJ8t6FV

EncodeSignerToPEM formatted the signer itself with %s. multikey signers
are backed by byte slices with no String method, so the raw private key
material was interpolated into the error text and leaked anywhere that
error was logged. Format the signer's key DID instead.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017uKN5QJLWhCX6jTTJ8t6FV
Copilot AI lite review requested due to automatic review settings August 11, 2026 18:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates identity.EncodeSignerToPEM() error messages to identify the signer using KeyDID() (a stable DID) rather than relying on the signer's default string representation, improving consistency and debuggability across error reports.

Changes:

  • Updated the private-key marshaling error to include signer.KeyDID().
  • Updated the PEM encoding error to include signer.KeyDID().

馃挕 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@frrist frrist left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My preference would be to define String() string on the interface embedded in a multikey, then implement it on all its implementations over in ucantone. This will fix all instances in dependencies where a signer operand is passeed to fmt.*. Here's a diff of the change that can be applied in ucantone:

diff --git a/multikey/ed25519/signer.go b/multikey/ed25519/signer.go
index 2a398c8..3e114e8 100644
--- a/multikey/ed25519/signer.go
+++ b/multikey/ed25519/signer.go
@@ -5,15 +5,16 @@ import (
        "crypto/rand"
        "fmt"
 
+       "github.com/multiformats/go-multibase"
+       "github.com/multiformats/go-multicodec"
+       "github.com/multiformats/go-varint"
+
        "github.com/fil-forge/ucantone/did"
        "github.com/fil-forge/ucantone/multikey"
        "github.com/fil-forge/ucantone/multikey/ed25519/verifier"
        "github.com/fil-forge/ucantone/ucan"
        "github.com/fil-forge/ucantone/varsig"
        "github.com/fil-forge/ucantone/varsig/algorithm/eddsa"
-       "github.com/multiformats/go-multibase"
-       "github.com/multiformats/go-multicodec"
-       "github.com/multiformats/go-varint"
 )
 
 const Code = multicodec.Ed25519Priv
@@ -94,6 +95,10 @@ func FromRaw(b []byte) (Signer, error) {
 
 type Signer []byte
 
+func (s Signer) String() string {
+       return s.verifier().KeyDID().String()
+}
+
 var _ multikey.Signer = (Signer)(nil)
 
 func (s Signer) Code() multicodec.Code {
diff --git a/multikey/secp256k1/signer.go b/multikey/secp256k1/signer.go
index 7c2224d..c9c3236 100644
--- a/multikey/secp256k1/signer.go
+++ b/multikey/secp256k1/signer.go
@@ -5,16 +5,17 @@ import (
        "crypto/sha256"
        "fmt"
 
+       "github.com/multiformats/go-multibase"
+       "github.com/multiformats/go-multicodec"
+       "github.com/multiformats/go-varint"
+       "gitlab.com/yawning/secp256k1-voi/secec"
+
        "github.com/fil-forge/ucantone/did"
        "github.com/fil-forge/ucantone/multikey"
        "github.com/fil-forge/ucantone/multikey/secp256k1/verifier"
        "github.com/fil-forge/ucantone/ucan"
        "github.com/fil-forge/ucantone/varsig"
        "github.com/fil-forge/ucantone/varsig/algorithm/ecdsa"
-       "github.com/multiformats/go-multibase"
-       "github.com/multiformats/go-multicodec"
-       "github.com/multiformats/go-varint"
-       "gitlab.com/yawning/secp256k1-voi/secec"
 )
 
 const Code = multicodec.Secp256k1Priv
@@ -99,6 +100,10 @@ func FromRaw(b []byte) (Signer, error) {
 
 type Signer []byte
 
+func (s Signer) String() string {
+       return s.verifier().KeyDID().String()
+}
+
 var _ multikey.Signer = (Signer)(nil)
 
 func (s Signer) SignatureAlgorithm() varsig.Algorithm {
diff --git a/ucan/ucan.go b/ucan/ucan.go
index 106c047..80b7437 100644
--- a/ucan/ucan.go
+++ b/ucan/ucan.go
@@ -43,6 +43,8 @@ type Signer interface {
 
        // Verifier returns a Verifier that can verify signatures from this Signer.
        Verifier() Verifier
+
+       String() string
 }
 
 // Signature encapsulates the bytes that comprise the signature as well as the

Approved in the event:

  • you feel strongly about the current design in this PR.
  • this bug is blocking your work.

@bajtos

bajtos commented Aug 12, 2026

Copy link
Copy Markdown
Member Author

Great suggestion!

@bajtos

bajtos commented Aug 12, 2026

Copy link
Copy Markdown
Member Author

Closing in favour of fil-forge/ucantone#48

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants