Add secp256k1eth pkcs11 support [1/4] by Eric-Warehime · Pull Request #24 · cosmos/kms

Eric-Warehime · 2026-07-15T03:09:54Z

Refactors the pkcs11 implementation to align with awskms--moves the signer to its own file.

Adds secp256k1eth signing support in pkcs11. This involves refactoring some of the ecdsasig library to support formats different from the DER encoding that awskms returns.

greptile-apps · 2026-07-15T03:13:55Z

Greptile Summary

This PR refactors the PKCS#11 signer (moving it into its own signer.go) and adds secp256k1eth signing support via the CKM_ECDSA mechanism. The ecdsasig library is extended with a decodeRSCompact/RecoverCompact path that handles the raw 64-byte r‖s output PKCS#11 tokens return, complementing the existing DER path used by AWS KMS.

ecdsasig: RecoverableSig → RecoverDER, decodeRS → decodeRSDER; new decodeRSCompact + RecoverCompact for raw r‖s input; shared private recoverSig helper extracts the key-recovery loop.
pkcs11/algo.go: new secp256k1eth entry in the algo registry with CKM_ECDSA mechanism, decodeSecp256k1Pub, and a recoverSig wrapper; fixSig signature widened to (raw, digest, pub []byte).
pkcs11/signer.go: Signer, Open, Sign, Close extracted verbatim from pkcs11.go; Sign now forwards signBytes and cached pub into fixSig for the recovery calculation.

Confidence Score: 3/5

The refactor and new secp256k1eth support are well-structured, but RecoverCompact lacks the digest length guard present in RecoverDER, which can cause silent misbehaviour if a non-32-byte hash is passed.

The new RecoverCompact function omits the len(digest) != 32 check that its sibling RecoverDER enforces before calling the shared recoverSig helper. The PKCS#11 Sign path passes raw signBytes as the digest; if those bytes are not exactly 32 bytes, key recovery silently exhausts both recovery candidates and returns a cryptic error rather than a clear diagnostic. The rest of the change — the refactor into signer.go, the algo registry extension, and the DER→compact split — is clean and low-risk.

signing/ecdsasig/ecdsasig.go — RecoverCompact needs the digest length guard; signing/pkcs11/algo.go has two minor comment issues.

Important Files Changed

Filename	Overview
signing/ecdsasig/ecdsasig.go	Added `decodeRSCompact` and `RecoverCompact` for raw r‖s signatures, and extracted shared `recoverSig` helper; `RecoverCompact` is missing the 32-byte digest length guard that `RecoverDER` enforces.
signing/pkcs11/algo.go	Added secp256k1eth algorithm entry with CKM_ECDSA mechanism, `decodeSecp256k1Pub`, and `recoverSig`; `fixSig` signature widened to pass digest and pub; two minor comment issues (stale "only entry for now", typo "aa").
signing/pkcs11/signer.go	Signer extracted from pkcs11.go into its own file; `Sign` now passes `signBytes` and `s.pub` to `fixSig` so secp256k1eth can perform key recovery — straightforward refactor with no logic changes.
signing/pkcs11/pkcs11.go	Signer code moved to signer.go; remaining helpers (selectSlot, findObject, resolvePIN, acquireModule) unchanged.
signing/awskms/algo.go	Single rename: `ecdsasig.RecoverableSig` → `ecdsasig.RecoverDER`; no logic changes.
signing/ecdsasig/ecdsasig_test.go	Tests updated to cover `RecoverCompact` with compact signature helpers `compactSig` and `highSCompact`; existing DER tests renamed to match the new function names.
signing/pkcs11/algo_test.go	Updated `fixSig` call-site to pass the new `(sig, nil, nil)` signature; no logic changes.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Caller
    participant Signer as pkcs11.Signer.Sign
    participant Token as PKCS#11 Token (CKM_ECDSA)
    participant fixSig as algo.fixSig (recoverSig)
    participant RC as ecdsasig.RecoverCompact
    participant RS as ecdsasig.recoverSig

    Caller->>Signer: Sign(ctx, signBytes [32-byte hash])
    Signer->>Token: SignInit(CKM_ECDSA, privH)
    Signer->>Token: Sign(signBytes)
    Token-->>Signer: raw sig (64-byte r‖s)
    Signer->>fixSig: fixSig(raw, signBytes, pub)
    fixSig->>fixSig: secp256k1.ParsePubKey(pub)
    fixSig->>RC: RecoverCompact(raw, signBytes, dpub)
    RC->>RC: decodeRSCompact(raw) → r, s (low-S normalised)
    RC->>RS: recoverSig(r, s, signBytes, dpub)
    loop "v = 0, 1"
        RS->>RS: ecdsa.RecoverCompact(27+v ‖ r ‖ s, signBytes)
        RS->>RS: "recovered == dpub?"
    end
    RS-->>RC: 65-byte r‖s‖v
    RC-->>fixSig: 65-byte r‖s‖v
    fixSig-->>Signer: 65-byte r‖s‖v
    Signer-->>Caller: 65-byte recoverable sig

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Caller
    participant Signer as pkcs11.Signer.Sign
    participant Token as PKCS#11 Token (CKM_ECDSA)
    participant fixSig as algo.fixSig (recoverSig)
    participant RC as ecdsasig.RecoverCompact
    participant RS as ecdsasig.recoverSig

    Caller->>Signer: Sign(ctx, signBytes [32-byte hash])
    Signer->>Token: SignInit(CKM_ECDSA, privH)
    Signer->>Token: Sign(signBytes)
    Token-->>Signer: raw sig (64-byte r‖s)
    Signer->>fixSig: fixSig(raw, signBytes, pub)
    fixSig->>fixSig: secp256k1.ParsePubKey(pub)
    fixSig->>RC: RecoverCompact(raw, signBytes, dpub)
    RC->>RC: decodeRSCompact(raw) → r, s (low-S normalised)
    RC->>RS: recoverSig(r, s, signBytes, dpub)
    loop "v = 0, 1"
        RS->>RS: ecdsa.RecoverCompact(27+v ‖ r ‖ s, signBytes)
        RS->>RS: "recovered == dpub?"
    end
    RS-->>RC: 65-byte r‖s‖v
    RC-->>fixSig: 65-byte r‖s‖v
    fixSig-->>Signer: 65-byte r‖s‖v
    Signer-->>Caller: 65-byte recoverable sig

Comments Outside Diff (1)

signing/pkcs11/algo.go, line 31-32 (link)

Stale comment — secp256k1eth is now a second entry in the registry.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

_{Reviews (1): Last reviewed commit: "Remove leftover" | Re-trigger Greptile}

greptile-apps · 2026-07-15T03:13:59Z

+// RecoverCompact decodes an r‖s signature (2x32 bytes)
+// and returns r‖s‖v (65 bytes) with recovery byte and low-S normalized.
+func RecoverCompact(rs, digest []byte, pub *secp256k1.PublicKey) ([]byte, error) {
+	r, s, err := decodeRSCompact(rs)
+	if err != nil {
+		return nil, err
+	}
+	return recoverSig(r, s, digest, pub)
+}


Missing 32-byte digest guard in RecoverCompact

RecoverDER explicitly validates len(digest) != 32 before calling recoverSig, but the new RecoverCompact skips that check entirely. Both ultimately call ecdsa.RecoverCompact(compact, digest), which expects a 32-byte hash. When the PKCS#11 signer passes signBytes as the digest argument and those bytes are not exactly 32 bytes (e.g., if a caller mistakenly passes full consensus sign-bytes), key recovery will silently fall through the v = 0/1 loop and return the opaque error "no 0/1 recovery id recovers the public key" instead of the actionable "digest must be 32 bytes" message that RecoverDER would emit.

greptile-apps · 2026-07-15T03:14:01Z

+	return ecdsasig.RecoverCompact(raw, digest, dpub)
+}
+
 // decodeEd25519Pub turns a CKA_EC_POINT value into aa byte array.


Typo: "aa" should be "a".

Suggested change

// decodeEd25519Pub turns a CKA_EC_POINT value into aa byte array.

// decodeEd25519Pub turns a CKA_EC_POINT value into a byte array.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Eric-Warehime added 3 commits July 14, 2026 21:48

Add secp256k1eth to pkcs11

7c39761

Merge remote-tracking branch 'origin/main' into eric/pkcs11-all

33afa4f

Remove leftover

37f38b8

Eric-Warehime requested a review from a team as a code owner July 15, 2026 03:09

greptile-apps Bot reviewed Jul 15, 2026

View reviewed changes

Eric-Warehime changed the title ~~Add secp256k1eth pkcs11 support~~ Add secp256k1eth pkcs11 support [1/4] Jul 15, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add secp256k1eth pkcs11 support [1/4]#24

Add secp256k1eth pkcs11 support [1/4]#24
Eric-Warehime wants to merge 3 commits into
mainfrom
eric/pkcs11-all

Eric-Warehime commented Jul 15, 2026

Uh oh!

greptile-apps Bot commented Jul 15, 2026 •

edited

Loading

Sequence Diagram

Comments Outside Diff (1)

Uh oh!

greptile-apps Bot Jul 15, 2026

Uh oh!

greptile-apps Bot Jul 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

	// decodeEd25519Pub turns a CKA_EC_POINT value into aa byte array.
	// decodeEd25519Pub turns a CKA_EC_POINT value into a byte array.

Uh oh!

Conversation

Eric-Warehime commented Jul 15, 2026

Uh oh!

greptile-apps Bot commented Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 3/5

Important Files Changed

Sequence Diagram

Comments Outside Diff (1)

Uh oh!

greptile-apps Bot Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

greptile-apps Bot commented Jul 15, 2026 •

edited

Loading