feat: AWS KMS Signer #4289
feat: AWS KMS Signer #4289
Conversation
d4mr
requested review from
joaquim-verges,
gregfromstl and
jnsdls
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Your org requires the Graphite merge queue for merging into mainAdd the label “merge-queue” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix. You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link. |
|
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
| export async function getAwsKmsAccount( | ||
| options: AwsKmsAccountOptions, | ||
| ): Promise<Account> { | ||
| if (typeof Buffer === "undefined") { |
There was a problem hiding this comment.
because we don't polyfill Buffer, and we don't expect folks to use AWS KMS on browsers, I have explicitly safeguarded against browser usage
size-limit report 📦
|
| @@ -0,0 +1,129 @@ | |||
| import type { KMSClientConfig } from "@aws-sdk/client-kms"; | |||
| import { KmsSigner } from "aws-kms-signer"; | |||
| import type { | |||
There was a problem hiding this comment.
just used private-key.ts as reference, is there a different import I should do?
| }): Promise<Hex> { | ||
| let messageHash: Hex; | ||
| if (typeof message === "string") { | ||
| const prefixedMessage = `\x19Ethereum Signed Message:\n${message.length}${message}`; |
There was a problem hiding this comment.
This is the required prefix for eth_sign & personal_sign according to spec (EIP191): ethereum/go-ethereum#3731 (comment)
https://docs.walletconnect.com/advanced/multichain/rpc-reference/ethereum-rpc#personal_sign
(sorry for the poor sources, can't find good ones after eth migrated their wikis)
Although this raises another question:
SignableMessage can be string or { raw: Hex | ByteArray }. If a raw SignableMessage is passed along, I am not adding a prefix, because the expectation is that its already processed.
This is based on a large assumption I made around the interface. I am not certain this is the expectation from the interface. What do you think?
(to be clear, signing a message requires this prefix. If you sign without this prefix and try to verifyMessage it will fail)
| }); | ||
|
|
||
| beforeAll(async () => { | ||
| account = await getAwsKmsAccount({ |
There was a problem hiding this comment.
So this will always hit a live KMS account? And is there any way to keep that address constant?
There was a problem hiding this comment.
Same KMS keyId would get you the same address. One way to enforce this on a test would be to also expose TEST_AWS_KMS_EXPECTED_ADDRESS and verify it matches.
gregfromstl
added
TS SDK
Co-authored-by: greg <[email protected]> Signed-off-by: Prithvish Baidya <[email protected]>
PR-Codex overview
This PR adds AWS KMS integration for secure key management, including configuration setup, account creation, and signing operations.
Detailed summary