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

Add local keymanager API #151

Closed
wants to merge 29 commits into from
Closed
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
730a0d5
Add validator manager API
dapplion Jun 4, 2021
d433e14
Fix issues
dapplion Jun 5, 2021
0ff09a3
Simplify routes
dapplion Oct 4, 2021
51861f8
Generalize language
dapplion Oct 4, 2021
2fd107b
Delete accounts routes
dapplion Oct 4, 2021
37490ab
Remove validator-oapi.yaml
dapplion Oct 4, 2021
db508c2
Merge branch 'master' into validator-manager-api
dapplion Oct 4, 2021
817a24e
Rename accounts for keystores
dapplion Oct 5, 2021
b920615
Remove slashing_protection_last_entry property
dapplion Oct 5, 2021
44c772a
Merge branch 'validator-manager-api' of github.com:dapplion/eth2.0-AP…
dapplion Oct 5, 2021
c0feb5f
Address PR comments
dapplion Oct 13, 2021
6f56ae9
Update key-manager-oapi.yaml
dapplion Oct 15, 2021
bd3ffe7
Update key-manager-oapi.yaml
dapplion Oct 27, 2021
5e8dded
Update key-manager-oapi.yaml
dapplion Oct 27, 2021
82ba2b9
Update key-manager-oapi.yaml
dapplion Oct 27, 2021
5d27a29
Merge branch 'master' into validator-manager-api
dapplion Oct 27, 2021
e1f5232
Clarify formats
dapplion Oct 27, 2021
3168f46
Remove keystore notation for keys
dapplion Oct 27, 2021
ec9f7d4
Allow to import with an array of passwords
dapplion Oct 27, 2021
43581ac
Fix indentation
dapplion Nov 2, 2021
aa3ff0a
Add not_deletable property
dapplion Nov 2, 2021
84465af
Clarify DeleteKeysResponse
dapplion Nov 4, 2021
c6e0daf
Update not_deletable prop name
dapplion Nov 4, 2021
ef02cd2
Wrap response in data prop
dapplion Nov 4, 2021
08f627f
Add type array to password requests
dapplion Nov 4, 2021
be49b64
Update ImportKeystores description
dapplion Nov 4, 2021
3b14523
Remove request. prefix in ImportKeystores description
dapplion Nov 4, 2021
32f7d68
Update DeleteKeysResponse enum values
dapplion Nov 5, 2021
c048b4c
Merge branch 'master' into validator-manager-api
dapplion Nov 10, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
225 changes: 225 additions & 0 deletions key-manager-oapi.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,225 @@
openapi: "3.0.3"

info:
title: "Eth2 key manager API"
description: |
API specification for a key manager client, which enables users to manage keystores.

The key manager API is served by the binary holding the validator keys. This binary may be a remote signer or a validator client.

All routes SHOULD be exposed through a secure channel, such as with HTTPs, an SSH tunnel, a VPN, etc.

All requests by default send and receive JSON, and as such should have either or both of the "Content-Type: application/json"
and "Accept: application/json" headers.

All sensitive routes are to be authenticated with a token. This token should be provided by the user via a secure channel:
- Log the token to stdout when running the binary with the key manager API enabled
- Read the token from a file available to the binary
version: "Dev - Eth2Spec v1.0.1"
contact:
name: Ethereum Github
url: https://github.com/ethereum/eth2.0-apis/issues
license:
name: "Apache 2.0"
url: "https://www.apache.org/licenses/LICENSE-2.0.html"

servers:
- url: "{server_url}"
variables:
server_url:
description: "key manager API url"
default: "https://public-mainnet-node.ethereum.org"

paths:
/eth/v1/keystores:
Copy link

Choose a reason for hiding this comment

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

Should we use a separate prefix for these APIs to make it more clear that beacon nodes are not expected to serve them in general? (e.g. /eth-validator/)

Copy link
Contributor

Choose a reason for hiding this comment

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

good point. i agree with this, though still wasn't sure about /eth-validator/
I realized that eth/v1 was already being used though for validators

"/eth/v1/validator/duties/attester/{epoch}",
"/eth/v1/validator/duties/proposer/{epoch}",
"/eth/v1/validator/duties/sync/{epoch}",
"/eth/v1/validator/blocks/{slot}",
"/eth/v2/validator/blocks/{slot}",
"/eth/v1/validator/attestation_data",
"/eth/v1/validator/aggregate_attestation",
"/eth/v1/validator/beacon_committee_subscriptions",
"/eth/v1/validator/sync_committee_subscriptions",
"/eth/v1/validator/aggregate_and_proofs",
"/eth/v1/validator/sync_committee_contribution",
"/eth/v1/validator/contribution_and_proofs",
so I didn't know what made sense in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would be nice to define what the /eth namespace means. If it's beacon node, beacon chain or ethereum in general

Copy link
Contributor

Choose a reason for hiding this comment

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

All those validator endpoints under /eth/v1 are supplied by the beacon node (all the standard apis so far are). So /eth is effectively beacon node and we need to pick something else for the validator client. /eth-validator is probably the right option though it's longer than I'd like. Tempting to just use /eth-vc or even /ethvc

get:
operationId: ListKeys
summary: List Keys.
description: |
List all validating pubkeys known to and decrypted by this keymanager binary
security:
- cookieAuth: []
responses:
"200":
description: Success response
content:
application/json:
schema:
title: ListKeysResponse
type: object
properties:
data:
type: array
items:
type: object
properties:
dapplion marked this conversation as resolved.
Show resolved Hide resolved
validating_pubkey:
dapplion marked this conversation as resolved.
Show resolved Hide resolved
$ref: "#/components/types/Pubkey"
derivation_path:
type: string
description: The derivation path (if present in the imported keystore).
readonly:
type: boolean
required: false
description: The key associated with this pubkey cannot be deleted from the API

"401":
$ref: "#/components/responses/Unauthorized"
"403":
$ref: "#/components/responses/Forbidden"
"500":
$ref: "#/components/responses/InternalError"

post:
Copy link

Choose a reason for hiding this comment

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

Lighthouse has a cool feature where you can tell the validator client to start working with a new validator that will perform signatures over the remote signer protocol:

https://lighthouse-book.sigmaprime.io/api-vc-endpoints.html#post-lighthousevalidatorsweb3signer

Besides making this standard more feature-complete, having this functionality is quite important for implementing UIs based on the remote signer protocol (because it solves the problem of adding new keys at run-time to your remote signer without requiring polling from the validator client).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm tempted to say that a standardised API for remote signers should be separate from the keystore API.

The details around remote signer interfaces aren't nailed down (e.g. Web3Signer vs Dirk's eth2-signer-api vs EIP-3030), and I think we could get a long way with just a standard keystore API. The keystore API will enable UIs for solo stakers and home stakers first, and then we can circle back to remote signer APIs in a future revision.

In the meantime we should make a note that any remote signer validators should be left untouched by keystore API methods, and should not appear in the list returned by GET.

operationId: ImportKeystores
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to talk to my team on import keystores, again would require the prysm wallet password I believe as the way we cache is appending keys to another keystore called the prysm wallet

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, that could pose a problem. I guess the VC doesn't keep the password in memory while it's running?

Copy link
Contributor

@michaelsproul michaelsproul Oct 25, 2021

Choose a reason for hiding this comment

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

If it doesn't keep the password in memory then we could define an additional "validator password" concept in the standard, although it would be nice to keep the standard simple by avoiding this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@michaelsproul sorry late response on this. confirmed it does keep in memory, just that it would need to be done prior to calling this API...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DAppNode uses a Prysm flag to persist a wallet password locally to allow non-interactive restarts. I'm not sure how to tackle this issue without such mechanism

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the GUI backend could pass that same flag to Prysm on init, and then it'll be in memory and everything "just works"?

summary: Import Keystores.
description: |
Import keystores generated by the Eth2.0 deposit CLI tooling. `passwords[i]` must unlock `keystores[i]`.

Users SHOULD send slashing_protection data associated with the imported pubkeys. MUST follow the format defined in
EIP-3076: Slashing Protection Interchange Format.
security:
- cookieAuth: []
requestBody:
content:
application/json:
schema:
type: object
properties:
keystores:
type: array
description: JSON-encoded keystore files generated with the Launchpad.
items:
type: object
passwords:
type: array
description: Passwords to unlock imported keystore files. `passwords[i]` must unlock `keystores[i]`.
items:
type: string
slashing_protection:
type: object
required: false
description: Slashing protection of the imported keys data in EIP-3076 JSON format.
Comment on lines +99 to +102
Copy link
Contributor

Choose a reason for hiding this comment

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

we're going to have to define this here if we want it listed in the schema...

Copy link
Contributor

Choose a reason for hiding this comment

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

responses:
"200":
description: Success response
content:
application/json:
schema:
title: ImportKeystoresResponse
type: object
properties:
data:
type: array
description: Status result of each `request.keystores` with same length and order of `request.keystores`
type: object
properties:
status:
type: string
description: |
- imported: Keystore successfully decrypted and imported to keymanager permanent storage
- duplicate: Keystore's pubkey is already known to the keymanager
- error: Any other status different to the above: decrypting error, I/O errors, etc.
enum:
- imported
- duplicate
- error
message:
type: string
description: error message if status == error
"401":
$ref: "#/components/responses/Unauthorized"
"403":
$ref: "#/components/responses/Forbidden"
"500":
$ref: "#/components/responses/InternalError"

delete:
operationId: DeleteKeys
Copy link
Contributor

Choose a reason for hiding this comment

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

question on intended behavior, if you send an empty request body is the intention to return the current state of slashing protection data? Also is the slashing protection data only on keys that are inactive or returning the data on the active ones as well 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

probably never mind on the second part, it doesn't hurt to have more data right 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it's very risky to return slashing protection data for keys that haven't been deleted, as it represents stale information that could get you slashed.

Slashing protection data should only be returned for keys from the request that were successfully deleted (status == deleted). We should clarify that in the description

summary: Delete Keys.
description: |
DELETE must delete all keys from `request.pubkeys` that are known to the keymanager and exist in its
persistent storage. Additionally, DELETE must fetch the slashing protection data for the requested keys from
persistent storage, which must be retained (and not deleted) after the response has been sent. Therefore in the
case of two identical delete requests being made, both will have access to slashing protection data.

In a single atomic sequential operation the keymanager must:
1. Guarantee that key(s) can not produce any more signature; only then
2. Delete key(s) and serialize its associated slashing protection data

DELETE should never return a 404 response, even if all pubkeys from request.pubkeys have no extant keystores
nor slashing protection data.
security:
- cookieAuth: []
requestBody:
content:
application/json:
schema:
type: object
properties:
pubkeys:
type: array
description: List of public keys to delete.
items:
$ref: "#/components/types/Pubkey"
responses:
"200":
description: Success response
content:
application/json:
schema:
title: DeleteKeysResponse
type: object
properties:
data:
type: array
description: Deletion status of all keys in `request.pubkeys` in the same order.
type: object
properties:
status:
type: string
description: |
- deleted: Successfully deleted from persistent storage and not able to sign any messages
- not-found: Pubkey not known to keymanager
- error: Any other status different to the above: I/O errors, etc.
enum:
- deleted
- not-found
dapplion marked this conversation as resolved.
Show resolved Hide resolved
- error
message:
type: string
description: error message if status == error
slashing_protection:
type: object
description: |
JSON representation of the slash protection data in format defined in EIP-3076: Slashing
Protection Interchange Format.

"401":
$ref: "#/components/responses/Unauthorized"
"403":
$ref: "#/components/responses/Forbidden"
"500":
$ref: "#/components/responses/InternalError"

components:
responses:
Unauthorized:
$ref: "./types/http.yaml#/Unauthorized"
Forbidden:
$ref: "./types/http.yaml#/Forbidden"
InternalError:
$ref: "./types/http.yaml#/InternalError"

securitySchemes:
cookieAuth: # arbitrary name for the security scheme; will be used in the "security" key later
type: apiKey
in: cookie
name: SESSION # cookie name
Comment on lines +216 to +220
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the rough consensus around cookies vs tokens fell in favour of tokens (see #151 (comment)). Prysm have implemented a JWT, and Lighthouse has a bearer token that's compatible.

We can specify token auth using the template here: https://swagger.io/docs/specification/authentication/bearer-authentication/

Copy link
Contributor

Choose a reason for hiding this comment

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

yup in favor of a token for now as it keeps it more simple.


types:
Pubkey:
type: string
pattern: "^0x[a-fA-F0-9]{96}$"
description: "The validator's BLS public key, uniquely identifying them. _48-bytes, hex encoded with 0x prefix, case insensitive._"
example: "0x93247f2209abcacf57b75a51dafae777f9dd38bc7053d1af526f220a7489a6d3a2753e5f3e8b1cfe39b56f43611df74a"
35 changes: 35 additions & 0 deletions types/http.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -124,3 +124,38 @@ IndexedErrorMessage:
type: string
example: "invalid signature"

Unauthorized:
description: "Unauthorized, no token is found"
content:
application/json:
schema:
type: object
properties:
code:
description: "Either specific error code in case of invalid request or http status code"
type: number
example: 401
message:
description: "Message describing error"
type: string
example:
code: 401
message: "Token not found"

Forbidden:
description: "Forbidden, a token is found but is invalid"
content:
application/json:
schema:
type: object
properties:
code:
description: "Either specific error code in case of invalid request or http status code"
type: number
example: 403
message:
description: "Message describing error"
type: string
example:
code: 403
message: "Token invalid"