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.

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

feat(key-manager): add generic signing capabilities #529

Merged
merged 7 commits into from
Jun 3, 2021

Conversation

mirceanis
Copy link
Member

@mirceanis mirceanis commented May 19, 2021

This PR extends the signing capabilities of the key-manager plugin.

The key-manager plugin now exposes a keyManagerSign method that accepts extra alg, enc parameters and supports extension of the parameter object.
The enc parameter indicates the encoding of the data string, defaulting to utf-8 if absent.
The alg option should indicate an algorithm to be used.
Depending on the capabilities of the KMS (one of the AbstractKeyManagementSystem implementations), different signatures can be requested.

For example:

  • a KMS implementation that has direct access to key material for a Secp256k1 key should be able to support:
    • ES256K, ES256K-R, eth_signTransaction, eth_signTypedData, eth_signMessage
  • a KMS implementation that is based on a web3 abstraction, would only be able to do:
    • eth_signTransaction, eth_signTypedData, and eth_signMessage

The capabilities of the KMS are recorded in the IKey.meta.algorithms array during key creation or import.
Further improvements can be made to programmatically filter for KMS or key capabilities.

This PR relates to #468

Examples:

Sign a JWS payload with ES256K:

// header with `"alg": "ES256K"`
const jwsBundle = `${encodedHeader}.${encodedPayload}`
// for this alg,the result is base64url encoded
const signature = await agent.keyManagerSign({
  algorithm: 'EdDSA',
  data: jwsBundle,
  encoding: 'utf-8', // this is the default encoding, so it could be omitted in this case
  keyRef: key.kid
})
const jwsString = `${encodedHeader}..${signature}`

Sign an ETH transaction:

// RLP encode the transaction fields
const txData = serialize({
  to: '0xce31a19193d4b23f4e9d6163d7247243bAF801c3',
  value: 300000,
  gasLimit: 43092000,
  gasPrice: 20000000000,
  nonce: 1,
})

// compute the raw transaction string
// for transaction signing, the output is the `0x` prefixed hex string representing the signed raw transaction.
const rawTx = await agent.keyManagerSign({
  algorithm: 'eth_signTransaction',
  data: txData,
  encoding: 'hex',
  keyRef: key.kid
})
// rawTx = '0xf869018504a817c80084029.......'

@codecov
Copy link

codecov bot commented May 19, 2021

Codecov Report

Merging #529 (b49b57c) into next (88264db) will increase coverage by 4.17%.
The diff coverage is 59.88%.

@@            Coverage Diff             @@
##             next     #529      +/-   ##
==========================================
+ Coverage   67.58%   71.75%   +4.17%     
==========================================
  Files          62       72      +10     
  Lines        1530     1972     +442     
  Branches      247      326      +79     
==========================================
+ Hits         1034     1415     +381     
- Misses        400      555     +155     
+ Partials       96        2      -94     

@mirceanis mirceanis force-pushed the feat/add-generic-signing-to-kms branch 2 times, most recently from 3391fab to 402e151 Compare May 27, 2021 09:31
@mirceanis mirceanis marked this pull request as ready for review May 27, 2021 12:13
Copy link
Member

@awoie awoie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general it looks great!

It would be great to have data be of type byte array, e.g., to support CBOR in the future.

packages/core/src/types/IKeyManager.ts Outdated Show resolved Hide resolved
packages/core/src/types/IKeyManager.ts Show resolved Hide resolved
packages/kms-local/src/key-management-system.ts Outdated Show resolved Hide resolved
Copy link
Member

@awoie awoie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more of my JOSE naming schizophrenia :)

packages/core/src/types/IIdentifier.ts Show resolved Hide resolved
packages/core/src/types/IKeyManager.ts Outdated Show resolved Hide resolved
@mirceanis mirceanis force-pushed the feat/add-generic-signing-to-kms branch from ec5c276 to ab80721 Compare June 1, 2021 14:14
@mirceanis mirceanis requested a review from awoie June 1, 2021 14:28
@mirceanis mirceanis force-pushed the feat/add-generic-signing-to-kms branch from ab80721 to 65192a3 Compare June 2, 2021 10:16
Copy link
Member

@awoie awoie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. It just strikes me a bit that 'Ed25519' and 'EdDSA' count as two separate algorithms although they are the same.

@mirceanis mirceanis merged commit 5f10a1b into next Jun 3, 2021
@mirceanis mirceanis deleted the feat/add-generic-signing-to-kms branch June 3, 2021 12:39
@mirceanis
Copy link
Member Author

LGTM. It just strikes me a bit that 'Ed25519' and 'EdDSA' count as two separate algorithms although they are the same.

Yes, they are identical. I added both for compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants