Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded KMS-backed signer support (AWS & GCP), made Signer.Sign context-aware, introduced a signer factory, wired KMS signers into init/run and block/DA signing flows, added docs/tests/E2E, and updated dependencies and many call sites to propagate context. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App (init/run)
participant Ctx as Cmd Context
participant Factory as Signer Factory
participant Config as Config Validator
participant KMS as KMS Service
participant Signer as Signer Impl
App->>Ctx: cmd.Context()
App->>Factory: NewSignerForInit(ctx, config, passphrase)
Factory->>Config: validate signer type & KMS config
alt validation error
Config-->>Factory: error
Factory-->>App: error
else file signer
Factory->>Signer: Create/load FileSystemSigner(...)
Signer-->>Factory: FileSystemSigner
else KMS signer
Factory->>KMS: NewKmsSigner(ctx, key, opts)
KMS->>KMS: load creds & client
KMS->>KMS: GetPublicKey(ctx) -> parse & cache
KMS-->>Factory: KmsSigner
end
Factory-->>App: signer
sequenceDiagram
participant Executor as Block Executor
participant Signer as Signer (File/AWS/GCP)
participant KMS as KMS Service
participant Cache as PubKey Cache
Executor->>Executor: ProduceBlock(ctx)
Executor->>Signer: Sign(ctx, headerBytes)
alt File signer
Signer->>Signer: local ed25519.Sign(...)
Signer-->>Executor: signature
else KMS signer
Signer->>Signer: check ctx.Err()
Signer->>KMS: Sign(ctx, message) with timeout
alt retryable error
KMS->>KMS: exponential backoff & retry
else non-retryable
KMS-->>Signer: error
end
KMS-->>Signer: signature
Signer->>Cache: return cached pubKey
Signer-->>Executor: signature
end
Executor->>Executor: continue block flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @alpe's task in 2m 33s —— View job Code Review: feat: Add KMS signer backend
This is a solid, well-structured PR. The architecture is clean, context propagation is thorough, and the fail-fast public key loading is a nice pattern. Several issues remain from previous review rounds and a few new ones need attention. Critical / Must-Fix1. Default timeout mismatch: 1s vs 10s (both AWS and GCP)
// aws/signer.go:87 — should be 10s to match docs
o := Options{Timeout: 1 * time.Second, MaxRetries: 3}2. GCP
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3171 +/- ##
==========================================
+ Coverage 61.14% 61.45% +0.31%
==========================================
Files 117 120 +3
Lines 12082 12431 +349
==========================================
+ Hits 7387 7640 +253
- Misses 3868 3938 +70
- Partials 827 853 +26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
types/utils_test.go (1)
82-82: Consider usingt.Context()for consistency.While
context.Background()works correctly here, other test files in this PR uset.Context()which is more idiomatic for tests. It automatically cancels when the test completes, providing better cleanup semantics.🔧 Suggested change
- firstSignedHeader, err := types.GetFirstSignedHeader(context.Background(), noopSigner, tc.chainID) + firstSignedHeader, err := types.GetFirstSignedHeader(t.Context(), noopSigner, tc.chainID)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@types/utils_test.go` at line 82, Replace the call to context.Background() with the test's cancellable context by using t.Context() when invoking types.GetFirstSignedHeader; update the specific invocation firstSignedHeader, err := types.GetFirstSignedHeader(context.Background(), noopSigner, tc.chainID) to pass t.Context() instead so the test uses the per-test context and cancels automatically when the test finishes.pkg/cmd/init_test.go (1)
49-50: Line 49 now overstates the scenario.This subtest is specifically exercising an unknown signer type (
remote), not non-file signers in general. Renaming the case description will keep the test intent aligned with what it actually covers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/init_test.go` around lines 49 - 50, Update the test case description to accurately reflect that this subtest exercises an unknown "remote" signer type: change the comment and/or header text that currently reads "Case 3: Non-File signer, Aggregator -> Error (unknown signer type)" to something like "Case 3: Remote signer, Aggregator -> Error (unknown signer type)" so the t.Run("RemoteSigner_Aggregator", ...) intent matches the comment.pkg/cmd/run_node.go (1)
140-142: Consider enhancing AWS KMS log with key ID for observability.Including the key ID (or a truncated version) in the log message would help operators verify which KMS key is in use during startup.
🔧 Optional enhancement
if nodeConfig.Signer.SignerType == "awskms" { - logger.Info().Msg("initialized AWS KMS signer via factory") + logger.Info(). + Str("kms_key_id", nodeConfig.Signer.KmsKeyID). + Msg("initialized AWS KMS signer via factory") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/run_node.go` around lines 140 - 142, The AWS KMS startup log only notes the signer type; update the block that checks nodeConfig.Signer.SignerType == "awskms" to also read the KMS key identifier from the signer config (e.g., nodeConfig.Signer.KeyID or nodeConfig.Signer.AwsKmsKeyID), truncate it for safety (for example to the first 8 characters) and include that truncated key id in the logger.Info() message so operators can see which KMS key was used while avoiding full secret exposure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/config/config.go`:
- Around line 305-306: The SignerType struct tag comment currently lists "(file,
awskms)" but the CLI flag description includes "(file, grpc, awskms)"; make them
consistent by either adding "grpc" to the SignerType field comment (SignerType
string `mapstructure:"signer_type" yaml:"signer_type" comment:"Type of remote
signer to use (file, grpc, awskms)"`) or by removing "grpc" from the CLI flag
help text if grpc is not supported—update the comment associated with SignerType
(and SignerPath documentation if needed) or the flag description in the CLI
registration so both list the same valid signer types.
In `@pkg/signer/aws/README.md`:
- Around line 3-15: Update the README for the awskms signer to add explicit IAM
permissions and KMS key configuration required: state that the IAM principal
needs kms:GetPublicKey and kms:Sign on the configured key, and that the KMS key
must be created with KeyUsage=SIGN_VERIFY and KeySpec=ECC_NIST_EDWARDS25519;
mention that the implementation of signer.Signer calls GetPublicKey eagerly
during initialization (and fails fast on misconfiguration) and uses Sign for
signing operations so those permissions/settings are mandatory for successful
initialization and runtime signing.
In `@pkg/signer/aws/signer.go`:
- Around line 31-37: The documented default Timeout in the Options struct (10s)
is inconsistent with the fallback used in kmsSignerFromClient (1s); update the
implementation to use a single default (e.g., defaultTimeout = 10 * time.Second)
and ensure kmsSignerFromClient and any other fallback logic use that constant
when opts is nil or opts.Timeout <= 0; modify the fallback for MaxRetries
similarly if needed and verify all uses (notably kmsSignerFromClient and the
related sign/constructor paths) reference the same default constants instead of
hardcoded values.
- Around line 207-216: GetAddress currently returns the backing slice s.address
from KmsSigner allowing callers to mutate cached state after the RLock is
released; while holding the read lock (in KmsSigner.GetAddress) allocate a new
byte slice, copy the contents of s.address into it, and return that copy instead
of the original slice (keep the existing nil check and error path, but ensure
the copy is created before releasing s.mu.RUnlock so callers cannot corrupt the
signer's cached identity).
In `@pkg/signer/factory.go`:
- Around line 33-35: Reject empty signer_path in the signer factory before
calling filepath.Abs: retrieve the raw value used (config.Signer.SignerPath or
the trimmed value from strings.TrimSuffix(config.Signer.SignerPath,
"signer.json")), check if it is empty (after trimming) and return a clear error
instead of calling filepath.Abs; only call filepath.Abs when the trimmed signer
path is non-empty and then continue with existing resolution logic that assigns
to signerPath. Ensure the validation happens in the same initialization area
where signerPath is computed (the code using filepath.Abs and signerPath) so
misconfiguration cannot fall back to the process CWD.
In `@types/utils.go`:
- Around line 192-198: GetRandomNextSignedHeader is building
newSignedHeader.Header via GetRandomNextHeader but then calls GetSignature with
the old signedHeader.Header, producing a signature that won't verify; change the
call to GetSignature(ctx, newSignedHeader.Header, signer), assign the returned
signature to newSignedHeader.Signature (or the appropriate field on
SignedHeader), and preserve the existing error check/return so the function
returns the newly built SignedHeader with a signature over its own Header.
---
Nitpick comments:
In `@pkg/cmd/init_test.go`:
- Around line 49-50: Update the test case description to accurately reflect that
this subtest exercises an unknown "remote" signer type: change the comment
and/or header text that currently reads "Case 3: Non-File signer, Aggregator ->
Error (unknown signer type)" to something like "Case 3: Remote signer,
Aggregator -> Error (unknown signer type)" so the
t.Run("RemoteSigner_Aggregator", ...) intent matches the comment.
In `@pkg/cmd/run_node.go`:
- Around line 140-142: The AWS KMS startup log only notes the signer type;
update the block that checks nodeConfig.Signer.SignerType == "awskms" to also
read the KMS key identifier from the signer config (e.g.,
nodeConfig.Signer.KeyID or nodeConfig.Signer.AwsKmsKeyID), truncate it for
safety (for example to the first 8 characters) and include that truncated key id
in the logger.Info() message so operators can see which KMS key was used while
avoiding full secret exposure.
In `@types/utils_test.go`:
- Line 82: Replace the call to context.Background() with the test's cancellable
context by using t.Context() when invoking types.GetFirstSignedHeader; update
the specific invocation firstSignedHeader, err :=
types.GetFirstSignedHeader(context.Background(), noopSigner, tc.chainID) to pass
t.Context() instead so the test uses the per-test context and cancels
automatically when the test finishes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a074a34e-fd54-4bb5-acf1-846e6f9e5871
⛔ Files ignored due to path filters (6)
apps/evm/go.sumis excluded by!**/*.sumapps/grpc/go.sumis excluded by!**/*.sumapps/testapp/go.sumis excluded by!**/*.sumexecution/evm/test/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumtest/e2e/go.sumis excluded by!**/*.sum
📒 Files selected for processing (45)
CHANGELOG.mdapps/evm/cmd/init.goapps/evm/go.modapps/grpc/cmd/init.goapps/grpc/go.modapps/testapp/cmd/init.goapps/testapp/go.modblock/internal/executing/executor.goblock/internal/submitting/da_submitter.goblock/internal/submitting/da_submitter_integration_test.goblock/internal/submitting/da_submitter_test.goblock/internal/submitting/submitter_test.goblock/internal/syncing/da_retriever_strict_test.goblock/internal/syncing/da_retriever_test.goblock/internal/syncing/p2p_handler_test.goblock/internal/syncing/syncer_test.godocs/.vitepress/config.tsdocs/guides/operations/aws-kms-signer.mdgo.modpkg/cmd/init.gopkg/cmd/init_test.gopkg/cmd/run_node.gopkg/config/config.gopkg/config/config_test.gopkg/config/defaults.gopkg/signer/aws/README.mdpkg/signer/aws/signer.gopkg/signer/aws/signer_test.gopkg/signer/factory.gopkg/signer/factory_test.gopkg/signer/file/README.mdpkg/signer/file/doc.gopkg/signer/file/example_test.gopkg/signer/file/file_signer_test.gopkg/signer/file/local.gopkg/signer/noop/signer.gopkg/signer/noop/signer_test.gopkg/signer/signer.gopkg/store/store_adapter_test.gopkg/sync/sync_service_test.gotest/e2e/evm_aws_kms_e2e_test.gotest/e2e/go.modtypes/signed_header_test.gotypes/utils.gotypes/utils_test.go
| This package implements `signer.Signer` using AWS KMS. | ||
|
|
||
| It uses KMS for `Sign` operations and caches the public key/address in memory after initialization. | ||
|
|
||
| ## Requirements | ||
|
|
||
| - AWS credentials must be available via the standard AWS SDK credential chain. | ||
| - The configured KMS key must be an asymmetric **Ed25519** key. | ||
|
|
||
| ## Configuration | ||
|
|
||
| Set `evnode.signer.signer_type` to `awskms` and provide at least `kms_key_id`. | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd pkg/signer/aws && find . -type f -name "*.md" -o -name "*.go" | head -20Repository: evstack/ev-node
Length of output: 99
🏁 Script executed:
cat -n pkg/signer/aws/README.mdRepository: evstack/ev-node
Length of output: 1086
🏁 Script executed:
cd pkg/signer/aws && ls -laRepository: evstack/ev-node
Length of output: 352
🏁 Script executed:
cd pkg/signer/aws && wc -l *.go | tail -1Repository: evstack/ev-node
Length of output: 70
🏁 Script executed:
cat -n pkg/signer/aws/signer.goRepository: evstack/ev-node
Length of output: 8302
🏁 Script executed:
cat -n pkg/signer/aws/signer_test.go | head -100Repository: evstack/ev-node
Length of output: 3771
🌐 Web query:
AWS KMS Ed25519 key creation KeyUsage SIGN_VERIFY KeySpec ECC_NIST_EDWARDS25519
💡 Result:
Yes, AWS KMS supports creating an asymmetric key with KeySpec 'ECC_NIST_EDWARDS25519' (Ed25519 on Edwards25519 curve) and KeyUsage 'SIGN_VERIFY'. This feature was announced in November 2025 and is available as of 2026-03-19. To create such a key, use the CreateKey API or AWS CLI command: aws kms create-key \ --key-spec ECC_NIST_EDWARDS25519 \ --key-usage SIGN_VERIFY This creates a customer-managed KMS key for signing and verification using EdDSA (Ed25519). The key supports signing algorithms like ED25519_SHA_512 (requires MessageType: RAW) and ED25519_PH_SHA_512 (requires MessageType: DIGEST). It cannot be used for encryption/decryption or key agreement. KeySpec 'ECC_NIST_EDWARDS25519' is listed in official AWS KMS documentation for asymmetric NIST-standard elliptic curve keys, valid only for SIGN_VERIFY usage.
Citations:
- 1: https://aws.amazon.com/about-aws/whats-new/2025/11/aws-kms-edwards-curve-digital-signature-algorithm/#:~:text=AWS%20Key%20Management%20Service%20(KMS,the%20Edwards25519%20curve%20(Ed25519).
- 2: https://docs.aws.amazon.com/kms/latest/APIReference/API_CreateKey.html
- 3: https://docs.aws.amazon.com/kms/latest/developerguide/symm-asymm-choose-key-spec.html
- 4: https://docs.aws.amazon.com/kms/latest/APIReference/API_KeyMetadata.html
- 5: https://sdk.amazonaws.com/kotlin/api/latest/kms/aws.sdk.kotlin.services.kms.model/-create-key-request/-builder/key-spec.html
- 6: https://docs.aws.amazon.com/cli/latest/reference/kms/create-key.html
- 7: https://docs.aws.amazon.com/kms/latest/developerguide/example_kms_CreateKey_section.html
Add KMS permission and key configuration requirements to the README.
The README should explicitly document the IAM permissions and KMS key settings required for this signer to function. Without these details, users following the setup instructions will encounter failures during signer initialization.
Add to the Requirements section:
- IAM permissions:
kms:GetPublicKeyandkms:Signon the specified KMS key - KMS key configuration: created with
KeyUsage=SIGN_VERIFYandKeySpec=ECC_NIST_EDWARDS25519
The code calls GetPublicKey eagerly during initialization (failing fast on misconfiguration) and uses Sign for all signing operations, both of which require these permissions and key settings to succeed. Users without these configured will see initialization errors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/signer/aws/README.md` around lines 3 - 15, Update the README for the
awskms signer to add explicit IAM permissions and KMS key configuration
required: state that the IAM principal needs kms:GetPublicKey and kms:Sign on
the configured key, and that the KMS key must be created with
KeyUsage=SIGN_VERIFY and KeySpec=ECC_NIST_EDWARDS25519; mention that the
implementation of signer.Signer calls GetPublicKey eagerly during initialization
(and fails fast on misconfiguration) and uses Sign for signing operations so
those permissions/settings are mandatory for successful initialization and
runtime signing.
| // Options configures optional KmsSigner behaviour. | ||
| type Options struct { | ||
| // Timeout for individual KMS Sign API calls. Default: 10s. | ||
| Timeout time.Duration | ||
| // MaxRetries for transient KMS failures during Sign. Default: 3. | ||
| MaxRetries int | ||
| } |
There was a problem hiding this comment.
Align the implementation with the documented default timeout.
Options says the default sign timeout is 10s, but kmsSignerFromClient falls back to 1s when opts is nil or Timeout <= 0. That makes the exported constructor much more aggressive than advertised.
Proposed fix
- o := Options{Timeout: 1 * time.Second, MaxRetries: 3}
+ o := Options{Timeout: 10 * time.Second, MaxRetries: 3}Also applies to: 87-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/signer/aws/signer.go` around lines 31 - 37, The documented default
Timeout in the Options struct (10s) is inconsistent with the fallback used in
kmsSignerFromClient (1s); update the implementation to use a single default
(e.g., defaultTimeout = 10 * time.Second) and ensure kmsSignerFromClient and any
other fallback logic use that constant when opts is nil or opts.Timeout <= 0;
modify the fallback for MaxRetries similarly if needed and verify all uses
(notably kmsSignerFromClient and the related sign/constructor paths) reference
the same default constants instead of hardcoded values.
pkg/signer/factory.go
Outdated
| // Resolve signer path; allow absolute, relative to node root, or relative to CWD if resolution fails | ||
| signerPath, err := filepath.Abs(strings.TrimSuffix(config.Signer.SignerPath, "signer.json")) | ||
| if err != nil { |
There was a problem hiding this comment.
Reject an empty signer_path before calling filepath.Abs.
filepath.Abs("") resolves to the process working directory, so a misconfigured file signer can silently create/load ./signer.json from wherever the node was started. That's a risky fallback for key material.
Proposed fix
- signerPath, err := filepath.Abs(strings.TrimSuffix(config.Signer.SignerPath, "signer.json"))
+ signerPath := strings.TrimSpace(config.Signer.SignerPath)
+ if signerPath == "" {
+ return nil, fmt.Errorf("signer_path is required when using local file signer")
+ }
+ signerPath = strings.TrimSuffix(signerPath, "signer.json")
+ if signerPath == "" {
+ signerPath = "."
+ }
+ signerPath, err := filepath.Abs(filepath.Clean(signerPath))
if err != nil {
return nil, err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Resolve signer path; allow absolute, relative to node root, or relative to CWD if resolution fails | |
| signerPath, err := filepath.Abs(strings.TrimSuffix(config.Signer.SignerPath, "signer.json")) | |
| if err != nil { | |
| // Resolve signer path; allow absolute, relative to node root, or relative to CWD if resolution fails | |
| signerPath := strings.TrimSpace(config.Signer.SignerPath) | |
| if signerPath == "" { | |
| return nil, fmt.Errorf("signer_path is required when using local file signer") | |
| } | |
| signerPath = strings.TrimSuffix(signerPath, "signer.json") | |
| if signerPath == "" { | |
| signerPath = "." | |
| } | |
| signerPath, err := filepath.Abs(filepath.Clean(signerPath)) | |
| if err != nil { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/signer/factory.go` around lines 33 - 35, Reject empty signer_path in the
signer factory before calling filepath.Abs: retrieve the raw value used
(config.Signer.SignerPath or the trimmed value from
strings.TrimSuffix(config.Signer.SignerPath, "signer.json")), check if it is
empty (after trimming) and return a clear error instead of calling filepath.Abs;
only call filepath.Abs when the trimmed signer path is non-empty and then
continue with existing resolution logic that assigns to signerPath. Ensure the
validation happens in the same initialization area where signerPath is computed
(the code using filepath.Abs and signerPath) so misconfiguration cannot fall
back to the process CWD.
* Add remote signer GCO KMS * Review feedback * Minor updates
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
docs/guides/operations/aws-kms-signer.md (1)
12-33: Add an explicit least-privilege warning for the example IAM policy.The example policy is broad (
Resource: "*") and includes key-management actions; please label it as bootstrap/admin-only and recommend scoped runtime permissions (already partially covered in Line 35–38) to reduce accidental over-permissioning in production.Also applies to: 35-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guides/operations/aws-kms-signer.md` around lines 12 - 33, Add a clear least-privilege warning above the example IAM policy JSON that currently uses "Resource": "*" and includes key-management actions (e.g., "kms:CreateKey", "kms:PutKeyPolicy", "kms:EnableKey", "kms:Sign"); label this policy as bootstrap/admin-only and explicitly state it must not be used in production, then reference and recommend the scoped runtime permissions described later (the smaller set in the docs) as the safe alternative and suggest replacing wildcard Resource with specific KMS key ARNs for production.test/e2e/evm_kms_e2e_test.go (1)
32-32: Callingflag.Parse()multiple times can cause issues
flag.Parse()is called in bothTestEvmSequencerWithAWSKMSSignerE2E(line 32) andTestEvmSequencerWithGCPKMSSignerE2E(line 72). When both tests run in the same test binary execution, this can cause problems as flags are already parsed. The Go testing framework already handles flag parsing before tests run.♻️ Remove redundant flag.Parse() calls
func TestEvmSequencerWithAWSKMSSignerE2E(t *testing.T) { if testing.Short() { t.Skip("skip e2e in short mode") } - flag.Parse() kmsKeyID := os.Getenv("EVNODE_E2E_AWS_KMS_KEY_ID")func TestEvmSequencerWithGCPKMSSignerE2E(t *testing.T) { if testing.Short() { t.Skip("skip e2e in short mode") } - flag.Parse() kmsKeyName := os.Getenv("EVNODE_E2E_GCP_KMS_KEY_NAME")Also applies to: 72-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/evm_kms_e2e_test.go` at line 32, The tests call flag.Parse() redundantly in TestEvmSequencerWithAWSKMSSignerE2E and TestEvmSequencerWithGCPKMSSignerE2E which can break when the test binary parses flags once; remove the flag.Parse() calls from both test functions (search for TestEvmSequencerWithAWSKMSSignerE2E and TestEvmSequencerWithGCPKMSSignerE2E) so the standard Go test harness handles flag parsing, and run go test to confirm no other tests rely on manual flag parsing.pkg/signer/gcp/signer.go (2)
262-272: Inconsistent mutex pattern between GetPublic and GetAddress
GetPublicmanually unlocks before the nil check whileGetAddressusesdefer s.mu.RUnlock(). For consistency and safety, consider usingdeferin both methods.♻️ Use defer for consistency
func (s *KmsSigner) GetPublic() (crypto.PubKey, error) { s.mu.RLock() - pubKey := s.pubKey - s.mu.RUnlock() + defer s.mu.RUnlock() + pubKey := s.pubKey if pubKey == nil { return nil, fmt.Errorf("public key not loaded") } return pubKey, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/signer/gcp/signer.go` around lines 262 - 272, GetPublic currently acquires a read lock and manually unlocks before checking pubKey; change it to mirror GetAddress by calling s.mu.RLock() then immediately deferring s.mu.RUnlock(), assign pubKey := s.pubKey, and perform the nil check after the defer so the lock-unlock pattern is consistent and safe (update the GetPublic method accordingly to use defer s.mu.RUnlock()).
194-201: Consider usingdefer cancel()for the per-call timeout contextThe
cancel()function is called immediately after the API call, but if there's a panic between context creation and the cancel call, the context could leak. Usingdeferis the idiomatic pattern.♻️ Use defer for cancel()
callCtx, cancel := context.WithTimeout(ctx, timeout) + defer cancel() dataCRC32C := int64(crc32.Checksum(message, castagnoliTable)) out, err := s.client.AsymmetricSign(callCtx, &kmspb.AsymmetricSignRequest{ Name: s.keyName, Data: message, DataCrc32C: wrapperspb.Int64(dataCRC32C), }) - cancel()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/signer/gcp/signer.go` around lines 194 - 201, The context cancel function created by callCtx, cancel := context.WithTimeout(ctx, timeout) is invoked immediately with cancel() which can leak the context on panic; after creating callCtx and cancel, replace the immediate cancel() call with defer cancel() so the context is always cancelled when the surrounding function returns (keep the call to s.client.AsymmetricSign using callCtx and the DataCrc32C wrapper as-is).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/evm/go.mod`:
- Line 239: Update the grpc dependency entry to google.golang.org/grpc v1.79.3
in the go.mod (replace the existing google.golang.org/grpc v1.79.2 // indirect
line), then run go mod tidy (or go get google.golang.org/grpc@v1.79.3) to update
the lockfiles and verify module downloads; this addresses the CVE by ensuring
the project uses the patched grpc version.
In `@docs/guides/operations/aws-kms-signer.md`:
- Around line 72-75: Remove the misleading expected startup log "initialized AWS
KMS signer via factory" from the docs and either delete that sentence or replace
it with the actual log emitted by the AWS signer initialization path; update the
guide text that references the signer factory so it reflects the real behavior
of the AWS signer initialization path (see aws signer implementation in
pkg/signer/aws/signer.go and the factory behavior in pkg/signer/factory.go) so
operators are not told to expect a nonexistent log line.
In `@go.mod`:
- Line 48: The go.mod currently pins the vulnerable module
"google.golang.org/grpc v1.79.2"; update that dependency to a patched gRPC
release that fixes GHSA-p77j-4mvh-x3m3 (e.g., upgrade to a patched version >=
v1.79.3 or the latest patched release), run "go get
google.golang.org/grpc@<patched-version>" and "go mod tidy", run tests, and
ensure any other references (such as the duplicate pin in apps/evm/go.mod) are
likewise updated and no replace directives or transitive dependencies re-pin the
vulnerable v1.79.2.
---
Nitpick comments:
In `@docs/guides/operations/aws-kms-signer.md`:
- Around line 12-33: Add a clear least-privilege warning above the example IAM
policy JSON that currently uses "Resource": "*" and includes key-management
actions (e.g., "kms:CreateKey", "kms:PutKeyPolicy", "kms:EnableKey",
"kms:Sign"); label this policy as bootstrap/admin-only and explicitly state it
must not be used in production, then reference and recommend the scoped runtime
permissions described later (the smaller set in the docs) as the safe
alternative and suggest replacing wildcard Resource with specific KMS key ARNs
for production.
In `@pkg/signer/gcp/signer.go`:
- Around line 262-272: GetPublic currently acquires a read lock and manually
unlocks before checking pubKey; change it to mirror GetAddress by calling
s.mu.RLock() then immediately deferring s.mu.RUnlock(), assign pubKey :=
s.pubKey, and perform the nil check after the defer so the lock-unlock pattern
is consistent and safe (update the GetPublic method accordingly to use defer
s.mu.RUnlock()).
- Around line 194-201: The context cancel function created by callCtx, cancel :=
context.WithTimeout(ctx, timeout) is invoked immediately with cancel() which can
leak the context on panic; after creating callCtx and cancel, replace the
immediate cancel() call with defer cancel() so the context is always cancelled
when the surrounding function returns (keep the call to s.client.AsymmetricSign
using callCtx and the DataCrc32C wrapper as-is).
In `@test/e2e/evm_kms_e2e_test.go`:
- Line 32: The tests call flag.Parse() redundantly in
TestEvmSequencerWithAWSKMSSignerE2E and TestEvmSequencerWithGCPKMSSignerE2E
which can break when the test binary parses flags once; remove the flag.Parse()
calls from both test functions (search for TestEvmSequencerWithAWSKMSSignerE2E
and TestEvmSequencerWithGCPKMSSignerE2E) so the standard Go test harness handles
flag parsing, and run go test to confirm no other tests rely on manual flag
parsing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d986867e-d2c9-40c3-968d-ac865e15dbe4
⛔ Files ignored due to path filters (6)
apps/evm/go.sumis excluded by!**/*.sumapps/grpc/go.sumis excluded by!**/*.sumapps/testapp/go.sumis excluded by!**/*.sumexecution/evm/test/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumtest/e2e/go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
apps/evm/go.modapps/grpc/go.modapps/testapp/go.moddocs/guides/operations/aws-kms-signer.mddocs/guides/operations/gcp-kms-signer.mdexecution/evm/test/go.modgo.modpkg/cmd/run_node.gopkg/config/config.gopkg/config/config_test.gopkg/config/defaults.gopkg/signer/aws/README.mdpkg/signer/factory.gopkg/signer/factory_test.gopkg/signer/gcp/README.mdpkg/signer/gcp/signer.gopkg/signer/gcp/signer_test.gotest/e2e/evm_kms_e2e_test.gotest/e2e/go.mod
✅ Files skipped from review due to trivial changes (5)
- execution/evm/test/go.mod
- docs/guides/operations/gcp-kms-signer.md
- pkg/signer/gcp/README.md
- pkg/signer/aws/README.md
- pkg/signer/factory_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- pkg/config/config_test.go
- pkg/config/defaults.go
- apps/grpc/go.mod
- test/e2e/go.mod
- apps/testapp/go.mod
- pkg/signer/factory.go
- pkg/cmd/run_node.go
- pkg/config/config.go
| You should see a startup log line: | ||
|
|
||
| `initialized AWS KMS signer via factory` | ||
|
|
There was a problem hiding this comment.
Remove or replace the expected startup log line; it does not exist in current code paths.
Line 72–75 tells operators to expect initialized AWS KMS signer via factory, but the AWS signer initialization path and signer factory path do not emit that log (pkg/signer/aws/signer.go:52-110, pkg/signer/factory.go:50-77). This creates a false negative during bring-up.
Proposed doc fix
-You should see a startup log line:
-
-`initialized AWS KMS signer via factory`
+Confirm startup succeeded by checking that `evnode` starts without signer initialization errors.
+If startup fails, use the exact errors in the troubleshooting section below.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| You should see a startup log line: | |
| `initialized AWS KMS signer via factory` | |
| Confirm startup succeeded by checking that `evnode` starts without signer initialization errors. | |
| If startup fails, use the exact errors in the troubleshooting section below. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/guides/operations/aws-kms-signer.md` around lines 72 - 75, Remove the
misleading expected startup log "initialized AWS KMS signer via factory" from
the docs and either delete that sentence or replace it with the actual log
emitted by the AWS signer initialization path; update the guide text that
references the signer factory so it reflects the real behavior of the AWS signer
initialization path (see aws signer implementation in pkg/signer/aws/signer.go
and the factory behavior in pkg/signer/factory.go) so operators are not told to
expect a nonexistent log line.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkg/config/config.go (1)
315-315:⚠️ Potential issue | 🟡 MinorMinor inconsistency in signer type documentation persists.
The struct field comment on line 315 lists
(file, kms)but the flag description on line 622 lists(file, grpc, kms). These should be consistent. The previous review flagged a similar inconsistency that was marked as addressed.📝 Proposed fix
Either update the struct comment to include
grpcif it's a valid type:- SignerType string `mapstructure:"signer_type" yaml:"signer_type" comment:"Type of remote signer to use (file, kms)"` + SignerType string `mapstructure:"signer_type" yaml:"signer_type" comment:"Type of remote signer to use (file, grpc, kms)"`Or update the flag description to remove
grpcif it's not supported:- cmd.Flags().String(FlagSignerType, def.Signer.SignerType, "type of signer to use (file, grpc, kms)") + cmd.Flags().String(FlagSignerType, def.Signer.SignerType, "type of signer to use (file, kms)"),
Also applies to: 622-622
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/config.go` at line 315, The SignerType struct field comment on SignerType (`SignerType string 'mapstructure:"signer_type" yaml:"signer_type" comment:"Type of remote signer to use (file, kms)"'`) is inconsistent with the flag description that lists `(file, grpc, kms)`; make them consistent by either adding `grpc` to the struct field comment or removing `grpc` from the flag description (whichever reflects actual supported signer types), and update both the SignerType field comment and the corresponding flag description text so both list the same valid signer types.pkg/signer/aws/signer.go (1)
31-37:⚠️ Potential issue | 🟡 MinorAlign the implementation with the documented default timeout.
Optionsdocuments the default sign timeout as10s(line 33), butkmsSignerFromClientfalls back to1swhenoptsis nil orTimeout <= 0(line 87). This makes the behavior more aggressive than advertised.📝 Proposed fix
- o := Options{Timeout: 1 * time.Second, MaxRetries: 3} + o := Options{Timeout: 10 * time.Second, MaxRetries: 3},
Also applies to: 87-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/signer/aws/signer.go` around lines 31 - 37, The Options struct documents a default Timeout of 10s but kmsSignerFromClient currently falls back to 1s when opts is nil or Timeout <= 0; update kmsSignerFromClient to use 10*time.Second as the fallback/default for Options.Timeout (and keep existing handling for opts == nil and Timeout <= 0), referencing the Options type and the kmsSignerFromClient function so the behavior matches the documented default; also scan the same initialization logic in that function (the block handling opts.Timeout and opts.MaxRetries) to ensure the MaxRetries default remains unchanged or is explicitly set if needed.
🧹 Nitpick comments (1)
pkg/signer/aws/signer.go (1)
164-172: Unbounded exponential backoff may cause excessive delays.The backoff calculation
100<<uint(attempt-1)grows without a cap. ForMaxRetries=3(4 attempts), the delays are 100ms, 200ms, 400ms which is fine. However, if someone configures a higherMaxRetries, attempt 10 would compute ~51 seconds of backoff.The GCP signer implementation (lines 225-243) includes a
maxRetryBackoff = 5 * time.Secondcap. Consider adding the same safeguard here for consistency.📝 Proposed fix
+const maxRetryBackoff = 5 * time.Second + func (s *KmsSigner) Sign(ctx context.Context, message []byte) ([]byte, error) { ... for attempt := 0; attempt < maxAttempts; attempt++ { ... if attempt > 0 { - // Exponential backoff: 100ms, 200ms, 400ms, ... - backoff := time.Duration(100<<uint(attempt-1)) * time.Millisecond + // Exponential backoff with cap: 100ms, 200ms, 400ms, ... up to 5s. + backoff := time.Duration(100<<uint(attempt-1)) * time.Millisecond + if backoff > maxRetryBackoff { + backoff = maxRetryBackoff + } select {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/signer/aws/signer.go` around lines 164 - 172, The exponential backoff in the KMS signing retry loop (where attempt > 0 and backoff is computed as time.Duration(100<<uint(attempt-1))*time.Millisecond) is unbounded and can produce excessive delays for large MaxRetries; update the retry backoff to cap the computed value at a reasonable maximum (e.g. maxRetryBackoff = 5 * time.Second) before using time.After, so compute backoff, if backoff > maxRetryBackoff set backoff = maxRetryBackoff, then use the capped backoff in the select along with the existing ctx cancellation handling in the Sign/KMS retry loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/signer/gcp/signer.go`:
- Around line 53-66: The documented default timeout for Options is 10s but
kmsSignerFromClient currently falls back to 1s; update the logic so the
effective timeout matches the doc: make Options.timeout() return 10*time.Second
when o is nil or o.Timeout <= 0 (or otherwise ensure kmsSignerFromClient uses
that 10s default), and keep maxRetries behavior consistent (use
Options.maxRetries() or a similar default if nil). Update only the timeout
fallback (change 1s -> 10s) by modifying the timeout() helper or the fallback in
kmsSignerFromClient to use 10*time.Second.
---
Duplicate comments:
In `@pkg/config/config.go`:
- Line 315: The SignerType struct field comment on SignerType (`SignerType
string 'mapstructure:"signer_type" yaml:"signer_type" comment:"Type of remote
signer to use (file, kms)"'`) is inconsistent with the flag description that
lists `(file, grpc, kms)`; make them consistent by either adding `grpc` to the
struct field comment or removing `grpc` from the flag description (whichever
reflects actual supported signer types), and update both the SignerType field
comment and the corresponding flag description text so both list the same valid
signer types.
In `@pkg/signer/aws/signer.go`:
- Around line 31-37: The Options struct documents a default Timeout of 10s but
kmsSignerFromClient currently falls back to 1s when opts is nil or Timeout <= 0;
update kmsSignerFromClient to use 10*time.Second as the fallback/default for
Options.Timeout (and keep existing handling for opts == nil and Timeout <= 0),
referencing the Options type and the kmsSignerFromClient function so the
behavior matches the documented default; also scan the same initialization logic
in that function (the block handling opts.Timeout and opts.MaxRetries) to ensure
the MaxRetries default remains unchanged or is explicitly set if needed.
---
Nitpick comments:
In `@pkg/signer/aws/signer.go`:
- Around line 164-172: The exponential backoff in the KMS signing retry loop
(where attempt > 0 and backoff is computed as
time.Duration(100<<uint(attempt-1))*time.Millisecond) is unbounded and can
produce excessive delays for large MaxRetries; update the retry backoff to cap
the computed value at a reasonable maximum (e.g. maxRetryBackoff = 5 *
time.Second) before using time.After, so compute backoff, if backoff >
maxRetryBackoff set backoff = maxRetryBackoff, then use the capped backoff in
the select along with the existing ctx cancellation handling in the Sign/KMS
retry loop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c27ce2dd-c0f3-4b55-8419-0c29ec16f3b3
📒 Files selected for processing (9)
docs/guides/operations/aws-kms-signer.mdpkg/config/config.gopkg/signer/aws/signer.gopkg/signer/aws/signer_test.gopkg/signer/factory.gopkg/signer/gcp/signer.gotest/e2e/evm_kms_e2e_test.gotypes/utils.gotypes/utils_test.go
✅ Files skipped from review due to trivial changes (1)
- docs/guides/operations/aws-kms-signer.md
🚧 Files skipped from review as they are similar to previous changes (3)
- types/utils_test.go
- pkg/signer/aws/signer_test.go
- pkg/signer/factory.go
| // Options configures optional KmsSigner behavior. | ||
| type Options struct { | ||
| // CredentialsFile is an optional path to a Google credentials JSON file. | ||
| // If empty, Application Default Credentials are used. | ||
| CredentialsFile string | ||
| // Timeout for individual KMS Sign API calls. Default: 10s. | ||
| Timeout time.Duration | ||
| // MaxRetries for transient KMS failures during Sign. Default: 3. | ||
| MaxRetries int | ||
| } | ||
|
|
||
| func (o *Options) timeout() time.Duration { return o.Timeout } | ||
|
|
||
| func (o *Options) maxRetries() int { return o.MaxRetries } |
There was a problem hiding this comment.
Align the implementation with the documented default timeout.
Options documents the default sign timeout as 10s (line 58), but kmsSignerFromClient falls back to 1s when opts is nil or Timeout <= 0 (line 108). This makes the behavior more aggressive than advertised.
📝 Proposed fix
- o := Options{Timeout: 1 * time.Second, MaxRetries: 3}
+ o := Options{Timeout: 10 * time.Second, MaxRetries: 3}Also applies to: 108-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/signer/gcp/signer.go` around lines 53 - 66, The documented default
timeout for Options is 10s but kmsSignerFromClient currently falls back to 1s;
update the logic so the effective timeout matches the doc: make
Options.timeout() return 10*time.Second when o is nil or o.Timeout <= 0 (or
otherwise ensure kmsSignerFromClient uses that 10s default), and keep maxRetries
behavior consistent (use Options.maxRetries() or a similar default if nil).
Update only the timeout fallback (change 1s -> 10s) by modifying the timeout()
helper or the fallback in kmsSignerFromClient to use 10*time.Second.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/signer/gcp/signer.go (1)
36-50: Expose a shutdown path for the KMS client.
kms.NewKeyManagementClientreturns a client withClose(), and the package docs say callers should close it when the client is no longer needed.NewKmsSignerowns that client today, butKMSClient/KmsSignerdo not expose any close semantics, so these gRPC connections can only be reclaimed by process exit. (pkg.go.dev)♻️ Suggested direction
type KMSClient interface { AsymmetricSign(ctx context.Context, req *kmspb.AsymmetricSignRequest) (*kmspb.AsymmetricSignResponse, error) GetPublicKey(ctx context.Context, req *kmspb.GetPublicKeyRequest) (*kmspb.PublicKey, error) + Close() error } @@ func (c *cloudKMSClient) GetPublicKey(ctx context.Context, req *kmspb.GetPublicKeyRequest) (*kmspb.PublicKey, error) { return c.client.GetPublicKey(ctx, req) } + +func (c *cloudKMSClient) Close() error { + return c.client.Close() +} @@ type KmsSigner struct { client KMSClient keyName string opts Options mu sync.RWMutex pubKey crypto.PubKey address []byte } + +func (s *KmsSigner) Close() error { + if s.client == nil { + return nil + } + return s.client.Close() +}Then wire
Close()into the node shutdown path where the signer is owned.Also applies to: 80-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/signer/gcp/signer.go` around lines 36 - 50, The KMS client created by NewKmsSigner isn't being closed; add a Close method to the KMSClient interface and implement it on cloudKMSClient to call the underlying kms.KeyManagementClient.Close(); then add a Close method on KmsSigner that forwards to its KMSClient.Close (or type-asserts to the interface) so callers of NewKmsSigner can cleanly shutdown the gRPC connection from the node shutdown path; update NewKmsSigner users to call KmsSigner.Close when the signer is disposed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/signer/gcp/signer.go`:
- Around line 133-147: The fetchPublicKey and verifySignResponse paths must
validate KMS response names and CRC32C integrity fields: in fetchPublicKey
(function fetchPublicKey, call s.client.GetPublicKey and examine the returned
out) verify out.GetName() exactly matches s.keyName and is non-empty, compute
the CRC32C (Castagnoli) of out.GetPem() and compare it to the returned
out.GetPemCrc32C (or equivalent CRC field) and return an error on mismatch
before parsing the PEM; similarly, in verifySignResponse (the handler that
consumes the SignResponse/VerifyResponse), validate response.GetName() matches
s.keyName, compute CRC32C of the data that was signed and compare to
response.GetVerifiedDataCrc32C and compute CRC32C of response.GetSignature() and
compare to response.GetSignatureCrc32C (or their protobuf wrappers), returning
an error if any mismatch or missing fields are detected so corrupted or
mismatched KMS responses are rejected and never cached/accepted.
---
Nitpick comments:
In `@pkg/signer/gcp/signer.go`:
- Around line 36-50: The KMS client created by NewKmsSigner isn't being closed;
add a Close method to the KMSClient interface and implement it on cloudKMSClient
to call the underlying kms.KeyManagementClient.Close(); then add a Close method
on KmsSigner that forwards to its KMSClient.Close (or type-asserts to the
interface) so callers of NewKmsSigner can cleanly shutdown the gRPC connection
from the node shutdown path; update NewKmsSigner users to call KmsSigner.Close
when the signer is disposed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 649390a9-8b6d-43a8-b65a-997d2e56b437
⛔ Files ignored due to path filters (1)
execution/evm/test/go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
docs/guides/operations/gcp-kms-signer.mdexecution/evm/test/go.modpkg/signer/gcp/signer.go
✅ Files skipped from review due to trivial changes (2)
- execution/evm/test/go.mod
- docs/guides/operations/gcp-kms-signer.md
| func (s *KmsSigner) fetchPublicKey(ctx context.Context) error { | ||
| out, err := s.client.GetPublicKey(ctx, &kmspb.GetPublicKeyRequest{Name: s.keyName}) | ||
| if err != nil { | ||
| return fmt.Errorf("KMS GetPublicKey failed: %w", err) | ||
| } | ||
|
|
||
| block, _ := pem.Decode([]byte(out.GetPem())) | ||
| if block == nil { | ||
| return fmt.Errorf("failed to decode PEM public key") | ||
| } | ||
|
|
||
| pub, err := x509.ParsePKIXPublicKey(block.Bytes) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to parse KMS public key: %w", err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -E "signer|kms" | head -20Repository: evstack/ev-node
Length of output: 640
🏁 Script executed:
fd "signer.go" --type fRepository: evstack/ev-node
Length of output: 171
🏁 Script executed:
wc -l pkg/signer/gcp/signer.go 2>/dev/null || echo "File not found, searching..."Repository: evstack/ev-node
Length of output: 87
🏁 Script executed:
find . -name "signer.go" -path "*/gcp/*" 2>/dev/nullRepository: evstack/ev-node
Length of output: 85
🏁 Script executed:
cat -n pkg/signer/gcp/signer.goRepository: evstack/ev-node
Length of output: 10499
🏁 Script executed:
rg "kmspb\." pkg/signer/gcp/signer.go | head -20Repository: evstack/ev-node
Length of output: 745
🏁 Script executed:
cat -n pkg/signer/gcp/signer_test.go | head -100Repository: evstack/ev-node
Length of output: 3973
🏁 Script executed:
rg "GetName|GetPemCrc32C" pkg/signer/gcp/ 2>/dev/null || echo "Not found in gcp package"Repository: evstack/ev-node
Length of output: 83
🌐 Web query:
Google Cloud KMS data integrity guidelines GetPublicKey response validation
💡 Result:
Google Cloud KMS provides data integrity guidelines for the GetPublicKey response through CRC32C checksum verification and resource name matching. These are detailed in the official documentation at https://cloud.google.com/kms/docs/data-integrity-guidelines and the API reference. Key validation steps for the PublicKey response: 1. Verify the 'name' field matches the requested CryptoKeyVersion resource name (e.g., projects//locations//keyRings//cryptoKeys//cryptoKeyVersions/*). Mismatch indicates request corruption in-transit. 2. Compute the CRC32C checksum of the 'pem' field (for non-PQC keys) and compare it to 'pemCrc32c.value'. For specified publicKeyFormat (PQC/X-Wing), verify CRC32C of 'publicKey.data' against 'publicKey.crc32c_checksum.value'. If checksums mismatch, discard the response, retry a limited number of times. Persistent mismatch may indicate client-side CRC32C computation error. Code examples from docs (Go, Node.js, Python) demonstrate this: Go: crc32c := func(data []byte) uint32 { t := crc32.MakeTable(crc32.Castagnoli) return crc32.Checksum(data, t) } if int64(crc32c([]byte(result.Pem))) != result.PemCrc32c.Value { return fmt.Errorf("getPublicKey: response corrupted in-transit") } Node.js: const crc32c = require('fast-crc32c'); if (crc32c.calculate(publicKey.pem) !== Number(publicKey.pemCrc32c.value)) { throw new Error('GetPublicKey: response corrupted in-transit'); } Python: if not public_key.pem_crc32c == crc32c(public_key.pem.encode("utf-8")): raise Exception("The response received from the server was corrupted in-transit.") These checks ensure end-to-end integrity against client-side corruption or transit issues, supplementing Google Cloud's automatic protections.
Citations:
- 1: https://cloud.google.com/kms/docs/reference/rest/v1/projects.locations.keyRings.cryptoKeys.cryptoKeyVersions/getPublicKey
- 2: https://docs.cloud.google.com/kms/docs/retrieve-public-key
- 3: https://docs.cloud.google.com/kms/docs/reference/rest/v1/projects.locations.keyRings.cryptoKeys.cryptoKeyVersions/getPublicKey
- 4: https://cloud.google.com/kms/docs/samples/kms-get-public-key
- 5: https://docs.cloud.google.com/kms/docs/data-integrity-guidelines
🏁 Script executed:
grep -n "GetName\|GetPemCrc32C\|GetVerifiedDataCrc32C\|GetSignatureCrc32C" pkg/signer/gcp/signer.goRepository: evstack/ev-node
Length of output: 146
🏁 Script executed:
rg "type PublicKey\|type AsymmetricSignResponse" --type go | head -20Repository: evstack/ev-node
Length of output: 41
🏁 Script executed:
rg -A 5 "GetSignatureCrc32C\(\)" pkg/signer/gcp/signer.goRepository: evstack/ev-node
Length of output: 253
🏁 Script executed:
python3 << 'EOF'
# Verify the proposed fix logic makes sense
# The code shows signature_crc32c is validated:
# signatureCRC32C := out.GetSignatureCrc32C()
# if signatureCRC32C == nil { return error }
# if signatureCRC32C.GetValue() != expectedCRC32C { return error }
# The proposed fix follows the exact same pattern for pem_crc32c:
# if out.GetPemCrc32C() == nil { return error }
# if got, want := out.GetPemCrc32C().GetValue(), int64(crc32.Checksum(...)...)
# if got != want { return error }
# This is consistent pattern - the fix is sound.
print("Pattern validation: CONSISTENT - proposed fix mirrors existing signature_crc32c validation")
print("\nGoogle Cloud KMS documentation confirms:")
print("1. Name field validation is required for GetPublicKey and AsymmetricSign")
print("2. pem_crc32c validation is required for GetPublicKey response")
print("3. These prevent in-transit corruption and request/response mismatches")
EOFRepository: evstack/ev-node
Length of output: 398
Add integrity checks for Cloud KMS response fields.
The fetchPublicKey and verifySignResponse functions must validate the name field (to detect request/response mismatch) and the CRC32C checksum fields as required by Google Cloud KMS data integrity guidelines. Currently, corrupted or mismatched responses can be cached as this node's identity or returned as valid signatures without detection.
🛡️ Proposed fix
func (s *KmsSigner) fetchPublicKey(ctx context.Context) error {
out, err := s.client.GetPublicKey(ctx, &kmspb.GetPublicKeyRequest{Name: s.keyName})
if err != nil {
return fmt.Errorf("KMS GetPublicKey failed: %w", err)
}
+ if out.GetName() != s.keyName {
+ return fmt.Errorf("KMS GetPublicKey integrity check failed: unexpected key name %q", out.GetName())
+ }
+ if out.GetPemCrc32C() == nil {
+ return fmt.Errorf("KMS GetPublicKey integrity check failed: pem_crc32c is missing")
+ }
+ if got, want := out.GetPemCrc32C().GetValue(), int64(crc32.Checksum([]byte(out.GetPem()), castagnoliTable)); got != want {
+ return fmt.Errorf("KMS GetPublicKey integrity check failed: pem_crc32c mismatch")
+ }
block, _ := pem.Decode([]byte(out.GetPem()))
@@
- if err := verifySignResponse(out); err != nil {
+ if err := verifySignResponse(out, s.keyName); err != nil {
lastErr = err
continue
}
@@
-func verifySignResponse(out *kmspb.AsymmetricSignResponse) error {
+func verifySignResponse(out *kmspb.AsymmetricSignResponse, expectedName string) error {
+ if out.GetName() != expectedName {
+ return fmt.Errorf("KMS Sign integrity check failed: unexpected key name %q", out.GetName())
+ }
if !out.GetVerifiedDataCrc32C() {
return fmt.Errorf("KMS Sign integrity check failed: verified_data_crc32c is false")
}Per coding guideline: "Validate all inputs from external sources in Go code". See https://cloud.google.com/kms/docs/data-integrity-guidelines.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/signer/gcp/signer.go` around lines 133 - 147, The fetchPublicKey and
verifySignResponse paths must validate KMS response names and CRC32C integrity
fields: in fetchPublicKey (function fetchPublicKey, call s.client.GetPublicKey
and examine the returned out) verify out.GetName() exactly matches s.keyName and
is non-empty, compute the CRC32C (Castagnoli) of out.GetPem() and compare it to
the returned out.GetPemCrc32C (or equivalent CRC field) and return an error on
mismatch before parsing the PEM; similarly, in verifySignResponse (the handler
that consumes the SignResponse/VerifyResponse), validate response.GetName()
matches s.keyName, compute CRC32C of the data that was signed and compare to
response.GetVerifiedDataCrc32C and compute CRC32C of response.GetSignature() and
compare to response.GetSignatureCrc32C (or their protobuf wrappers), returning
an error if any mismatch or missing fields are detected so corrupted or
mismatched KMS responses are rejected and never cached/accepted.
|
1 participant
Resolves #3163
Overview
Includes AWS and GCP remote signers
Manual test:
See:
Example
Setup AWS permission:
{ "Version": "2012-10-17", "Statement": [ { "Sid": "AllowKeyCreation", "Effect": "Allow", "Action": [ "kms:CreateKey", "kms:TagResource", "kms:EnableKey", "kms:PutKeyPolicy", "kms:GetPublicKey", "kms:Sign", "kms:ListKeys", "kms:ListAliases" ], "Resource": "*" } ] }Create KMS key:
Copy
KeyIdfrom response.Summary by CodeRabbit
New Features
Configuration
Documentation