Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adding support for an AWS KMS provider #122

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Adding support for an AWS KMS provider #122

wants to merge 10 commits into from
+573 −9

Conversation

ddaws
Copy link

@ddaws ddaws commented Jan 15, 2025 •
edited
Loading

Description

This PR adds AWS KMS support to the op-signer service, allowing users to choose between AWS KMS and Google Cloud KMS for signing operations. The implementation maintains backward compatibility by defaulting to GCP if no provider is specified.

provider: "AWS" # or "GCP" (defaults to "GCP" if not specified)
auth:
  - name: "test-client"
    key: "test-key"

TODO

  • Integration tests against AWS KMS

Changes

  • Add provider type configuration in YAML config
  • Add tests for config parsing and validation of new provider type
  • Create provider factory pattern for KMS provider selection
  • Update service initialization to handle provider errors
  • Add AWS KMS signature provider implementation
  • Add AWS SDK dependencies

Breaking Changes

None. Existing configurations without a provider specified will continue to use GCP KMS.

Tests

Config Tests (op-signer/service/config_test.go):

  1. TestReadConfig_DefaultKeyProvider
    • Tests that when no provider is specified in the config, it defaults to GCP
    • Creates a temporary config file without a provider field
    • Verifies that config.ProviderType equals provider.KeyProviderGCP
  2. TestReadConfig_ExplicitKeyProvider
    • Tests that when AWS is explicitly specified as the provider, it's properly parsed
    • Creates a temporary config file with provider: "AWS"
    • Verifies that config.ProviderType equals provider.KeyProviderAWS
  3. TestReadConfig_InvalidKeyProvider
    • Tests that an invalid provider type results in an error
    • Creates a temporary config file with provider: "INVALID"
    • Verifies that an error is returned containing "invalid provider"

AWS KMS Tests (op-signer/service/provider/aws_kms_test.go):

  1. TestAWSKMSSignatureProvider_GetPublicKey
    • Tests public key retrieval from AWS KMS
  2. TestAWSKMSSignatureProvider_SignDigest
    • Tests signature generation and verification
    • Verifies 65-byte signature length
    • Tests signature recoverability
    • Verifies recovered public key matches original
    • Validates signature using go-ethereum's implementation

Utility Tests (op-signer/service/provider/utils_test.go):

  1. TestMarshalAndUnmarshalECDSAPublicKey
    • Tests secp256k1 public key DER encoding/decoding
  2. TestMarshalAndParseECDSAPublicKey
    • Validates compatibility with AWS KMS format
  3. TestConvertEthereumSignatureToDER
    • Tests signature format conversion
  4. TestConvertToEthereumSignatureRecoverable: Tests signature recoverability
    • Verifies DER to compact format conversion
    • Validates signature recovery with original message
    • Confirms compatibility with Ethereum's signature format

Additional context

Metadata

ddaws added 4 commits January 15, 2025 12:54
@ddaws
feat(op-signer): add KeyProvider enum for cloud provider selection
df7cec5 
Add KeyProvider type to specify cloud KMS provider (AWS or GCP) in AuthConfig.
Defaults to GCP if not specified for backwards compatibility. Includes tests
for config parsing and validation.

- Add KeyProvider type with AWS and GCP options
- Add validation for KeyProvider values
- Default to GCP when type is not specified
- Add unit tests for config parsing
- Update example config.yaml with explicit GCP type
@ddaws
refactor(op-signer): move KeyProvider to top-level config
aac0d7a 
Move the KeyProvider configuration from individual AuthConfig entries to the
top-level SignerServiceConfig. This simplifies the configuration by having a
single provider type for all auth configs.

- Rename config field to `provider` in yaml
- Move KeyProvider from AuthConfig to SignerServiceConfig
- Update config validation to check top-level provider
- Update tests to reflect new config structure
- Maintain backwards compatibility with GCP default
@ddaws
feat(op-signer): add provider factory for signature providers
6d9eee4 
Add factory function to create SignatureProvider instances based on provider type.
Updates service initialization to use the new factory pattern.

- Add NewSignatureProvider factory function in provider package
- Update NewSignerService to handle provider creation errors
- Update app.go to handle potential service creation errors
- Move ProviderType enum from config to provider package
@ddaws
feat(op-signer): add AWS KMS signature provider
0badbd2 
Add AWS KMS implementation of SignatureProvider interface to support AWS KMS
for signing operations. Updates provider factory to support both GCP and AWS.

- Add AWSKMSSignatureProvider with Sign and GetPublicKey methods
- Add AWS SDK dependencies
- Update provider factory to support AWS KMS
- Reuse existing signature conversion utilities
- Add test-friendly constructor with mock client
@ddaws ddaws requested a review from a team as a code owner January 15, 2025 05:00
@ddaws ddaws requested a review from Inphi January 15, 2025 05:00
@ddaws ddaws marked this pull request as draft January 15, 2025 05:01
ddaws added 6 commits January 15, 2025 13:08
@ddaws
chore(deps): add AWS SDK dependencies
b78337e 
Add AWS SDK dependencies required for AWS KMS implementation:
- github.com/aws/aws-sdk-go-v2/config
- github.com/aws/aws-sdk-go-v2 (indirect)

These dependencies are needed for the AWS KMS signature provider functionality.
@ddaws
chore(lint): fixed spelling
90e8f03
@ddaws
feat(op-signer): add DER encoding utilities for secp256k1 keys
51911ee 
Add utility functions to marshal and unmarshal secp256k1 public keys in DER
format for AWS KMS integration. Includes comprehensive tests to verify encoding
compatibility.

- Add marshalECDSAPublicKey function for DER encoding
- Add unmarshalECDSAPublicKey function for DER decoding
- Add tests to verify marshal/unmarshal roundtrip
- Add tests to verify compatibility with AWS KMS format
@ddaws
feat(op-signer): add signature format conversion utilities
6564016 
Add utilities for converting between Ethereum's compact recoverable signature
format and DER format, with comprehensive tests.

- Add convertCompactRecoverableSignatureToDER function
- Add test for DER conversion roundtrip
- Add test for signature recoverability
- Add test for signature format compatibility with go-ethereum

This enables proper signature format handling between AWS KMS (DER) and
Ethereum (compact recoverable)
@ddaws
test(op-signer): add AWS KMS signature provider tests
7d63efc 
Add comprehensive tests for AWS KMS signature provider implementation:
- Add mock AWS KMS client for testing
- Test GetPublicKey with DER-encoded secp256k1 keys
- Test SignDigest with signature verification and recovery
- Verify compatibility with Ethereum signature format
- Test signature malleability and length requirements
@ddaws
fix(op-signer): use %w verb for error wrapping in DER conversion
410a131 
Update error formatting in convertCompactRecoverableSignatureToDER to use %w
instead of %v to properly wrap and preserve the original error context.
@ddaws ddaws changed the title Draft: Adding support for an AWS KMS provider Adding support for an AWS KMS provider Jan 16, 2025
@ddaws ddaws marked this pull request as ready for review January 22, 2025 03:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Inphi Inphi Awaiting requested review from Inphi Inphi is a code owner automatically assigned from ethereum-optimism/infra-reviewers

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@ddaws