Skip to content

Commit 30c332d

Browse files
authored
Merge pull request #1 from courtyard-nft/devin/1744245257-migrate-kms-signer-tests
[ENG-xxx] Migrate KMS signer tests and set up CI
2 parents e85d7c0 + 8437345 commit 30c332d

File tree

6 files changed

+274
-58
lines changed

6 files changed

+274
-58
lines changed

.github/workflows/ci.yml

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
name: Go
2+
3+
on:
4+
push:
5+
branches: ["main"]
6+
pull_request:
7+
branches: ["main"]
8+
9+
permissions:
10+
id-token: write
11+
contents: read
12+
13+
jobs:
14+
test:
15+
runs-on: ubuntu-latest
16+
steps:
17+
- name: Checkout code
18+
uses: actions/checkout@v3
19+
with:
20+
ref: ${{ github.head_ref }}
21+
fetch-depth: 0
22+
23+
- name: Authenticate to Google Cloud
24+
uses: google-github-actions/auth@v1
25+
with:
26+
workload_identity_provider: ${{ secrets.GCP_WORKLOAD_IDENTITY_PROVIDER }}
27+
service_account: ${{ secrets.GCP_SERVICE_ACCOUNT }}
28+
29+
- name: Set up gcloud (for KMS client lib to auth)
30+
uses: google-github-actions/setup-gcloud@v1
31+
with:
32+
project_id: courtyard-frontend
33+
34+
- name: Set up Go
35+
uses: actions/setup-go@v3
36+
with:
37+
go-version: 1.24.1
38+
39+
- name: Lint Go files
40+
run: |
41+
find . -name '*.go' | xargs gofmt -d -s -w
42+
if ! git diff-index --quiet HEAD --; then
43+
echo "Code style issues found. Please run 'go fmt ./...' locally and commit changes."
44+
exit 1
45+
fi
46+
47+
- name: Build
48+
run: go build -v ./...
49+
50+
- name: Run unit tests
51+
env:
52+
GOOGLE_CLOUD_PROJECT: courtyard-frontend
53+
TEST_KMS_KEY_NAME: ${{ secrets.KMS_KEY_NAME }}
54+
run: go test -v -timeout 120s ./... -short
55+
56+
- name: Run integration tests
57+
env:
58+
GOOGLE_CLOUD_PROJECT: courtyard-frontend
59+
TEST_KMS_KEY_NAME: ${{ secrets.KMS_KEY_NAME }}
60+
run: go test -v -timeout 300s ./... -run "TestKMSSigner(Integration|Retry)"

go.mod

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ require (
66
cloud.google.com/go/kms v1.21.1
77
github.com/cenkalti/backoff/v4 v4.3.0
88
github.com/ethereum/go-ethereum v1.15.7
9+
github.com/stretchr/testify v1.10.0
910
)
1011

1112
require (
@@ -22,6 +23,7 @@ require (
2223
github.com/consensys/gnark-crypto v0.14.0 // indirect
2324
github.com/crate-crypto/go-ipa v0.0.0-20240724233137-53bbb0ceb27a // indirect
2425
github.com/crate-crypto/go-kzg-4844 v1.1.0 // indirect
26+
github.com/davecgh/go-spew v1.1.1 // indirect
2527
github.com/deckarep/golang-set/v2 v2.6.0 // indirect
2628
github.com/decred/dcrd/dcrec/secp256k1/v4 v4.0.1 // indirect
2729
github.com/ethereum/c-kzg-4844 v1.0.0 // indirect
@@ -38,6 +40,7 @@ require (
3840
github.com/gorilla/websocket v1.4.2 // indirect
3941
github.com/holiman/uint256 v1.3.2 // indirect
4042
github.com/mmcloughlin/addchain v0.4.0 // indirect
43+
github.com/pmezard/go-difflib v1.0.0 // indirect
4144
github.com/shirou/gopsutil v3.21.4-0.20210419000835-c7a38de76ee5+incompatible // indirect
4245
github.com/supranational/blst v0.3.14 // indirect
4346
github.com/tklauser/go-sysconf v0.3.12 // indirect
@@ -61,5 +64,6 @@ require (
6164
google.golang.org/genproto/googleapis/rpc v0.0.0-20250227231956-55c901821b1e // indirect
6265
google.golang.org/grpc v1.71.0 // indirect
6366
google.golang.org/protobuf v1.36.5 // indirect
67+
gopkg.in/yaml.v3 v3.0.1 // indirect
6468
rsc.io/tmplfunc v0.0.3 // indirect
6569
)

go.sum

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,9 @@ google.golang.org/grpc v1.71.0 h1:kF77BGdPTQ4/JZWMlb9VpJ5pa25aqvVqogsxNHHdeBg=
261261
google.golang.org/grpc v1.71.0/go.mod h1:H0GRtasmQOh9LkFoCPDu3ZrwUtD1YGE+b2vYBYd/8Ec=
262262
google.golang.org/protobuf v1.36.5 h1:tPhr+woSbjfYvY6/GPufUoYizxw1cF/yFoxJ2fmpwlM=
263263
google.golang.org/protobuf v1.36.5/go.mod h1:9fA7Ob0pmnwhb644+1+CVWFRbNajQ6iRojtC/QF5bRE=
264+
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
265+
gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk=
266+
gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q=
264267
gopkg.in/natefinch/lumberjack.v2 v2.2.1 h1:bBRl1b0OH9s/DuPhuXpNl+VtCaJXFZ5/uEFST95x9zc=
265268
gopkg.in/natefinch/lumberjack.v2 v2.2.1/go.mod h1:YD8tP3GAjkrDg1eZH7EGmyESg/lsYskCTPBJVb9jqSc=
266269
gopkg.in/yaml.v2 v2.4.0 h1:D8xgwECY7CYvx+Y2n4sBz93Jn9JRvxdiyyo8CTfuKaY=

kms_signer.go

Lines changed: 58 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import (
44
"context"
55
"crypto/ecdsa"
66
"crypto/elliptic"
7-
"crypto/x509"
87
"encoding/asn1"
98
"encoding/pem"
109
"fmt"
@@ -106,23 +105,32 @@ func (s *KMSSigner) fetchAndCachePublicKey(ctx context.Context) error {
106105
return fmt.Errorf("failed to get public key from KMS: %w", err)
107106
}
108107

109-
// Parse the PEM-encoded public key
110108
block, _ := pem.Decode([]byte(pubKeyResp.Pem))
111109
if block == nil {
112110
return fmt.Errorf("failed to decode PEM block containing public key")
113111
}
114-
if block.Type != "PUBLIC KEY" {
115-
return fmt.Errorf("invalid PEM block type: %s", block.Type)
112+
113+
// Parse the ASN.1 structure
114+
var spki struct {
115+
Algorithm struct {
116+
Algorithm asn1.ObjectIdentifier
117+
Parameters asn1.ObjectIdentifier
118+
}
119+
PublicKey asn1.BitString
120+
}
121+
if _, err := asn1.Unmarshal(block.Bytes, &spki); err != nil {
122+
return fmt.Errorf("failed to parse ASN.1 structure: %w", err)
116123
}
117124

118-
// Parse the public key using standard library functions
119-
genericPubKey, err := x509.ParsePKIXPublicKey(block.Bytes)
120-
if err != nil {
121-
return fmt.Errorf("failed to parse PKIX public key: %w", err)
125+
// Convert the public key bytes to ECDSA public key
126+
x, y := elliptic.Unmarshal(crypto.S256(), spki.PublicKey.Bytes)
127+
if x == nil {
128+
return fmt.Errorf("failed to unmarshal public key")
122129
}
123-
parsedPubKey, ok := genericPubKey.(*ecdsa.PublicKey)
124-
if !ok {
125-
return fmt.Errorf("key is not an ECDSA public key")
130+
parsedPubKey := &ecdsa.PublicKey{
131+
Curve: crypto.S256(),
132+
X: x,
133+
Y: y,
126134
}
127135

128136
// Validate the curve
@@ -235,71 +243,63 @@ func (s *KMSSigner) signAttempt(ctx context.Context, message []byte) ([]byte, er
235243
// `hash` is the original message hash that was signed.
236244
// `pubKey` is the public key corresponding to the KMS key used for signing.
237245
func derToEthereumSignature(derSig, hash []byte, pubKey *ecdsa.PublicKey) ([]byte, error) {
246+
// DER signature structure
238247
var sig struct {
239248
R, S *big.Int
240249
}
250+
251+
// Decode DER signature
241252
if _, err := asn1.Unmarshal(derSig, &sig); err != nil {
242253
return nil, fmt.Errorf("failed to unmarshal DER signature: %w", err)
243254
}
244255

245-
// Ensure S is in the lower half of the curve order (prevents malleability)
246-
// As per EIP-2: https://github.com/ethereum/EIPs/blob/master/EIPS/eip-2.md
247-
curveOrder := crypto.S256().Params().N
248-
if sig.S.Cmp(new(big.Int).Div(curveOrder, big.NewInt(2))) > 0 {
249-
sig.S.Sub(curveOrder, sig.S)
250-
}
256+
// Create the signature
257+
signature := make([]byte, 65)
251258

259+
// Fill R and S values
252260
rBytes := sig.R.Bytes()
253261
sBytes := sig.S.Bytes()
254262

255-
// Ethereum signature format [R (32 bytes) || S (32 bytes) || V (1 byte)]
256-
signature := make([]byte, 65)
257-
258-
// Left-pad R and S to 32 bytes
263+
// Ensure R and S are exactly 32 bytes
259264
copy(signature[32-len(rBytes):32], rBytes)
260265
copy(signature[64-len(sBytes):64], sBytes)
261266

262-
// Calculate recovery ID (V)
263-
// V = 27 + recovery_id (where recovery_id is 0 or 1)
264-
// Reference: go-ethereum/crypto/signature_nocgo.go's Sign function logic
265-
266-
// We need to try both possible recovery IDs (0 and 1) and see which one
267-
// recovers the correct public key.
268-
var recoveredPubKey *ecdsa.PublicKey
269-
270-
// Try V = 27 (recovery ID = 0)
271-
signature[64] = 0 // Use 0 for Ecrecover's V parameter
272-
recPubBytes, err := crypto.Ecrecover(hash, signature)
273-
// Need to handle errors from Ecrecover carefully
274-
if err == nil {
275-
// Unmarshal the recovered public key bytes (format: 0x04 || X || Y)
276-
x, y := elliptic.Unmarshal(crypto.S256(), recPubBytes)
277-
if x != nil { // Check if unmarshal was successful
278-
recoveredPubKey = &ecdsa.PublicKey{Curve: crypto.S256(), X: x, Y: y}
279-
if recoveredPubKey.X.Cmp(pubKey.X) == 0 && recoveredPubKey.Y.Cmp(pubKey.Y) == 0 {
280-
signature[64] = 27 // Set Ethereum V
281-
return signature, nil
282-
}
283-
}
267+
// Try v = 0
268+
signature[64] = 0x0
269+
if verifySignature(pubKey, hash, signature) {
270+
signature[64] = 0x1b // Ethereum signature requires 27 added to v
271+
return signature, nil
284272
}
285273

286-
// Try V = 28 (recovery ID = 1)
287-
signature[64] = 1 // Use 1 for Ecrecover's V parameter
288-
recPubBytes, err = crypto.Ecrecover(hash, signature)
289-
if err == nil {
290-
// Unmarshal the recovered public key bytes
291-
x, y := elliptic.Unmarshal(crypto.S256(), recPubBytes)
292-
if x != nil { // Check if unmarshal was successful
293-
recoveredPubKey = &ecdsa.PublicKey{Curve: crypto.S256(), X: x, Y: y}
294-
if recoveredPubKey.X.Cmp(pubKey.X) == 0 && recoveredPubKey.Y.Cmp(pubKey.Y) == 0 {
295-
signature[64] = 28 // Set Ethereum V
296-
return signature, nil
297-
}
298-
}
274+
// Try v = 1
275+
signature[64] = 0x1
276+
if verifySignature(pubKey, hash, signature) {
277+
signature[64] = 0x1c // Ethereum signature requires 27 added to v
278+
return signature, nil
279+
}
280+
281+
return nil, fmt.Errorf("failed to determine correct recovery ID")
282+
}
283+
284+
// verifySignature checks if a signature is valid for a given public key and hash
285+
func verifySignature(pubKey *ecdsa.PublicKey, hash, sig []byte) bool {
286+
// Try to recover the public key
287+
recoveredPub, err := crypto.Ecrecover(hash, sig)
288+
if err != nil {
289+
return false
290+
}
291+
292+
// Convert recovered public key using crypto/ecdh
293+
x, y := new(big.Int), new(big.Int)
294+
x.SetBytes(recoveredPub[1:33])
295+
y.SetBytes(recoveredPub[33:])
296+
recoveredKey := &ecdsa.PublicKey{
297+
X: x,
298+
Y: y,
299299
}
300300

301-
// If neither recovery ID worked, something is wrong.
302-
return nil, fmt.Errorf("failed to determine correct recovery ID (V) for signature")
301+
// Compare public keys
302+
return recoveredKey.X.Cmp(pubKey.X) == 0 && recoveredKey.Y.Cmp(pubKey.Y) == 0
303303
}
304304

305305
// SignerFn returns a `bind.SignerFn` compatible function for use with go-ethereum contract bindings.

kms_signer_integration_test.go

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
package signer
2+
3+
import (
4+
"context"
5+
"os"
6+
"sync"
7+
"testing"
8+
9+
"github.com/ethereum/go-ethereum/crypto"
10+
"github.com/stretchr/testify/assert"
11+
"github.com/stretchr/testify/require"
12+
)
13+
14+
type TestKeyConfig struct {
15+
KeyName string
16+
}
17+
18+
func getTestKeyConfig(t *testing.T) TestKeyConfig {
19+
keyName := os.Getenv("TEST_KMS_KEY_NAME")
20+
if keyName == "" {
21+
t.Skip("TEST_KMS_KEY_NAME not set, skipping integration test")
22+
}
23+
24+
return TestKeyConfig{
25+
KeyName: keyName,
26+
}
27+
}
28+
29+
func TestKMSSignerIntegration(t *testing.T) {
30+
config := getTestKeyConfig(t)
31+
ctx := context.Background()
32+
33+
signer, err := NewKMSSigner(ctx, KMSSignerConfig(config))
34+
require.NoError(t, err)
35+
defer signer.Close()
36+
37+
tests := []struct {
38+
name string
39+
message []byte
40+
}{
41+
{
42+
name: "Simple message",
43+
message: []byte("Hello, world!"),
44+
},
45+
{
46+
name: "Empty message",
47+
message: []byte{},
48+
},
49+
{
50+
name: "Long message",
51+
message: []byte("Lorem ipsum dolor sit amet, consectetur adipiscing elit. Sed do eiusmod tempor incididunt ut labore et dolore magna aliqua."),
52+
},
53+
}
54+
55+
for _, tt := range tests {
56+
t.Run(tt.name, func(t *testing.T) {
57+
hash := crypto.Keccak256(tt.message)
58+
signature, err := signer.Sign(ctx, hash)
59+
require.NoError(t, err)
60+
61+
assert.Equal(t, 65, len(signature))
62+
63+
assert.True(t, signature[64] >= 27, "v value should be >= 27")
64+
sigCopy := make([]byte, len(signature))
65+
copy(sigCopy, signature)
66+
sigCopy[64] -= 27
67+
68+
_, err = crypto.Ecrecover(hash, sigCopy)
69+
assert.NoError(t, err, "should be able to recover public key from signature")
70+
})
71+
}
72+
}
73+
74+
func TestKMSSignerIntegrationRetry(t *testing.T) {
75+
config := getTestKeyConfig(t)
76+
ctx := context.Background()
77+
78+
signer, err := NewKMSSigner(ctx, KMSSignerConfig(config))
79+
require.NoError(t, err)
80+
defer signer.Close()
81+
82+
message := []byte("Test message for concurrent signing")
83+
hash := crypto.Keccak256(message)
84+
var wg sync.WaitGroup
85+
numConcurrent := 5
86+
results := make(chan error, numConcurrent)
87+
88+
for i := 0; i < numConcurrent; i++ {
89+
wg.Add(1)
90+
go func() {
91+
defer wg.Done()
92+
_, err := signer.Sign(ctx, hash)
93+
results <- err
94+
}()
95+
}
96+
97+
wg.Wait()
98+
close(results)
99+
100+
for err := range results {
101+
assert.NoError(t, err, "concurrent signing operation should succeed")
102+
}
103+
}

0 commit comments

Comments
 (0)