Skip to content

Commit 9467819

Browse files
alpealpe
committed
Review feedback
1 parent 737d500 commit 9467819

2 files changed

Lines changed: 191 additions & 7 deletions

File tree

‎pkg/signer/gcp/signer.go‎

Lines changed: 59 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"encoding/pem"
1111
"errors"
1212
"fmt"
13+
"hash/crc32"
1314
"net"
1415
"sync"
1516
"time"
@@ -20,6 +21,14 @@ import (
2021
"google.golang.org/api/option"
2122
"google.golang.org/grpc/codes"
2223
"google.golang.org/grpc/status"
24+
"google.golang.org/protobuf/types/known/wrapperspb"
25+
)
26+
27+
var castagnoliTable = crc32.MakeTable(crc32.Castagnoli)
28+
29+
const (
30+
baseRetryBackoff = 100 * time.Millisecond
31+
maxRetryBackoff = 5 * time.Second
2332
)
2433

2534
// KMSClient is the subset of the Google Cloud KMS client API that KmsSigner needs.
@@ -173,8 +182,8 @@ func (s *KmsSigner) Sign(ctx context.Context, message []byte) ([]byte, error) {
173182

174183
for attempt := 0; attempt < maxAttempts; attempt++ {
175184
if attempt > 0 {
176-
// Exponential backoff: 100ms, 200ms, 400ms, ...
177-
backoff := time.Duration(100<<uint(attempt-1)) * time.Millisecond
185+
// Exponential backoff with cap: 100ms, 200ms, 400ms, ... up to 5s.
186+
backoff := retryBackoff(attempt)
178187
select {
179188
case <-ctx.Done():
180189
return nil, fmt.Errorf("KMS Sign canceled: %w", ctx.Err())
@@ -183,9 +192,11 @@ func (s *KmsSigner) Sign(ctx context.Context, message []byte) ([]byte, error) {
183192
}
184193

185194
callCtx, cancel := context.WithTimeout(ctx, timeout)
195+
dataCRC32C := int64(crc32.Checksum(message, castagnoliTable))
186196
out, err := s.client.AsymmetricSign(callCtx, &kmspb.AsymmetricSignRequest{
187-
Name: s.keyName,
188-
Data: message,
197+
Name: s.keyName,
198+
Data: message,
199+
DataCrc32C: wrapperspb.Int64(dataCRC32C),
189200
})
190201
cancel()
191202

@@ -197,12 +208,56 @@ func (s *KmsSigner) Sign(ctx context.Context, message []byte) ([]byte, error) {
197208
continue
198209
}
199210

211+
if err := verifySignResponse(out); err != nil {
212+
lastErr = err
213+
continue
214+
}
215+
200216
return out.GetSignature(), nil
201217
}
202218

203219
return nil, fmt.Errorf("KMS Sign failed after %d attempts: %w", maxAttempts, lastErr)
204220
}
205221

222+
func retryBackoff(attempt int) time.Duration {
223+
if attempt <= 1 {
224+
return baseRetryBackoff
225+
}
226+
227+
backoff := baseRetryBackoff
228+
for i := 1; i < attempt; i++ {
229+
if backoff >= maxRetryBackoff/2 {
230+
return maxRetryBackoff
231+
}
232+
backoff *= 2
233+
}
234+
235+
if backoff > maxRetryBackoff {
236+
return maxRetryBackoff
237+
}
238+
239+
return backoff
240+
}
241+
242+
func verifySignResponse(out *kmspb.AsymmetricSignResponse) error {
243+
if !out.GetVerifiedDataCrc32C() {
244+
return fmt.Errorf("KMS Sign integrity check failed: verified_data_crc32c is false")
245+
}
246+
247+
signatureCRC32C := out.GetSignatureCrc32C()
248+
if signatureCRC32C == nil {
249+
return fmt.Errorf("KMS Sign integrity check failed: signature_crc32c is missing")
250+
}
251+
252+
signature := out.GetSignature()
253+
expectedCRC32C := int64(crc32.Checksum(signature, castagnoliTable))
254+
if signatureCRC32C.GetValue() != expectedCRC32C {
255+
return fmt.Errorf("KMS Sign integrity check failed: signature_crc32c mismatch")
256+
}
257+
258+
return nil
259+
}
260+
206261
// GetPublic returns the cached public key.
207262
func (s *KmsSigner) GetPublic() (crypto.PubKey, error) {
208263
s.mu.RLock()

‎pkg/signer/gcp/signer_test.go‎

Lines changed: 132 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,14 +6,17 @@ import (
66
"crypto/x509"
77
"encoding/pem"
88
"fmt"
9+
"hash/crc32"
910
"sync/atomic"
1011
"testing"
12+
"time"
1113

1214
"cloud.google.com/go/kms/apiv1/kmspb"
1315
"github.com/stretchr/testify/assert"
1416
"github.com/stretchr/testify/require"
1517
"google.golang.org/grpc/codes"
1618
"google.golang.org/grpc/status"
19+
"google.golang.org/protobuf/types/known/wrapperspb"
1720
)
1821

1922
// mockKMSClient is a test double implementing KMSClient.
@@ -98,19 +101,26 @@ func TestSign_Success(t *testing.T) {
98101
_, publicKeyPEM := generateTestEd25519PEM(t)
99102

100103
expectedSig := []byte("test-signature-bytes")
104+
expectedMsg := []byte("hello world")
101105
mock := &mockKMSClient{
102106
publicKeyPEM: publicKeyPEM,
103107
signFn: func(_ context.Context, req *kmspb.AsymmetricSignRequest) (*kmspb.AsymmetricSignResponse, error) {
104108
assert.Equal(t, "projects/p/locations/global/keyRings/r/cryptoKeys/k/cryptoKeyVersions/1", req.Name)
105-
assert.Equal(t, []byte("hello world"), req.Data)
106-
return &kmspb.AsymmetricSignResponse{Signature: expectedSig}, nil
109+
assert.Equal(t, expectedMsg, req.Data)
110+
require.NotNil(t, req.DataCrc32C)
111+
assert.Equal(t, int64(crc32.Checksum(expectedMsg, castagnoliTable)), req.DataCrc32C.GetValue())
112+
return &kmspb.AsymmetricSignResponse{
113+
Signature: expectedSig,
114+
VerifiedDataCrc32C: true,
115+
SignatureCrc32C: wrapperspb.Int64(int64(crc32.Checksum(expectedSig, castagnoliTable))),
116+
}, nil
107117
},
108118
}
109119

110120
s, err := kmsSignerFromClient(context.Background(), mock, "projects/p/locations/global/keyRings/r/cryptoKeys/k/cryptoKeyVersions/1", nil)
111121
require.NoError(t, err)
112122

113-
sig, err := s.Sign(context.Background(), []byte("hello world"))
123+
sig, err := s.Sign(context.Background(), expectedMsg)
114124
require.NoError(t, err)
115125
assert.Equal(t, expectedSig, sig)
116126
}
@@ -177,6 +187,125 @@ func TestSign_NonRetryableError_NoRetries(t *testing.T) {
177187
assert.Equal(t, int32(1), atomic.LoadInt32(&calls), "non-retryable errors should fail fast")
178188
}
179189

190+
func TestSign_IntegrityCheckVerifiedDataFalse_RetriesAndFails(t *testing.T) {
191+
_, publicKeyPEM := generateTestEd25519PEM(t)
192+
193+
var calls int32
194+
mock := &mockKMSClient{
195+
publicKeyPEM: publicKeyPEM,
196+
signFn: func(_ context.Context, _ *kmspb.AsymmetricSignRequest) (*kmspb.AsymmetricSignResponse, error) {
197+
atomic.AddInt32(&calls, 1)
198+
sig := []byte("sig")
199+
return &kmspb.AsymmetricSignResponse{
200+
Signature: sig,
201+
VerifiedDataCrc32C: false,
202+
SignatureCrc32C: wrapperspb.Int64(int64(crc32.Checksum(sig, castagnoliTable))),
203+
}, nil
204+
},
205+
}
206+
207+
s, err := kmsSignerFromClient(
208+
context.Background(),
209+
mock,
210+
"projects/p/locations/global/keyRings/r/cryptoKeys/k/cryptoKeyVersions/1",
211+
&Options{MaxRetries: 1},
212+
)
213+
require.NoError(t, err)
214+
215+
_, err = s.Sign(context.Background(), []byte("hello world"))
216+
require.Error(t, err)
217+
assert.Contains(t, err.Error(), "verified_data_crc32c is false")
218+
assert.Equal(t, int32(2), atomic.LoadInt32(&calls), "integrity failures should be retried")
219+
}
220+
221+
func TestSign_IntegrityCheckSignatureCRC32CMismatch_RetriesAndFails(t *testing.T) {
222+
_, publicKeyPEM := generateTestEd25519PEM(t)
223+
224+
var calls int32
225+
mock := &mockKMSClient{
226+
publicKeyPEM: publicKeyPEM,
227+
signFn: func(_ context.Context, _ *kmspb.AsymmetricSignRequest) (*kmspb.AsymmetricSignResponse, error) {
228+
atomic.AddInt32(&calls, 1)
229+
return &kmspb.AsymmetricSignResponse{
230+
Signature: []byte("sig"),
231+
VerifiedDataCrc32C: true,
232+
SignatureCrc32C: wrapperspb.Int64(12345),
233+
}, nil
234+
},
235+
}
236+
237+
s, err := kmsSignerFromClient(
238+
context.Background(),
239+
mock,
240+
"projects/p/locations/global/keyRings/r/cryptoKeys/k/cryptoKeyVersions/1",
241+
&Options{MaxRetries: 1},
242+
)
243+
require.NoError(t, err)
244+
245+
_, err = s.Sign(context.Background(), []byte("hello world"))
246+
require.Error(t, err)
247+
assert.Contains(t, err.Error(), "signature_crc32c mismatch")
248+
assert.Equal(t, int32(2), atomic.LoadInt32(&calls), "integrity failures should be retried")
249+
}
250+
251+
func TestSign_IntegrityCheckRecoversOnRetry(t *testing.T) {
252+
_, publicKeyPEM := generateTestEd25519PEM(t)
253+
254+
var calls int32
255+
expectedSig := []byte("valid-signature")
256+
mock := &mockKMSClient{
257+
publicKeyPEM: publicKeyPEM,
258+
signFn: func(_ context.Context, _ *kmspb.AsymmetricSignRequest) (*kmspb.AsymmetricSignResponse, error) {
259+
attempt := atomic.AddInt32(&calls, 1)
260+
if attempt == 1 {
261+
return &kmspb.AsymmetricSignResponse{
262+
Signature: []byte("corrupted"),
263+
VerifiedDataCrc32C: false,
264+
SignatureCrc32C: wrapperspb.Int64(1),
265+
}, nil
266+
}
267+
return &kmspb.AsymmetricSignResponse{
268+
Signature: expectedSig,
269+
VerifiedDataCrc32C: true,
270+
SignatureCrc32C: wrapperspb.Int64(int64(crc32.Checksum(expectedSig, castagnoliTable))),
271+
}, nil
272+
},
273+
}
274+
275+
s, err := kmsSignerFromClient(
276+
context.Background(),
277+
mock,
278+
"projects/p/locations/global/keyRings/r/cryptoKeys/k/cryptoKeyVersions/1",
279+
&Options{MaxRetries: 2},
280+
)
281+
require.NoError(t, err)
282+
283+
got, err := s.Sign(context.Background(), []byte("hello world"))
284+
require.NoError(t, err)
285+
assert.Equal(t, expectedSig, got)
286+
assert.Equal(t, int32(2), atomic.LoadInt32(&calls), "second attempt should succeed")
287+
}
288+
289+
func TestRetryBackoff_Capped(t *testing.T) {
290+
testCases := []struct {
291+
name string
292+
attempt int
293+
expected time.Duration
294+
}{
295+
{name: "attempt 1", attempt: 1, expected: 100 * time.Millisecond},
296+
{name: "attempt 2", attempt: 2, expected: 200 * time.Millisecond},
297+
{name: "attempt 6", attempt: 6, expected: 3200 * time.Millisecond},
298+
{name: "attempt 7 capped", attempt: 7, expected: 5 * time.Second},
299+
{name: "attempt 10 capped", attempt: 10, expected: 5 * time.Second},
300+
}
301+
302+
for _, tc := range testCases {
303+
t.Run(tc.name, func(t *testing.T) {
304+
assert.Equal(t, tc.expected, retryBackoff(tc.attempt))
305+
})
306+
}
307+
}
308+
180309
func TestGetPublic_Cached(t *testing.T) {
181310
pub, publicKeyPEM := generateTestEd25519PEM(t)
182311

0 commit comments

Comments
 (0)