Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion signing/awskms/algo.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ func recoverableSig(raw, digest, pub []byte) ([]byte, error) {
if err != nil {
return nil, fmt.Errorf("parse secp256k1 public key: %w", err)
}
return ecdsasig.RecoverableSig(raw, digest, dpub)
return ecdsasig.RecoverDER(raw, digest, dpub)
}

// decodeEd25519Pub turns the DER SubjectPublicKeyInfo returned by KMS
Expand Down
47 changes: 41 additions & 6 deletions signing/ecdsasig/ecdsasig.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@ import (
"github.com/decred/dcrd/dcrec/secp256k1/v4/ecdsa"
)

// decodeRS parses a canonical DER ECDSA signature into (r, s) as mod-N scalars
// decodeRSDER parses a canonical DER ECDSA signature into (r, s) as mod-N scalars
// and normalizes s to low-S. ModNScalar reduces mod the curve order, so range
// validation rides on the overflow flag from SetByteSlice.
func decodeRS(der []byte) (r, s secp256k1.ModNScalar, err error) {
func decodeRSDER(der []byte) (r, s secp256k1.ModNScalar, err error) {
var sig struct{ R, S *big.Int }
rest, err := asn1.Unmarshal(der, &sig)
if err != nil {
Expand All @@ -34,10 +34,39 @@ func decodeRS(der []byte) (r, s secp256k1.ModNScalar, err error) {
return r, s, nil
}

// decodeRSCompact parses a raw fixed-width r‖s signature (2×32 bytes)
// into (r, s) as mod-N scalars and normalizes s to low-S.
func decodeRSCompact(rs []byte) (r, s secp256k1.ModNScalar, err error) {
if len(rs) != 64 {
return r, s, fmt.Errorf("ecdsasig: raw signature must be 64 bytes, got %d", len(rs))
}
if r.SetByteSlice(rs[:32]) || s.SetByteSlice(rs[32:]) {
return r, s, fmt.Errorf("ecdsasig: signature r or s is >= curve order")
}
if r.IsZero() || s.IsZero() {
return r, s, fmt.Errorf("ecdsasig: signature r and s must be nonzero")
}
// Low-S: collapse the malleable high half so the result is canonical.
if s.IsOverHalfOrder() {
s.Negate()
}
return r, s, nil
}

// 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) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

for my knowledge, what is the difference here between or when would i want to use RecoverCompact vs RecoverDER? They both seem to be doing the same thing (taking a 64 byte (r||s) signature and recovering the v, returning (r||s||v). I'm assuming the input signature differs in some way but I can't exactly tell how from the comments. (this is just a skill issue question)

r, s, err := decodeRSCompact(rs)
if err != nil {
return nil, err
}
return recoverSig(r, s, digest, pub)
}
Comment on lines +56 to +64

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 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.


// ConsensusSig converts a DER (r,s) signature into the 64-byte r‖s low-S form
// cometbft secp256k1 consensus verification requires.
func ConsensusSig(der []byte) ([]byte, error) {
r, s, err := decodeRS(der)
r, s, err := decodeRSDER(der)
if err != nil {
return nil, err
}
Expand All @@ -48,20 +77,26 @@ func ConsensusSig(der []byte) ([]byte, error) {
return out, nil
}

// RecoverableSig converts a DER (r,s) signature into the 65-byte
// RecoverDER converts a DER (r,s) signature into the 65-byte
// r‖s‖v recoverable form. Since the underlying signer does not return v,
// it is found by trial-recovering pub from the (low-S normalized) signature
// with each candidate.
// An error is returned if neither candidate recovers pub, including the X-overflow case
// (recid 2/3), which the SignerService 0/1-recovery-id protocol cannot carry.
func RecoverableSig(der, digest []byte, pub *secp256k1.PublicKey) ([]byte, error) {
func RecoverDER(der, digest []byte, pub *secp256k1.PublicKey) ([]byte, error) {
if len(digest) != 32 {
return nil, fmt.Errorf("ecdsasig: digest must be 32 bytes, got %d", len(digest))
}
r, s, err := decodeRS(der)
r, s, err := decodeRSDER(der)
if err != nil {
return nil, err
}
return recoverSig(r, s, digest, pub)
}

// recoverSig takes r,s ModNScalar as well as the associated pubkey and
// returns 65 byte r‖s‖v signature with low-S normalized and recover byte set
func recoverSig(r, s secp256k1.ModNScalar, digest []byte, pub *secp256k1.PublicKey) ([]byte, error) {
rb, sb := r.Bytes(), s.Bytes()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
rb, sb := r.Bytes(), s.Bytes()
if len(digest) != 32 {
return nil, fmt.Errorf("ecdsasig: digest must be 32 bytes, got %d", len(digest))
}
rb, sb := r.Bytes(), s.Bytes()


// decred compact form is <27+recid>‖R‖S; isCompressedKey=false ⇒ no +4.
Expand Down
34 changes: 26 additions & 8 deletions signing/ecdsasig/ecdsasig_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,30 @@ func highSDER(t *testing.T, der []byte) []byte {
return out
}

func TestRecoverableSigRecoversPubKey(t *testing.T) {
// compactSig signs digest and returns the raw 64-byte r‖s encoding, standing
// in for what a PKCS#11 token returns from CKM_ECDSA.
func compactSig(t *testing.T, priv *secp256k1.PrivateKey, digest []byte) []byte {
t.Helper()
return ecdsa.SignCompact(priv, digest, true)[1:]
}

// highSCompact returns the signature with s replaced by N-s, producing a
// non-canonical high-S signature.
func highSCompact(t *testing.T, rs []byte) []byte {
t.Helper()
var s secp256k1.ModNScalar
s.SetByteSlice(rs[32:])
s.Negate()
neg := s.Bytes()
return append(append([]byte(nil), rs[:32]...), neg[:]...)
}

func TestRecoverCompactRecoversPubKey(t *testing.T) {
priv := testKey(t)
digest := sha256.Sum256([]byte("eth digest"))
der := derSig(t, priv, digest[:])
rs := compactSig(t, priv, digest[:])

sig, err := RecoverableSig(der, digest[:], priv.PubKey())
sig, err := RecoverCompact(rs, digest[:], priv.PubKey())
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -87,17 +105,17 @@ func TestRecoverableSigRecoversPubKey(t *testing.T) {
}
}

func TestRecoverableSigNormalizesHighS(t *testing.T) {
func TestRecoverCompactNormalizesHighS(t *testing.T) {
priv := testKey(t)
digest := sha256.Sum256([]byte("eth digest"))
low := derSig(t, priv, digest[:])
high := highSDER(t, low)
low := compactSig(t, priv, digest[:])
high := highSCompact(t, low)

a, err := RecoverableSig(low, digest[:], priv.PubKey())
a, err := RecoverCompact(low, digest[:], priv.PubKey())
if err != nil {
t.Fatal(err)
}
b, err := RecoverableSig(high, digest[:], priv.PubKey())
b, err := RecoverCompact(high, digest[:], priv.PubKey())
if err != nil {
t.Fatal(err)
}
Expand Down
38 changes: 36 additions & 2 deletions signing/pkcs11/algo.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import (
"fmt"

"github.com/cosmos/kms/config"
"github.com/cosmos/kms/signing/ecdsasig"
"github.com/decred/dcrd/dcrec/secp256k1/v4"
"github.com/miekg/pkcs11"
)

Expand All @@ -23,7 +25,7 @@ type keyAlgo struct {
name config.Algorithm
mechanism func() []*pkcs11.Mechanism
decodePub func(ckaECPoint []byte) ([]byte, error)
fixSig func(raw []byte) ([]byte, error)
fixSig func(raw, digest, pub []byte) ([]byte, error)
}

// algos is the registry of supported key algorithms, keyed by the config
Expand All @@ -33,10 +35,24 @@ var algos = map[config.Algorithm]keyAlgo{
name: config.AlgoED25519,
mechanism: func() []*pkcs11.Mechanism { return []*pkcs11.Mechanism{pkcs11.NewMechanism(ckmEDDSA, nil)} },
decodePub: decodeEd25519Pub,
fixSig: func(raw []byte) ([]byte, error) { return raw, nil },
fixSig: func(raw, digest, pub []byte) ([]byte, error) { return raw, nil },
},
config.AlgoSecp256k1Eth: {
name: config.AlgoSecp256k1Eth,
mechanism: func() []*pkcs11.Mechanism { return []*pkcs11.Mechanism{pkcs11.NewMechanism(pkcs11.CKM_ECDSA, nil)} },
decodePub: decodeSecp256k1Pub,
fixSig: recoverSig,
},
}

func recoverSig(raw, digest, pub []byte) ([]byte, error) {
dpub, err := secp256k1.ParsePubKey(pub)
if err != nil {
return nil, fmt.Errorf("parse secp256k1 public key: %w", err)
}
return ecdsasig.RecoverCompact(raw, digest, dpub)
}

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 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!

// PKCS#11 v3.0 encodes the point as a DER OCTET STRING wrapping the 32-byte key
// (0x04 0x20 <32 bytes>); some tokens return the raw 32 bytes. Both are accepted.
Expand All @@ -51,3 +67,21 @@ func decodeEd25519Pub(ckaECPoint []byte) ([]byte, error) {
}
return raw, nil
}

// decodeSecp256k1Pub turns a CKA_EC_POINT value into the 33-byte compressed
// public key. PKCS#11 encodes the point as a DER OCTET STRING wrapping the
// SEC1 point (0x04‖X‖Y) either uncompressed or compressed. Both are accepted.
func decodeSecp256k1Pub(ckaECPoint []byte) ([]byte, error) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

probably want some tests for this

raw := ckaECPoint
// DER OCTET STRING (0x04) of length 65 uncompressed, or 33 compressed)
// wrapping the SEC1 point. A bare uncompressed point also starts with x04

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
// wrapping the SEC1 point. A bare uncompressed point also starts with x04
// wrapping the SEC1 point. A bare uncompressed point also starts with 0x04

// but is 65 bytes, never 67 or 35.
if wrapped := len(raw) - 2; (wrapped == 65 || wrapped == 33) && raw[0] == 0x04 && int(raw[1]) == wrapped {
raw = raw[2:]
}
pub, err := secp256k1.ParsePubKey(raw)
if err != nil {
return nil, fmt.Errorf("secp256k1 CKA_EC_POINT: %w", err)
}
return pub.SerializeCompressed(), nil
}
2 changes: 1 addition & 1 deletion signing/pkcs11/algo_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func TestEd25519DecodePub_BadLength(t *testing.T) {

func TestEd25519FixSig_Identity(t *testing.T) {
sig := []byte("a-64-byte-ed25519-signature-placeholder-value-for-testing-only!!")
out, err := algos["ed25519"].fixSig(sig)
out, err := algos["ed25519"].fixSig(sig, nil, nil)
require.NoError(t, err)
require.Equal(t, sig, out)
}
139 changes: 0 additions & 139 deletions signing/pkcs11/pkcs11.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,11 @@
package pkcs11

import (
"context"
"fmt"
"os"
"strings"
"sync"

"github.com/cosmos/kms/config"
"github.com/cosmos/kms/signing"
"github.com/miekg/pkcs11"
)

Expand All @@ -32,103 +29,6 @@ type Config struct {
Algorithm config.Algorithm
}

// Signer signs on a PKCS#11 token. It owns a single
// long-lived session; the mutex serializes signing (PKCS#11 sessions are not
// safe for concurrent use) and guards Close.
type Signer struct {
mod *pkcs11.Ctx
module string // module path, used to release the shared context on Close
session pkcs11.SessionHandle
privH pkcs11.ObjectHandle
pub []byte
algo keyAlgo

mu sync.Mutex
closed bool
}

var _ signing.Signer = (*Signer)(nil)

// Open loads the PKCS#11 module, logs into the selected token, locates the key,
// and caches its public key. Any failure is returned (fatal at startup for the
// chain). On success the returned Signer holds an open, logged-in session that
// must be released with Close.
func Open(cfg Config) (s *Signer, err error) {
algoName := cfg.Algorithm
if algoName == "" {
algoName = config.AlgoED25519
}
algo, ok := algos[algoName]
if !ok {
return nil, fmt.Errorf("pkcs11: unknown algorithm %q", algoName)
}

pin, err := resolvePIN(cfg)
if err != nil {
return nil, err
}

mod, err := acquireModule(cfg.Module)
if err != nil {
return nil, err
}
// Release our module reference on any error past this point.
defer func() {
if err != nil {
releaseModule(cfg.Module)
}
}()

slot, err := selectSlot(mod, cfg)
if err != nil {
return nil, err
}

session, err := mod.OpenSession(slot, pkcs11.CKF_SERIAL_SESSION)
if err != nil {
return nil, fmt.Errorf("pkcs11: open session on slot %d: %w", slot, err)
}
defer func() {
if err != nil {
_ = mod.CloseSession(session)
}
}()

// Login is per-application (shared across sessions on a slot): a concurrent
// signer on the same token may already hold the login, which is fine.
if err = mod.Login(session, pkcs11.CKU_USER, pin); err != nil {
if ce, ok := err.(pkcs11.Error); !ok || ce != pkcs11.CKR_USER_ALREADY_LOGGED_IN {
return nil, fmt.Errorf("pkcs11: login: %w", err)
}
err = nil
}

privH, err := findObject(mod, session, pkcs11.CKO_PRIVATE_KEY, cfg)
if err != nil {
return nil, fmt.Errorf("pkcs11: find private key: %w", err)
}
pubH, err := findObject(mod, session, pkcs11.CKO_PUBLIC_KEY, cfg)
if err != nil {
return nil, fmt.Errorf("pkcs11: find public key: %w", err)
}

attrs, err := mod.GetAttributeValue(session, pubH, []*pkcs11.Attribute{
pkcs11.NewAttribute(pkcs11.CKA_EC_POINT, nil),
})
if err != nil {
return nil, fmt.Errorf("pkcs11: read public key: %w", err)
}
if len(attrs) == 0 {
return nil, fmt.Errorf("pkcs11: public key has no CKA_EC_POINT")
}
pub, err := algo.decodePub(attrs[0].Value)
if err != nil {
return nil, fmt.Errorf("pkcs11: decode public key: %w", err)
}

return &Signer{mod: mod, module: cfg.Module, session: session, privH: privH, pub: pub, algo: algo}, nil
}

// selectSlot returns the slot to use: the explicit Slot when set, otherwise the
// slot whose token CKA_LABEL matches TokenLabel.
func selectSlot(mod *pkcs11.Ctx, cfg Config) (uint, error) {
Expand Down Expand Up @@ -194,45 +94,6 @@ func keySelector(cfg Config) string {
}
}

// PubKey returns the public key cached at Open.
func (s *Signer) PubKey() []byte { return s.pub }

// Scheme returns the config.Algorithm.
func (s *Signer) Scheme() config.Algorithm { return s.algo.name }

// Sign signs the canonical consensus sign-bytes on the token.
func (s *Signer) Sign(_ context.Context, signBytes []byte) ([]byte, error) {
s.mu.Lock()
defer s.mu.Unlock()
if s.closed {
return nil, fmt.Errorf("pkcs11: signer is closed")
}
if err := s.mod.SignInit(s.session, s.algo.mechanism(), s.privH); err != nil {
return nil, fmt.Errorf("pkcs11: sign init: %w", err)
}
raw, err := s.mod.Sign(s.session, signBytes)
if err != nil {
return nil, fmt.Errorf("pkcs11: sign: %w", err)
}
return s.algo.fixSig(raw)
}

// Close logs out, closes the session, and tears down the module. It is
// idempotent.
func (s *Signer) Close() error {
s.mu.Lock()
defer s.mu.Unlock()
if s.closed {
return nil
}
s.closed = true
_ = s.mod.CloseSession(s.session)
// releaseModule finalizes and unloads the module once the last signer using
// it has closed (Finalize tears down login state and sessions).
releaseModule(s.module)
return nil
}

// resolvePIN returns the user PIN from whichever source the config specifies.
// The PIN is read at open time (not stored in config files): an env var is read
// from the process environment; a file is read and stripped of trailing
Expand Down
Loading