chore: upgrade crypto library to use newer methods from Go's stdlib #230

Copilot · 2025-10-17T20:15:24Z

Overview

This PR modernizes the ECDSA crypto implementation to use the recommended ASN.1 variants of Sign and Verify functions from Go's standard library, replacing the legacy implementation that exposed r and s as separate *big.Int parameters.

Motivation

The current implementation uses the legacy ecdsa.Sign() and ecdsa.Verify() functions which are no longer the recommended practice. As noted in the Go crypto/ecdsa documentation, the ASN.1 variants (SignASN1 and VerifyASN1) should be used for new code as they provide a more standard representation of ECDSA signatures.

Changes

Core Implementation (`security/crypto/ecdsa.go`)

Before:

type ECDSASignature struct {
	r, s   *big.Int
	signer hotstuff.ID
}

r, s, err := ecdsa.Sign(rand.Reader, ec.privateKey(), hash[:])
if !ecdsa.Verify(pk, hash[:], sig.R(), sig.S()) { ... }

After:

type ECDSASignature struct {
	sig    []byte
	signer hotstuff.ID
}

sig, err := ecdsa.SignASN1(rand.Reader, ec.privateKey(), hash[:])
if !ecdsa.VerifyASN1(pk, hash[:], sig.sig) { ... }

Key Updates

Internal representation: Signatures are now stored as ASN.1-encoded byte slices instead of separate r and s values
Signing: Uses ecdsa.SignASN1() which returns ASN.1 DER-encoded signatures
Verification: Uses ecdsa.VerifyASN1() which accepts ASN.1 DER-encoded signatures
Simplified API: RestoreECDSASignature(sig []byte, signer hotstuff.ID) now accepts ASN.1-encoded bytes directly
Protobuf wire format: Updated to use a single Sig bytes field instead of separate R and S fields, matching the pattern used by EDDSA

Protobuf Changes (`internal/proto/hotstuffpb/hotstuff.proto`)

Before:

message ECDSASignature {
  uint32 Signer = 1;
  bytes R = 2;
  bytes S = 3;
}

After:

message ECDSASignature {
  uint32 Signer = 1;
  bytes Sig = 2;
}

Signatures now go on the wire as raw ASN.1-encoded []byte, simplifying the implementation and matching the pattern used by other signature types.

Comprehensive Test Suite (`security/crypto/ecdsa_test.go`)

Added table-driven tests following best practices, covering:

Sign and verify operations with various message types (empty, long, binary, multi-replica)
Failure scenarios with tampered messages
Signature combining across multiple replicas
Batch verification with different and duplicate messages
Signature restoration from ASN.1-encoded bytes
ASN.1 encoding verification
Multiple elliptic curves (P256, P384, P521)
Signature randomization properties
Performance benchmarks

Compatibility

✅ All existing tests pass - Including security/cert tests that depend on ECDSA signatures.

✅ Security verified - CodeQL analysis reports 0 alerts.

Note: The protobuf wire format has changed to use a single Sig field instead of separate R and S fields. This is a wire format change that requires all nodes to be upgraded together.

Testing

# Run ECDSA tests
go test -v ./security/crypto/... -run TestECDSA

# Run all tests
go test -short ./...

# Run security analysis
codeql analyze

All tests pass successfully with the new implementation.

Statistics

Lines added: ~700 (tests and implementation changes)
Lines removed: ~600 (legacy implementation and protobuf code)
Test coverage: 9 test cases with 30+ subtests
Security alerts: 0

Fixes #229

Original prompt

This section details on the original issue you should resolve

<issue_title>chore: upgrade crypto library to use newer methods from Go's stdlib</issue_title>
<issue_description>We currently use the legacy implementation of the stdlib's ecdsa, which exposes the r and s big.Int parameters.
type ECDSASignature struct {
	r, s   *big.Int
	signer hotstuff.ID
}
This is no longer recommended practice. We should overhaul our ecdsa crypto implementation to use the more modern ecdsa package from the stdlib. Specifically, we should use the ASN1 variants of the Sign and Verify functions.

We should also add some table-driven tests for the crypto logic.
</issue_description>

<agent_instructions>Follow best-practice table-driven tests. Ask me if you have questions or suggestions for related, but unspecified, improvements. </agent_instructions>

Comments on the Issue (you are @copilot in this section)

Fixes #229

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Co-authored-by: meling <[email protected]>

meling

@copilot I've not reviewed the tests yet, but update them accordingly based on my comments.

meling · 2025-10-17T22:02:16Z

security/crypto/ecdsa.go

 )

+// ecdsaASN1Sig represents the ASN.1 structure of an ECDSA signature
+type ecdsaASN1Sig struct {


@copilot I don't think we need this. We do not store the signatures in a specific format. They go on the wire are a []byte

Removed the ecdsaASN1Sig struct. Now signatures go on the wire as []byte directly in the protobuf. (commit: 5e7c8b9)

meling · 2025-10-17T22:02:25Z

security/crypto/ecdsa.go

 // ECDSASignature is an ECDSA signature.
 type ECDSASignature struct {
-	r, s   *big.Int
+	sig    []byte // ASN.1 encoded signature


Remove the comment.

Removed the comment. (commit: 5e7c8b9)

meling · 2025-10-17T22:04:44Z

security/crypto/ecdsa.go


 // RestoreECDSASignature restores an existing signature.
 // It should not be used to create new signatures, use Sign instead.
 func RestoreECDSASignature(r, s *big.Int, signer hotstuff.ID) *ECDSASignature {


We should change the signature to remove the 'r' and 's' arguments; instead, we should pass in the []byte we receive from the network. See the QuorumSignatureFromProto function in internal/hotstuffpb/convert.go

Changed the signature to RestoreECDSASignature(sig []byte, signer hotstuff.ID) and updated the QuorumSignatureFromProto function to pass the []byte received from the network. (commit: 5e7c8b9)

meling · 2025-10-17T22:05:14Z

security/crypto/ecdsa.go

 // It should not be used to create new signatures, use Sign instead.
 func RestoreECDSASignature(r, s *big.Int, signer hotstuff.ID) *ECDSASignature {
-	return &ECDSASignature{r, s, signer}
+	// Encode r and s as ASN.1 for internal storage


Remove marshalling logic.

Removed the marshalling logic. (commit: 5e7c8b9)

meling · 2025-10-17T22:05:59Z

security/crypto/ecdsa.go


 // R returns the r value of the signature.
+// This method is primarily used for protobuf serialization and decodes the ASN.1 signature on each call.
 func (sig ECDSASignature) R() *big.Int {


Remove this method.

Removed the R() method. (commit: 5e7c8b9)

meling · 2025-10-17T22:06:06Z

security/crypto/ecdsa.go


 // S returns the s value of the signature.
+// This method is primarily used for protobuf serialization and decodes the ASN.1 signature on each call.
 func (sig ECDSASignature) S() *big.Int {


Remove this method.

Removed the S() method. (commit: 5e7c8b9)

Co-authored-by: meling <[email protected]>

Copilot · 2025-10-17T22:22:33Z

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

esm.ubuntu.com
- Triggering command: /usr/lib/apt/methods/https (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Configure Actions setup steps to set up my environment, which run before the firewall is enabled
Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)

…ary-implementation

Creating a Multi signature for ecdsa we now need to do this: crypto.NewMulti(restoredSig) instead of this: crypto.Multi[*crypto.ECDSASignature]{signer: restoredSig}

Ran the modernizer to clean up some quick fixes.

Instead of manually initilizaing a Multi[T] type to create a new signature, we use the NewMulti constructor instead.

Initial plan

5e9f2ef

Copilot AI assigned Copilot and meling Oct 17, 2025

Copilot started work on behalf of meling October 17, 2025 20:15 View session

Copilot AI and others added 3 commits October 17, 2025 20:31

Upgrade ECDSA to use modern ASN.1 methods and add comprehensive tests

8674212

Co-authored-by: meling <[email protected]>

Address code review feedback - add documentation and clarifications

b85b693

Co-authored-by: meling <[email protected]>

Final verification - all tests pass and security checks complete

8f753a5

Co-authored-by: meling <[email protected]>

Copilot AI changed the title ~~[WIP] Update crypto library to use newer ecdsa methods~~ chore: upgrade crypto library to use newer methods from Go's stdlib Oct 17, 2025

Copilot AI requested a review from meling October 17, 2025 20:38

Copilot finished work on behalf of meling October 17, 2025 20:38

meling requested changes Oct 17, 2025

View reviewed changes

Copilot started work on behalf of meling October 17, 2025 22:09 View session

Address review feedback: simplify ECDSA signature to use raw ASN.1 bytes

de93c22

Co-authored-by: meling <[email protected]>

Copilot AI requested a review from meling October 17, 2025 22:22

Copilot finished work on behalf of meling October 17, 2025 22:22

meling mentioned this pull request Oct 19, 2025

Replace map-based Multi signature with slice-based version for better performance #231

Merged

meling added 4 commits October 19, 2025 12:18

Merge branch 'package-restructuring' into copilot/upgrade-crypto-libr…

1a002ca

…ary-implementation

fix(ecdsa_test): adjust Multi signature construction after merge

cc492ad

Creating a Multi signature for ecdsa we now need to do this: crypto.NewMulti(restoredSig) instead of this: crypto.Multi[*crypto.ECDSASignature]{signer: restoredSig}

chore(crypto): use for range loops and b.Loop()

b2e81dc

Ran the modernizer to clean up some quick fixes.

refactor(ecdsa, eddsa): use NewMulti to create new signature

9241421

Instead of manually initilizaing a Multi[T] type to create a new signature, we use the NewMulti constructor instead.

chore: upgrade crypto library to use newer methods from Go's stdlib #230

Are you sure you want to change the base?

chore: upgrade crypto library to use newer methods from Go's stdlib #230

Conversation

Copilot AI commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Motivation

Changes

Core Implementation (security/crypto/ecdsa.go)

Key Updates

Protobuf Changes (internal/proto/hotstuffpb/hotstuff.proto)

Comprehensive Test Suite (security/crypto/ecdsa_test.go)

Compatibility

Testing

Statistics

Comments on the Issue (you are @copilot in this section)

Uh oh!

meling left a comment

Choose a reason for hiding this comment

Uh oh!

meling Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

meling Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

meling Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

meling Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

meling Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

meling Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented Oct 17, 2025

I tried to connect to the following addresses, but was blocked by firewall rules:

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Copilot AI commented Oct 17, 2025 •

edited

Loading

Core Implementation (`security/crypto/ecdsa.go`)

Protobuf Changes (`internal/proto/hotstuffpb/hotstuff.proto`)

Comprehensive Test Suite (`security/crypto/ecdsa_test.go`)