Conversation
📝 WalkthroughWalkthroughThis pull request adds AWS KMS signer backend support by introducing context-aware signing throughout the codebase. It implements a new AWS KMS signer package, establishes a signer factory pattern for creating signers based on configuration, updates the core Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 1s —— View job Code Review: feat: Add AWS KMS signer backend
Overall this is a solid, well-scoped implementation. The interface-based design ( Issues🐛 Default timeout inconsistency —
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3171 +/- ##
==========================================
+ Coverage 61.14% 61.23% +0.09%
==========================================
Files 117 119 +2
Lines 12082 12235 +153
==========================================
+ Hits 7387 7492 +105
- Misses 3868 3904 +36
- Partials 827 839 +12
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
| SignerType string `mapstructure:"signer_type" yaml:"signer_type" comment:"Type of remote signer to use (file, awskms)"` | ||
| SignerPath string `mapstructure:"signer_path" yaml:"signer_path" comment:"Path to the signer file or address"` |
There was a problem hiding this comment.
Minor inconsistency in signer type documentation.
The struct field comment on line 305 lists (file, awskms) but the flag description on line 577 lists (file, grpc, awskms). These should be consistent.
📝 Proposed fix
Either update the struct comment to match the flag:
- SignerType string `mapstructure:"signer_type" yaml:"signer_type" comment:"Type of remote signer to use (file, awskms)"`
+ SignerType string `mapstructure:"signer_type" yaml:"signer_type" comment:"Type of remote signer to use (file, grpc, awskms)"`Or update the flag description if grpc is not a valid type:
- cmd.Flags().String(FlagSignerType, def.Signer.SignerType, "type of signer to use (file, grpc, awskms)")
+ cmd.Flags().String(FlagSignerType, def.Signer.SignerType, "type of signer to use (file, awskms)")📝 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.
| SignerType string `mapstructure:"signer_type" yaml:"signer_type" comment:"Type of remote signer to use (file, awskms)"` | |
| SignerPath string `mapstructure:"signer_path" yaml:"signer_path" comment:"Path to the signer file or address"` | |
| SignerType string `mapstructure:"signer_type" yaml:"signer_type" comment:"Type of remote signer to use (file, grpc, awskms)"` | |
| SignerPath string `mapstructure:"signer_path" yaml:"signer_path" comment:"Path to the signer file or address"` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/config/config.go` around lines 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.
| 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.
| // GetAddress returns the cached address derived from the public key. | ||
| func (s *KmsSigner) GetAddress() ([]byte, error) { | ||
| s.mu.RLock() | ||
| defer s.mu.RUnlock() | ||
|
|
||
| if s.address == nil { | ||
| return nil, fmt.Errorf("address not loaded") | ||
| } | ||
|
|
||
| return s.address, nil |
There was a problem hiding this comment.
Return a copy of the cached address.
GetAddress currently exposes the backing slice stored on the signer. Any caller can mutate that slice after the lock is released and corrupt the signer's cached identity.
Proposed fix
func (s *KmsSigner) GetAddress() ([]byte, error) {
s.mu.RLock()
- defer s.mu.RUnlock()
-
if s.address == nil {
+ s.mu.RUnlock()
return nil, fmt.Errorf("address not loaded")
}
- return s.address, nil
+ address := append([]byte(nil), s.address...)
+ s.mu.RUnlock()
+ return address, nil
}📝 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.
| // GetAddress returns the cached address derived from the public key. | |
| func (s *KmsSigner) GetAddress() ([]byte, error) { | |
| s.mu.RLock() | |
| defer s.mu.RUnlock() | |
| if s.address == nil { | |
| return nil, fmt.Errorf("address not loaded") | |
| } | |
| return s.address, nil | |
| // GetAddress returns the cached address derived from the public key. | |
| func (s *KmsSigner) GetAddress() ([]byte, error) { | |
| s.mu.RLock() | |
| if s.address == nil { | |
| s.mu.RUnlock() | |
| return nil, fmt.Errorf("address not loaded") | |
| } | |
| address := append([]byte(nil), s.address...) | |
| s.mu.RUnlock() | |
| return address, nil | |
| } |
🤖 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 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).
| // 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.
| func GetRandomNextSignedHeader(ctx context.Context, signedHeader *SignedHeader, signer signer.Signer, chainID string) (*SignedHeader, error) { | ||
| newSignedHeader := &SignedHeader{ | ||
| Header: GetRandomNextHeader(signedHeader.Header, chainID), | ||
| Signer: signedHeader.Signer, | ||
| } | ||
|
|
||
| signature, err := GetSignature(signedHeader.Header, signer) | ||
| signature, err := GetSignature(ctx, signedHeader.Header, signer) |
There was a problem hiding this comment.
Sign the newly built header here.
GetRandomNextSignedHeader creates newSignedHeader.Header but still signs signedHeader.Header. The returned SignedHeader therefore carries a signature for different bytes and won't verify.
Proposed fix
- signature, err := GetSignature(ctx, signedHeader.Header, signer)
+ signature, err := GetSignature(ctx, newSignedHeader.Header, signer)📝 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.
| func GetRandomNextSignedHeader(ctx context.Context, signedHeader *SignedHeader, signer signer.Signer, chainID string) (*SignedHeader, error) { | |
| newSignedHeader := &SignedHeader{ | |
| Header: GetRandomNextHeader(signedHeader.Header, chainID), | |
| Signer: signedHeader.Signer, | |
| } | |
| signature, err := GetSignature(signedHeader.Header, signer) | |
| signature, err := GetSignature(ctx, signedHeader.Header, signer) | |
| func GetRandomNextSignedHeader(ctx context.Context, signedHeader *SignedHeader, signer signer.Signer, chainID string) (*SignedHeader, error) { | |
| newSignedHeader := &SignedHeader{ | |
| Header: GetRandomNextHeader(signedHeader.Header, chainID), | |
| Signer: signedHeader.Signer, | |
| } | |
| signature, err := GetSignature(ctx, newSignedHeader.Header, signer) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@types/utils.go` around lines 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.
1 participant
Part of #3163
Overview
Manual test:
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
Documentation
Refactor