Repository navigation
Add AWS KMS support for account keys - #232
Conversation
Adds an `aws-kms` key type that mirrors the existing `google-kms` key
type: the key is declared in flow.json with a `resourceID`, and signing
is delegated to the KMS provider.
- config.KeyTypeAWSKMS ("aws-kms")
- AWSKMSKey signs through the Flow Go SDK awskms package
- the region is read from the key ARN, so it does not have to be
configured separately
- credentials are resolved from the default AWS configuration
- Validate returns an actionable error when credentials cannot be
resolved, instead of starting an interactive login
AWS KMS signs ECDSA keys with ECDSA_SHA_256 only, so the hash algorithm
is not a choice for this key type: it is fixed to SHA2_256, and a
configuration asking for anything else is rejected. Leaving it to the
default SHA3_256 would produce signatures that no verifier can check.
Only the canonical key ARN form is accepted
(arn:aws:kms:<region>:<account-id>:key/<key-id>), because key aliases and
other KMS resource types cannot be reconstructed from a parsed key and
the signing key has to be addressed by its ARN.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
vishalchangrani
left a comment
There was a problem hiding this comment.
Looked through this carefully. Two strong correctness catches worth calling out:
Hash algorithm pin to SHA2_256. This one would have been easy to miss. AWS KMS only signs ECDSA with ECDSA_SHA_256, and awskms.parseHashAlgorithm in flow-go-sdk returns SHA2_256 for both supported key specs. If the config falls through to flowkit's default (SHA3_256), Key.HashAlgo() reports SHA3_256 but the signer actually produces SHA2_256 — every signature gets rejected on-chain. Pinning in both awsKMSKeyFromConfig and at JSON parse time (with the explicit error on non-SHA2_256) is the right call.
ARN regex guards an SDK crash. awskms.KeyFromResourceARN in flow-go-sdk does strings.Split(parts[5], "/")[1] without bounds-checking — give it arn:aws:kms:us-west-2:111122223333:key (no key ID) and it panics. Rejecting aliases, short account IDs, and missing region/account up front is a solid defensive layer. Might be worth a quick follow-up patch to the SDK itself.
A few non-blocking suggestions:
-
Non-standard AWS partitions. The regex only accepts
arn:aws:. China and GovCloud usearn:aws-cn:/arn:aws-us-gov:. Likely irrelevant to our migration, but easy to open up:^arn:aws(-cn|-us-gov)?:kms:.... -
Hash enforcement asymmetric. JSON path errors on
SHA3_256; a Go caller constructingconfig.AccountKey{Type: KeyTypeAWSKMS, HashAlgo: crypto.SHA3_256}directly gets a silent override. Fine if intentional (strict at user-facing boundary, lenient internally), but worth a one-line comment onawsKMSKeyFromConfignoting the override so future readers aren't surprised. -
Validate()cost.awsCfg.Credentials.Retrieve()can trigger an IMDS roundtrip on EC2/ECS, and runs even for accounts not being used in the current command. The GCP equivalent is heavier (can pop a browser) so there's precedent, but it may add noticeable startup latency on short-lived commands with many AWS accounts inflow.json. Worth keeping in mind if someone later reports slowflowinvocations.
Overall LGTM — this is a strict upgrade over a naive port of the GCP path.
- Note on awsKMSKeyFromConfig that overriding the configured hash algorithm is deliberate, and that the JSON layer is the strict boundary - Say why aws-cn and aws-us-gov ARNs are rejected: the SDK rebuilds the ARN as arn:aws:kms:... and sends that as the KeyId, so the wrong partition reaches the service. Supporting them needs an SDK change. - Pin the partition rejection in the ARN test table
|
Pushed Partitions. I kept the rejection rather than opening the regex up, because accepting them would only move the failure later. Hash enforcement. Added the comment on
SDK crash. Agreed the bounds check belongs upstream — |
|
Verified end-to-end against AWS KMS on Flow testnet. CI: Setup
The account's key is the public half of that KMS key. The transaction has the KMS account as proposer and authorizer, with a separate account as payer. Result:
On the hash algorithm. The account had to be registered with |
What
Adds an
aws-kmsaccount key type, mirroring the existinggoogle-kmskey type. The key is declared inflow.jsonwith aresourceID, and signing is delegated to AWS KMS.config.KeyTypeAWSKMS("aws-kms")accounts.AWSKMSKeysigns throughgithub.com/onflow/flow-go-sdk/crypto/awskms~/.aws, SSO)Validatereturns an actionable error when credentials cannot be resolved, rather than starting an interactive loginNotes
The hash algorithm is fixed to SHA2_256. AWS KMS signs ECDSA keys with
ECDSA_SHA_256only:awskmssetsSigningAlgorithmSpecEcdsaSha256, and itsparseHashAlgorithmreturnsSHA2_256for both supported key specs. flowkit defaults to SHA3_256, so the hash algorithm is now derived from the key type instead of the configuration, and a configuration asking for anything else is rejected. Without this,HashAlgo()reports a value that no verifier can reproduce.Only the canonical key ARN is accepted (
arn:aws:kms:<region>:<account-id>:key/<key-id>). Key aliases and other KMS resource types cannot be reconstructed from a parsed key, and the signing key has to be addressed by its ARN.Verification
go test ./...passes;gofmt,go vet,check-headers,check-schema, andgo mod tidyare cleanflowbinary built against this branch loads aflow.jsonholding both anaws-kmsand agoogle-kmsaccountaws-kmshash algorithm survives aflow.jsonwrite-back asSHA2_256